-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add validation enforcing that network subdomain adheres to RFC 1035 #278
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielvegamyhre 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be considered a breaking change.
/hold target for v0.3.0 release? |
@@ -91,6 +91,11 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) { | |||
for _, errMessage := range validation.IsDNS1123Subdomain(js.Spec.Network.Subdomain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is RFC 1035 more emcompassing than DNS 1123? ie does validating DNS1035 mean that we also satisfy DNS 1123?
/retest |
@@ -91,6 +91,11 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) { | |||
for _, errMessage := range validation.IsDNS1123Subdomain(js.Spec.Network.Subdomain) { | |||
allErrs = append(allErrs, fmt.Errorf(errMessage)) | |||
} | |||
|
|||
// Since subdomain name is also used as service name, it must adhere to RFC 1035 as well. | |||
for _, errMessage := range validation.IsDNS1035Label(js.Spec.Network.Subdomain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the subdomain field in the podspec is validated at: https://github.com/kubernetes/kubernetes/blob/6c39a3724d5ff33564d31d5097704f15852c036f/pkg/apis/core/validation/validation.go#L3990
we should do the same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, you are trying to validate for service too, sgtm.
/lgtm
/hold cancel |
Fixes #277
cc @kannon92 what do you think of this? It seems a bit odd that we are validating against 2 different RFC standards for the subdomain name, since it's used as both as the service name and subdomain name, but this may be simpler than finding a new way to derive a service name and validating that?