Add changes to validate name and generateName for revision template (#…

…5110) * Add changes to validate name and generateName for revision template * Fix review comment * Fix review comment * Added testcase for v1alpha1/revision_validation.go * Rebase * fix conflict issue
knative · Nov 11, 2019 · 2a1f3b9 · 2a1f3b9
1 parent 55556a8
commit 2a1f3b9
Show file tree

Hide file tree

Showing 9 changed files with 523 additions and 50 deletions.
diff --git a/pkg/apis/serving/metadata_validation.go b/pkg/apis/serving/metadata_validation.go
@@ -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"
@@ -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 {
+	if generateName != "" {
+		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
+}
diff --git a/pkg/apis/serving/metadata_validation_test.go b/pkg/apis/serving/metadata_validation_test.go
@@ -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"`
@@ -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{
@@ -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,
@@ -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,
@@ -399,7 +399,7 @@ func TestAnnotationUpdate(t *testing.T) {
 			},
 			Spec: getSpec("foo"),
 		},
-		prev: &WithPod{
+		prev: &withPod{
 			Spec: getSpec("foo"),
 		},
 		want: map[string]string{
@@ -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,
@@ -418,7 +418,7 @@ func TestAnnotationUpdate(t *testing.T) {
 			},
 			Spec: getSpec("bar"),
 		},
-		prev: &WithPod{
+		prev: &withPod{
 			Spec: getSpec("foo"),
 		},
 		want: map[string]string{
@@ -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)
+			}
+		})
+	}
+}
diff --git a/pkg/apis/serving/v1/configuration_validation_test.go b/pkg/apis/serving/v1/configuration_validation_test.go
@@ -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{
@@ -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{
@@ -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.

diff --git a/pkg/apis/serving/v1/revision_validation.go b/pkg/apis/serving/v1/revision_validation.go
@@ -18,7 +18,6 @@ package v1
 
 import (
 	"context"
-	"fmt"
 	"strings"
 
 	"knative.dev/pkg/apis"
@@ -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
 }

diff --git a/pkg/apis/serving/v1/revision_validation_test.go b/pkg/apis/serving/v1/revision_validation_test.go
@@ -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{