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 1/3] 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) + } + }) + } +} From 488d7650f410e19ce23aa2021ed6c0c2f64a5b75 Mon Sep 17 00:00:00 2001 From: kidddddddddddddddddddddd <1062602710@qq.com> Date: Tue, 14 Mar 2023 09:42:41 +0800 Subject: [PATCH 2/3] update description --- pkg/apis/networking/validation/validation.go | 2 +- pkg/apis/networking/validation/validation_test.go | 2 +- pkg/registry/networking/ingress/strategy.go | 5 +++-- pkg/registry/networking/ingress/strategy_test.go | 11 +++-------- 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index cf1d171db3b..309c9ba61d5 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -278,7 +278,7 @@ func ValidateIngressCreate(ingress *networking.Ingress) field.ErrorList { annotationVal, annotationIsSet := ingress.Annotations[annotationIngressClass] if annotationIsSet && ingress.Spec.IngressClassName != nil && annotationVal != *ingress.Spec.IngressClassName { annotationPath := field.NewPath("annotations").Child(annotationIngressClass) - allErrs = append(allErrs, field.Invalid(annotationPath, annotationVal, "ingressClass annotation and IngressClassName should have the same value")) + allErrs = append(allErrs, field.Invalid(annotationPath, annotationVal, "must match `ingressClassName` when both are specified")) } return allErrs } diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index a702c8513a0..9e1497b3b40 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -1005,7 +1005,7 @@ func TestValidateIngressCreate(t *testing.T) { ingress.Spec.IngressClassName = utilpointer.String("bar") ingress.Annotations = map[string]string{annotationIngressClass: "foo"} }, - expectedErrs: field.ErrorList{field.Invalid(field.NewPath("annotations").Child(annotationIngressClass), "foo", "ingressClass annotation and IngressClassName should have the same value")}, + expectedErrs: field.ErrorList{field.Invalid(field.NewPath("annotations").Child(annotationIngressClass), "foo", "must match `ingressClassName` when both are specified")}, }, "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 b9706397d1a..a23763b9f3d 100644 --- a/pkg/registry/networking/ingress/strategy.go +++ b/pkg/registry/networking/ingress/strategy.go @@ -18,6 +18,7 @@ package ingress import ( "context" + "fmt" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" @@ -101,8 +102,8 @@ func (ingressStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) 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") + if annotationIsSet && ingress.Spec.IngressClassName == nil { + warnings = append(warnings, fmt.Sprintf("annotation %q is deprecated, please use 'spec.ingressClassName' instead", annotationIngressClass)) } return warnings } diff --git a/pkg/registry/networking/ingress/strategy_test.go b/pkg/registry/networking/ingress/strategy_test.go index bb783935c81..082eda85487 100644 --- a/pkg/registry/networking/ingress/strategy_test.go +++ b/pkg/registry/networking/ingress/strategy_test.go @@ -17,6 +17,7 @@ limitations under the License. package ingress import ( + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -175,17 +176,11 @@ func TestWarningsOnCreate(t *testing.T) { 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": { + "ingressClass annotation set, IngressClassName not set": { tweakIngress: func(ingress *networking.Ingress) { ingress.Annotations = map[string]string{annotationIngressClass: "foo"} }, + expectedWarnings: []string{fmt.Sprintf("annotation %q is deprecated, please use 'spec.ingressClassName' instead", annotationIngressClass)}, }, "IngressClassName set": { tweakIngress: func(ingress *networking.Ingress) { From ac626f8abcb3b041494db61ce0bfa5313c900d9d Mon Sep 17 00:00:00 2001 From: kidddddddddddddddddddddd <1062602710@qq.com> Date: Wed, 15 Mar 2023 09:47:10 +0800 Subject: [PATCH 3/3] remove test in strategy_test --- .../networking/ingress/strategy_test.go | 62 ------------------- 1 file changed, 62 deletions(-) diff --git a/pkg/registry/networking/ingress/strategy_test.go b/pkg/registry/networking/ingress/strategy_test.go index 082eda85487..2fd552ae0fe 100644 --- a/pkg/registry/networking/ingress/strategy_test.go +++ b/pkg/registry/networking/ingress/strategy_test.go @@ -17,14 +17,11 @@ limitations under the License. package ingress import ( - "fmt" "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 { @@ -141,62 +138,3 @@ 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 set, IngressClassName not set": { - tweakIngress: func(ingress *networking.Ingress) { - ingress.Annotations = map[string]string{annotationIngressClass: "foo"} - }, - expectedWarnings: []string{fmt.Sprintf("annotation %q is deprecated, please use 'spec.ingressClassName' instead", annotationIngressClass)}, - }, - "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) - } - }) - } -}