⚠️ Remove DefaulterRemoveUnknownOrOmitableFields mutating webhook option (again) (#12404)

* Remove DefaulterRemoveUnknownOrOmitableFields mutating webhook option (again)

* Address feedback

* Address feedback

* More feedback

Author: fabriziopandini

.golangci.yml

@@ -354,7 +354,7 @@ linters:
       - linters:
           - revive
         # Ignoring stylistic checks for generated code
-        path: .*(api|types)\/.*\/conversion.*\.go$
+        path: .*(api|types|test)\/.*\/conversion.*\.go$
         # By convention, receiver names in a method should reflect their identity.
         text: 'receiver-naming: receiver name'
       - linters:

Makefile

@@ -307,6 +307,14 @@ generate-manifests-core: $(CONTROLLER_GEN) $(KUSTOMIZE) ## Generate manifests e.
 		paths=./util/test/builder/... \
 		crd:crdVersions=v1 \
 		output:crd:dir=./util/test/builder/crd
+	$(CONTROLLER_GEN) \
+		paths=./internal/topology/upgrade/test/t1/... \
+		crd:crdVersions=v1 \
+		output:crd:dir=./internal/topology/upgrade/test/t1/crd
+	$(CONTROLLER_GEN) \
+		paths=./internal/topology/upgrade/test/t2/... \
+		crd:crdVersions=v1 \
+		output:crd:dir=./internal/topology/upgrade/test/t2/crd
 	$(CONTROLLER_GEN) \
 		paths=./controllers/crdmigrator/test/t1/... \
 		crd:crdVersions=v1 \
@@ -398,13 +406,14 @@ generate-go-deepcopy-core: $(CONTROLLER_GEN) ## Generate deepcopy go code for co
 		paths=./api/ipam/... \
 		paths=./api/runtime/... \
 		paths=./api/runtime/hooks/... \
+		paths=./cmd/clusterctl/... \
+		paths=./controllers/crdmigrator/test/... \
 		paths=./internal/api/addons/... \
 		paths=./internal/api/core/... \
 		paths=./internal/runtime/test/... \
-		paths=./cmd/clusterctl/... \
+		paths=./internal/topology/upgrade/test/... \
 		paths=./util/test/builder/... \
-		paths=./util/deprecated/v1beta1/test/builder/... \
-		paths=./controllers/crdmigrator/test/...
+		paths=./util/deprecated/v1beta1/test/builder/...
 
 .PHONY: generate-go-deepcopy-kubeadm-bootstrap
 generate-go-deepcopy-kubeadm-bootstrap: $(CONTROLLER_GEN) ## Generate deepcopy go code for kubeadm bootstrap
@@ -454,13 +463,14 @@ generate-go-conversions-core: ## Run all generate-go-conversions-core-* targets
 
 .PHONY: generate-go-conversions-core-api
 generate-go-conversions-core-api: $(CONVERSION_GEN) ## Generate conversions go code for core api
-	$(MAKE) clean-generated-conversions SRC_DIRS="./api/core/v1beta1,./internal/api/core/v1alpha3,./internal/api/core/v1alpha4"
+	$(MAKE) clean-generated-conversions SRC_DIRS="./api/core/v1beta1,./internal/api/core/v1alpha3,./internal/api/core/v1alpha4,./internal/topology/upgrade/test/t2/v1beta1"
 	$(CONVERSION_GEN) \
 		--output-file=zz_generated.conversion.go \
 		--go-header-file=./hack/boilerplate/boilerplate.generatego.txt \
 		./internal/api/core/v1alpha3 \
 		./internal/api/core/v1alpha4 \
-		./api/core/v1beta1
+		./api/core/v1beta1 \
+		./internal/topology/upgrade/test/t2/v1beta1
 
 .PHONY: generate-go-conversions-addons-api
 generate-go-conversions-addons-api: $(CONVERSION_GEN) ## Generate conversions go code for addons api

bootstrap/kubeadm/internal/webhooks/kubeadmconfig.go

@@ -33,7 +33,7 @@ import (
 func (webhook *KubeadmConfig) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(&bootstrapv1.KubeadmConfig{}).
-		WithDefaulter(webhook, admission.DefaulterRemoveUnknownOrOmitableFields).
+		WithDefaulter(webhook).
 		WithValidator(webhook).
 		Complete()
 }

bootstrap/kubeadm/internal/webhooks/kubeadmconfigtemplate.go

@@ -21,7 +21,7 @@ import (
 	"fmt"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	runtime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -32,7 +32,7 @@ import (
 func (webhook *KubeadmConfigTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(&bootstrapv1.KubeadmConfigTemplate{}).
-		WithDefaulter(webhook, admission.DefaulterRemoveUnknownOrOmitableFields).
+		WithDefaulter(webhook).
 		WithValidator(webhook).
 		Complete()
 }

controllers/crdmigrator/crd_migrator_test.go

@@ -18,7 +18,6 @@ package crdmigrator
 
 import (
 	"context"
-	"fmt"
 	"path"
 	"path/filepath"
 	goruntime "runtime"
@@ -29,7 +28,6 @@ import (
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -38,14 +36,12 @@ import (
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apimachinery/pkg/util/sets"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/client-go/util/retry"
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/config"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -59,6 +55,7 @@ import (
 )
 
 func TestReconcile(t *testing.T) {
+	_, filename, _, _ := goruntime.Caller(0) //nolint:dogsled
 	crdName := "testclusters.test.cluster.x-k8s.io"
 	crdObjectKey := client.ObjectKey{Name: crdName}
 
@@ -170,7 +167,7 @@ func TestReconcile(t *testing.T) {
 			}()
 
 			t.Logf("T1: Install CRDs")
-			g.Expect(installCRDs(ctx, env.GetClient(), "test/t1/crd")).To(Succeed())
+			g.Expect(env.ApplyCRDs(ctx, filepath.Join(path.Dir(filename), "test", "t1", "crd"))).To(Succeed())
 			validateStoredVersions(t, g, crdObjectKey, "v1beta1")
 
 			t.Logf("T1: Start Manager")
@@ -211,7 +208,7 @@ func TestReconcile(t *testing.T) {
 			stopManager(cancelManager, managerStopped)
 
 			t.Logf("T2: Install CRDs")
-			g.Expect(installCRDs(ctx, env.GetClient(), "test/t2/crd")).To(Succeed())
+			g.Expect(env.ApplyCRDs(ctx, filepath.Join(path.Dir(filename), "test", "t2", "crd"))).To(Succeed())
 			validateStoredVersions(t, g, crdObjectKey, "v1beta1", "v1beta2")
 
 			t.Logf("T2: Start Manager")
@@ -245,7 +242,7 @@ func TestReconcile(t *testing.T) {
 			stopManager(cancelManager, managerStopped)
 
 			t.Logf("T3: Install CRDs")
-			g.Expect(installCRDs(ctx, env.GetClient(), "test/t3/crd")).To(Succeed())
+			g.Expect(env.ApplyCRDs(ctx, filepath.Join(path.Dir(filename), "test", "t3", "crd"))).To(Succeed())
 			// Stored versions didn't change.
 			if skipCRDMigrationPhases.Has(StorageVersionMigrationPhase) {
 				validateStoredVersions(t, g, crdObjectKey, "v1beta1", "v1beta2")
@@ -284,7 +281,7 @@ func TestReconcile(t *testing.T) {
 			stopManager(cancelManager, managerStopped)
 
 			t.Logf("T4: Install CRDs")
-			err = installCRDs(ctx, env.GetClient(), "test/t4/crd")
+			err = env.ApplyCRDs(ctx, filepath.Join(path.Dir(filename), "test", "t4", "crd"))
 			if skipCRDMigrationPhases.Has(StorageVersionMigrationPhase) {
 				// If storage version migration was skipped before, we now cannot deploy CRDs that remove v1beta1.
 				g.Expect(err).To(HaveOccurred())
@@ -489,65 +486,6 @@ func stopManager(cancelManager context.CancelFunc, managerStopped chan struct{})
 	<-managerStopped
 }
 
-func installCRDs(ctx context.Context, c client.Client, crdPath string) error {
-	// Get the root of the current file to use in CRD paths.
-	_, filename, _, _ := goruntime.Caller(0) //nolint:dogsled
-
-	installOpts := envtest.CRDInstallOptions{
-		Scheme:       env.GetScheme(),
-		MaxTime:      10 * time.Second,
-		PollInterval: 100 * time.Millisecond,
-		Paths: []string{
-			filepath.Join(path.Dir(filename), "..", "..", "controllers", "crdmigrator", crdPath),
-		},
-		ErrorIfPathMissing: true,
-	}
-
-	// Read the CRD YAMLs into options.CRDs.
-	if err := envtest.ReadCRDFiles(&installOpts); err != nil {
-		return fmt.Errorf("unable to read CRD files: %w", err)
-	}
-
-	// Apply the CRDs.
-	if err := applyCRDs(ctx, c, installOpts.CRDs); err != nil {
-		return fmt.Errorf("unable to create CRD instances: %w", err)
-	}
-
-	// Wait for the CRDs to appear in discovery.
-	if err := envtest.WaitForCRDs(env.GetConfig(), installOpts.CRDs, installOpts); err != nil {
-		return fmt.Errorf("something went wrong waiting for CRDs to appear as API resources: %w", err)
-	}
-
-	return nil
-}
-
-func applyCRDs(ctx context.Context, c client.Client, crds []*apiextensionsv1.CustomResourceDefinition) error {
-	for _, crd := range crds {
-		existingCrd := crd.DeepCopy()
-		err := c.Get(ctx, client.ObjectKey{Name: crd.GetName()}, existingCrd)
-		switch {
-		case apierrors.IsNotFound(err):
-			if err := c.Create(ctx, crd); err != nil {
-				return fmt.Errorf("unable to create CRD %s: %w", crd.GetName(), err)
-			}
-		case err != nil:
-			return fmt.Errorf("unable to get CRD %s to check if it exists: %w", crd.GetName(), err)
-		default:
-			if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-				if err := c.Get(ctx, client.ObjectKey{Name: crd.GetName()}, existingCrd); err != nil {
-					return err
-				}
-				// Note: Intentionally only overwriting spec and thus preserving metadata labels, annotations, etc.
-				existingCrd.Spec = crd.Spec
-				return c.Update(ctx, existingCrd)
-			}); err != nil {
-				return fmt.Errorf("unable to update CRD %s: %w", crd.GetName(), err)
-			}
-		}
-	}
-	return nil
-}
-
 type noopWebhookServer struct {
 	webhook.Server
 }

controlplane/kubeadm/internal/webhooks/kubeadm_control_plane.go

@@ -46,7 +46,7 @@ import (
 func (webhook *KubeadmControlPlane) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(&controlplanev1.KubeadmControlPlane{}).
-		WithDefaulter(webhook, admission.DefaulterRemoveUnknownOrOmitableFields).
+		WithDefaulter(webhook).
 		WithValidator(webhook).
 		Complete()
 }

controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go

@@ -36,7 +36,7 @@ import (
 func (webhook *KubeadmControlPlaneTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(&controlplanev1.KubeadmControlPlaneTemplate{}).
-		WithDefaulter(webhook, admission.DefaulterRemoveUnknownOrOmitableFields).
+		WithDefaulter(webhook).
 		WithValidator(webhook).
 		Complete()
 }

internal/test/envtest/environment.go

@@ -44,6 +44,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
+	"k8s.io/client-go/util/retry"
 	"k8s.io/component-base/logs"
 	logsv1 "k8s.io/component-base/logs/api/v1"
 	"k8s.io/klog/v2"
@@ -618,3 +619,60 @@ func verifyPanicMetrics() error {
 
 	return nil
 }
+
+// ApplyCRDs allows you to add or replace CRDs after test env has been started.
+func (e *Environment) ApplyCRDs(ctx context.Context, crdPath string) error {
+	installOpts := envtest.CRDInstallOptions{
+		Scheme:       e.GetScheme(),
+		MaxTime:      10 * time.Second,
+		PollInterval: 100 * time.Millisecond,
+		Paths: []string{
+			crdPath,
+		},
+		ErrorIfPathMissing: true,
+	}
+
+	// Read the CRD YAMLs into options.CRDs.
+	if err := envtest.ReadCRDFiles(&installOpts); err != nil {
+		return fmt.Errorf("unable to read CRD files: %w", err)
+	}
+
+	// Apply the CRDs.
+	if err := applyCRDs(ctx, e.GetClient(), installOpts.CRDs); err != nil {
+		return fmt.Errorf("unable to create CRD instances: %w", err)
+	}
+
+	// Wait for the CRDs to appear in discovery.
+	if err := envtest.WaitForCRDs(e.GetConfig(), installOpts.CRDs, installOpts); err != nil {
+		return fmt.Errorf("something went wrong waiting for CRDs to appear as API resources: %w", err)
+	}
+
+	return nil
+}
+
+func applyCRDs(ctx context.Context, c client.Client, crds []*apiextensionsv1.CustomResourceDefinition) error {
+	for _, crd := range crds {
+		existingCrd := crd.DeepCopy()
+		err := c.Get(ctx, client.ObjectKey{Name: crd.GetName()}, existingCrd)
+		switch {
+		case apierrors.IsNotFound(err):
+			if err := c.Create(ctx, crd); err != nil {
+				return fmt.Errorf("unable to create CRD %s: %w", crd.GetName(), err)
+			}
+		case err != nil:
+			return fmt.Errorf("unable to get CRD %s to check if it exists: %w", crd.GetName(), err)
+		default:
+			if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+				if err := c.Get(ctx, client.ObjectKey{Name: crd.GetName()}, existingCrd); err != nil {
+					return err
+				}
+				// Note: Intentionally only overwriting spec and thus preserving metadata labels, annotations, etc.
+				existingCrd.Spec = crd.Spec
+				return c.Update(ctx, existingCrd)
+			}); err != nil {
+				return fmt.Errorf("unable to update CRD %s: %w", crd.GetName(), err)
+			}
+		}
+	}
+	return nil
+}