Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use lowercase hostnames for node names #65487

Merged
merged 1 commit into from Jun 29, 2018

Conversation

Projects
None yet
6 participants
@dshcherb
Copy link
Contributor

dshcherb commented Jun 26, 2018

What this PR does / why we need it:

Uppercase hostnames used in charms result in (lowercase) node name lookup errors. This happens when /etc/hostname contains uppercase characters and gethostname or getfqdn return those characters.

Special notes for your reviewer:

Discovered in a field deployment where hostnames are all uppercase.

Release note:

Hostnames are now converted to lowercase before being used for node lookups in the kubernetes-worker charm.
use lowercase hostnames for node names
Usage of names containing uppercase characters returned by calls to
gethostname and getfqdn in requests to apiserver related to nodes
results in 404 errors. Node names are lowercase in K8s itself so charms
should make sure to use lowercase names well as it results in errors.

pkg/util/node/node.go has code to convert hostnames to lowercase in
GetHostname and that function is used to form node names.
@Cynerva

This comment has been minimized.

Copy link
Contributor

Cynerva commented Jun 26, 2018

/ok-to-test

@@ -1058,9 +1058,9 @@ def get_node_name():
elif is_state('endpoint.openstack.ready'):
cloud_provider = 'openstack'
if cloud_provider == 'aws':
return getfqdn()
return getfqdn().lower()

This comment has been minimized.

@Cynerva

Cynerva Jun 27, 2018

Contributor

I'm not sure about this one. It looks to me like the node name is only lower-cased when no cloud provider is set.

Here's where the hostname gets lower-cased:

// GetHostname returns OS's hostname if 'hostnameOverride' is empty; otherwise, return 'hostnameOverride'.
func GetHostname(hostnameOverride string) string {
hostname := hostnameOverride
if hostname == "" {
nodename, err := os.Hostname()
if err != nil {
glog.Fatalf("Couldn't determine hostname: %v", err)
}
hostname = nodename
}
return strings.ToLower(strings.TrimSpace(hostname))
}

Here's what kubelet does with it:

hostname := nodeutil.GetHostname(hostnameOverride)
// Query the cloud provider for our node name, default to hostname
nodeName := types.NodeName(hostname)
cloudIPs := []net.IP{}
cloudNames := []string{}
if kubeDeps.Cloud != nil {
var err error
instances, ok := kubeDeps.Cloud.Instances()
if !ok {
return nil, fmt.Errorf("failed to get instances from cloud provider")
}
nodeName, err = instances.CurrentNodeName(context.TODO(), hostname)

In short: if there's a cloud provider, kubelet will get the node name from that instead of from GetHostname.

For the AWS cloud provider, I don't see it being lower-cased:

func mapInstanceToNodeName(i *ec2.Instance) types.NodeName {
return types.NodeName(aws.StringValue(i.PrivateDnsName))
}

I don't actually know if it's possible for AWS's PrivateDnsName to contain uppercase characters, but I would like to err on the side of caution and assume it might.

@dshcherb can we reduce the scope of this change to only apply when no cloud-provider is set? I would feel safest with something like this instead:

if cloud_provider == '':
    return gethostname().lower()
elif cloud_provider == 'aws':
    return getfqdn()
else:
    return gethostname()

This comment has been minimized.

@dshcherb

dshcherb Jun 27, 2018

Author Contributor

The K8s docs seem to be pretty clear on conventions:

https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
"By convention, the names of Kubernetes resources should be up to maximum length of 253 characters and consist of lower case alphanumeric characters, -, and ., but certain resources have more specific restrictions."
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/identifiers.md - this has a bit vague references to resources specifically but mentions DNS labels and lowercase nevertheless.

I have also followed some code paths (from "node create" to the request validation) - looks like an uppercase node (resource) name would not pass ValidateNodeName check as it requires a node name to be have the following regex:
const dns1035LabelFmt string = "[a-z]([-a-z0-9]*[a-z0-9])?"

https://gist.github.com/dshcherb/1ba9b9cb62f8753de210bc459115c07a

I could reduce the scope but I don't think this is necessary due to the above.

Thoughts?

This comment has been minimized.

@Cynerva

Cynerva Jun 28, 2018

Contributor

Thanks @dshcherb. You're right - it looks like the apiserver does adhere strictly to that convention:

$ cat test-node.yaml 
apiVersion: v1
kind: Node
metadata:
  name: TEST-NODE

$ kubectl create -f test-node.yaml 
The Node "TEST-NODE" is invalid: metadata.name: Invalid value: "TEST-NODE": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

I'm +1 to this as-is then - the charm should always convert it to lower case. Thanks for setting me straight.

@Cynerva

This comment has been minimized.

Copy link
Contributor

Cynerva commented Jun 28, 2018

/lgtm

Good work, thanks 👍

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 28, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cynerva, dshcherb

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:

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 29, 2018

Automatic merge from submit-queue (batch tested with PRs 60150, 65467, 65487, 65595, 65374). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 4859645 into kubernetes:master Jun 29, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation dshcherb authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@marpaia

This comment has been minimized.

Copy link
Member

marpaia commented Jul 2, 2018

/kind bug

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Aug 2, 2018

/sig node
adding sig label for release notes tools to fetch.
please adjust if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.