Merge pull request #6096 from sbueringer/pr-fix-kcp-rollout-main

k8s-ci-robot · web-flow · commit 07a9e532d14e · 2022-02-18T08:34:25.000-08:00
🐛 KCP: don't rollout machines when format is defaulted
diff --git a/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go b/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go
@@ -79,7 +79,6 @@ type KubeadmConfigSpec struct {
 
 	// Format specifies the output format of the bootstrap data
 	// +optional
-	// +kubebuilder:default=cloud-config
 	Format Format `json:"format,omitempty"`
 
 	// Verbosity is the number for the kubeadm log level verbosity.
diff --git a/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go b/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go
@@ -43,6 +43,22 @@ func (c *KubeadmConfig) SetupWebhookWithManager(mgr ctrl.Manager) error {
 		Complete()
 }
 
+// +kubebuilder:webhook:verbs=create;update,path=/mutate-bootstrap-cluster-x-k8s-io-v1beta1-kubeadmconfig,mutating=true,failurePolicy=fail,groups=bootstrap.cluster.x-k8s.io,resources=kubeadmconfigs,versions=v1beta1,name=default.kubeadmconfig.bootstrap.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
+
+var _ webhook.Defaulter = &KubeadmConfig{}
+
+// Default implements webhook.Defaulter so a webhook will be registered for the type.
+func (c *KubeadmConfig) Default() {
+	DefaultKubeadmConfigSpec(&c.Spec)
+}
+
+// DefaultKubeadmConfigSpec defaults a KubeadmConfigSpec.
+func DefaultKubeadmConfigSpec(r *KubeadmConfigSpec) {
+	if r.Format == "" {
+		r.Format = CloudConfig
+	}
+}
+
 // +kubebuilder:webhook:verbs=create;update,path=/validate-bootstrap-cluster-x-k8s-io-v1beta1-kubeadmconfig,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=bootstrap.cluster.x-k8s.io,resources=kubeadmconfigs,versions=v1beta1,name=validation.kubeadmconfig.bootstrap.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 
 var _ webhook.Validator = &KubeadmConfig{}
diff --git a/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook_test.go b/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook_test.go
@@ -25,9 +25,41 @@ import (
 	"k8s.io/utils/pointer"
 
 	"sigs.k8s.io/cluster-api/feature"
+	utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
 )
 
-func TestClusterValidate(t *testing.T) {
+func TestKubeadmConfigDefault(t *testing.T) {
+	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)()
+
+	g := NewWithT(t)
+
+	kubeadmConfig := &KubeadmConfig{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo",
+		},
+		Spec: KubeadmConfigSpec{},
+	}
+	updateDefaultingKubeadmConfig := kubeadmConfig.DeepCopy()
+	updateDefaultingKubeadmConfig.Spec.Verbosity = pointer.Int32Ptr(4)
+	t.Run("for KubeadmConfig", utildefaulting.DefaultValidateTest(updateDefaultingKubeadmConfig))
+
+	kubeadmConfig.Default()
+
+	g.Expect(kubeadmConfig.Spec.Format).To(Equal(CloudConfig))
+
+	ignitionKubeadmConfig := &KubeadmConfig{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo",
+		},
+		Spec: KubeadmConfigSpec{
+			Format: Ignition,
+		},
+	}
+	ignitionKubeadmConfig.Default()
+	g.Expect(ignitionKubeadmConfig.Spec.Format).To(Equal(Ignition))
+}
+
+func TestKubeadmConfigValidate(t *testing.T) {
 	cases := map[string]struct {
 		in                    *KubeadmConfig
 		enableIgnitionFeature bool
diff --git a/bootstrap/kubeadm/api/v1beta1/kubeadmconfigtemplate_webhook.go b/bootstrap/kubeadm/api/v1beta1/kubeadmconfigtemplate_webhook.go
@@ -30,6 +30,15 @@ func (r *KubeadmConfigTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error
 		Complete()
 }
 
+// +kubebuilder:webhook:verbs=create;update,path=/mutate-bootstrap-cluster-x-k8s-io-v1beta1-kubeadmconfigtemplate,mutating=true,failurePolicy=fail,groups=bootstrap.cluster.x-k8s.io,resources=kubeadmconfigtemplates,versions=v1beta1,name=default.kubeadmconfigtemplate.bootstrap.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
+
+var _ webhook.Defaulter = &KubeadmConfigTemplate{}
+
+// Default implements webhook.Defaulter so a webhook will be registered for the type.
+func (r *KubeadmConfigTemplate) Default() {
+	DefaultKubeadmConfigSpec(&r.Spec.Template.Spec)
+}
+
 // +kubebuilder:webhook:verbs=create;update,path=/validate-bootstrap-cluster-x-k8s-io-v1beta1-kubeadmconfigtemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=bootstrap.cluster.x-k8s.io,resources=kubeadmconfigtemplates,versions=v1beta1,name=validation.kubeadmconfigtemplate.bootstrap.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 
 var _ webhook.Validator = &KubeadmConfigTemplate{}
diff --git a/bootstrap/kubeadm/api/v1beta1/kubeadmconfigtemplate_webhook_test.go b/bootstrap/kubeadm/api/v1beta1/kubeadmconfigtemplate_webhook_test.go
@@ -21,10 +21,29 @@ import (
 
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
 
 	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
+	utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
 )
 
+func TestKubeadmConfigTemplateDefault(t *testing.T) {
+	g := NewWithT(t)
+
+	kubeadmConfigTemplate := &bootstrapv1.KubeadmConfigTemplate{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo",
+		},
+	}
+	updateDefaultingKubeadmConfigTemplate := kubeadmConfigTemplate.DeepCopy()
+	updateDefaultingKubeadmConfigTemplate.Spec.Template.Spec.Verbosity = pointer.Int32Ptr(4)
+	t.Run("for KubeadmConfigTemplate", utildefaulting.DefaultValidateTest(updateDefaultingKubeadmConfigTemplate))
+
+	kubeadmConfigTemplate.Default()
+
+	g.Expect(kubeadmConfigTemplate.Spec.Template.Spec.Format).To(Equal(bootstrapv1.CloudConfig))
+}
+
 func TestKubeadmConfigTemplateValidation(t *testing.T) {
 	cases := map[string]struct {
 		in *bootstrapv1.KubeadmConfigTemplate
diff --git a/bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigs.yaml b/bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigs.yaml
@@ -2464,7 +2464,6 @@ spec:
                   type: object
                 type: array
               format:
-                default: cloud-config
                 description: Format specifies the output format of the bootstrap data
                 enum:
                 - cloud-config
diff --git a/bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigtemplates.yaml b/bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigtemplates.yaml
@@ -2478,7 +2478,6 @@ spec:
                           type: object
                         type: array
                       format:
-                        default: cloud-config
                         description: Format specifies the output format of the bootstrap
                           data
                         enum:
diff --git a/bootstrap/kubeadm/config/default/webhookcainjection_patch.yaml b/bootstrap/kubeadm/config/default/webhookcainjection_patch.yaml
@@ -1,5 +1,12 @@
 ---
 apiVersion: admissionregistration.k8s.io/v1
+kind: MutatingWebhookConfiguration
+metadata:
+  name: mutating-webhook-configuration
+  annotations:
+    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
+---
+apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration
 metadata:
   name: validating-webhook-configuration
diff --git a/bootstrap/kubeadm/config/webhook/manifests.yaml b/bootstrap/kubeadm/config/webhook/manifests.yaml
@@ -1,5 +1,54 @@
 ---
 apiVersion: admissionregistration.k8s.io/v1
+kind: MutatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: mutating-webhook-configuration
+webhooks:
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /mutate-bootstrap-cluster-x-k8s-io-v1beta1-kubeadmconfig
+  failurePolicy: Fail
+  name: default.kubeadmconfig.bootstrap.cluster.x-k8s.io
+  rules:
+  - apiGroups:
+    - bootstrap.cluster.x-k8s.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - kubeadmconfigs
+  sideEffects: None
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /mutate-bootstrap-cluster-x-k8s-io-v1beta1-kubeadmconfigtemplate
+  failurePolicy: Fail
+  name: default.kubeadmconfigtemplate.bootstrap.cluster.x-k8s.io
+  rules:
+  - apiGroups:
+    - bootstrap.cluster.x-k8s.io
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - kubeadmconfigtemplates
+  sideEffects: None
+---
+apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration
 metadata:
   creationTimestamp: null
diff --git a/controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook.go b/controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook.go
@@ -68,6 +68,8 @@ func defaultKubeadmControlPlaneSpec(s *KubeadmControlPlaneSpec, namespace string
 		s.Version = "v" + s.Version
 	}
 
+	bootstrapv1.DefaultKubeadmConfigSpec(&s.KubeadmConfigSpec)
+
 	s.RolloutStrategy = defaultRolloutStrategy(s.RolloutStrategy)
 }
 
diff --git a/controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook_test.go b/controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook_test.go
@@ -62,6 +62,7 @@ func TestKubeadmControlPlaneDefault(t *testing.T) {
 	t.Run("for KubeadmControlPlane", utildefaulting.DefaultValidateTest(updateDefaultingValidationKCP))
 	kcp.Default()
 
+	g.Expect(kcp.Spec.KubeadmConfigSpec.Format).To(Equal(bootstrapv1.CloudConfig))
 	g.Expect(kcp.Spec.MachineTemplate.InfrastructureRef.Namespace).To(Equal(kcp.Namespace))
 	g.Expect(kcp.Spec.Version).To(Equal("v1.18.3"))
 	g.Expect(kcp.Spec.RolloutStrategy.Type).To(Equal(RollingUpdateStrategyType))
diff --git a/controlplane/kubeadm/api/v1beta1/kubeadmcontrolplanetemplate_webhook.go b/controlplane/kubeadm/api/v1beta1/kubeadmcontrolplanetemplate_webhook.go
@@ -26,6 +26,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 
+	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
 	"sigs.k8s.io/cluster-api/feature"
 )
 
@@ -43,6 +44,8 @@ var _ webhook.Defaulter = &KubeadmControlPlaneTemplate{}
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type.
 func (r *KubeadmControlPlaneTemplate) Default() {
+	bootstrapv1.DefaultKubeadmConfigSpec(&r.Spec.Template.Spec.KubeadmConfigSpec)
+
 	r.Spec.Template.Spec.RolloutStrategy = defaultRolloutStrategy(r.Spec.Template.Spec.RolloutStrategy)
 }
 
diff --git a/controlplane/kubeadm/api/v1beta1/kubeadmcontrolplanetemplate_webhook_test.go b/controlplane/kubeadm/api/v1beta1/kubeadmcontrolplanetemplate_webhook_test.go
@@ -24,9 +24,40 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilfeature "k8s.io/component-base/featuregate/testing"
 
+	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
 	"sigs.k8s.io/cluster-api/feature"
+	utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
 )
 
+func TestKubeadmControlPlaneTemplateDefault(t *testing.T) {
+	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)()
+
+	g := NewWithT(t)
+
+	kcpTemplate := &KubeadmControlPlaneTemplate{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo",
+		},
+		Spec: KubeadmControlPlaneTemplateSpec{
+			Template: KubeadmControlPlaneTemplateResource{
+				Spec: KubeadmControlPlaneTemplateResourceSpec{
+					MachineTemplate: &KubeadmControlPlaneTemplateMachineTemplate{
+						NodeDrainTimeout: &metav1.Duration{Duration: 10 * time.Second},
+					},
+				},
+			},
+		},
+	}
+	updateDefaultingValidationKCPTemplate := kcpTemplate.DeepCopy()
+	updateDefaultingValidationKCPTemplate.Spec.Template.Spec.MachineTemplate.NodeDrainTimeout = &metav1.Duration{Duration: 20 * time.Second}
+	t.Run("for KubeadmControlPlaneTemplate", utildefaulting.DefaultValidateTest(updateDefaultingValidationKCPTemplate))
+	kcpTemplate.Default()
+
+	g.Expect(kcpTemplate.Spec.Template.Spec.KubeadmConfigSpec.Format).To(Equal(bootstrapv1.CloudConfig))
+	g.Expect(kcpTemplate.Spec.Template.Spec.RolloutStrategy.Type).To(Equal(RollingUpdateStrategyType))
+	g.Expect(kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate.MaxSurge.IntVal).To(Equal(int32(1)))
+}
+
 func TestKubeadmControlPlaneTemplateValidationFeatureGateEnabled(t *testing.T) {
 	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)()
 
diff --git a/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml b/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml
@@ -2923,7 +2923,6 @@ spec:
                       type: object
                     type: array
                   format:
-                    default: cloud-config
                     description: Format specifies the output format of the bootstrap
                       data
                     enum:
diff --git a/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanetemplates.yaml b/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanetemplates.yaml
@@ -1707,7 +1707,6 @@ spec:
                               type: object
                             type: array
                           format:
-                            default: cloud-config
                             description: Format specifies the output format of the
                               bootstrap data
                             enum:
diff --git a/controlplane/kubeadm/internal/filters.go b/controlplane/kubeadm/internal/filters.go
@@ -162,6 +162,13 @@ func matchInitOrJoinConfiguration(machineConfig *bootstrapv1.KubeadmConfig, kcp
 	// to allow a comparison with the KubeadmConfig referenced from the machine.
 	kcpConfig := getAdjustedKcpConfig(kcp, machineConfig)
 
+	// Default both KubeadmConfigSpecs before comparison.
+	// *Note* This assumes that newly added default values never
+	// introduce a semantic difference to the unset value.
+	// But that is something that is ensured by our API guarantees.
+	bootstrapv1.DefaultKubeadmConfigSpec(kcpConfig)
+	bootstrapv1.DefaultKubeadmConfigSpec(&machineConfig.Spec)
+
 	// cleanups all the fields that are not relevant for the comparison.
 	cleanupConfigFields(kcpConfig, machineConfig)
 
diff --git a/controlplane/kubeadm/internal/filters_test.go b/controlplane/kubeadm/internal/filters_test.go
@@ -281,6 +281,52 @@ func TestMatchInitOrJoinConfiguration(t *testing.T) {
 		kcp := &controlplanev1.KubeadmControlPlane{}
 		g.Expect(matchInitOrJoinConfiguration(nil, kcp)).To(BeTrue())
 	})
+	t.Run("returns true if one format is empty and the other one cloud-config", func(t *testing.T) {
+		g := NewWithT(t)
+		kcp := &controlplanev1.KubeadmControlPlane{
+			Spec: controlplanev1.KubeadmControlPlaneSpec{
+				KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
+					Format: bootstrapv1.CloudConfig,
+				},
+			},
+		}
+		m := &clusterv1.Machine{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "KubeadmConfig",
+				APIVersion: clusterv1.GroupVersion.String(),
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "default",
+				Name:      "test",
+			},
+			Spec: clusterv1.MachineSpec{
+				Bootstrap: clusterv1.Bootstrap{
+					ConfigRef: &corev1.ObjectReference{
+						Kind:       "KubeadmConfig",
+						Namespace:  "default",
+						Name:       "test",
+						APIVersion: bootstrapv1.GroupVersion.String(),
+					},
+				},
+			},
+		}
+		machineConfigs := map[string]*bootstrapv1.KubeadmConfig{
+			m.Name: {
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "KubeadmConfig",
+					APIVersion: bootstrapv1.GroupVersion.String(),
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "test",
+				},
+				Spec: bootstrapv1.KubeadmConfigSpec{
+					Format: "",
+				},
+			},
+		}
+		g.Expect(matchInitOrJoinConfiguration(machineConfigs[m.Name], kcp)).To(BeTrue())
+	})
 	t.Run("returns true if InitConfiguration is equal", func(t *testing.T) {
 		g := NewWithT(t)
 		kcp := &controlplanev1.KubeadmControlPlane{

Original file line number	Diff line number	Diff line change
`@@ -68,6 +68,8 @@ func defaultKubeadmControlPlaneSpec(s *KubeadmControlPlaneSpec, namespace string`
`68`	`68`	`s.Version = "v" + s.Version`
`69`	`69`	`}`
`70`	`70`
	`71`	`+ bootstrapv1.DefaultKubeadmConfigSpec(&s.KubeadmConfigSpec)`
	`72`	`+`
`71`	`73`	`s.RolloutStrategy = defaultRolloutStrategy(s.RolloutStrategy)`
`72`	`74`	`}`
`73`	`75`