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

Add changes to validate name and generateName for revision template #5110

Merged
merged 6 commits into from
Nov 11, 2019
+523 −50
Merged

Add changes to validate name and generateName for revision template #5110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
37806f3
Add changes to validate name and generateName for revision template
savitaashture Aug 9, 2019
7f83cf8
Fix review comment
savitaashture Aug 9, 2019
ae41cc7
Fix review comment
savitaashture Aug 13, 2019
c4d7ccd
Added testcase for v1alpha1/revision_validation.go
savitaashture Aug 28, 2019
2b4c297
Rebase
savitaashture Sep 18, 2019
8ccdcae
fix conflict issue
savitaashture Oct 24, 2019
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
37 changes: 37 additions & 0 deletions pkg/apis/serving/metadata_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ package serving

import (
"context"
"fmt"
"strconv"
"strings"

"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/validation"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
"knative.dev/serving/pkg/apis/autoscaling"
Expand Down Expand Up @@ -132,3 +134,38 @@ func SetUserInfo(ctx context.Context, oldSpec, newSpec, resource interface{}) {
}
}
}

// ValidateRevisionName validates name and generateName for the revisionTemplate
func ValidateRevisionName(ctx context.Context, name, generateName string) *apis.FieldError {
savitaashture marked this conversation as resolved.
Show resolved Hide resolved
if generateName != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we allow to use generateName before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we provide valid or invalid generateName in service spec it will override in the generateName when it creates revision

But if at all user give invalid generateName something like below

apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: "hello"
spec:
  template:
    metadata:
      generateName: hi@
    spec:
      containers:
       - image: savita3020/helloworld

user will not be notified and service will be created successfully by overriding generateName in revision as shown below

kubectl get rev -n test hello-kphn8 -o yaml

apiVersion: serving.knative.dev/v1alpha1
kind: Revision
metadata:
  annotations:
    serving.knative.dev/creator: kube:admin
    serving.knative.dev/lastPinned: "1565333856"
  creationTimestamp: "2019-08-09T06:57:24Z"
  generateName: hello-
  generation: 1
  labels:
    serving.knative.dev/configuration: hello
    serving.knative.dev/configurationGeneration: "1"
    serving.knative.dev/route: hello
    serving.knative.dev/service: hello
  name: hello-kphn8

To just to make sure i have added condition check.

if msgs := validation.NameIsDNS1035Label(generateName, true); len(msgs) > 0 {
return apis.ErrInvalidValue(
fmt.Sprintf("not a DNS 1035 label prefix: %v", msgs),
"metadata.generateName")
}
}
if name != "" {
if msgs := validation.NameIsDNS1035Label(name, false); len(msgs) > 0 {
return apis.ErrInvalidValue(
fmt.Sprintf("not a DNS 1035 label: %v", msgs),
"metadata.name")
}
om := apis.ParentMeta(ctx)
prefix := om.Name + "-"
if om.Name != "" {
// Even if there is GenerateName, allow the use
// of Name post-creation.
} else if om.GenerateName != "" {
// We disallow bringing your own name when the parent
// resource uses generateName (at creation).
return apis.ErrDisallowedFields("metadata.name")
}

if !strings.HasPrefix(name, prefix) {
return apis.ErrInvalidValue(
fmt.Sprintf("%q must have prefix %q", name, prefix),
"metadata.name")
}
}
return nil
}
72 changes: 62 additions & 10 deletions pkg/apis/serving/metadata_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ func TestValidateClusterVisibilityLabel(t *testing.T) {

}

type WithPod struct {
type withPod struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec corev1.PodSpec `json:"spec,omitempty"`
Expand All @@ -334,12 +334,12 @@ func TestAnnotationCreate(t *testing.T) {
tests := []struct {
name string
user string
this *WithPod
this *withPod
want map[string]string
}{{
name: "create annotation",
user: u1,
this: &WithPod{
this: &withPod{
Spec: getSpec("foo"),
},
want: map[string]string{
Expand All @@ -349,7 +349,7 @@ func TestAnnotationCreate(t *testing.T) {
}, {
name: "create annotation should override user provided annotations",
user: u1,
this: &WithPod{
this: &withPod{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
CreatorAnnotation: u2,
Expand Down Expand Up @@ -384,13 +384,13 @@ func TestAnnotationUpdate(t *testing.T) {
tests := []struct {
name string
user string
prev *WithPod
this *WithPod
prev *withPod
this *withPod
want map[string]string
}{{
name: "update annotation without spec changes",
user: u2,
this: &WithPod{
this: &withPod{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
CreatorAnnotation: u1,
Expand All @@ -399,7 +399,7 @@ func TestAnnotationUpdate(t *testing.T) {
},
Spec: getSpec("foo"),
},
prev: &WithPod{
prev: &withPod{
Spec: getSpec("foo"),
},
want: map[string]string{
Expand All @@ -409,7 +409,7 @@ func TestAnnotationUpdate(t *testing.T) {
}, {
name: "update annotation with spec changes",
user: u2,
this: &WithPod{
this: &withPod{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
CreatorAnnotation: u1,
Expand All @@ -418,7 +418,7 @@ func TestAnnotationUpdate(t *testing.T) {
},
Spec: getSpec("bar"),
},
prev: &WithPod{
prev: &withPod{
Spec: getSpec("foo"),
},
want: map[string]string{
Expand All @@ -441,3 +441,55 @@ func TestAnnotationUpdate(t *testing.T) {
})
}
}

func TestValidateRevisionName(t *testing.T) {
cases := []struct {
name string
revName string
revGenerateName string
objectMeta metav1.ObjectMeta
expectErr error
}{{
name: "invalid revision generateName - dots",
revGenerateName: "foo.bar",
expectErr: apis.ErrInvalidValue("not a DNS 1035 label prefix: [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])?')]",
"metadata.generateName"),
}, {
name: "invalid revision name - dots",
revName: "foo.bar",
expectErr: apis.ErrInvalidValue("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])?')]",
"metadata.name"),
}, {
name: "invalid name (not prefixed)",
objectMeta: metav1.ObjectMeta{
Name: "bar",
},
revName: "foo",
expectErr: apis.ErrInvalidValue(`"foo" must have prefix "bar-"`,
"metadata.name"),
}, {
name: "invalid name (with generateName)",
objectMeta: metav1.ObjectMeta{
GenerateName: "foo-bar-",
},
revName: "foo-bar-foo",
expectErr: apis.ErrDisallowedFields("metadata.name"),
}, {
name: "valid name",
objectMeta: metav1.ObjectMeta{
Name: "valid",
},
revName: "valid-name",
expectErr: (*apis.FieldError)(nil),
}}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
ctx := context.Background()
ctx = apis.WithinParent(ctx, c.objectMeta)
if err := ValidateRevisionName(ctx, c.revName, c.revGenerateName); !reflect.DeepEqual(c.expectErr, err) {
t.Errorf("Expected: '%#v', Got: '%#v'", c.expectErr, err)
}
})
}
}
92 changes: 91 additions & 1 deletion pkg/apis/serving/v1/configuration_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,28 @@ func TestConfigurationValidation(t *testing.T) {
},
},
want: nil,
}, {
name: "invalid name",
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "",
},
Spec: ConfigurationSpec{
Template: RevisionTemplateSpec{
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "hellworld",
}},
},
},
},
},
},
want: &apis.FieldError{
Message: "name or generateName is required",
Paths: []string{"metadata.name"},
},
}, {
name: "valid BYO name (with generateName)",
c: &Configuration{
Expand All @@ -121,7 +143,7 @@ func TestConfigurationValidation(t *testing.T) {
},
},
},
want: nil,
want: apis.ErrDisallowedFields("spec.template.metadata.name"),
}, {
name: "invalid BYO name (not prefixed)",
c: &Configuration{
Expand All @@ -145,6 +167,74 @@ func TestConfigurationValidation(t *testing.T) {
},
want: apis.ErrInvalidValue(`"foo" must have prefix "byo-name-"`,
"spec.template.metadata.name"),
}, {
name: "invalid name for configuration spec",
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "byo-name",
},
Spec: ConfigurationSpec{
Template: RevisionTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Name: "foo.bar",
},
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "hellworld",
}},
},
},
},
},
},
want: apis.ErrInvalidValue("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])?')]",
"spec.template.metadata.name"),
}, {
name: "invalid generate name for configuration spec",
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "byo-name",
},
Spec: ConfigurationSpec{
Template: RevisionTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "foo.bar",
},
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "hellworld",
}},
},
},
},
},
},
want: apis.ErrInvalidValue("not a DNS 1035 label prefix: [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])?')]",
"spec.template.metadata.generateName"),
}, {
name: "valid generate name for configuration spec",
c: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "byo-name",
},
Spec: ConfigurationSpec{
Template: RevisionTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "valid-generatename",
},
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "hellworld",
}},
},
},
},
},
},
want: nil,
}}

// TODO(dangerd): PodSpec validation failures.
Expand Down
17 changes: 1 addition & 16 deletions pkg/apis/serving/v1/revision_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package v1

import (
"context"
"fmt"
"strings"

"knative.dev/pkg/apis"
Expand Down Expand Up @@ -62,21 +61,7 @@ func (rts *RevisionTemplateSpec) Validate(ctx context.Context) *apis.FieldError

// If the RevisionTemplateSpec has a name specified, then check that
// it follows the requirements on the name.
if rts.Name != "" {
var prefix string
if om := apis.ParentMeta(ctx); om.Name == "" {
prefix = om.GenerateName
} else {
prefix = om.Name + "-"
}

if !strings.HasPrefix(rts.Name, prefix) {
errs = errs.Also(apis.ErrInvalidValue(
fmt.Sprintf("%q must have prefix %q", rts.Name, prefix),
"metadata.name"))
}
}

errs = errs.Also(serving.ValidateRevisionName(ctx, rts.Name, rts.GenerateName))
errs = errs.Also(serving.ValidateQueueSidecarAnnotation(rts.Annotations).ViaField("metadata.annotations"))
return errs
}
Expand Down
50 changes: 50 additions & 0 deletions pkg/apis/serving/v1/revision_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,56 @@ func TestRevisionTemplateSpecValidation(t *testing.T) {
},
},
want: nil,
}, {
name: "valid name for revision template",
rts: &RevisionTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
// When user provides empty string in the name field it will behave like no name provided.
Name: "",
},
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "helloworld",
}},
},
},
},
want: nil,
}, {
name: "invalid name for revision template",
rts: &RevisionTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
// We let users bring their own revision name.
Name: "parent-@foo-bar",
},
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "helloworld",
}},
},
},
},
want: apis.ErrInvalidValue("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])?')]",
"metadata.name"),
}, {
name: "invalid generate name for revision template",
rts: &RevisionTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
// We let users bring their own revision generate name.
GenerateName: "parent-@foo-bar",
},
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "helloworld",
}},
},
},
},
want: apis.ErrInvalidValue("not a DNS 1035 label prefix: [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])?')]",
"metadata.generateName"),
}, {
name: "invalid metadata.annotations for scale",
rts: &RevisionTemplateSpec{
Expand Down
Loading