Strengthen our resource name validation.#3019
Strengthen our resource name validation.#3019knative-prow-robot merged 1 commit intoknative:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
knative-prow-robot
added
the
size/L
knative-prow-robot
left a comment
knative-prow-robot
left a comment
There was a problem hiding this comment.
@mattmoor: 0 warnings.
Details
In response to this:
This strengthen's our resource name validation to include DNS-1035 checks, which match the underlying Kubernetes resources that we create.
Fixes: #2942
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.
mattmoor
force-pushed
the
dns-1035-names
branch
from
92b5b84 to
eef1610
Compare
| want: (&apis.FieldError{Message: "Invalid resource name: special character . must not be present", Paths: []string{"metadata.name"}}). | ||
| Also(apis.ErrMissingField("spec")), | ||
| want: (&apis.FieldError{ | ||
| Message: "not a DNS 1035 label: [a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name', or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')]", |
There was a problem hiding this comment.
Something like test/util.go:Error1035 = ...?
When k8s changes this string, it'll be a single place to change.
There was a problem hiding this comment.
That actually creates an import cycle, and I'm not sure of a good place to put it that could also be shared by autoscaling (we could share it across pkg/apis/serving/v1alpha1 via a private variable).
| name := meta.GetName() | ||
|
|
||
| if strings.Contains(name, ".") { | ||
| msgs := validation.IsDNS1035Label(name) |
There was a problem hiding this comment.
What is the experience mutating an existing object that satisfied the old regex, but breaks the new one? Does it only error if the change touches the metadata, or will it error if anything on the object changes?
| r: &Revision{}, | ||
| r: &Revision{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "valid", |
There was a problem hiding this comment.
Is this now required for this test to work, or is this just filling in some forgotten boilerplate on the test table?
There was a problem hiding this comment.
Yes, the K8s function rejects empty names, so the validation before was overly weak.
| Message: "not a DNS 1035 label: [must be no more than 63 characters]", | ||
| Paths: []string{"metadata.name"}, | ||
| }, | ||
| }} |
There was a problem hiding this comment.
do you want a test with both error conditions?
Also you can use validation.go:MaxLenError to generate check string.
There was a problem hiding this comment.
Not really :)
I don't think we need 100% path coverage of a (hopefully) well exercised library.
I can add it if you feel strongly (assuming you don't want ~5 versions of that same test).
There was a problem hiding this comment.
FWIW, you can collapse 2 tests into 1 :-)
This strengthen's our resource name validation to include DNS-1035 checks, which match the underlying Kubernetes resources that we create. Fixes: knative#2942
mattmoor
force-pushed
the
dns-1035-names
branch
from
eef1610 to
dd9dbeb
Compare
|
/retest |
vagababov
left a comment
vagababov
left a comment
There was a problem hiding this comment.
/lgtm
/hold for dan to approve.
|
/hold to actually hold |
knative-prow-robot
added
the
do-not-merge/hold
|
/lgtm |
knative-prow-robot
removed
the
do-not-merge/hold
5 participants
This strengthen's our resource name validation to include DNS-1035 checks, which match the underlying Kubernetes resources that we create.
Fixes: #2942