From 113355a5a23c1b80d7f27748af8c9b463de4957f Mon Sep 17 00:00:00 2001 From: kidddddddddddddddddddddd <1062602710@qq.com> Date: Wed, 1 Feb 2023 14:16:43 +0800 Subject: [PATCH] loosen check rules for ingress creation --- pkg/apis/networking/validation/validation.go | 4 +- .../networking/validation/validation_test.go | 11 ++- pkg/registry/networking/ingress/strategy.go | 14 +++- .../networking/ingress/strategy_test.go | 67 +++++++++++++++++++ 4 files changed, 91 insertions(+), 5 deletions(-) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 5a3046cef15..cf1d171db3b 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -276,9 +276,9 @@ func ValidateIngressCreate(ingress *networking.Ingress) field.ErrorList { } allErrs = append(allErrs, validateIngress(ingress, opts)...) annotationVal, annotationIsSet := ingress.Annotations[annotationIngressClass] - if annotationIsSet && ingress.Spec.IngressClassName != nil { + if annotationIsSet && ingress.Spec.IngressClassName != nil && annotationVal != *ingress.Spec.IngressClassName { annotationPath := field.NewPath("annotations").Child(annotationIngressClass) - allErrs = append(allErrs, field.Invalid(annotationPath, annotationVal, "can not be set when the class field is also set")) + allErrs = append(allErrs, field.Invalid(annotationPath, annotationVal, "ingressClass annotation and IngressClassName should have the same value")) } return allErrs } diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index 7397f3a08c6..a702c8513a0 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -993,12 +993,19 @@ func TestValidateIngressCreate(t *testing.T) { }, expectedErrs: field.ErrorList{}, }, - "class field and annotation set": { + "class field and annotation set with same value": { + tweakIngress: func(ingress *networking.Ingress) { + ingress.Spec.IngressClassName = utilpointer.String("foo") + ingress.Annotations = map[string]string{annotationIngressClass: "foo"} + }, + expectedErrs: field.ErrorList{}, + }, + "class field and annotation set with different value": { tweakIngress: func(ingress *networking.Ingress) { ingress.Spec.IngressClassName = utilpointer.String("bar") ingress.Annotations = map[string]string{annotationIngressClass: "foo"} }, - expectedErrs: field.ErrorList{field.Invalid(field.NewPath("annotations").Child(annotationIngressClass), "foo", "can not be set when the class field is also set")}, + expectedErrs: field.ErrorList{field.Invalid(field.NewPath("annotations").Child(annotationIngressClass), "foo", "ingressClass annotation and IngressClassName should have the same value")}, }, "valid regex path": { tweakIngress: func(ingress *networking.Ingress) { diff --git a/pkg/registry/networking/ingress/strategy.go b/pkg/registry/networking/ingress/strategy.go index fc934d796d1..b9706397d1a 100644 --- a/pkg/registry/networking/ingress/strategy.go +++ b/pkg/registry/networking/ingress/strategy.go @@ -29,6 +29,10 @@ import ( "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) +const ( + annotationIngressClass = "kubernetes.io/ingress.class" +) + // ingressStrategy implements verification logic for Replication Ingress. type ingressStrategy struct { runtime.ObjectTyper @@ -93,7 +97,15 @@ func (ingressStrategy) Validate(ctx context.Context, obj runtime.Object) field.E } // WarningsOnCreate returns warnings for the creation of the given object. -func (ingressStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { return nil } +func (ingressStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { + var warnings []string + ingress := obj.(*networking.Ingress) + _, annotationIsSet := ingress.Annotations[annotationIngressClass] + if annotationIsSet && ingress.Spec.IngressClassName != nil { + warnings = append(warnings, "ingressClass annotation and IngressClassName should not be set at the same time") + } + return warnings +} // Canonicalize normalizes the object after validation. func (ingressStrategy) Canonicalize(obj runtime.Object) { diff --git a/pkg/registry/networking/ingress/strategy_test.go b/pkg/registry/networking/ingress/strategy_test.go index 2fd552ae0fe..bb783935c81 100644 --- a/pkg/registry/networking/ingress/strategy_test.go +++ b/pkg/registry/networking/ingress/strategy_test.go @@ -19,9 +19,11 @@ package ingress import ( "testing" + "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/kubernetes/pkg/apis/networking" + utilpointer "k8s.io/utils/pointer" ) func newIngress() networking.Ingress { @@ -138,3 +140,68 @@ func TestIngressStatusStrategy(t *testing.T) { t.Errorf("Unexpected error %v", errs) } } + +func TestWarningsOnCreate(t *testing.T) { + ctx := genericapirequest.NewDefaultContext() + if !StatusStrategy.NamespaceScoped() { + t.Errorf("Ingress must be namespace scoped") + } + if StatusStrategy.AllowCreateOnUpdate() { + t.Errorf("Ingress should not allow create on update") + } + + serviceBackend := &networking.IngressServiceBackend{ + Name: "defaultbackend", + Port: networking.ServiceBackendPort{ + Number: 80, + }, + } + defaultBackend := networking.IngressBackend{ + Service: serviceBackend, + } + baseIngress := &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test123", + Namespace: "test123", + ResourceVersion: "1234", + }, + Spec: networking.IngressSpec{ + DefaultBackend: &defaultBackend, + Rules: []networking.IngressRule{}, + }, + } + + testCases := map[string]struct { + tweakIngress func(ingress *networking.Ingress) + expectedWarnings []string + }{ + "ingressClass annotation and IngressClassName set": { + tweakIngress: func(ingress *networking.Ingress) { + ingress.Spec.IngressClassName = utilpointer.String("foo") + ingress.Annotations = map[string]string{annotationIngressClass: "foo"} + }, + expectedWarnings: []string{"ingressClass annotation and IngressClassName should not be set at the same time"}, + }, + "ingressClass annotation set": { + tweakIngress: func(ingress *networking.Ingress) { + ingress.Annotations = map[string]string{annotationIngressClass: "foo"} + }, + }, + "IngressClassName set": { + tweakIngress: func(ingress *networking.Ingress) { + ingress.Spec.IngressClassName = utilpointer.String("foo") + }, + }, + } + + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + newIngress := baseIngress.DeepCopy() + testCase.tweakIngress(newIngress) + warnings := Strategy.WarningsOnCreate(ctx, newIngress) + if diff := cmp.Diff(warnings, testCase.expectedWarnings); diff != "" { + t.Errorf("warings does not match (-want,+got):\n%s", diff) + } + }) + } +}