fix crd finalizer validation

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 {
+func (a customResourceStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string {
-	return nil
+	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 {
+func (a customResourceStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string {
-	return nil
+	return generateWarningsFromObj(obj, old)
 }
 
 // GetAttrs returns labels and fields of a given object for filtering purposes.

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.
+var standardFinalizers = sets.NewString(
-func (customResourceValidator) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string {
+	metav1.FinalizerOrphanDependents,
-	return nil
+	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 {

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: