Skip to content

Commit

Permalink
Fix validation issue of lastModifier for configuration and routes (#6072
Browse files Browse the repository at this point in the history 
)

* Fix validation issue of lastModifier for configuration and routes

* fix review comment and add unit test case
  • Loading branch information
@savitaashture @knative-prow-robot
savitaashture authored and knative-prow-robot committed Nov 27, 2019
1 parent fe2316c commit 2c32e73
Show file tree
Hide file tree
Showing 12 changed files with 360 additions and 12 deletions.
8 changes: 6 additions & 2 deletions pkg/apis/serving/v1/configuration_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,12 @@ func (c *Configuration) Validate(ctx context.Context) (errs *apis.FieldError) {

if apis.IsInUpdate(ctx) {
original := apis.GetBaseline(ctx).(*Configuration)
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, c.Spec, original.GetAnnotations(),
c.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
// Don't validate annotations(creator and lastModifier) when configuration owned by service
// validate only when configuration created independently.
if c.OwnerReferences == nil {
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, c.Spec, original.GetAnnotations(),
c.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
}
err := c.Spec.Template.VerifyNameChange(ctx, original.Spec.Template)
errs = errs.Also(err.ViaField("spec.template"))
}
Expand Down
54 changes: 54 additions & 0 deletions pkg/apis/serving/v1/configuration_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -820,6 +820,60 @@ func TestConfigurationAnnotationUpdate(t *testing.T) {
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}, {
name: "no validation for lastModifier annotation even after update without spec changes as configuration owned by service",
this: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u3,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1",
Kind: serving.GroupName,
}},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
prev: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}, {
name: "no validation for creator annotation even after update as configuration owned by service",
this: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u3,
serving.UpdaterAnnotation: u1,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1",
Kind: serving.GroupName,
}},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
prev: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
8 changes: 6 additions & 2 deletions pkg/apis/serving/v1/route_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,12 @@ func (r *Route) Validate(ctx context.Context) *apis.FieldError {

if apis.IsInUpdate(ctx) {
original := apis.GetBaseline(ctx).(*Route)
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, r.Spec, original.GetAnnotations(),
r.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
// Don't validate annotations(creator and lastModifier) when route owned by service
// validate only when route created independently.
if r.OwnerReferences == nil {
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, r.Spec, original.GetAnnotations(),
r.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
}
}
return errs
}
Expand Down
54 changes: 54 additions & 0 deletions pkg/apis/serving/v1/route_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,60 @@ func TestRouteAnnotationUpdate(t *testing.T) {
Spec: getRouteSpec("old"),
},
want: nil,
}, {
name: "no validation for lastModifier annotation even after update without spec changes as route owned by service",
this: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u3,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1",
Kind: serving.GroupName,
}},
},
Spec: getRouteSpec("old"),
},
prev: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getRouteSpec("old"),
},
want: nil,
}, {
name: "no validation for creator annotation even after update as route owned by service",
this: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u3,
serving.UpdaterAnnotation: u1,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1",
Kind: serving.GroupName,
}},
},
Spec: getRouteSpec("old"),
},
prev: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getRouteSpec("old"),
},
want: nil,
}}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
8 changes: 6 additions & 2 deletions pkg/apis/serving/v1alpha1/configuration_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,12 @@ func (c *Configuration) Validate(ctx context.Context) (errs *apis.FieldError) {

if apis.IsInUpdate(ctx) {
original := apis.GetBaseline(ctx).(*Configuration)
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, c.Spec, original.GetAnnotations(),
c.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
// Don't validate annotations(creator and lastModifier) when configuration owned by service
// validate only when configuration created independently.
if c.GetOwnerReferences() == nil {
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, c.Spec, original.GetAnnotations(),
c.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
}
err := c.Spec.GetTemplate().VerifyNameChange(ctx,
original.Spec.GetTemplate())
errs = errs.Also(err.ViaField("spec.revisionTemplate"))
Expand Down
54 changes: 54 additions & 0 deletions pkg/apis/serving/v1alpha1/configuration_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,60 @@ func TestConfigurationAnnotationUpdate(t *testing.T) {
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}, {
name: "no validation for lastModifier annotation even after update as configuration owned by service",
this: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u3,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1alpha1",
Kind: serving.GroupName,
}},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
prev: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}, {
name: "no validation for creator annotation even after update as configuration owned by service",
this: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u3,
serving.UpdaterAnnotation: u1,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1alpha1",
Kind: serving.GroupName,
}},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
prev: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
8 changes: 6 additions & 2 deletions pkg/apis/serving/v1alpha1/route_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,12 @@ func (r *Route) Validate(ctx context.Context) *apis.FieldError {

if apis.IsInUpdate(ctx) {
original := apis.GetBaseline(ctx).(*Route)
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, r.Spec, original.GetAnnotations(),
r.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
// Don't validate annotations(creator and lastModifier) when route owned by service
// validate only when route created independently.
if r.OwnerReferences == nil {
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, r.Spec, original.GetAnnotations(),
r.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
}
}
return errs
}
Expand Down
54 changes: 54 additions & 0 deletions pkg/apis/serving/v1alpha1/route_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,60 @@ func TestRouteAnnotationUpdate(t *testing.T) {
Spec: getRouteSpec("old"),
},
want: nil,
}, {
name: "no validation for lastModifier annotation even after update without spec changes as route owned by service",
this: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u3,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1alpha1",
Kind: serving.GroupName,
}},
},
Spec: getRouteSpec("old"),
},
prev: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getRouteSpec("old"),
},
want: nil,
}, {
name: "no validation for creator annotation even after update as route owned by service",
this: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u3,
serving.UpdaterAnnotation: u1,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1alpha1",
Kind: serving.GroupName,
}},
},
Spec: getRouteSpec("old"),
},
prev: &Route{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getRouteSpec("old"),
},
want: nil,
}}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
8 changes: 6 additions & 2 deletions pkg/apis/serving/v1beta1/configuration_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,12 @@ func (c *Configuration) Validate(ctx context.Context) (errs *apis.FieldError) {

if apis.IsInUpdate(ctx) {
original := apis.GetBaseline(ctx).(*Configuration)
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, c.Spec, original.GetAnnotations(),
c.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
// Don't validate annotations(creator and lastModifier) when configuration owned by service
// validate only when configuration created independently.
if c.OwnerReferences == nil {
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, c.Spec, original.GetAnnotations(),
c.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
}
err := c.Spec.Template.VerifyNameChange(ctx, original.Spec.Template)
errs = errs.Also(err.ViaField("spec.template"))
}
Expand Down
54 changes: 54 additions & 0 deletions pkg/apis/serving/v1beta1/configuration_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,60 @@ func TestConfigurationAnnotationUpdate(t *testing.T) {
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}, {
name: "no validation for lastModifier annotation even after update as configuration owned by service",
this: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u3,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1beta1",
Kind: serving.GroupName,
}},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
prev: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}, {
name: "no validation for creator annotation even after update as configuration owned by service",
this: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u3,
serving.UpdaterAnnotation: u1,
},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "v1beta1",
Kind: serving.GroupName,
}},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
prev: &Configuration{
ObjectMeta: metav1.ObjectMeta{
Name: "valid",
Annotations: map[string]string{
serving.CreatorAnnotation: u1,
serving.UpdaterAnnotation: u1,
},
},
Spec: getConfigurationSpec("helloworld:foo"),
},
want: nil,
}}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
8 changes: 6 additions & 2 deletions pkg/apis/serving/v1beta1/route_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,12 @@ func (r *Route) Validate(ctx context.Context) *apis.FieldError {

if apis.IsInUpdate(ctx) {
original := apis.GetBaseline(ctx).(*Route)
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, r.Spec, original.GetAnnotations(),
r.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
// Don't validate annotations(creator and lastModifier) when route owned by service
// validate only when route created independently.
if r.OwnerReferences == nil {
errs = errs.Also(apis.ValidateCreatorAndModifier(original.Spec, r.Spec, original.GetAnnotations(),
r.GetAnnotations(), serving.GroupName).ViaField("metadata.annotations"))
}
}
return errs
}
Expand Down
Loading

0 comments on commit 2c32e73

Please sign in to comment.