Remove more defaulting from KubeadmConfig/KubeadmConfigTemplate/KCP/KCPTemplate

Author: sbueringer

api/bootstrap/kubeadm/v1beta2/kubeadmconfig_types.go

@@ -134,28 +134,6 @@ type KubeadmConfigSpec struct {
 	Ignition *IgnitionSpec `json:"ignition,omitempty"`
 }
 
-// Default defaults a KubeadmConfigSpec.
-func (c *KubeadmConfigSpec) Default() {
-	if c.Format == "" {
-		c.Format = CloudConfig
-	}
-	if c.InitConfiguration != nil && c.InitConfiguration.NodeRegistration.ImagePullPolicy == "" {
-		c.InitConfiguration.NodeRegistration.ImagePullPolicy = "IfNotPresent"
-	}
-	if c.JoinConfiguration != nil && c.JoinConfiguration.NodeRegistration.ImagePullPolicy == "" {
-		c.JoinConfiguration.NodeRegistration.ImagePullPolicy = "IfNotPresent"
-	}
-	if c.JoinConfiguration != nil && c.JoinConfiguration.Discovery.File != nil {
-		if kfg := c.JoinConfiguration.Discovery.File.KubeConfig; kfg != nil {
-			if kfg.User.Exec != nil {
-				if kfg.User.Exec.APIVersion == "" {
-					kfg.User.Exec.APIVersion = "client.authentication.k8s.io/v1"
-				}
-			}
-		}
-	}
-}
-
 // Validate ensures the KubeadmConfigSpec is valid.
 func (c *KubeadmConfigSpec) Validate(pathPrefix *field.Path) field.ErrorList {
 	var allErrs field.ErrorList

bootstrap/kubeadm/config/webhook/manifests.yaml

@@ -4,27 +4,6 @@ kind: MutatingWebhookConfiguration
 metadata:
   name: mutating-webhook-configuration
 webhooks:
-- admissionReviewVersions:
-  - v1
-  - v1beta1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /mutate-bootstrap-cluster-x-k8s-io-v1beta2-kubeadmconfig
-  failurePolicy: Fail
-  name: default.kubeadmconfig.bootstrap.cluster.x-k8s.io
-  rules:
-  - apiGroups:
-    - bootstrap.cluster.x-k8s.io
-    apiVersions:
-    - v1beta2
-    operations:
-    - CREATE
-    - UPDATE
-    resources:
-    - kubeadmconfigs
-  sideEffects: None
 - admissionReviewVersions:
   - v1
   - v1beta1

bootstrap/kubeadm/defaulting/defaulting.go

@@ -0,0 +1,44 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// Package defaulting contains defaulting code that is used in CABPK and KCP.
+package defaulting
+
+import bootstrapv1 "sigs.k8s.io/cluster-api/api/bootstrap/kubeadm/v1beta2"
+
+// ApplyPreviousKubeadmConfigDefaults defaults a KubeadmConfig with default values we used in the past.
+// This is done in multiple places (webhooks and KCP controller) to ensure no rollouts are triggered now that
+// we removed this defaulting from webhooks.
+func ApplyPreviousKubeadmConfigDefaults(c *bootstrapv1.KubeadmConfigSpec) {
+	if c.Format == "" {
+		c.Format = bootstrapv1.CloudConfig
+	}
+	if c.InitConfiguration != nil && c.InitConfiguration.NodeRegistration.ImagePullPolicy == "" {
+		c.InitConfiguration.NodeRegistration.ImagePullPolicy = "IfNotPresent"
+	}
+	if c.JoinConfiguration != nil && c.JoinConfiguration.NodeRegistration.ImagePullPolicy == "" {
+		c.JoinConfiguration.NodeRegistration.ImagePullPolicy = "IfNotPresent"
+	}
+	if c.JoinConfiguration != nil && c.JoinConfiguration.Discovery.File != nil {
+		if kfg := c.JoinConfiguration.Discovery.File.KubeConfig; kfg != nil {
+			if kfg.User.Exec != nil {
+				if kfg.User.Exec.APIVersion == "" {
+					kfg.User.Exec.APIVersion = "client.authentication.k8s.io/v1"
+				}
+			}
+		}
+	}
+}

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -1053,10 +1053,14 @@ func (r *KubeadmConfigReconciler) resolveDiscoveryKubeConfig(cfg *bootstrapv1.Fi
 		}
 	}
 	if cfg.KubeConfig.User.Exec != nil {
+		apiVersion := cfg.KubeConfig.User.Exec.APIVersion
+		if apiVersion == "" {
+			apiVersion = "client.authentication.k8s.io/v1"
+		}
 		user.Exec = &clientcmdv1.ExecConfig{
 			Command:            cfg.KubeConfig.User.Exec.Command,
 			Args:               cfg.KubeConfig.User.Exec.Args,
-			APIVersion:         cfg.KubeConfig.User.Exec.APIVersion,
+			APIVersion:         apiVersion,
 			ProvideClusterInfo: ptr.Deref(cfg.KubeConfig.User.Exec.ProvideClusterInfo, false),
 			InteractiveMode:    "Never",
 		}
@@ -1356,6 +1360,11 @@ func (r *KubeadmConfigReconciler) computeClusterConfigurationAndAdditionalData(c
 func (r *KubeadmConfigReconciler) storeBootstrapData(ctx context.Context, scope *Scope, data []byte) error {
 	log := ctrl.LoggerFrom(ctx)
 
+	format := scope.Config.Spec.Format
+	if format == "" {
+		format = bootstrapv1.CloudConfig
+	}
+
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      scope.Config.Name,
@@ -1375,7 +1384,7 @@ func (r *KubeadmConfigReconciler) storeBootstrapData(ctx context.Context, scope
 		},
 		Data: map[string][]byte{
 			"value":  data,
-			"format": []byte(scope.Config.Spec.Format),
+			"format": []byte(format),
 		},
 		Type: clusterv1.ClusterSecretType,
 	}

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go

@@ -827,7 +827,8 @@ func TestBootstrapDataFormat(t *testing.T) {
 			clusterInitialized: true,
 		},
 		{
-			name: "Empty format field",
+			name:   "Empty format field",
+			format: bootstrapv1.CloudConfig,
 		},
 	}
 
@@ -2315,8 +2316,7 @@ func TestKubeadmConfigReconciler_ResolveDiscoveryFileKubeConfig(t *testing.T) {
 								KubeConfig: &bootstrapv1.FileDiscoveryKubeConfig{
 									User: bootstrapv1.KubeConfigUser{
 										Exec: &bootstrapv1.KubeConfigAuthExec{
-											APIVersion: "client.authentication.k8s.io/v1",
-											Command:    "/usr/bin/bootstrap",
+											Command: "/usr/bin/bootstrap",
 											Env: []bootstrapv1.KubeConfigAuthExecEnv{
 												{Name: "ENV_TEST", Value: "value"},
 											},

bootstrap/kubeadm/internal/webhooks/kubeadmconfig.go

@@ -33,31 +33,16 @@ import (
 func (webhook *KubeadmConfig) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(&bootstrapv1.KubeadmConfig{}).
-		WithDefaulter(webhook).
 		WithValidator(webhook).
 		Complete()
 }
 
-// +kubebuilder:webhook:verbs=create;update,path=/mutate-bootstrap-cluster-x-k8s-io-v1beta2-kubeadmconfig,mutating=true,failurePolicy=fail,groups=bootstrap.cluster.x-k8s.io,resources=kubeadmconfigs,versions=v1beta2,name=default.kubeadmconfig.bootstrap.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 // +kubebuilder:webhook:verbs=create;update,path=/validate-bootstrap-cluster-x-k8s-io-v1beta2-kubeadmconfig,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=bootstrap.cluster.x-k8s.io,resources=kubeadmconfigs,versions=v1beta2,name=validation.kubeadmconfig.bootstrap.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 
 // KubeadmConfig implements a validation and defaulting webhook for KubeadmConfig.
 type KubeadmConfig struct{}
 
 var _ webhook.CustomValidator = &KubeadmConfig{}
-var _ webhook.CustomDefaulter = &KubeadmConfig{}
-
-// Default implements webhook.Defaulter so a webhook will be registered for the type.
-func (webhook *KubeadmConfig) Default(_ context.Context, obj runtime.Object) error {
-	c, ok := obj.(*bootstrapv1.KubeadmConfig)
-	if !ok {
-		return apierrors.NewBadRequest(fmt.Sprintf("expected a KubeadmConfig but got a %T", obj))
-	}
-
-	c.Spec.Default()
-
-	return nil
-}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
 func (webhook *KubeadmConfig) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) {

bootstrap/kubeadm/internal/webhooks/kubeadmconfig_test.go

@@ -27,43 +27,10 @@ import (
 
 	bootstrapv1 "sigs.k8s.io/cluster-api/api/bootstrap/kubeadm/v1beta2"
 	"sigs.k8s.io/cluster-api/feature"
-	"sigs.k8s.io/cluster-api/internal/webhooks/util"
 )
 
 var ctx = ctrl.SetupSignalHandler()
 
-func TestKubeadmConfigDefault(t *testing.T) {
-	utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)
-
-	g := NewWithT(t)
-
-	kubeadmConfig := &bootstrapv1.KubeadmConfig{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "foo",
-		},
-		Spec: bootstrapv1.KubeadmConfigSpec{},
-	}
-	updateDefaultingKubeadmConfig := kubeadmConfig.DeepCopy()
-	updateDefaultingKubeadmConfig.Spec.Verbosity = ptr.To[int32](4)
-	webhook := &KubeadmConfig{}
-	t.Run("for KubeadmConfig", util.CustomDefaultValidateTest(ctx, updateDefaultingKubeadmConfig, webhook))
-
-	g.Expect(webhook.Default(ctx, kubeadmConfig)).To(Succeed())
-
-	g.Expect(kubeadmConfig.Spec.Format).To(Equal(bootstrapv1.CloudConfig))
-
-	ignitionKubeadmConfig := &bootstrapv1.KubeadmConfig{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "foo",
-		},
-		Spec: bootstrapv1.KubeadmConfigSpec{
-			Format: bootstrapv1.Ignition,
-		},
-	}
-	g.Expect(webhook.Default(ctx, ignitionKubeadmConfig)).To(Succeed())
-	g.Expect(ignitionKubeadmConfig.Spec.Format).To(Equal(bootstrapv1.Ignition))
-}
-
 func TestKubeadmConfigValidate(t *testing.T) {
 	cases := map[string]struct {
 		in                    *bootstrapv1.KubeadmConfig

bootstrap/kubeadm/internal/webhooks/kubeadmconfigtemplate.go

@@ -27,6 +27,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	bootstrapv1 "sigs.k8s.io/cluster-api/api/bootstrap/kubeadm/v1beta2"
+	"sigs.k8s.io/cluster-api/bootstrap/kubeadm/defaulting"
+	"sigs.k8s.io/cluster-api/util/topology"
 )
 
 func (webhook *KubeadmConfigTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error {
@@ -44,13 +46,20 @@ func (webhook *KubeadmConfigTemplate) SetupWebhookWithManager(mgr ctrl.Manager)
 type KubeadmConfigTemplate struct{}
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type.
-func (webhook *KubeadmConfigTemplate) Default(_ context.Context, obj runtime.Object) error {
+func (webhook *KubeadmConfigTemplate) Default(ctx context.Context, obj runtime.Object) error {
 	c, ok := obj.(*bootstrapv1.KubeadmConfigTemplate)
 	if !ok {
 		return apierrors.NewBadRequest(fmt.Sprintf("expected a KubeadmConfigTemplate but got a %T", obj))
 	}
 
-	c.Spec.Template.Spec.Default()
+	req, err := admission.RequestFromContext(ctx)
+	if err != nil {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected an admission.Request inside context: %v", err))
+	}
+
+	if topology.IsDryRunRequest(req, c) {
+		defaulting.ApplyPreviousKubeadmConfigDefaults(&c.Spec.Template.Spec)
+	}
 
 	return nil
 }

bootstrap/kubeadm/internal/webhooks/kubeadmconfigtemplate_test.go

@@ -21,8 +21,10 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	admissionv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	bootstrapv1 "sigs.k8s.io/cluster-api/api/bootstrap/kubeadm/v1beta2"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
@@ -40,10 +42,23 @@ func TestKubeadmConfigTemplateDefault(t *testing.T) {
 	updateDefaultingKubeadmConfigTemplate := kubeadmConfigTemplate.DeepCopy()
 	updateDefaultingKubeadmConfigTemplate.Spec.Template.Spec.Verbosity = ptr.To[int32](4)
 	webhook := &KubeadmConfigTemplate{}
-	t.Run("for KubeadmConfigTemplate", util.CustomDefaultValidateTest(ctx, updateDefaultingKubeadmConfigTemplate, webhook))
+	t.Run("for KubeadmConfigTemplate", util.CustomDefaultValidateTest(admission.NewContextWithRequest(ctx, admission.Request{}), updateDefaultingKubeadmConfigTemplate, webhook))
 
-	g.Expect(webhook.Default(ctx, kubeadmConfigTemplate)).To(Succeed())
+	// Expect no defaulting.
+	original := kubeadmConfigTemplate.DeepCopy()
+	g.Expect(webhook.Default(admission.NewContextWithRequest(ctx, admission.Request{}), kubeadmConfigTemplate)).To(Succeed())
+	g.Expect(kubeadmConfigTemplate).To(BeComparableTo(original))
 
+	// Expect defaulting for dry-run request.
+	ctx = admission.NewContextWithRequest(ctx, admission.Request{
+		AdmissionRequest: admissionv1.AdmissionRequest{
+			DryRun: ptr.To(true),
+		},
+	})
+	kubeadmConfigTemplate.Annotations = map[string]string{
+		clusterv1.TopologyDryRunAnnotation: "",
+	}
+	g.Expect(webhook.Default(ctx, kubeadmConfigTemplate)).To(Succeed())
 	g.Expect(kubeadmConfigTemplate.Spec.Template.Spec.Format).To(Equal(bootstrapv1.CloudConfig))
 }
 

controllers/external/util.go

@@ -193,7 +193,8 @@ type GenerateTemplateInput struct {
 func GenerateTemplate(in *GenerateTemplateInput) (*unstructured.Unstructured, error) {
 	template, found, err := unstructured.NestedMap(in.Template.Object, "spec", "template")
 	if !found {
-		return nil, errors.Errorf("missing Spec.Template on %v %q", in.Template.GroupVersionKind(), in.Template.GetName())
+		// If spec.template is not set we are intentionally using an empty object here.
+		template = nil
 	} else if err != nil {
 		return nil, errors.Wrapf(err, "failed to retrieve Spec.Template map on %v %q", in.Template.GroupVersionKind(), in.Template.GetName())
 	}