Fix shared subnet/vpc tags #3184

justinsb · 2017-08-11T10:20:40Z

Stop setting the Name tag on a shared subnet/vpc
Stop setting the legacy KubernetesCluster tag on a shared subnet/vpc
that is new enough (>=1.6); we rely on the shared tags instead
Set tags on shared subnets; i.e. we do set the shared tag on a
shared subnet; that is important for ELBs
Set tags on shared VPCs; i.e. we do set the shared tag on a shared
VPC; that is not used but consistent with subnets.
Add tests for shared subnet

justinsb · 2017-08-11T10:29:57Z

@KashifSaadat this is the PR that may make it so that we don't need #3064.

But https://github.com/kubernetes/kops/pull/3184/files#diff-ee8d33deb9549322c28a9ef65ed04473R241 is the tag we talked about that we still need on a shared subnet. It's not a conflict any more, but it does require a tag for each cluster using the subnet.

Something I think would also be a nice alternative would be to separate out the network objects into their own "thing" - likely their own top level API object like a Cluster - and then we could tag the VPCs and Subnets with a tag like k8s.io/network-id=myname, and then the instances would also reference the network in that way (i.e. we don't need a per-cluster tag on the network objects). But that requires changes in kubernetes/kubernetes, so would have to wait till 1.8 at the earliest.

Let me know if the shared tag is a problem for your use-case and we can discuss more!

KashifSaadat · 2017-08-11T11:05:41Z

upup/pkg/fi/cloudup/awstasks/vpc.go

@@ -143,11 +144,10 @@ func (_ *VPC) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *VPC) error {
 			if featureflag.VPCSkipEnableDNSSupport.Enabled() {
 				glog.Warningf("VPC did not have EnableDNSSupport=true, but ignoring because of VPCSkipEnableDNSSupport feature-flag")
 			} else {
+				// TODO: We could easily just allow kops to fix this...


Would be good to automatically rectify this issue but could cause issues if users are not expecting any change to occur to a shared resource. Maybe allow users to specify an auto-fix flag and cover other similar cases if there are any?

KashifSaadat · 2017-08-11T11:07:21Z

This is excellent. The cluster tag addition shouldn't be an issue for our use-case, will give it a test. Thanks for the work @justinsb! 👍

KashifSaadat · 2017-08-16T12:53:38Z

Tested and this works really well. Couple notes:

The dry-run still mentions a modification will be made to the Name tag of the InternetGateway resource (when applying the changes, the Internet Gateway is not modified). This needs to be ignored, or the same behaviour applied with adding owned and shared tags.
If users are managing the VPC/Subnet state independently via other methods such as Terraform, those scripts may attempt to drop the newly added tags (unless manually imported into their scripts / state files).

chrislovecnm

What happens when the kops install user cannot modify subnets or the vpc? i.e. add tags?

k8s-github-robot · 2017-08-19T00:03:37Z

@justinsb PR needs rebase

justinsb · 2017-08-20T16:12:22Z

What happens when the kops install user cannot modify subnets or the vpc?

For subnets, k8s needs it if you're going to be creating ELBs. I'd like to fix this (#3191) but need to clear some of the backlog first...

chrislovecnm · 2017-08-20T17:24:21Z

@justinsb on the vpc as well?

justinsb · 2017-08-22T07:31:32Z

@chrislovecnm It is required on the subnet. It is not required on the VPC, but it seems inconsistent to do one but not the other. I would like to get it so that you don't need permissions on either (#3191), but until then it doesn't seem like the one permission is going to be much worse than the other...

chrislovecnm · 2017-10-05T06:38:35Z

@geojaz can you take a look please? We need to work out these details

blakebarnett · 2017-10-06T21:11:21Z

Testing some of this now, I noticed that kops doesn't seem to respect kubernetes.io/cluster/<name>=shared on the main routing table for the cluster (the one named <cluster_name>)

Otherwise setting all the tags properly shared subnets/rtb/ngw seems to work for me.

chrislovecnm · 2017-10-09T23:26:50Z

@blakebarnett the route table does not have a shared bool in the model. Bug.

chrislovecnm · 2017-10-09T23:28:05Z

@KashifSaadat / @justinsb what do we need to resolve? Can we get a rebase and get this merged?

justinsb · 2017-10-22T21:20:03Z

Rebased.

AFAIK @chrislovecnm you're the only -1 on this.

* Stop setting the Name tag on a shared subnet/vpc * Stop setting the legacy KubernetesCluster tag on a shared subnet/vpc that is new enough (>=1.6); we rely on the shared tags instead * Set tags on shared subnets; i.e. we _do_ set the shared tag on a shared subnet; that is important for ELBs * Set tags on shared VPCs; i.e. we _do_ set the shared tag on a shared VPC; that is not used but consistent with subnets. * Add tests for shared subnet

KashifSaadat

LGTM, are we able to get this merged in? :)

chrislovecnm · 2017-11-01T14:39:27Z

/lgtm

k8s-github-robot · 2017-11-01T14:41:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-01T14:41:19Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-01T15:23:29Z

Automatic merge from submit-queue.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2017

justinsb force-pushed the test_does_not_change_tags branch from ec57745 to c80c051 Compare August 11, 2017 10:23

KashifSaadat reviewed Aug 11, 2017

View reviewed changes

KashifSaadat mentioned this pull request Aug 16, 2017

Building cluster in existing VPC takes over VPC and IGW #3203

Closed

chrislovecnm reviewed Aug 18, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2017

KashifSaadat mentioned this pull request Aug 19, 2017

Ignore shared services (vpcs, subnets) showing up in a dry-run #3064

Closed

KashifSaadat mentioned this pull request Sep 13, 2017

Using shared VPC results in incorrect modification message #476

Closed

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2017

justinsb added this to the 1.8.0 milestone Sep 25, 2017

chrislovecnm added the blocks-next label Oct 5, 2017

justinsb force-pushed the test_does_not_change_tags branch from c80c051 to daabe45 Compare October 22, 2017 21:18

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2017

justinsb added 2 commits October 23, 2017 11:30

Sort Tags consistently to avoid test flakes

9cf22ae

justinsb force-pushed the test_does_not_change_tags branch from daabe45 to 9cf22ae Compare October 23, 2017 15:39

KashifSaadat approved these changes Nov 1, 2017

View reviewed changes

k8s-ci-robot assigned chrislovecnm Nov 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2017

k8s-github-robot merged commit 08c34b6 into kubernetes:master Nov 1, 2017

kashook mentioned this pull request Jan 12, 2018

kubernetes.io/cluster/<name>=shared tag being added to VPC that isn't managed by kops #4265

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shared subnet/vpc tags #3184

Fix shared subnet/vpc tags #3184

justinsb commented Aug 11, 2017

justinsb commented Aug 11, 2017

KashifSaadat Aug 11, 2017 •

edited

Loading

KashifSaadat commented Aug 11, 2017 •

edited

Loading

KashifSaadat commented Aug 16, 2017

chrislovecnm left a comment

k8s-github-robot commented Aug 19, 2017

justinsb commented Aug 20, 2017

chrislovecnm commented Aug 20, 2017

justinsb commented Aug 22, 2017

chrislovecnm commented Oct 5, 2017

blakebarnett commented Oct 6, 2017

chrislovecnm commented Oct 9, 2017

chrislovecnm commented Oct 9, 2017

justinsb commented Oct 22, 2017

KashifSaadat left a comment

chrislovecnm commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

Fix shared subnet/vpc tags #3184

Fix shared subnet/vpc tags #3184

Conversation

justinsb commented Aug 11, 2017

justinsb commented Aug 11, 2017

KashifSaadat Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

KashifSaadat commented Aug 11, 2017 • edited Loading

KashifSaadat commented Aug 16, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Aug 19, 2017

justinsb commented Aug 20, 2017

chrislovecnm commented Aug 20, 2017

justinsb commented Aug 22, 2017

chrislovecnm commented Oct 5, 2017

blakebarnett commented Oct 6, 2017

chrislovecnm commented Oct 9, 2017

chrislovecnm commented Oct 9, 2017

justinsb commented Oct 22, 2017

KashifSaadat left a comment

Choose a reason for hiding this comment

chrislovecnm commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

k8s-github-robot commented Nov 1, 2017

KashifSaadat Aug 11, 2017 •

edited

Loading

KashifSaadat commented Aug 11, 2017 •

edited

Loading