Adding Alternative Node Naming Convention #61878

nathanjsweet · 2018-03-29T05:11:33Z

What this PR does / why we need it:
Currently node names in aws are derived from the private-dns entry of the ec2 instance.
This PR offers administrators, who might have differing dns toplogies in their VPCs,
the option to manually configure their node names. This PR also directly addresses a
bug in K8s that prevents the Node Admission Controller from working properly when the AWS cloud provider is selected.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #11543
Fixes #47695
Fixes #52241

Addresses #48929

Is there someone where I need to document this?

Allow node names in AWS to be configured explicitly with a tag

/sig aws

Currently node names in aws are derived from the private-dns entry of the ec2 instance. This PR offers administrators, who might have differing dns toplogies in their VPCs, the option to manually configure their node names. This PR also directly addresses a bug in K8s that prevents the Node Admission Controller from working properly when the AWS cloud provider is selected.

DanyC97 · 2018-04-03T12:39:40Z

@liggitt is this part of the work you mentioned #54482 (comment) ?

liggitt · 2018-04-03T13:21:55Z

I hadn't seen this PR in particular, but this type of change is what I was anticipating (though I didn't know whether @kubernetes/sig-aws-misc wanted to drive this while the cloudprovider was still in-tree)

nathanjsweet · 2018-04-20T21:05:49Z

Okay, I can confirm this is working now. It took me a while to get to testing it end-to-end.
@jsafrane can I assign this to you?

nathanjsweet · 2018-04-20T21:06:58Z

Oh yeah the cloud config looks like this:

[Global]
ExplicitNodeNames=true

liggitt · 2018-04-20T21:14:48Z

pkg/cloudprovider/providers/aws/aws.go

+			}
+		}
+		glog.Infof("Tag %s not found for instance %s", TagNameKubernetesNodeNamePrefix, aws.StringValue(i.InstanceId))
+	}
 	return types.NodeName(aws.StringValue(i.PrivateDnsName))


I assume a custom tag value is mutable on an instance. If so, changing the value of the custom tag after a node has been added to the cluster will cause problems... docs should make that clear

Can you point me to where the docs are?

I meant the docs to be added around this new option should call out the sharp edges :)

liggitt · 2018-04-20T21:15:45Z

pkg/cloudprovider/providers/aws/aws.go

+				if len(value) > 0 {
+					return types.NodeName(aws.StringValue(tag.Value))
+				}
+				glog.Infof("Tag %s was found for instance %s, but had no value", TagNameKubernetesNodeNamePrefix, aws.StringValue(i.InstanceId))


will silently falling back to the private dns name lead to scenarios where there's a race to apply the custom tag before the kubelet starts on up on the node and registers itself using the private dns name?

Enabling this feature and not having the tags in place will cause problems, but I think if you decide to enable this feature it's on you to make sure you don't mess up the names. This is already the case with the cloud-provider flag which requires the node-name and hostname to match (something that is undocumented).

Also, my guess is that kubelet doesn't register if this happens.

This is already the case with the cloud-provider flag which requires the node-name and hostname to match

in that case, the names aren't mutable... the race between kubelet start and tag addition is my main concern

my guess is that kubelet doesn't register if this happens

if it's the first startup in a tls-bootstrapping setup, the kublet will use the private dns name to get it's initial credential and register, then start failing as soon as the tag appears and it's idea of who it is changes

Enabling this feature and not having the tags in place will cause problems

This must be clearly documented. And IMO kubelet should exit when it starts on a node with ExplicitNodeNames enabled and without the right tag on the node.

And IMO kubelet should exit when it starts on a node with ExplicitNodeNames enabled

Yeah, okay. I'll make that change.

jsafrane · 2018-04-23T08:08:24Z

/ok-to-test

jsafrane · 2018-04-23T08:12:22Z

Is there someone where I need to document this?

Yes! I could not find any documentation about AWS cloud.conf format and keys/values. I am afraid it's you who's supposed to create one.

It belongs here: https://kubernetes.io/docs/concepts/cluster-administration/cloud-providers/#aws
(source: https://github.com/kubernetes/website/tree/master/docs/concepts/cluster-administration)

…ster

nathanjsweet · 2018-05-01T03:54:31Z

@jsafrane okay I gave mapInstanceToNodeName and error. I'm fairly confident that should address the issues.

liggitt · 2018-05-01T04:20:56Z

Does this PR resolve #52241 ?

nathanjsweet · 2018-05-01T05:02:10Z

@liggitt Yes it would.

nckturner

I think this would be an awesome improvement, but if it is possible I'd like to see it done without additional configuration options. I think this is possible by defaulting to private DNS if tags are not present nodes, but I'd love to hear other's thoughts.

nckturner · 2018-05-02T19:05:14Z

pkg/cloudprovider/providers/aws/aws.go

-func mapInstanceToNodeName(i *ec2.Instance) types.NodeName {
-	return types.NodeName(aws.StringValue(i.PrivateDnsName))
+func mapInstanceToNodeName(i *ec2.Instance, explicitNames bool) (types.NodeName, error) {
+	if explicitNames {


I like the tags idea, but rather than differentiating between the old way (private DNS name) and the new way (tagged name) via a config option, I'd prefer if we can allow the new behavior in a backwards compatible way, without the need for a new config option. I.e., default to private DNS name but use a tagged name if present on the node.

nckturner · 2018-05-02T19:10:59Z

pkg/cloudprovider/providers/aws/aws.go

-	return types.NodeName(aws.StringValue(i.PrivateDnsName))
+func mapInstanceToNodeName(i *ec2.Instance, explicitNames bool) (types.NodeName, error) {
+	if explicitNames {
+		for _, tag := range i.Tags {


I.e. check to see if a tag exists that matches kubernetes.io/nodeName, and if not, we fall back to private DNS instead of erroring.

nckturner · 2018-05-02T19:13:33Z

pkg/cloudprovider/providers/aws/aws.go

 	filters := []*ec2.Filter{
-		newEc2Filter("private-dns-name", privateDNSName),
+		newEc2Filter(nodeFilter, privateDNSName),


Here we would have to attempt with the tag filter, check to see if any instances were returned, then try private DNS.

liggitt · 2018-05-02T19:37:36Z

I think this is possible by defaulting to private DNS if tags are not present nodes, but I'd love to hear other's thoughts.

switching a node name after the fact is disruptive enough that it doesn't seem great to infer and fallback this way. see discussion (now hidden) #61878 (comment) of the race conditions between adding tags and kubelet startup that falling back to the dns name can cause

nathanjsweet · 2018-06-14T16:07:24Z

Sorry for the bombardment, I accidentally rebased higher up the tree than I should have.

nathanjsweet · 2018-06-21T16:53:22Z

I can no longer carry this PR forward, as I have left Datica Inc. Datica has signed the CNCF agreement though, so I don't know if that means a maintainer can fork this and carry it forward. Sorry for the inconvenience.

nathanjsweet · 2018-06-21T16:54:14Z

pkg/cloudprovider/providers/aws/volumes.go

@@ -141,7 +141,10 @@ func (c *Cloud) checkIfAttachedToNode(diskName KubernetesVolumeID, nodeName type
 		}

 		awsDiskInfo.ec2Instance = instanceInfo
-		awsDiskInfo.nodeName = mapInstanceToNodeName(instanceInfo)
+		awsDiskInfo.nodeName, err = mapInstanceToNodeName(instanceInfo, c.cfg.Global.ExplicitNodeNames)


This is going to be a problem, if someone switches naming topology midway through, then disks could potentially get orphaned.

karlhungus · 2018-07-10T16:43:38Z

/retest

karlhungus · 2018-07-10T16:44:27Z

I was going to offer help here, but I can't see any comments that haven't already been addressed

karlhungus · 2018-07-10T17:52:26Z

I'm happy to take a stab at addressing this: #61878 (comment) (documentation) comment

nathanjsweet · 2018-07-11T08:55:09Z

@karlhungus There needs to be dire warnings in the documentation that the node naming convention can never be switched after cloud resources have been deployed by Kubernetes without orphaning those resources (I suppose you could just manually delete them), however disks would get orphaned.

liggitt · 2018-07-27T19:33:15Z

pkg/cloudprovider/providers/aws/aws.go

+		// as the assigned node name, and want to explicitly set the names of their nodes, via
+		// tags. This configuration indicates that all node names have been explicitly
+		// set with the tag "kubernetes.io/node-name".
+		ExplicitNodeNames bool


looking at this cold, a name like useTagsForNodeNames might be clearer

v1k0d3n · 2018-09-06T19:44:29Z

as a service provider, it would be a great option to see implemented. is there any status update, or is there a way we can throw our hat in the ring and lend a hand with this issue? it seems like this fell off the wagon...

liggitt · 2018-09-06T20:45:33Z

I'd recommend getting on the agenda in a sig-aws and/or sig-cloudprovider meeting for visibility and blessing to help get it over the finish line. Would also be good to make sure it's a pattern that could be imitated by other providers and make the maintainers of those providers aware of this effort.

nickperry · 2018-11-23T19:42:36Z

What's happening with this one? We really need this at our site.

nathanjsweet · 2018-11-27T15:53:15Z

@liggitt With the benefit of time I am starting to think that this PR is a bad idea. I can't think of another K8s feature that requires the admin to understand such a significant amount of how the underlying cloud-controller works. I think a better solution would be for the cloud controller to include an interface for changing node names (this would require them to persist) so that if the admin decides to change the node naming convention it is easy to do so. Another reason I think that is a better solution is that this current PR only fixes the problem for new clusters. Old clusters really can't make use of this PR. I'd be willing to draft something like this up (I'll be working on K8s full time again in January), but WG should I propose this idea to?

liggitt · 2018-11-27T15:58:30Z

I'd be willing to draft something like this up (I'll be working on K8s full time again in January), but WG should I propose this idea to?

cloud providers have been consolidated under sig-cloud-provider, to improve consistency in how k8s interfaces with cloud providers. That group would be the venue for discussing this

liggitt · 2018-11-27T16:00:36Z

I think a better solution would be for the cloud controller to include an interface for changing node names (this would require them to persist) so that if the admin decides to change the node naming convention it is easy to do so.

as an aside, changing an existing node's name is likely to be just as disruptive as removing and creating a new node. API object names are immutable, as is the spec.nodeName field in pods.

justinsb · 2018-11-27T16:12:20Z

I think it would be great to have the cloudprovider pass Node objects, instead of NodeNames, because then we could get the instance id from the node object (the ProviderID field IIRC). Then the cloudprovider wouldn't care about the Node Name at all.

It's also good because it avoids having to map from a NodeName -> an AWS instance ID, which requires caching and/or API calls.

As @liggitt points out, we can work on this as part of the cloudprovider extraction; and we probably won't be able to rename Nodes, but we can at least remove the cloudprovider constraint on the Name. I think at that point it's easy to wrap kubelet in a script to build a name from tags, IP, instance ID, etc.

sigil66 · 2018-11-29T17:25:29Z

@justinsb is there a target release for the cloud provider extraction project?

fejta-bot · 2019-02-27T18:07:10Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

nathanjsweet · 2019-02-27T18:11:49Z

This PR is stale and not happening.

andrewsykim · 2019-02-27T18:22:48Z

@justinsb is there a target release for the cloud provider extraction project?

Provider extraction efforts are still ongoing and will be ongoing for the next few releases. I personally am not opposed to addressing this along-side the extraction effort. Feel free to add this to the next SIG cloud provider meeting agenda

pierluigilenoci · 2019-02-28T09:40:49Z

/remove-lifecycle stale

cabrinha · 2019-04-28T21:11:53Z

@justinsb is there a target release for the cloud provider extraction project?

Provider extraction efforts are still ongoing and will be ongoing for the next few releases. I personally am not opposed to addressing this along-side the extraction effort. Feel free to add this to the next SIG cloud provider meeting agenda

Can you add it to that agenda? It seems like this issue is getting lost in the noise.

andrewsykim · 2019-04-29T14:26:10Z

@cabrinha best place to continue this discussion would be our mailing list https://groups.google.com/forum/#!forum/kubernetes-sig-cloud-provider

k8s-ci-robot requested review from jsafrane and justinsb March 29, 2018 05:11

DanyC97 mentioned this pull request Apr 3, 2018

--hostname-override ignored when --cloud-provider is specified #54482

Open

nathanjsweet changed the title ~~WIP: Adding Alternative Node Naming Convention~~ Adding Alternative Node Naming Convention Apr 20, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2018

liggitt reviewed Apr 20, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 23, 2018

add error to mapInstanceToNodeName so that kubelet won't falsely regi…

3fa831f

…ster

liggitt mentioned this pull request May 1, 2018

Add alternative to PrivateDnsName on AWS cloud provider #52241

Closed

nckturner reviewed May 2, 2018

View reviewed changes

nathanjsweet commented Jun 21, 2018

View reviewed changes

karlhungus mentioned this pull request Jul 12, 2018

cloud_providers: aws, node naming updates kubernetes/website#9489

Closed

liggitt reviewed Jul 27, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2018

miracle2k mentioned this pull request Oct 15, 2018

Customize how node names are generated kubernetes/kops#5767

Closed

jorhett mentioned this pull request Dec 19, 2018

Fix cloud_provider hostnames puppetlabs/puppetlabs-kubernetes#223

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 27, 2019

nathanjsweet closed this Feb 27, 2019

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 28, 2019

liggitt mentioned this pull request May 1, 2019

AWS: Allow detected node name to be overridden by cloud config. #76689

Closed

Adding Alternative Node Naming Convention #61878

Adding Alternative Node Naming Convention #61878

Conversation

nathanjsweet commented Mar 29, 2018 • edited Loading

DanyC97 commented Apr 3, 2018

liggitt commented Apr 3, 2018

nathanjsweet commented Apr 20, 2018

nathanjsweet commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Apr 20, 2018 • edited Loading

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

liggitt Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Apr 23, 2018

jsafrane commented Apr 23, 2018

nathanjsweet commented May 1, 2018

liggitt commented May 1, 2018

nathanjsweet commented May 1, 2018

nckturner 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

liggitt commented May 2, 2018

nathanjsweet commented Jun 14, 2018

nathanjsweet commented Jun 21, 2018

Choose a reason for hiding this comment

karlhungus commented Jul 10, 2018

karlhungus commented Jul 10, 2018

karlhungus commented Jul 10, 2018

nathanjsweet commented Jul 11, 2018

Choose a reason for hiding this comment

v1k0d3n commented Sep 6, 2018

liggitt commented Sep 6, 2018

nickperry commented Nov 23, 2018

nathanjsweet commented Nov 27, 2018

liggitt commented Nov 27, 2018

liggitt commented Nov 27, 2018

justinsb commented Nov 27, 2018

sigil66 commented Nov 29, 2018

fejta-bot commented Feb 27, 2019

nathanjsweet commented Feb 27, 2019

andrewsykim commented Feb 27, 2019 • edited Loading

pierluigilenoci commented Feb 28, 2019

cabrinha commented Apr 28, 2019

andrewsykim commented Apr 29, 2019

nathanjsweet commented Mar 29, 2018 •

edited

Loading

liggitt Apr 20, 2018 •

edited

Loading

liggitt Apr 20, 2018 •

edited

Loading

andrewsykim commented Feb 27, 2019 •

edited

Loading