This repository has been archived by the owner on Feb 5, 2020. It is now read-only.
modules/aws,azure: use the new tag format for k8s 1.6 #469
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can one of the admins verify this patch? |
bison
reviewed
May 3, 2017
modules/aws/vpc/vpc-private.tf
Outdated
| @@ -32,7 +32,7 @@ resource "aws_subnet" "worker_subnet" { | |||
| "Name", "worker-${ "${length(var.worker_azs)}" > 0 ? | |||
| "${var.worker_azs[count.index]}" : | |||
| "${data.aws_availability_zones.azs.names[count.index]}" }", | |||
| "KubernetesCluster", "${var.cluster_name}", | |||
| "kubernetes.io/cluster/${var.cluster_name}", "owned", | |||
There was a problem hiding this comment.
I'm not really sure if anything in Kubernetes actually uses the owned vs. shared values at the moment, but in any case, it might make more sense to mark subnets as shared. That was actually one of the main use cases for moving to this style of tagging -- being able to share subnets between clusters.
|
@bison No, only in 1.6. |
|
Approved if @bison approves it. I don't have the required background for this. |
Quentin-M
approved these changes
May 4, 2017
|
wait for green and merge |
|
retriggered stalled build, got green, hence merging. |
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Jul 26, 2018
The tags I'm adding lack Name and
kubernetes.io/cluster/${var.cluster_name}, which are part of the
tagging for most other resources created in this module. But Chance,
pointed out that Kubernetes doesn't manage EIPs, so we don't need to
set the kubernetes.io tags at all. And naming EIPs doesn't add much
information; after setup completes you can get all the naming
information you need from the associated NAT gateway.
Setting the cluster ID makes it easier for us to reap leaked resources
after tearing down a cluster (for example, if a test which creates and
deletes a cluster is evicted before completing deletion). And passing
extra_tags through allows us to set expiration dates and such for our
new resources, which will also help with cleanup.
Not related to this commit, but for future reference Chance elaborated
on the use of 'shared' for VPC-related resource. Those are from
e4cd883a (modules/aws/vpc: set shared rather than owned for
VPC/Subnets, 2017-05-03, coreos/tectonic-installer#469), and Chance's
use-case is:
1. User creates a new cluster with the installer, not thinking about
VPC reuse.
2. User creates a second cluster sharing the same VPC and subnets.
Ideally those users would create the VPCs and subnets externally with
shared tags and pass them through to the installer by ID. Then it
would be clear that the installer did not create them, and you could
safely delete either cluster without affecting the shared resources.
But for users without the foresight to create shared VPCs, having
terraform create "shared" VPCs directly gives you similar results (as
long as you don't delete the first cluster and break everything ;).
frobware
pushed a commit
to frobware/installer
that referenced
this pull request
Sep 17, 2018
The tags I'm adding lack Name and
kubernetes.io/cluster/${var.cluster_name}, which are part of the
tagging for most other resources created in this module. But Chance,
pointed out that Kubernetes doesn't manage EIPs, so we don't need to
set the kubernetes.io tags at all. And naming EIPs doesn't add much
information; after setup completes you can get all the naming
information you need from the associated NAT gateway.
Setting the cluster ID makes it easier for us to reap leaked resources
after tearing down a cluster (for example, if a test which creates and
deletes a cluster is evicted before completing deletion). And passing
extra_tags through allows us to set expiration dates and such for our
new resources, which will also help with cleanup.
Not related to this commit, but for future reference Chance elaborated
on the use of 'shared' for VPC-related resource. Those are from
e4cd883a (modules/aws/vpc: set shared rather than owned for
VPC/Subnets, 2017-05-03, coreos/tectonic-installer#469), and Chance's
use-case is:
1. User creates a new cluster with the installer, not thinking about
VPC reuse.
2. User creates a second cluster sharing the same VPC and subnets.
Ideally those users would create the VPCs and subnets externally with
shared tags and pass them through to the installer by ID. Then it
would be clear that the installer did not create them, and you could
safely delete either cluster without affecting the shared resources.
But for users without the foresight to create shared VPCs, having
terraform create "shared" VPCs directly gives you similar results (as
long as you don't delete the first cluster and break everything ;).
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Jan 11, 2019
Centralize extra-tag inclusion on aws/main.tf. This reduces the
number of places we need to think about what tags should be ;).
Also keep kubernetes.io/cluster/{name} localized in the aws module.
See 8a37f72 (modules/aws/bootstrap: Pull AWS bootstrap setup into a
module, 2018-09-05, openshift#217) for why we need to keep it on the bootstrap
instance. But the bootstrap resources will be removed after the
bootstrap-complete event comes through, and we don't want Kubernetes
controllers trying to pick them up.
This commit updates the internal Route 53 zone from KubernetesCluster
to kubernetes.io/cluster/{name}: owned, catching it up to
kubernetes/kubernetes@0b5ae539 (AWS: Support shared tag, 2017-02-18,
kubernetes/kubernetes#41695). That tag originally landed on the zone
back in 75fb49a (platforms/aws: apply tags to internal route53 zone,
2017-05-02, coreos/tectonic-installer#465).
Only the master instances need the clusterid tag, as described in
6c7a5f0 (Tag master machines for adoption by machine controller,
2018-10-17, openshift#479).
A number of VPC resources have moved from "shared" to "owned". The
shared values are from 45dfc2b (modules/aws,azure: use the new tag
format for k8s 1.6, 2017-05-04, coreos/tectonic-installer#469). The
commit message doesn't have much to say for motivation, but Brad Ison
said [1]:
I'm not really sure if anything in Kubernetes actually uses the
owned vs. shared values at the moment, but in any case, it might
make more sense to mark subnets as shared. That was actually one of
the main use cases for moving to this style of tagging -- being able
to share subnets between clusters.
But we aren't sharing these resources; see 6f55e67 (terraform/aws:
remove option to use an existing vpc in aws, 2018-11-11, openshift#654).
[1]: coreos/tectonic-installer#469 (comment)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #402