Don't blame DNS spec on Kubernetes requirement for lower-case DNS labels. #39675

pires · 2017-01-10T15:00:27Z

What this PR does / why we need it: #39635 was rejected because it wasn't clear to the author (me) that lower-case DNS labels are in fact a Kubernetes requirement rather than from the DNS RFC 1035 or/and DNS RFC 1123.

Special notes for your reviewer: @thockin this is a first pass to make the error messages clearer about the fact that DNS specs are not to blame.

k8s-reviewable · 2017-01-10T15:00:39Z

This change is

pires · 2017-01-11T01:36:14Z

@k8s-bot bazel test this

pires · 2017-01-11T01:36:31Z

@k8s-bot unit test this

thockin · 2017-01-11T06:01:08Z

pkg/util/validation/validation.go

@@ -88,7 +88,7 @@ func IsValidLabelValue(value string) []string {
 }

 const dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
-const dns1123LabelErrMsg string = "a valid DNS (RFC 1123) label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character"
+const dns1123LabelErrMsg string = "a DNS label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character"


the problem is that we have at least 2 errors that say "a DNS label". How about "a DNS-1123" and "DNS-1035" ?

Good point. Fixed!

pires · 2017-01-11T12:34:27Z

@k8s-bot bazel test this

pires · 2017-01-11T12:34:32Z

@k8s-bot unit test this

k8s-ci-robot · 2017-01-11T13:07:31Z

Jenkins GCI GKE smoke e2e failed for commit 49c4f3e44b7d62baac786a9ed9af0dd13a00bc50. Full PR test history.

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

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. I understand the commands that are listed here.

k8s-ci-robot · 2017-01-11T13:08:01Z

Jenkins GKE smoke e2e failed for commit 49c4f3e44b7d62baac786a9ed9af0dd13a00bc50. Full PR test history.

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

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. I understand the commands that are listed here.

pires · 2017-01-12T19:44:51Z

@k8s-bot test this

thockin · 2017-01-12T20:04:46Z

Looks like you have more tests to fix up.

pires · 2017-01-12T21:06:40Z

@thockin there must be something wrong with the build as the tests that complain have been fixed in this PR.

pires · 2017-01-12T21:08:50Z

Example:

--- FAIL: TestValidateVolumes (0.00s)
	validation_test.go:1904: [6: "name not a DNS label"] expected error detail "a DNS-1123 label must consist of", got "a valid DNS (RFC 1123) label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name',  or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')"

$ grep -e "(RFC 1123)" ./pkg/util/validation/*.go
./pkg/util/validation/validation.go:// DNS (RFC 1123).
./pkg/util/validation/validation.go:// subdomain in DNS (RFC 1123).

k8s-ci-robot · 2017-01-12T21:24:46Z

Jenkins Bazel Build failed for commit 49c4f3e44b7d62baac786a9ed9af0dd13a00bc50. Full PR test history.

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

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. I understand the commands that are listed here.

k8s-ci-robot · 2017-01-12T21:29:29Z

Jenkins unit/integration failed for commit 49c4f3e44b7d62baac786a9ed9af0dd13a00bc50. 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.

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. I understand the commands that are listed here.

thockin · 2017-01-13T05:11:05Z

could you be tripped up by staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go ?

pires · 2017-01-13T10:52:04Z

Seems I can't fix it

I0113 02:03:40.675] Verifying hack/make-rules/../../hack/verify-readonly-packages.sh
W0113 02:03:47.628] Found pkg/util/validation/.readonly, but files changed compared to "master" branch.
I0113 02:03:47.729] FAILED   hack/make-rules/../../hack/verify-readonly-packages.sh	7s

pires · 2017-01-13T11:10:49Z

@deads2k you added /pkg/util/validation/.readonly. That means this PR is invalid?

deads2k · 2017-01-13T13:16:08Z

@deads2k you added /pkg/util/validation/.readonly. That means this PR is invalid?

The authoritative copy of pkg/util/validation is now under https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/apimachinery, so the change should be made there. We'll be removing the original packages as soon as we finish updating our godeps that vendor old packages.

A brief explanation. As we started splitting apart repos, they all wanted incompatible copies of pkg/runtime and other apimachinery packages that multiple layered interfaces. That precluded compatibility between the new packages (even client-go and kubernetes couldn't co-exist), so we factored out the apimachinery bits into a new repo that lives (for now) in staging and is synced externally.

pires · 2017-01-13T13:23:02Z

@deads2k so this needs to be put on hold until pkg/util/validation is removed?

deads2k · 2017-01-13T13:28:51Z

@deads2k so this needs to be put on hold until pkg/util/validation is removed?

Just remove your changes to pkg/util/validation (they aren't being use anywhere right now anyway), and this is good to merge.

deads2k · 2017-01-13T13:29:14Z

Leave the change to staging/src/k8s.io/apimachinery/pkg/util/validation

…els.

pires · 2017-01-13T13:41:50Z

Thank you @deads2k. PTAL

pires · 2017-01-13T13:54:39Z

@k8s-bot verify test this

deads2k · 2017-01-13T13:57:43Z

@k8s-bot verify test this

@pires systemic I'm afraid. go4.org is down I think.

k8s-ci-robot · 2017-01-13T14:04:49Z

Jenkins verification failed for commit 3856d91. Full PR test history.

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

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. I understand the commands that are listed here.

pires · 2017-01-14T08:47:55Z

@k8s-bot verify test this

k8s-github-robot · 2017-01-14T09:26:46Z

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

k8s-github-robot · 2017-01-14T10:07:30Z

Automatic merge from submit-queue

Otherwise pod spec is invalid and scans for long images will not run https://bugzilla.redhat.com/show_bug.cgi?id=1751267 Expanded comments to clarify lowercase requirement is by Kubernetes not the DNS RFC (kubernetes/kubernetes#39675). The RFC also says: > Host software MUST handle host names of up to 63 characters and > SHOULD handle host names of up to 255 characters so a bit ambiguous what's "valid DNS label" means but anyway Kubernetes chose 63 (some history on kubernetes/kubernetes#4825)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1751267 Setting "hostname" was an enhancement in ManageIQ#248 to improve image names but it introduced a regression: Pod spec could be invalid for long image names, causing scanning to not run at all. (Possibly also for weird characters in name, not sure if realistic but made code more robust to that too.) Expanded comments to clarify lowercase requirement is by Kubernetes not the DNS RFC (kubernetes/kubernetes#39675). The RFC also says: > Host software MUST handle host names of up to 63 characters and > SHOULD handle host names of up to 255 characters so a bit ambiguous what's "valid DNS label" means but anyway Kubernetes chose 63 (some history on kubernetes/kubernetes#4825)

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

k8s-github-robot assigned dchen1107 Jan 10, 2017

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jan 10, 2017

thockin reviewed Jan 11, 2017

View reviewed changes

pires force-pushed the dns_case_insensitive branch from 9d846db to 49c4f3e Compare January 11, 2017 12:29

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 11, 2017

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2017

thockin assigned thockin and unassigned dchen1107 Jan 12, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jan 12, 2017

luxas added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Jan 12, 2017

pires force-pushed the dns_case_insensitive branch from 49c4f3e to c897ed2 Compare January 13, 2017 09:39

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2017

Don't blame DNS spec on Kubernetes requirement for lower-case DNS lab…

3856d91

…els.

pires force-pushed the dns_case_insensitive branch from c897ed2 to 3856d91 Compare January 13, 2017 13:41

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2017

k8s-github-robot merged commit 616038d into kubernetes:master Jan 14, 2017

pires deleted the dns_case_insensitive branch January 14, 2017 10:27

cben mentioned this pull request Nov 17, 2019

Fix scanning pod fake "hostname" for long image names ManageIQ/manageiq-providers-kubernetes#351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't blame DNS spec on Kubernetes requirement for lower-case DNS labels. #39675

Don't blame DNS spec on Kubernetes requirement for lower-case DNS labels. #39675

pires commented Jan 10, 2017

k8s-reviewable commented Jan 10, 2017

pires commented Jan 11, 2017

pires commented Jan 11, 2017

thockin Jan 11, 2017

pires Jan 11, 2017 •

edited

pires commented Jan 11, 2017

pires commented Jan 11, 2017

k8s-ci-robot commented Jan 11, 2017

k8s-ci-robot commented Jan 11, 2017

pires commented Jan 12, 2017

thockin commented Jan 12, 2017

pires commented Jan 12, 2017

pires commented Jan 12, 2017

k8s-ci-robot commented Jan 12, 2017

k8s-ci-robot commented Jan 12, 2017

thockin commented Jan 13, 2017

pires commented Jan 13, 2017 •

edited

pires commented Jan 13, 2017

deads2k commented Jan 13, 2017

pires commented Jan 13, 2017

deads2k commented Jan 13, 2017

deads2k commented Jan 13, 2017

pires commented Jan 13, 2017

pires commented Jan 13, 2017

deads2k commented Jan 13, 2017

k8s-ci-robot commented Jan 13, 2017

pires commented Jan 14, 2017

k8s-github-robot commented Jan 14, 2017

k8s-github-robot commented Jan 14, 2017

Don't blame DNS spec on Kubernetes requirement for lower-case DNS labels. #39675

Don't blame DNS spec on Kubernetes requirement for lower-case DNS labels. #39675

Conversation

pires commented Jan 10, 2017

k8s-reviewable commented Jan 10, 2017

pires commented Jan 11, 2017

pires commented Jan 11, 2017

thockin Jan 11, 2017

Choose a reason for hiding this comment

pires Jan 11, 2017 • edited

Choose a reason for hiding this comment

pires commented Jan 11, 2017

pires commented Jan 11, 2017

k8s-ci-robot commented Jan 11, 2017

k8s-ci-robot commented Jan 11, 2017

pires commented Jan 12, 2017

thockin commented Jan 12, 2017

pires commented Jan 12, 2017

pires commented Jan 12, 2017

k8s-ci-robot commented Jan 12, 2017

k8s-ci-robot commented Jan 12, 2017

thockin commented Jan 13, 2017

pires commented Jan 13, 2017 • edited

pires commented Jan 13, 2017

deads2k commented Jan 13, 2017

pires commented Jan 13, 2017

deads2k commented Jan 13, 2017

deads2k commented Jan 13, 2017

pires commented Jan 13, 2017

pires commented Jan 13, 2017

deads2k commented Jan 13, 2017

k8s-ci-robot commented Jan 13, 2017

pires commented Jan 14, 2017

k8s-github-robot commented Jan 14, 2017

k8s-github-robot commented Jan 14, 2017

pires Jan 11, 2017 •

edited

pires commented Jan 13, 2017 •

edited