error out empty hostname #64815

dixudx · 2018-06-06T06:57:53Z

What this PR does / why we need it:
For linux, the hostname is read from file /proc/sys/kernel/hostname directly, which can be overwritten with whitespaces.

Should error out such invalid hostnames.

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 kubernetes/kubeadm#835

Special notes for your reviewer:
/cc luxas timothysc

Release note:

nodes: improve handling of erroneous host names

dixudx · 2018-06-06T07:01:46Z

os.Hostname is reading from file /proc/sys/kernel/hostname directly.

root@server-01:~# cat /proc/sys/kernel/hostname
server-01
root@server-01:~# vim /proc/sys/kernel/hostname 
root@server-01:~# echo "   " > /proc/sys/kernel/hostname 
root@server-01:~# cat /proc/sys/kernel/hostname

After overwritten, os.Hostname() will get hostname with whitespaces, which will be trimmed as empty string.

dixudx · 2018-06-06T07:06:43Z

@seh FYI.

seh · 2018-06-06T11:57:27Z

My hostnames don’t have any white space in them, but kubeadm still fails to detect them properly.

echo "|$(cat /proc/sys/kernel/hostname)|"
|ip-10-103-0-147|

Hence, this patch cannot fix kubernetes/kubeadm#835.

rosti · 2018-06-06T12:47:57Z

pkg/util/node/node.go

+	// For linux, the hostname is read from file /proc/sys/kernel/hostname directly
+	hostname = strings.TrimSpace(hostname)
+	if len(hostname) == 0 {
+		glog.Fatalf("Empty hostname is invalid")


I don't like the use of glog.Fatalf here. It calls os.Exit and I perceive this as a bad thing. It does not allow for any error handling or recovery here. A panic or returning an error might be better.

+1 we should Errorf and pass back the error like everything else.

@dixudx - re-ping are you going to fix this ^

timothysc · 2018-06-06T14:14:38Z

pkg/util/node/node.go

+	// For linux, the hostname is read from file /proc/sys/kernel/hostname directly
+	hostname = strings.TrimSpace(hostname)
+	if len(hostname) == 0 {
+		glog.Fatalf("Empty hostname is invalid")


+1 we should Errorf and pass back the error like everything else.

luxas

Just wanna mention I'm very hesitant to changing pkg/util/node for fixing a bug in kubeadm only. pkg/util/node is used in A LOT of other places. Can we solve @seh's issue in an other way?

dixudx · 2018-06-07T02:15:54Z

pkg/util/node is used in A LOT of other places.

Right. So kubeadm may not be the only victim. We should fix this root bug.

fixing a bug in kubeadm only

We could surface the fix in kubeadm. But IMO that's not a real fix.

luxas · 2018-06-08T08:01:16Z

@dixudx at this point, the only thing we can do is to try to surface the fix in kubeadm. We can't go ahead and do a large potentially very disruptive change now. If we have some easy fix locally in the kubeadm code to ease this problem we can do it, otherwise not.

dixudx · 2018-06-08T10:08:52Z

Hence, this patch cannot fix

@seh Have you tried to test locally with this fix.

seh · 2018-06-08T13:25:03Z

No, I haven't tested it. The solution to kubernetes/kubeadm#835 is not have kubeadm fail more frequently; it's to have kubeadm detect the machine's hostname properly. The machines in question have a hostname, those hostnames do not contain spaces, and the /proc/sys/kernel/hostname files do not contain any whitespace. Hence, this fix is not pertinent to the problem I described in kubernetes/kubeadm#835.

I'm not saying that your change here isn't worthy of inclusion, but let's not claim that it fixes kubernetes/kubeadm#835.

luxas · 2018-06-30T09:36:06Z

@dixudx should we close this?

dixudx · 2018-07-02T01:28:51Z

@luxas Shall we make this as a common check, not only for kubeadm.

luxas · 2018-07-02T14:36:25Z

@luxas Shall we make this as a common check, not only for kubeadm.

@dixudx I'm not sure what you mean here

dixudx · 2018-07-03T12:07:00Z

@luxas The hostname checking is necessary, not only for kubeadm. For users that deploying clusters using other scripts, tightening the checking will help avoid potential issues.

dixudx · 2018-07-03T12:10:07Z

@smarterclayton PTAL. Thanks

smarterclayton · 2018-07-03T15:10:27Z

The comment about returning an error is appropriate. Utility code should not invoke glog.Fatalf or panic.

timothysc

/lgtm
/approve

dixudx · 2018-07-27T02:15:17Z

/retest

dixudx · 2018-07-27T10:24:16Z

/retest

out of date.

timothysc · 2018-07-28T02:30:02Z

/test pull-kubernetes-local-e2e-containerized

poke @smarterclayton for approval.

dixudx · 2018-07-28T06:46:37Z

@timothysc @smarterclayton Rebased. PTAL. Thanks.

dixudx · 2018-07-30T02:39:48Z

/retest

timothysc

/lgtm

neolit123 · 2018-07-30T20:42:24Z

@dixudx suggesting release note:

nodes: improve handling of erroneous host names

smarterclayton · 2018-07-31T01:10:16Z

cmd/kubelet/app/server.go

@@ -784,7 +788,11 @@ func InitializeTLS(kf *options.KubeletFlags, kc *kubeletconfiginternal.KubeletCo
 			return nil, err
 		}
 		if !canReadCertAndKey {
-			cert, key, err := certutil.GenerateSelfSignedCertKey(nodeutil.GetHostname(kf.HostnameOverride), nil, nil)
+			nodeName, err := nodeutil.GetHostname(kf.HostnameOverride)


This is the wrong name for the variable.

smarterclayton · 2018-07-31T01:12:53Z

Can this break existing clusters that have empty hostnames? If I have a 1.11 cluster, won't my nodes start? If so, when they upgrade to 1.12 with this PR, wouldn't it break those clusters?

k8s-ci-robot · 2018-08-01T06:58:18Z

New changes are detected. LGTM label has been removed.

dixudx · 2018-08-01T07:20:59Z

Can this break existing clusters that have empty hostnames? If I have a 1.11 cluster, won't my nodes start? If so, when they upgrade to 1.12 with this PR, wouldn't it break those clusters?

@smarterclayton No, it won't break any existing clusters.

The reason is that, for running clusters, the node name won't be empty. Because k8s won't allow node with empty node name when registering, right? /api/v1/nodes/<nodeName> does not make sense for empty node name.

kubernetes/pkg/apis/core/validation/validation.go

Lines 4076 to 4080 in 8e2d37e

    
           // ValidateNode tests if required fields in the node are set. 
        
           func ValidateNode(node *core.Node) field.ErrorList { 
        
           	fldPath := field.NewPath("metadata") 
        
           	allErrs := ValidateObjectMeta(&node.ObjectMeta, false, ValidateNodeName, fldPath) 
        
           	allErrs = append(allErrs, ValidateNodeSpecificAnnotations(node.ObjectMeta.Annotations, fldPath.Child("annotations"))...)

This means for existing clusters with empty hostnames, the node name must be override already. And for those clusters with hostname override, this PR has no effect at all.

IMO, it's quite safe to merge this PR.

smarterclayton · 2018-08-01T14:32:11Z

ok

/approve

dixudx · 2018-08-02T02:43:02Z

ping @timothysc for lgtm. Thanks.

timothysc · 2018-08-03T14:42:49Z

/lgtm .

Thx for your patience @dixudx !

k8s-ci-robot · 2018-08-03T14:43:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, smarterclayton, timothysc

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/OWNERS~~ [smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-08-03T22:56:04Z

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

k8s-github-robot · 2018-08-04T00:13:31Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 6, 2018

k8s-ci-robot requested review from luxas and timothysc June 6, 2018 06:57

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jun 6, 2018

rosti reviewed Jun 6, 2018

View reviewed changes

timothysc suggested changes Jun 6, 2018

View reviewed changes

timothysc added the kind/bug Categorizes issue or PR as related to a bug. label Jun 6, 2018

luxas previously requested changes Jun 6, 2018

View reviewed changes

luxas assigned timothysc and luxas Jun 6, 2018

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

dixudx force-pushed the hostname_empty branch from 3596728 to dcf9d4c Compare July 3, 2018 12:05

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

dixudx force-pushed the hostname_empty branch from dcf9d4c to 102096b Compare July 25, 2018 12:21

timothysc approved these changes Jul 26, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2018

dixudx force-pushed the hostname_empty branch from 0e196ae to 5a50dae Compare July 28, 2018 06:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2018

timothysc approved these changes Jul 30, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2018

smarterclayton reviewed Jul 31, 2018

View reviewed changes

nodes: improve handling of erroneous host names

b3dfe0c

dixudx force-pushed the hostname_empty branch from 5a50dae to b3dfe0c Compare August 1, 2018 06:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2018

timothysc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2018

k8s-github-robot merged commit cb1ef9f into kubernetes:master Aug 4, 2018

dixudx deleted the hostname_empty branch August 4, 2018 03:48

error out empty hostname #64815

error out empty hostname #64815

Conversation

dixudx commented Jun 6, 2018 • edited Loading

dixudx commented Jun 6, 2018

dixudx commented Jun 6, 2018

seh commented Jun 6, 2018 • edited Loading

rosti Jun 6, 2018

Choose a reason for hiding this comment

timothysc Jun 6, 2018

Choose a reason for hiding this comment

timothysc Jul 9, 2018

Choose a reason for hiding this comment

timothysc Jun 6, 2018

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

dixudx commented Jun 7, 2018

luxas commented Jun 8, 2018

dixudx commented Jun 8, 2018

seh commented Jun 8, 2018

luxas commented Jun 30, 2018

dixudx commented Jul 2, 2018

luxas commented Jul 2, 2018

dixudx commented Jul 3, 2018

dixudx commented Jul 3, 2018

smarterclayton commented Jul 3, 2018

timothysc left a comment

Choose a reason for hiding this comment

dixudx commented Jul 27, 2018

dixudx commented Jul 27, 2018

timothysc commented Jul 28, 2018

dixudx commented Jul 28, 2018

dixudx commented Jul 30, 2018

timothysc left a comment

Choose a reason for hiding this comment

neolit123 commented Jul 30, 2018

smarterclayton Jul 31, 2018

Choose a reason for hiding this comment

smarterclayton commented Jul 31, 2018 • edited Loading

k8s-ci-robot commented Aug 1, 2018

dixudx commented Aug 1, 2018

smarterclayton commented Aug 1, 2018

dixudx commented Aug 2, 2018

timothysc commented Aug 3, 2018

k8s-ci-robot commented Aug 3, 2018

k8s-github-robot commented Aug 3, 2018

k8s-github-robot commented Aug 4, 2018

dixudx commented Jun 6, 2018 •

edited

Loading

seh commented Jun 6, 2018 •

edited

Loading

smarterclayton commented Jul 31, 2018 •

edited

Loading