Skip to content

🌱 Simplify cleanupConfigFields in KCP #12776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Simplify cleanupConfigFields in KCP #12776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 13 additions & 27 deletions controlplane/kubeadm/internal/filters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
78 changes: 57 additions & 21 deletions controlplane/kubeadm/internal/filters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down