Make ClusterID required for AWS. #49215

rrati · 2017-07-19T16:52:56Z

What this PR does / why we need it:
Makes ClusterID required for AWS and provides a flag to run in un-tagged mode

Release note:

A cluster using the AWS cloud provider will need to label existing nodes and resources with a ClusterID or the kube-controller-manager will not start.  To run without a ClusterID pass --allow-untagged-cloud=true to the kube-controller-manager on startup.

k8s-ci-robot

@spxtr: 12 new warning(s).

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2017-07-19T19:07:15Z

pkg/cloudprovider/providers/fake/fake.go

@@ -111,6 +111,10 @@ func (f *FakeCloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []s
 	return nameservers, searches
 }

+func (f *FakeCloud) HasClusterID() bool {


Golint comments: exported method FakeCloud.HasClusterID should have comment or be unexported. More info.

k8s-ci-robot · 2017-07-19T19:07:15Z

pkg/cloudprovider/providers/aws/aws.go

@@ -956,6 +956,11 @@ func (c *Cloud) Routes() (cloudprovider.Routes, bool) {
 	return c, true
 }

+// ClusterID returns true if the cluster has a clusterID


Golint comments: comment on exported method Cloud.HasClusterID should be of the form "HasClusterID ...". More info.

k8s-ci-robot · 2017-07-19T19:07:16Z

pkg/cloudprovider/providers/rackspace/rackspace.go

@@ -466,6 +466,11 @@ func (os *Rackspace) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []
 	return nameservers, searches
 }

+// ClusterID returns true if the cluster has a clusterID


Golint comments: comment on exported method Rackspace.HasClusterID should be of the form "HasClusterID ...". More info.

k8s-ci-robot · 2017-07-19T19:07:16Z

pkg/cloudprovider/providers/azure/azure.go

@@ -382,6 +382,11 @@ func (az *Cloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []stri
 	return nameservers, searches
 }

+// ClusterID returns true if the cluster has a clusterID


Golint comments: comment on exported method Cloud.HasClusterID should be of the form "HasClusterID ...". More info.

k8s-ci-robot · 2017-07-19T19:07:16Z

cmd/kube-controller-manager/app/controllermanager.go

+		if s.AllowUntaggedCloud == true {
+			glog.Warning("Detected a cluster without a ClusterID.  A ClusterID will be required in the future.  Please tag your cluster to avoid any future issues.")
+		} else {
+			return ControllerContext{}, fmt.Errorf("No ClusterID Found.  A ClusterID is required for the cloud provider to function properly.  This check can be bypassed by setting the allow-untagged-cloud option")


Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

k8s-ci-robot · 2017-07-19T19:07:16Z

pkg/cloudprovider/providers/gce/gce.go

@@ -409,6 +409,11 @@ func (gce *GCECloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []
 	return nameservers, srchOut
 }

+// ClusterID returns true if the cluster has a clusterID
+func (gce *GCECloud) HasClusterID() bool {


Golint naming: receiver name gce should be consistent with previous receiver name g for GCECloud. More info.

k8s-ci-robot · 2017-07-19T19:07:16Z

pkg/cloudprovider/providers/ovirt/ovirt.go

@@ -134,6 +134,11 @@ func (v *OVirtCloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []
 	return nameservers, searches
 }

+// ClusterID returns true if the cluster has a clusterID


Golint comments: comment on exported method OVirtCloud.HasClusterID should be of the form "HasClusterID ...". More info.

k8s-ci-robot · 2017-07-19T19:07:16Z

pkg/cloudprovider/providers/cloudstack/cloudstack.go

@@ -120,6 +120,11 @@ func (cs *CSCloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []st
 	return nameservers, searches
 }

+// ClusterID returns true if the cluster has a clusterID


Golint comments: comment on exported method CSCloud.HasClusterID should be of the form "HasClusterID ...". More info.

k8s-ci-robot · 2017-07-19T19:07:16Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -702,6 +702,11 @@ func (vs *VSphere) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []st
 	return nameservers, searches
 }

+// ClusterID returns true if the cluster has a clusterID


Golint comments: comment on exported method VSphere.HasClusterID should be of the form "HasClusterID ...". More info.

k8s-ci-robot · 2017-07-19T19:07:17Z

pkg/cloudprovider/providers/photon/photon.go

@@ -539,6 +539,11 @@ func (pc *PCCloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []st
 	return nameservers, searches
 }

+// ClusterID returns true if the cluster has a clusterID


Golint comments: comment on exported method PCCloud.HasClusterID should be of the form "HasClusterID ...". More info.

spxtr · 2017-07-19T20:08:29Z

/lint

k8s-ci-robot

@spxtr: 1 new warning(s).

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2017-07-19T20:08:44Z

pkg/cloudprovider/providers/gce/gce.go

@@ -409,6 +409,11 @@ func (gce *GCECloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut []
 	return nameservers, srchOut
 }

+// HasClusterID returns true if the cluster has a clusterID
+func (gce *GCECloud) HasClusterID() bool {


Golint naming: receiver name gce should be consistent with previous receiver name g for GCECloud. More info.

This is not correct. What I have is consistent with existing code

I see. It's g at one part of the file and gce for the rest. Your code is fine here, feel free to ignore the lint message :)

Or fix the other part of the file.

I could rather do that cleanup as a part of a separate PR. It really isn't related to this PR.

rrati · 2017-07-20T12:06:18Z

@kubernetes/sig-aws-misc @justinsb

rrati · 2017-07-20T19:04:37Z

/test pull-kubernetes-e2e-gce-etcd3

roberthbailey · 2017-07-20T20:17:34Z

cmd/kube-controller-manager/app/options/options.go

@@ -134,6 +134,7 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
 	fs.BoolVar(&s.UseServiceAccountCredentials, "use-service-account-credentials", s.UseServiceAccountCredentials, "If true, use individual service account credentials for each controller.")
 	fs.StringVar(&s.CloudProvider, "cloud-provider", s.CloudProvider, "The provider for cloud services.  Empty string for no provider.")
 	fs.StringVar(&s.CloudConfigFile, "cloud-config", s.CloudConfigFile, "The path to the cloud provider configuration file.  Empty string for no configuration file.")
+	fs.BoolVar(&s.AllowUntaggedCloud, "allow-untagged-cloud", false, "Allow the cluster to run without the cluster-id on cloud instances.  This is a legacy mode of operation and a cluster-id will be required in the future.")


Should we mark this flag as deprecated immediately?

This flag seems pointless if it defaults to false. People either have to change their config to pass --allow-untagged-cloud or change their config to pass clusterID. If you are going to break compatibility, just force people to get to where they need to go. By allowing them to pass --allow-untagged-cloud and later removing the flag, aren't you just breaking compatibility twice?

Whether to set the default to false or true is still being discussed in the AWS sig. However, imo, setting the flag to false forces the issue immediately and gives users time to do the labeling before the option goes away. The intent is for this option to only exist for a few releases. Having it default to false will inform the user while still allowing for an easy change to function in the old, but broken, behavior while they plan to do the required labeling.

@roberthbailey I believe that is the plan, but would prefer aws-sig members weigh in.

The thought behind the flag is that changing your infrastructure deployment tool to add the tags may be non-trivial.

So we are forcing people to add the tag, but we're also giving them a reasonable path to upgrade if they can't easily add that to their deployment tool.

I agree we can mark the flag deprecated right away.

I marked the new flag as deprecated

justinsb · 2017-08-04T17:33:51Z

pkg/kubeapiserver/options/cloudprovider.go

@@ -30,6 +30,7 @@ import (
 type CloudProviderOptions struct {
 	CloudConfigFile string
 	CloudProvider   string
+	AllowUntagged   bool


May be missing something, but is this one used?

Nope, this shouldn't be there. I'll remove

justinsb · 2017-08-04T17:34:23Z

Looks good from an AWS perspective.

rrati · 2017-08-08T19:15:29Z

/test pull-kubernetes-e2e-gce-etcd3

rrati · 2017-08-08T19:15:40Z

/test pull-kubernetes-verify

rrati · 2017-08-09T14:09:25Z

/test pull-kubernetes-verify

rrati · 2017-08-09T15:24:23Z

@roberthbailey @mikedanese PTAL

mikedanese · 2017-08-09T17:23:49Z

This needs a release note action required.

rrati · 2017-08-09T18:26:18Z

@mikedanese There is a release note in the original PR text. It hasn't been showing up for some reason

mikedanese · 2017-08-09T18:55:34Z

/lgtm

deads2k · 2017-08-09T19:32:49Z

/approve

k8s-github-robot · 2017-08-09T19:33:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mikedanese, rrati

Associated issue: 48954

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

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

~~cmd/OWNERS~~ [mikedanese]
~~hack/verify-flags/OWNERS~~ [deads2k]
~~pkg/apis/componentconfig/OWNERS~~ [mikedanese]
~~pkg/cloudprovider/OWNERS~~ [mikedanese]

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-08-10T14:34:45Z

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

rrati · 2017-08-10T15:18:03Z

@mikedanese Please re-lgtm

k8s-github-robot · 2017-08-10T15:25:29Z

Automatic merge from submit-queue

Automatic merge from submit-queue (batch tested with PRs 16443, 16331) Require cluster id for AWS Backporting of: kubernetes/kubernetes#48612 kubernetes/kubernetes#49215 Plus changes in master-controller startup to look for the flag to allow untagged clouds or exit.

mikedanese · 2017-09-28T17:34:20Z

Can we remove HasClusterID from cloudprovider interface once we've finished the deprecation of untagged cloud for aws?

Automatic merge from submit-queue Make ClusterID required for AWS. **What this PR does / why we need it**: Makes ClusterID required for AWS and provides a flag to run in un-tagged mode fixes kubernetes#48954 **Release note**: ```release-note A cluster using the AWS cloud provider will need to label existing nodes and resources with a ClusterID or the kube-controller-manager will not start. To run without a ClusterID pass --allow-untagged-cloud=true to the kube-controller-manager on startup. ```

andrewsykim · 2019-02-20T05:16:36Z

Sorry to be commenting on a really old PR, but I don't think we followed up and deprecated the necessary flags. I'm not even sure what the state of ClusterID is at this point. Is it safe to remove the --allow-untagged-cloud flag?

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

k8s-github-robot assigned roberthbailey and mikedanese Jul 19, 2017

rrati force-pushed the aws-require-cluster-id branch from f5d3987 to dfd943a Compare July 19, 2017 17:54

k8s-ci-robot reviewed Jul 19, 2017

View reviewed changes

rrati force-pushed the aws-require-cluster-id branch from dfd943a to eac5939 Compare July 19, 2017 19:50

k8s-ci-robot reviewed Jul 19, 2017

View reviewed changes

k8s-ci-robot added the sig/aws label Jul 20, 2017

roberthbailey reviewed Jul 20, 2017

View reviewed changes

justinsb reviewed Aug 4, 2017

View reviewed changes

rrati force-pushed the aws-require-cluster-id branch 2 times, most recently from 2660f72 to 458ad97 Compare August 7, 2017 16:42

Make ClusterID required for AWS. kubernetes#48954

926f070

roberthbailey removed their assignment Aug 9, 2017

mikedanese added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 9, 2017

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

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 9, 2017

rrati force-pushed the aws-require-cluster-id branch from 458ad97 to 926f070 Compare August 10, 2017 13:42

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

k8s-github-robot merged commit e9ab489 into kubernetes:master Aug 10, 2017

rrati mentioned this pull request Sep 13, 2017

Require cluster id for AWS openshift/origin#16331

Merged

MrHohn mentioned this pull request Sep 15, 2017

Only return true if a ProviderID exists for GCE #52546 #52555

Closed

andrewsykim mentioned this pull request Feb 20, 2019

Investigate usage/requirements for ClusterID kubernetes/cloud-provider#12

Closed

rrati mentioned this pull request Jul 10, 2020

REQUEST: New membership for rrati kubernetes/org#2016

Closed

6 tasks

Make ClusterID required for AWS. #49215

Make ClusterID required for AWS. #49215

Conversation

rrati commented Jul 19, 2017 • edited Loading

k8s-ci-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spxtr commented Jul 19, 2017

k8s-ci-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrati commented Jul 20, 2017

rrati commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Aug 4, 2017

rrati commented Aug 8, 2017

rrati commented Aug 8, 2017

rrati commented Aug 9, 2017

rrati commented Aug 9, 2017 • edited Loading

mikedanese commented Aug 9, 2017

rrati commented Aug 9, 2017

mikedanese commented Aug 9, 2017

deads2k commented Aug 9, 2017

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 10, 2017

rrati commented Aug 10, 2017

k8s-github-robot commented Aug 10, 2017

mikedanese commented Sep 28, 2017

andrewsykim commented Feb 20, 2019

rrati commented Jul 19, 2017 •

edited

Loading

rrati commented Aug 9, 2017 •

edited

Loading