Merge pull request #12776 from sbueringer/pr-simplify-cleanupConfigFields

🌱 Simplify cleanupConfigFields in KCP

Author: k8s-ci-robot

controlplane/kubeadm/internal/filters.go

@@ -288,42 +288,28 @@ func getAdjustedKcpConfig(kcp *controlplanev1.KubeadmControlPlane, machineConfig
 }
 
 // cleanupConfigFields cleanups all the fields that are not relevant for the comparison.
+// Note: This function assumes that old defaults have been applied to kcpConfig and machineConfig
+// as a consequence we can assume JoinConfiguration and JoinConfiguration.NodeRegistration are always defined.
 func cleanupConfigFields(kcpConfig *bootstrapv1.KubeadmConfigSpec, machineConfig *bootstrapv1.KubeadmConfig) {
-	// KCP ClusterConfiguration will only be compared with a machine's ClusterConfiguration annotation, so
-	// we are cleaning up from the reflect.DeepEqual comparison.
+	// KCP ClusterConfiguration will be compared in `matchClusterConfiguration` so we are cleaning it up here
+	// so it doesn't lead to a duplicate diff in `matchInitOrJoinConfiguration`.
 	kcpConfig.ClusterConfiguration = bootstrapv1.ClusterConfiguration{}
 	machineConfig.Spec.ClusterConfiguration = bootstrapv1.ClusterConfiguration{}
 
-	// If KCP JoinConfiguration is not present, set machine JoinConfiguration to nil (nothing can trigger rollout here).
-	// NOTE: this is required because CABPK applies an empty joinConfiguration in case no one is provided.
-	if !kcpConfig.JoinConfiguration.IsDefined() {
-		machineConfig.Spec.JoinConfiguration = bootstrapv1.JoinConfiguration{}
-	}
-
 	// Cleanup JoinConfiguration.Discovery from kcpConfig and machineConfig, because those info are relevant only for
 	// the join process and not for comparing the configuration of the machine.
-	emptyDiscovery := bootstrapv1.Discovery{}
-	if kcpConfig.JoinConfiguration.IsDefined() {
-		kcpConfig.JoinConfiguration.Discovery = emptyDiscovery
-	}
-	if machineConfig.Spec.JoinConfiguration.IsDefined() {
-		machineConfig.Spec.JoinConfiguration.Discovery = emptyDiscovery
-	}
-
-	// If KCP JoinConfiguration.ControlPlane is not present, set machine join configuration to nil (nothing can trigger rollout here).
-	// NOTE: this is required because CABPK applies an empty joinConfiguration.ControlPlane in case no one is provided.
-	if kcpConfig.JoinConfiguration.IsDefined() && kcpConfig.JoinConfiguration.ControlPlane == nil &&
-		machineConfig.Spec.JoinConfiguration.ControlPlane != nil {
+	// Note: Changes to Discovery will apply for the next join, but they will not lead to a rollout.
+	kcpConfig.JoinConfiguration.Discovery = bootstrapv1.Discovery{}
+	machineConfig.Spec.JoinConfiguration.Discovery = bootstrapv1.Discovery{}
+
+	// If KCP JoinConfiguration.ControlPlane is nil and the Machine JoinConfiguration.ControlPlane is empty,
+	// set Machine JoinConfiguration.ControlPlane to nil.
+	// NOTE: This is required because CABPK applies an empty JoinConfiguration.ControlPlane in case it is nil.
+	if kcpConfig.JoinConfiguration.ControlPlane == nil &&
+		reflect.DeepEqual(machineConfig.Spec.JoinConfiguration.ControlPlane, &bootstrapv1.JoinControlPlane{}) {
 		machineConfig.Spec.JoinConfiguration.ControlPlane = nil
 	}
 
-	// If KCP's join NodeRegistration is empty, set machine's node registration to empty as no changes should trigger rollout.
-	emptyNodeRegistration := bootstrapv1.NodeRegistrationOptions{}
-	if kcpConfig.JoinConfiguration.IsDefined() && reflect.DeepEqual(kcpConfig.JoinConfiguration.NodeRegistration, emptyNodeRegistration) &&
-		!reflect.DeepEqual(machineConfig.Spec.JoinConfiguration.NodeRegistration, emptyNodeRegistration) {
-		machineConfig.Spec.JoinConfiguration.NodeRegistration = emptyNodeRegistration
-	}
-
 	// Drop differences that do not lead to changes to Machines, but that might exist due
 	// to changes in how we serialize objects or how webhooks work.
 	dropOmittableFields(kcpConfig)

controlplane/kubeadm/internal/filters_test.go

@@ -333,22 +333,6 @@ func TestCleanupConfigFields(t *testing.T) {
 		g.Expect(kcpConfig.ClusterConfiguration.IsDefined()).To(BeFalse())
 		g.Expect(machineConfig.Spec.ClusterConfiguration.IsDefined()).To(BeFalse())
 	})
-	t.Run("JoinConfiguration gets removed from MachineConfig if it was not derived by KCPConfig", func(t *testing.T) {
-		g := NewWithT(t)
-		kcpConfig := &bootstrapv1.KubeadmConfigSpec{
-			JoinConfiguration: bootstrapv1.JoinConfiguration{}, // KCP not providing a JoinConfiguration
-		}
-		machineConfig := &bootstrapv1.KubeadmConfig{
-			Spec: bootstrapv1.KubeadmConfigSpec{
-				JoinConfiguration: bootstrapv1.JoinConfiguration{
-					ControlPlane: &bootstrapv1.JoinControlPlane{},
-				}, // Machine gets a default JoinConfiguration from CABPK
-			},
-		}
-		cleanupConfigFields(kcpConfig, machineConfig)
-		g.Expect(kcpConfig.JoinConfiguration.IsDefined()).To(BeFalse())
-		g.Expect(machineConfig.Spec.JoinConfiguration.IsDefined()).To(BeFalse())
-	})
 	t.Run("JoinConfiguration.Discovery gets removed because it is not relevant for compare", func(t *testing.T) {
 		g := NewWithT(t)
 		kcpConfig := &bootstrapv1.KubeadmConfigSpec{
@@ -367,7 +351,7 @@ func TestCleanupConfigFields(t *testing.T) {
 		g.Expect(kcpConfig.JoinConfiguration.Discovery).To(BeComparableTo(bootstrapv1.Discovery{}))
 		g.Expect(machineConfig.Spec.JoinConfiguration.Discovery).To(BeComparableTo(bootstrapv1.Discovery{}))
 	})
-	t.Run("JoinConfiguration.ControlPlane gets removed from MachineConfig if it was not derived by KCPConfig", func(t *testing.T) {
+	t.Run("JoinConfiguration.ControlPlane gets removed from MachineConfig if it was added by CABPK", func(t *testing.T) {
 		g := NewWithT(t)
 		kcpConfig := &bootstrapv1.KubeadmConfigSpec{
 			JoinConfiguration: bootstrapv1.JoinConfiguration{
@@ -385,23 +369,28 @@ func TestCleanupConfigFields(t *testing.T) {
 		g.Expect(kcpConfig.JoinConfiguration).ToNot(BeNil())
 		g.Expect(machineConfig.Spec.JoinConfiguration.ControlPlane).To(BeNil())
 	})
-	t.Run("JoinConfiguration.NodeRegistrationOptions gets removed from MachineConfig if it was not derived by KCPConfig", func(t *testing.T) {
+	t.Run("JoinConfiguration.ControlPlane gets not removed from MachineConfig if it is not empty", func(t *testing.T) {
 		g := NewWithT(t)
 		kcpConfig := &bootstrapv1.KubeadmConfigSpec{
 			JoinConfiguration: bootstrapv1.JoinConfiguration{
-				NodeRegistration: bootstrapv1.NodeRegistrationOptions{}, // NodeRegistrationOptions configuration missing in KCP
+				ControlPlane: nil, // Control plane configuration missing in KCP
 			},
 		}
 		machineConfig := &bootstrapv1.KubeadmConfig{
 			Spec: bootstrapv1.KubeadmConfigSpec{
 				JoinConfiguration: bootstrapv1.JoinConfiguration{
-					NodeRegistration: bootstrapv1.NodeRegistrationOptions{Name: "test"}, // Machine gets a some JoinConfiguration.NodeRegistrationOptions
+					ControlPlane: &bootstrapv1.JoinControlPlane{
+						LocalAPIEndpoint: bootstrapv1.APIEndpoint{
+							AdvertiseAddress: "1.2.3.4",
+							BindPort:         6443,
+						},
+					},
 				},
 			},
 		}
 		cleanupConfigFields(kcpConfig, machineConfig)
 		g.Expect(kcpConfig.JoinConfiguration).ToNot(BeNil())
-		g.Expect(machineConfig.Spec.JoinConfiguration.NodeRegistration).To(BeComparableTo(bootstrapv1.NodeRegistrationOptions{}))
+		g.Expect(machineConfig.Spec.JoinConfiguration.ControlPlane).ToNot(BeNil())
 	})
 	t.Run("drops omittable fields", func(t *testing.T) {
 		g := NewWithT(t)
@@ -609,6 +598,53 @@ func TestMatchInitOrJoinConfiguration(t *testing.T) {
     Files:     nil,
     DiskSetup: {},
     ... // 9 identical fields
+  }`))
+	})
+	t.Run("returns false if JoinConfiguration is NOT equal", func(t *testing.T) {
+		g := NewWithT(t)
+		kcp := &controlplanev1.KubeadmControlPlane{
+			Spec: controlplanev1.KubeadmControlPlaneSpec{
+				KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
+					ClusterConfiguration: bootstrapv1.ClusterConfiguration{},
+					InitConfiguration:    bootstrapv1.InitConfiguration{},
+					// JoinConfiguration not set anymore.
+				},
+			},
+		}
+		machineConfig := &bootstrapv1.KubeadmConfig{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "default",
+				Name:      "test",
+			},
+			Spec: bootstrapv1.KubeadmConfigSpec{
+				JoinConfiguration: bootstrapv1.JoinConfiguration{
+					NodeRegistration: bootstrapv1.NodeRegistrationOptions{
+						Name: "An old name", // This is a change
+					},
+				},
+			},
+		}
+		match, diff, err := matchInitOrJoinConfiguration(machineConfig, kcp)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(match).To(BeFalse())
+		g.Expect(diff).To(BeComparableTo(`&v1beta2.KubeadmConfigSpec{
+    ClusterConfiguration: {},
+    InitConfiguration:    {NodeRegistration: {ImagePullPolicy: "IfNotPresent"}},
+    JoinConfiguration: v1beta2.JoinConfiguration{
+      NodeRegistration: v1beta2.NodeRegistrationOptions{
+-       Name:      "An old name",
++       Name:      "",
+        CRISocket: "",
+        Taints:    nil,
+        ... // 4 identical fields
+      },
+      CACertPath: "",
+      Discovery:  {},
+      ... // 4 identical fields
+    },
+    Files:     nil,
+    DiskSetup: {},
+    ... // 9 identical fields
   }`))
 	})
 	t.Run("returns true if returns true if only omittable configurations are not equal", func(t *testing.T) {