From 13b52a18483ff57e7e352f8bae4ef289d2f3f699 Mon Sep 17 00:00:00 2001 From: nayihz Date: Fri, 28 Jul 2023 07:19:37 +0800 Subject: [PATCH] fix crd finalizer validation --- .../pkg/registry/customresource/strategy.go | 33 +++- .../pkg/registry/customresource/validator.go | 24 ++- .../apiserver/field_validation_test.go | 141 +++++++++++++++++- 3 files changed, 188 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go index b534c627343..0fdac7635b9 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" celconfig "k8s.io/apiserver/pkg/apis/cel" "k8s.io/apiserver/pkg/features" @@ -188,8 +189,32 @@ func (a customResourceStrategy) Validate(ctx context.Context, obj runtime.Object } // WarningsOnCreate returns warnings for the creation of the given object. -func (customResourceStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { - return nil +func (a customResourceStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { + return generateWarningsFromObj(obj, nil) +} + +func generateWarningsFromObj(obj, old runtime.Object) []string { + var allWarnings []string + fldPath := field.NewPath("metadata", "finalizers") + newObjAccessor, err := meta.Accessor(obj) + if err != nil { + return allWarnings + } + + newAdded := sets.NewString(newObjAccessor.GetFinalizers()...) + if old != nil { + oldObjAccessor, err := meta.Accessor(old) + if err != nil { + return allWarnings + } + newAdded = newAdded.Difference(sets.NewString(oldObjAccessor.GetFinalizers()...)) + } + + for _, finalizer := range newAdded.List() { + allWarnings = append(allWarnings, validateKubeFinalizerName(finalizer, fldPath)...) + } + + return allWarnings } // Canonicalize normalizes the object after validation. @@ -244,8 +269,8 @@ func (a customResourceStrategy) ValidateUpdate(ctx context.Context, obj, old run } // WarningsOnUpdate returns warnings for the given update. -func (customResourceStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil +func (a customResourceStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { + return generateWarningsFromObj(obj, old) } // GetAttrs returns labels and fields of a given object for filtering purposes. diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go index c39ad15270a..54be1db9752 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go @@ -22,11 +22,15 @@ import ( "math" "strings" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/validation" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" + apimachineryvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" @@ -96,9 +100,23 @@ func (a customResourceValidator) ValidateUpdate(ctx context.Context, obj, old ru return allErrs } -// WarningsOnUpdate returns warnings for the given update. -func (customResourceValidator) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil +var standardFinalizers = sets.NewString( + metav1.FinalizerOrphanDependents, + metav1.FinalizerDeleteDependents, + string(corev1.FinalizerKubernetes), +) + +func validateKubeFinalizerName(stringValue string, fldPath *field.Path) []string { + var allWarnings []string + for _, msg := range apimachineryvalidation.IsQualifiedName(stringValue) { + allWarnings = append(allWarnings, fmt.Sprintf("%s: %q: %s", fldPath.String(), stringValue, msg)) + } + if len(strings.Split(stringValue, "/")) == 1 { + if !standardFinalizers.Has(stringValue) { + allWarnings = append(allWarnings, fmt.Sprintf("%s: %q: prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers", fldPath.String(), stringValue)) + } + } + return allWarnings } func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, old runtime.Object, scale *apiextensions.CustomResourceSubresourceScale) field.ErrorList { diff --git a/test/integration/apiserver/field_validation_test.go b/test/integration/apiserver/field_validation_test.go index c7c32a14519..de9258ce5f1 100644 --- a/test/integration/apiserver/field_validation_test.go +++ b/test/integration/apiserver/field_validation_test.go @@ -425,13 +425,40 @@ spec: } }` + crdApplyFinalizerBody = ` +{ + "apiVersion": "%s", + "kind": "%s", + "metadata": { + "name": "%s", + "finalizers": %s + }, + "spec": { + "knownField1": "val1", + "ports": [{ + "name": "portName", + "containerPort": 8080, + "protocol": "TCP", + "hostPort": 8082 + }], + "embeddedObj": { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "my-cm", + "namespace": "my-ns" + } + } + } +}` + patchYAMLBody = ` apiVersion: %s kind: %s metadata: name: %s finalizers: - - test-finalizer + - test/finalizer spec: cronSpec: "* * * * */5" ports: @@ -569,6 +596,9 @@ func TestFieldValidation(t *testing.T) { t.Run("PatchCRDSchemaless", func(t *testing.T) { testFieldValidationPatchCRDSchemaless(t, rest, schemalessGVK, schemalessGVR) }) t.Run("ApplyCreateCRDSchemaless", func(t *testing.T) { testFieldValidationApplyCreateCRDSchemaless(t, rest, schemalessGVK, schemalessGVR) }) t.Run("ApplyUpdateCRDSchemaless", func(t *testing.T) { testFieldValidationApplyUpdateCRDSchemaless(t, rest, schemalessGVK, schemalessGVR) }) + t.Run("testFinalizerValidationApplyCreateCRD", func(t *testing.T) { + testFinalizerValidationApplyCreateAndUpdateCRD(t, rest, schemalessGVK, schemalessGVR) + }) } // testFieldValidationPost tests POST requests containing unknown fields with @@ -2135,7 +2165,7 @@ kind: %s metadata: name: %s finalizers: - - test-finalizer + - test/finalizer spec: cronSpec: "* * * * */5" ports: @@ -2887,6 +2917,111 @@ func testFieldValidationApplyUpdateCRDSchemaless(t *testing.T, rest rest.Interfa } } +func testFinalizerValidationApplyCreateAndUpdateCRD(t *testing.T, rest rest.Interface, gvk schema.GroupVersionKind, gvr schema.GroupVersionResource) { + var testcases = []struct { + name string + finalizer []string + updatedFinalizer []string + opts metav1.PatchOptions + expectUpdateWarnings []string + expectCreateWarnings []string + }{ + { + name: "create-crd-with-invalid-finalizer", + finalizer: []string{"invalid-finalizer"}, + expectCreateWarnings: []string{ + `metadata.finalizers: "invalid-finalizer": prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers`, + }, + }, + { + name: "create-crd-with-valid-finalizer", + finalizer: []string{"kubernetes.io/valid-finalizer"}, + }, + { + name: "update-crd-with-invalid-finalizer", + finalizer: []string{"invalid-finalizer"}, + updatedFinalizer: []string{"another-invalid-finalizer"}, + expectCreateWarnings: []string{ + `metadata.finalizers: "invalid-finalizer": prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers`, + }, + expectUpdateWarnings: []string{ + `metadata.finalizers: "another-invalid-finalizer": prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers`, + }, + }, + { + name: "update-crd-with-valid-finalizer", + finalizer: []string{"kubernetes.io/valid-finalizer"}, + updatedFinalizer: []string{"kubernetes.io/another-valid-finalizer"}, + }, + { + name: "update-crd-with-valid-finalizer-leaving-an-existing-invalid-finalizer", + finalizer: []string{"invalid-finalizer"}, + updatedFinalizer: []string{"kubernetes.io/another-valid-finalizer"}, + expectCreateWarnings: []string{ + `metadata.finalizers: "invalid-finalizer": prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers`, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + kind := gvk.Kind + apiVersion := gvk.Group + "/" + gvk.Version + + // create the CR as specified by the test case + name := fmt.Sprintf("apply-create-crd-%s", tc.name) + finalizerVal, _ := json.Marshal(tc.finalizer) + applyCreateBody := []byte(fmt.Sprintf(crdApplyFinalizerBody, apiVersion, kind, name, finalizerVal)) + + req := rest.Patch(types.ApplyPatchType). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(name). + Param("fieldManager", "apply_test"). + VersionedParams(&tc.opts, metav1.ParameterCodec) + result := req.Body(applyCreateBody).Do(context.TODO()) + if result.Error() != nil { + t.Fatalf("unexpected error: %v", result.Error()) + } + + if len(result.Warnings()) != len(tc.expectCreateWarnings) { + for _, r := range result.Warnings() { + t.Logf("received warning: %v", r) + } + t.Fatalf("unexpected number of warnings, expected: %d, got: %d", len(tc.expectCreateWarnings), len(result.Warnings())) + } + for i, expectedWarning := range tc.expectCreateWarnings { + if expectedWarning != result.Warnings()[i].Text { + t.Fatalf("expected warning: %s, got warning: %s", expectedWarning, result.Warnings()[i].Text) + } + } + + if len(tc.updatedFinalizer) != 0 { + finalizerVal, _ := json.Marshal(tc.updatedFinalizer) + applyUpdateBody := []byte(fmt.Sprintf(crdApplyFinalizerBody, apiVersion, kind, name, finalizerVal)) + updateReq := rest.Patch(types.ApplyPatchType). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(name). + Param("fieldManager", "apply_test"). + VersionedParams(&tc.opts, metav1.ParameterCodec) + result = updateReq.Body(applyUpdateBody).Do(context.TODO()) + + if result.Error() != nil { + t.Fatalf("unexpected error: %v", result.Error()) + } + + if len(result.Warnings()) != len(tc.expectUpdateWarnings) { + t.Fatalf("unexpected number of warnings, expected: %d, got: %d", len(tc.expectUpdateWarnings), len(result.Warnings())) + } + for i, expectedWarning := range tc.expectUpdateWarnings { + if expectedWarning != result.Warnings()[i].Text { + t.Fatalf("expected warning: %s, got warning: %s", expectedWarning, result.Warnings()[i].Text) + } + } + } + }) + } +} + func setupCRD(t testing.TB, config *rest.Config, apiGroup string, schemaless bool) *apiextensionsv1.CustomResourceDefinition { apiExtensionClient, err := apiextensionsclient.NewForConfig(config) if err != nil { @@ -3687,7 +3822,7 @@ kind: %s metadata: name: %s finalizers: - - test-finalizer + - test/finalizer spec: cronSpec: "* * * * */5" ports: