Populate Node.Status.Addresses with Hostname #25532

mkulke · 2016-05-12T12:58:19Z

This PR is supposed to address #22063

Currently NodeName has to be a resolvable dns address on the master to allow apiserver -> kubelet communication (exec, log, port-forward operations on a pod). In some situations this is unfortunate (see the discussions on the issue).

The PR aims to do the following:

Populate the Type: Hostname in the Node.Status.Addresses array, the type is already defined, but was not used so far.
Add logic to resolve a Node's Hostname when the apiserver initiates communication with the Kubelet, instead of using the Nodename string as Hostname.

The hostname of the node (as autodetected by the kubelet, specified via --hostname-override, or determined by the cloudprovider) is now recorded as an address of type "Hostname" in the status of the Node API object. The hostname is expected to be resolveable from the apiserver.

This change is

k8s-bot · 2016-05-12T12:58:37Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-12T12:59:14Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-12T13:00:19Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

liggitt · 2016-05-12T13:12:24Z

pkg/kubelet/kubelet.go

@@ -2867,6 +2867,7 @@ func (kl *Kubelet) syncNetworkStatus() {

 // Set addresses for the node.
 func (kl *Kubelet) setNodeAddress(node *api.Node) error {
+	hostnameAddress := api.NodeAddress{Type: api.NodeHostName, Address: kl.GetHostname()}


Is this hostname always resolvable from the master?

No, not necessarily: // GetHostname Returns the hostname as the kubelet sees it. Also, this can be overriden arbitrarily using --hostname-override on the kubelet. Maybe there are cases where this behaviour is undesired, e.g.: cloud-provider sets a resolvable Nodename, but the Node's hostname is actually not resolvable by the master, however it is preferred b/c the Addresses array is populated now. contrived?

That's not contrived at all, imo

I think it would be helpful to map out all the combinations (with and without cloud provider, with and without hostname override, with and without the hostname being resolvable by the master), and show when the hostname override is honored and who is responsible for setting it. I also wonder if some cloud providers need to be updated to set this info if they are known to determine hostnames that won't resolve

@liggitt: Ok, trying to outline it:

$ git grep -A3 "func .* CurrentNodeName" pkg/cloudprovider/providers/aws/aws.go:func (c *AWSCloud) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/aws/aws.go- return c.selfAWSInstance.nodeName, nil pkg/cloudprovider/providers/aws/aws.go-} pkg/cloudprovider/providers/aws/aws.go- -- pkg/cloudprovider/providers/fake/fake.go:func (f *FakeCloud) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/fake/fake.go- return hostname, nil pkg/cloudprovider/providers/fake/fake.go-} pkg/cloudprovider/providers/fake/fake.go- -- pkg/cloudprovider/providers/gce/gce.go:func (gce *GCECloud) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/gce/gce.go- return hostname, nil pkg/cloudprovider/providers/gce/gce.go-} pkg/cloudprovider/providers/gce/gce.go- -- pkg/cloudprovider/providers/mesos/mesos.go:func (c *MesosCloud) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/mesos/mesos.go- return hostname, nil pkg/cloudprovider/providers/mesos/mesos.go-} pkg/cloudprovider/providers/mesos/mesos.go- -- pkg/cloudprovider/providers/openstack/openstack.go:func (i *Instances) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/openstack/openstack.go- return hostname, nil pkg/cloudprovider/providers/openstack/openstack.go-} pkg/cloudprovider/providers/openstack/openstack.go- -- pkg/cloudprovider/providers/ovirt/ovirt.go:func (v *OVirtCloud) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/ovirt/ovirt.go- return hostname, nil pkg/cloudprovider/providers/ovirt/ovirt.go-} pkg/cloudprovider/providers/ovirt/ovirt.go- -- pkg/cloudprovider/providers/rackspace/rackspace.go:func (i *Instances) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/rackspace/rackspace.go- // Beware when changing this, nodename == hostname assumption is crucial to pkg/cloudprovider/providers/rackspace/rackspace.go- // apiserver => kubelet communication. pkg/cloudprovider/providers/rackspace/rackspace.go- return hostname, nil -- pkg/cloudprovider/providers/vsphere/vsphere.go:func (i *Instances) CurrentNodeName(hostname string) (string, error) { pkg/cloudprovider/providers/vsphere/vsphere.go- return i.localInstanceID, nil pkg/cloudprovider/providers/vsphere/vsphere.go-} pkg/cloudprovider/providers/vsphere/vsphere.go-

Most map directly to hostname, VSphere and AWS don't.

VSphere maps to VM name, not necessarily resolvable. Right now the VM name has to be configured to be a master-resolvable DNS name.

https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/vsphere/vsphere.go#L127

AWS gets its hostname from the API, using instance.PrivateDnsName, which is equal to an EC2 instance hostname:

https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L982

The current kubelet code will get a Node's hostname by executing uname -n, unless --hostname-override is set, then it will use this value.

In general I don't think this is wrong (actually it's the intended behavior IMO): It's better to require the Node's hostname to be master-resolvable than have a convention that the Nodename has to be resolvable.

However this could introduce a breaking change for clusters set up under the assumption: Nodename matters, hostname doesn't. To make sure this doesn't happen we can add the Type:Hostname Address to the address array in the affected cloud-providers code already, and add it in the kubelet code only when it's not set by the cloud-provider (actually this sanity-check should be there anyway).

Edit: The latter is not feasible without extending the Kubelet object to pass down information whether the hostname was overridden or not. I'd suggest leaving the address w/ Type:Hostname exclusively in the kubelet's domain, failing when the cloud provider sets it. The only breakage I can think of, right now, are VSphere clusters where the Hostname might not be resolvable, while the Nodename is.

justinsb · 2016-05-12T13:25:55Z

ok to test

justinsb · 2016-05-12T13:28:39Z

Getting e2e on this asap will be helpful, I think. I like the refactoring of the connection info into a class. I didn't know about NodeHostName either - that's much cleaner than looking at the IPs!

k8s-github-robot · 2016-05-13T21:07:58Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

justinsb · 2016-06-09T14:27:57Z

This is a fairly significant change (operationally, not in LOC). I propose we try to land it in 1.4.

@mkulke you'll probably have to ping me once 1.4 opens to remind me..

smarterclayton · 2016-06-09T14:29:11Z

I agree with that - the risk is very high.

k8s-bot · 2016-06-15T00:34:37Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

justinsb · 2016-07-15T13:38:19Z

pkg/registry/pod/strategy.go

 		// If pod has not been assigned a host, return an empty location
 		return nil, nil, nil
 	}
-	nodeScheme, nodePort, nodeTransport, err := connInfo.GetConnectionInfo(ctx, nodeHost)
+	connectionInfo, err := connInfo.GetConnectionInfo(ctx, nodeName)


Aha! So the thing is that here connInfo is a REST object, not a "raw" HTTPKubeletClient object. Because the one will convert the the nodeName and the other will not. I'm going to verify that; but I propose changing REST to implement a different interface, given the behaviours are different (convert node name vs not).

I don't think it needs a different interface... the inputs are a nodeName, the outputs are the connection information. In the absence of this new address info on a node, the fallback will be to continue using the node name as the host.

justinsb · 2016-07-15T13:42:05Z

I think this is great, and exactly the sort of thing that has made turn-up hard. (cc @mikedanese )

It is high risk, but we are at the stage where we can implement it I think.

But I think we should just the same logic as we have in 2d85e4a in the pod provider. Arguably then this is actually a bug fix, in that it is inconsistent that the pods package resolve the kubelet differently from how the nodes packages resolves it. That is arguable, but the concrete consequence would be that we should not have to change the kubelet/node registration at all, and that we should prefer the InternalIP. That is what InternalIP is for, I believe.

If there is a provider for whom this is unworkable, we have plenty of time to discover it, and even add yet-another-address-type if that is needed.

liggitt · 2016-07-15T14:22:44Z

pkg/registry/node/etcd/etcd.go

+}
+
+func (c *REST) GetConnectionInfo(ctx api.Context, nodeName string) (*client.ConnectionInfo, error) {
+	hostname, err := c.getKubeletHost(nodeName)


this is forcing two lookups of the node object from etcd... (once for the host, once for the port)... just fetch once

@liggitt: make sense, i'm retrieving both in a single query now.

liggitt · 2016-10-25T05:26:11Z

This PR records the kubelet hostname (autodetected or manually specified with --hostname-override) in Node status as an address of type Hostname.

Tagging @kubernetes/api-review-team / @kubernetes/kube-api for approval. Previous discussion at #33718 (comment).

This PR and #35497 are needed by 1.5 to preserve the ability to use a DNS name for master->node communications (typically for SSL cert purposes)

liggitt

Include the hostname address in the nodeIP-filtered list, and needs sign-off from @kubernetes/api-review-team / @kubernetes/kube-api , LGTM otherwise

liggitt · 2016-10-25T05:29:19Z

#33388 flake
@k8s-bot gci gke e2e test this

liggitt · 2016-10-31T17:59:47Z

@mkulke, can you make the last couple changes, would like to get this merged early this week ahead of code-freeze

smarterclayton · 2016-10-31T18:05:09Z

This looks reasonable to me - breaking backwards compatibility with node name lookups in existing clusters is deeply dangerous, so this appears to correctly preserve legacy behavior.

mkulke · 2016-11-01T00:26:57Z

@liggitt: ok, i addressed the review comments, squashed and rebased.

k8s-ci-robot · 2016-11-01T00:45:58Z

Jenkins GCE Node e2e failed for commit b7880e7. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-01T00:48:23Z

Jenkins unit/integration failed for commit b7880e7. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

liggitt · 2016-11-01T02:38:15Z

#35898 flake
@k8s-bot node e2e test this
@k8s-bot unit test this

liggitt · 2016-11-01T02:53:06Z

LGTM. Needs a nod from @kubernetes/api-review-team for the use of the Hostname node address type (previous discussion at #33718 (comment))

cc @bgrant0607 @cjcullen

bgrant0607 · 2016-11-01T21:16:21Z

ref #9267

bgrant0607 · 2016-11-01T21:28:50Z

@liggitt I'm fine with this, and happy if it gets us closer to moving away from resolving node resource names. Please document the required level of resolvability of the hostname (e.g., must be resolvable by the apiserver).

However, I haven't thought about what more we could do to determine whether it might break anyone, or all the different ways it might interact with Kubelet configuration.

At minimum, this needs a much more detailed release note than just the title of this PR.

Something to think about for the future: At some point we discussed adding info about which form of the address to use to verify node identity.

bgrant0607 · 2016-11-02T20:39:13Z

cc @caesarxuchao

liggitt · 2016-11-03T14:56:12Z

Updated release-note with description of source and expectations for the address. @mkulke can you open a doc PR as well for this page: http://kubernetes.io/docs/admin/node/#node-addresses

k8s-github-robot · 2016-11-03T16:41:18Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-03T17:22:36Z

Automatic merge from submit-queue

googlebot added the cla: yes label May 12, 2016

mkulke mentioned this pull request May 12, 2016

Apiserver to kubelet communication should not use NodeName as address. #22063

Closed

liggitt reviewed May 12, 2016
View reviewed changes

k8s-github-robot assigned smarterclayton May 12, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 12, 2016

mkulke force-pushed the resolve-nodename-in-kubelet-comm branch from 6874997 to ea05212 Compare May 13, 2016 19:07

k8s-github-robot added needs-ok-to-merge needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 13, 2016

mkulke force-pushed the resolve-nodename-in-kubelet-comm branch from 965c638 to 5e1c104 Compare May 19, 2016 20:38

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

smarterclayton assigned justinsb and unassigned smarterclayton Jun 6, 2016

justinsb added this to the v1.4 milestone Jun 9, 2016

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

mkulke force-pushed the resolve-nodename-in-kubelet-comm branch from 5e1c104 to 50c0a1b Compare July 9, 2016 08:18

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

justinsb reviewed Jul 15, 2016
View reviewed changes

liggitt reviewed Jul 15, 2016
View reviewed changes

justinsb mentioned this pull request Jul 15, 2016

Refactor: separate KubeletClient & ConnectionInfoGetter concepts #29011

Merged

liggitt added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 25, 2016

liggitt requested changes Oct 25, 2016

View reviewed changes

smarterclayton added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Oct 31, 2016

liggitt added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Oct 31, 2016

Populate NodeHostName status.

b7880e7

mkulke force-pushed the resolve-nodename-in-kubelet-comm branch from 0cfba15 to b7880e7 Compare November 1, 2016 00:25

liggitt approved these changes Nov 1, 2016

View reviewed changes

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2016

k8s-github-robot merged commit 43b79d6 into kubernetes:master Nov 3, 2016

mkulke mentioned this pull request Nov 3, 2016

Updated the NodeHostname entry. kubernetes/website#1617

Merged

justinsb mentioned this pull request Nov 4, 2016

Problems with duplicate Node Hostname records #36234

Closed

liggitt mentioned this pull request Nov 4, 2016

Don't add duplicate Hostname address #36231

Merged

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

danwinship mentioned this pull request Jul 11, 2019

Fix cloud reported hostname being overridden if nodeIP set #80003

Merged

Populate Node.Status.Addresses with Hostname #25532

Populate Node.Status.Addresses with Hostname #25532

Conversation

mkulke commented May 12, 2016 • edited by liggitt Loading

k8s-bot commented May 12, 2016

k8s-bot commented May 12, 2016

k8s-bot commented May 12, 2016

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

mkulke May 13, 2016 • edited Loading

Choose a reason for hiding this comment

justinsb commented May 12, 2016

justinsb commented May 12, 2016

k8s-github-robot commented May 13, 2016

justinsb commented Jun 9, 2016

smarterclayton commented Jun 9, 2016 via email

k8s-bot commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jul 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 25, 2016 • edited Loading

liggitt left a comment • edited Loading

Choose a reason for hiding this comment

liggitt commented Oct 25, 2016

liggitt commented Oct 31, 2016

smarterclayton commented Oct 31, 2016

mkulke commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

liggitt commented Nov 1, 2016

liggitt commented Nov 1, 2016

bgrant0607 commented Nov 1, 2016

bgrant0607 commented Nov 1, 2016

bgrant0607 commented Nov 2, 2016

liggitt commented Nov 3, 2016 • edited Loading

k8s-github-robot commented Nov 3, 2016

k8s-github-robot commented Nov 3, 2016

mkulke commented May 12, 2016 •

edited by liggitt

Loading

mkulke May 13, 2016 •

edited

Loading

liggitt commented Oct 25, 2016 •

edited

Loading

liggitt left a comment •

edited

Loading

liggitt commented Nov 3, 2016 •

edited

Loading