Skip to content

Strengthen our resource name validation. #3019

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

Merged
knative-prow-robot merged 1 commit into from
Jan 29, 2019
Merged

Strengthen our resource name validation. #3019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pkg/apis/autoscaling/v1alpha1/pa_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ func TestPodAutoscalerValidation(t *testing.T) {
name: "valid",
r: &PodAutoscaler{
ObjectMeta: v1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
"minScale": "2",
},
Expand All @@ -186,6 +187,7 @@ func TestPodAutoscalerValidation(t *testing.T) {
name: "bad scale bounds",
r: &PodAutoscaler{
ObjectMeta: v1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
autoscaling.MinScaleAnnotationKey: "FOO",
},
Expand All @@ -206,11 +208,18 @@ func TestPodAutoscalerValidation(t *testing.T) {
}).ViaField("annotations").ViaField("metadata"),
}, {
name: "empty spec",
r: &PodAutoscaler{},
r: &PodAutoscaler{
ObjectMeta: v1.ObjectMeta{
Name: "valid",
},
},
want: apis.ErrMissingField("spec"),
}, {
name: "nested spec error",
r: &PodAutoscaler{
ObjectMeta: v1.ObjectMeta{
Name: "valid",
},
Spec: PodAutoscalerSpec{
ConcurrencyModel: "BadValue",
ServiceName: "foo",
Expand Down
26 changes: 20 additions & 6 deletions pkg/apis/serving/v1alpha1/configuration_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ func TestConfigurationValidation(t *testing.T) {
}{{
name: "valid",
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
Spec: ConfigurationSpec{
RevisionTemplate: RevisionTemplateSpec{
Spec: RevisionSpec{
Expand All @@ -172,6 +175,9 @@ func TestConfigurationValidation(t *testing.T) {
}, {
name: "propagate revision failures",
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
Spec: ConfigurationSpec{
RevisionTemplate: RevisionTemplateSpec{
Spec: RevisionSpec{
Expand All @@ -186,7 +192,11 @@ func TestConfigurationValidation(t *testing.T) {
want: apis.ErrDisallowedFields("spec.revisionTemplate.spec.container.name"),
}, {
name: "empty spec",
c: &Configuration{},
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
},
want: apis.ErrMissingField("spec"),
}, {
name: "invalid name - dots",
Expand All @@ -195,17 +205,21 @@ func TestConfigurationValidation(t *testing.T) {
Name: "do.not.use.dots",
},
},
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])?')]",
Copy link
Copy Markdown
Contributor

@vagababov vagababov Jan 29, 2019

Choose a reason for hiding this comment

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

Something like test/util.go:Error1035 = ...?
When k8s changes this string, it'll be a single place to change.

Copy link
Copy Markdown
Contributor

@vagababov vagababov Jan 29, 2019

Choose a reason for hiding this comment

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

Same for the length.

Copy link
Copy Markdown
Member Author

@mattmoor mattmoor Jan 29, 2019

Choose a reason for hiding this comment

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

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).

Paths: []string{"metadata.name"},
}).Also(apis.ErrMissingField("spec")),
}, {
name: "invalid name - too long",
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: strings.Repeat("a", 65),
Name: strings.Repeat("a", 64),
},
},
want: (&apis.FieldError{Message: "Invalid resource name: length must be no more than 63 characters", Paths: []string{"metadata.name"}}).
Also(apis.ErrMissingField("spec")),
want: (&apis.FieldError{
Message: "not a DNS 1035 label: [must be no more than 63 characters]",
Paths: []string{"metadata.name"},
}).Also(apis.ErrMissingField("spec")),
}}

for _, test := range tests {
Expand Down
14 changes: 4 additions & 10 deletions pkg/apis/serving/v1alpha1/metadata_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,22 @@ package v1alpha1
import (
"fmt"
"strconv"
"strings"

"github.com/knative/pkg/apis"
"github.com/knative/serving/pkg/apis/autoscaling"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation"
)

// ValidateObjectMetadata validates that `metadata` stanza of the
// resources is correct.
func ValidateObjectMetadata(meta metav1.Object) *apis.FieldError {
name := meta.GetName()

if strings.Contains(name, ".") {
msgs := validation.IsDNS1035Label(name)
Copy link
Copy Markdown

@dgerd dgerd Jan 29, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

@mattmoor mattmoor Jan 29, 2019

Choose a reason for hiding this comment

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

you cannot change name

if len(msgs) > 0 {
return &apis.FieldError{
Message: "Invalid resource name: special character . must not be present",
Paths: []string{"name"},
}
}

if len(name) > 63 {
return &apis.FieldError{
Message: "Invalid resource name: length must be no more than 63 characters",
Message: fmt.Sprintf("not a DNS 1035 label: %v", msgs),
Paths: []string{"name"},
}
}
Expand Down
35 changes: 17 additions & 18 deletions pkg/apis/serving/v1alpha1/revision_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,9 @@ func TestRevisionValidation(t *testing.T) {
}{{
name: "valid",
r: &Revision{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
Spec: RevisionSpec{
Container: corev1.Container{
Image: "helloworld",
Expand All @@ -638,11 +641,18 @@ func TestRevisionValidation(t *testing.T) {
want: nil,
}, {
name: "empty spec",
r: &Revision{},
r: &Revision{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Copy link
Copy Markdown

@dgerd dgerd Jan 29, 2019

Choose a reason for hiding this comment

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

Is this now required for this test to work, or is this just filling in some forgotten boilerplate on the test table?

Copy link
Copy Markdown
Member Author

@mattmoor mattmoor Jan 29, 2019

Choose a reason for hiding this comment

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

Yes, the K8s function rejects empty names, so the validation before was overly weak.

},
},
want: apis.ErrMissingField("spec"),
}, {
name: "nested spec error",
r: &Revision{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
Spec: RevisionSpec{
Container: corev1.Container{
Name: "kevin",
Expand All @@ -653,10 +663,10 @@ func TestRevisionValidation(t *testing.T) {
},
want: apis.ErrDisallowedFields("spec.container.name"),
}, {
name: "invalid name - dots",
name: "invalid name - dots and too long",
r: &Revision{
ObjectMeta: metav1.ObjectMeta{
Name: "do.not.use.dots",
Name: "a" + strings.Repeat(".", 62) + "a",
},
Spec: RevisionSpec{
Container: corev1.Container{
Expand All @@ -665,7 +675,10 @@ func TestRevisionValidation(t *testing.T) {
ConcurrencyModel: "Multi",
},
},
want: &apis.FieldError{Message: "Invalid resource name: special character . must not be present", Paths: []string{"metadata.name"}},
want: &apis.FieldError{
Message: "not a DNS 1035 label: [must be no more than 63 characters 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])?')]",
Paths: []string{"metadata.name"},
},
}, {
name: "invalid metadata.annotations - scale bounds",
r: &Revision{
Expand All @@ -687,20 +700,6 @@ func TestRevisionValidation(t *testing.T) {
Message: fmt.Sprintf("%s=%v is less than %s=%v", autoscaling.MaxScaleAnnotationKey, 2, autoscaling.MinScaleAnnotationKey, 5),
Paths: []string{autoscaling.MaxScaleAnnotationKey, autoscaling.MinScaleAnnotationKey},
}).ViaField("annotations").ViaField("metadata"),
}, {
name: "invalid name - too long",
r: &Revision{
ObjectMeta: metav1.ObjectMeta{
Name: strings.Repeat("a", 65),
},
Spec: RevisionSpec{
Container: corev1.Container{
Image: "helloworld",
},
ConcurrencyModel: "Multi",
},
},
want: &apis.FieldError{Message: "Invalid resource name: length must be no more than 63 characters", Paths: []string{"metadata.name"}},
}}
Copy link
Copy Markdown
Contributor

@vagababov vagababov Jan 29, 2019

Choose a reason for hiding this comment

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

do you want a test with both error conditions?
Also you can use validation.go:MaxLenError to generate check string.

Copy link
Copy Markdown
Member Author

@mattmoor mattmoor Jan 29, 2019

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor

@vagababov vagababov Jan 29, 2019

Choose a reason for hiding this comment

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

FWIW, you can collapse 2 tests into 1 :-)

Copy link
Copy Markdown
Member Author

@mattmoor mattmoor Jan 29, 2019

Choose a reason for hiding this comment

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

Done for Revision.


for _, test := range tests {
Expand Down
30 changes: 25 additions & 5 deletions pkg/apis/serving/v1alpha1/route_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ func TestRouteValidation(t *testing.T) {
}{{
name: "valid",
r: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
Spec: RouteSpec{
Traffic: []TrafficTarget{{
RevisionName: "foo",
Expand All @@ -45,6 +48,9 @@ func TestRouteValidation(t *testing.T) {
}, {
name: "valid split",
r: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
Spec: RouteSpec{
Traffic: []TrafficTarget{{
Name: "prod",
Expand All @@ -61,6 +67,9 @@ func TestRouteValidation(t *testing.T) {
}, {
name: "invalid traffic entry",
r: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
},
Spec: RouteSpec{
Traffic: []TrafficTarget{{
Name: "foo",
Expand Down Expand Up @@ -88,7 +97,10 @@ func TestRouteValidation(t *testing.T) {
}},
},
},
want: &apis.FieldError{Message: "Invalid resource name: special character . must not be present", Paths: []string{"metadata.name"}},
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])?')]",
Paths: []string{"metadata.name"},
},
}, {
name: "invalid name - dots and spec percent is not 100",
r: &Route{
Expand All @@ -102,13 +114,18 @@ func TestRouteValidation(t *testing.T) {
}},
},
},
want: (&apis.FieldError{Message: "Invalid resource name: special character . must not be present", Paths: []string{"metadata.name"}}).
Also(&apis.FieldError{Message: "Traffic targets sum to 90, want 100", Paths: []string{"spec.traffic"}}),
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])?')]",
Paths: []string{"metadata.name"},
}).Also(&apis.FieldError{
Message: "Traffic targets sum to 90, want 100",
Paths: []string{"spec.traffic"},
}),
}, {
name: "invalid name - too long",
r: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: strings.Repeat("a", 65),
Name: strings.Repeat("a", 64),
},
Spec: RouteSpec{
Traffic: []TrafficTarget{{
Expand All @@ -117,7 +134,10 @@ func TestRouteValidation(t *testing.T) {
}},
},
},
want: &apis.FieldError{Message: "Invalid resource name: length must be no more than 63 characters", Paths: []string{"metadata.name"}},
want: &apis.FieldError{
Message: "not a DNS 1035 label: [must be no more than 63 characters]",
Paths: []string{"metadata.name"},
},
}}

for _, test := range tests {
Expand Down
Loading