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.

Sign up for GitHub

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

Jump to bottom

Allow leading * in ingress hostname #29204

Merged
merged 1 commit into from
Aug 4, 2016
Merged

Allow leading * in ingress hostname #29204

merged 1 commit into from
Aug 4, 2016

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Jul 19, 2016

fixes #29043

@k8s-bot
Copy link

k8s-bot commented Jul 19, 2016

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.

4 similar comments
@k8s-bot
Copy link

k8s-bot commented Jul 19, 2016

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
Copy link

k8s-bot commented Jul 19, 2016

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
Copy link

k8s-bot commented Jul 19, 2016

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
Copy link

k8s-bot commented Jul 19, 2016

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-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jul 19, 2016
@bprashanth bprashanth assigned bprashanth and unassigned thockin Jul 20, 2016
@k8s-github-robot k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only and removed retest-not-required-docs-only labels Jul 28, 2016
@bprashanth bprashanth mentioned this pull request Jul 29, 2016
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2016
bprashanth
bprashanth reviewed Aug 2, 2016
View reviewed changes
pkg/util/validation/validation.go
var wildcardDNS1123SubdomainRegexp = regexp.MustCompile("^\\*\\." + dns1123SubdomainFmt + "$")

// IsWildcardDNS1123Subdomain tests for a string that conforms to the definition of a
// wildcard subdomain in DNS (RFC 1034) and (RFC 1123).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the rfc specific the exact regex we're using, or is it an approximation of what we translate the rfc to mean?

Copy link
Member Author

@aledbf aledbf Aug 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is clear about the leading * https://tools.ietf.org/html/rfc1034#section-4.3.3

@bprashanth
Copy link
Contributor

Looks good overall, just some nits

@aledbf aledbf force-pushed the ingress-wildcard-hosts branch from b276b8f to 77139c4 Compare August 2, 2016 19:21
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2016
@bprashanth
Copy link
Contributor

LGTM thanks

@bprashanth
Copy link
Contributor

ok to test

@bprashanth bprashanth added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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 Aug 2, 2016
@aledbf aledbf force-pushed the ingress-wildcard-hosts branch from 77139c4 to d5ec799 Compare August 2, 2016 21:01
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2016
bprashanth
bprashanth reviewed Aug 3, 2016
View reviewed changes
pkg/util/validation/validation.go
@@ -139,6 +139,24 @@ func IsDNS1035Label(value string) []string {
return errs
}

// wildcard definition - RFC 1034 section 4.3.3.
const wildcardDNF1123SubdomainFmt = "\\*\\." + dns1123SubdomainFmt
Copy link
Contributor

@bprashanth bprashanth Aug 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify this regex with some examples in the comment?
According to https://tools.ietf.org/html/rfc2818#section-3.1, f*.com matches foo.com but not bar.com, but this regex will fail f*.com right? Also https://tools.ietf.org/html/rfc6125#section-6.4.3 states that only the leftmost label will tolerate a *, which I believe this regex handles. Users on slack noticed that the go library was recently modified to accomodate this (golang/go@e7fae68).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

I also added tests for IsWildcardDNS1123Subdomain

@aledbf aledbf force-pushed the ingress-wildcard-hosts branch from d5ec799 to 52073a5 Compare August 3, 2016 19:39
bprashanth
bprashanth reviewed Aug 3, 2016
View reviewed changes
pkg/util/validation/validation.go
// IsWildcardDNS1123Subdomain tests for a string that conforms to the definition of a
// wildcard subdomain in DNS (RFC 1034 section 4.3.3).
func IsWildcardDNS1123Subdomain(value string) []string {
wildcardDNS1123SubdomainRegexp := regexp.MustCompile("^\\*\\." + dns1123SubdomainFmt + "$")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did i miss your change? this will still reject "f*.com" right? https://play.golang.org/p/rVxw0LEFx1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes
validation_test.go:428: expected errors for "f*.com": [must match the regex \*\.[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* (e.g. '*.example.com')]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was looking at the wrong rfc, yeah f*.com is invalid.

@bprashanth
Copy link
Contributor

Just the nit. Your test failure looks like a flake but repushing will force a retest. Suggesting reporting on an existing issue, if one is found:

  gcloud auth login
!!! [0803 12:51:27] Max attempts reached. Will exit.
Build step 'Execute shell' marked build as failure
[xUnit] [INFO] - Starting to record.

@aledbf aledbf force-pushed the ingress-wildcard-hosts branch from 52073a5 to 317874d Compare August 3, 2016 22:08
@bprashanth
Copy link
Contributor

@k8s-bot test this github issue: #27455

@aledbf aledbf force-pushed the ingress-wildcard-hosts branch from 317874d to 60f4fbf Compare August 4, 2016 00:17
@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 4, 2016

GCE e2e build/test passed for commit 60f4fbf.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0410c33 into kubernetes:master Aug 4, 2016
@aledbf aledbf deleted the ingress-wildcard-hosts branch August 5, 2016 15:50
@knobunc knobunc mentioned this pull request Sep 12, 2016
Closed
@Mario54 Mario54 mentioned this pull request Jul 18, 2017
Closed
@liggitt liggitt mentioned this pull request Jul 30, 2019
Merged
@Miciah Miciah mentioned this pull request Aug 13, 2020
Merged
@bashofmann bashofmann mentioned this pull request Sep 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bprashanth bprashanth

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow leading * in ingress hostname and validate tls.hostname
6 participants
@aledbf @k8s-bot @bprashanth @k8s-github-robot @googlebot @thockin