Merge pull request #12065 from chrischdi/pr-allow-unsetting-etcd-local-extnal-dns

🌱 kcp: allow unsetting etcd.local, etcd.external and dns

Author: k8s-ci-robot

controlplane/kubeadm/internal/webhooks/kubeadm_control_plane.go

@@ -162,23 +162,16 @@ const minimumCertificatesExpiryDays = 7
 func (webhook *KubeadmControlPlane) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
 	// add a * to indicate everything beneath is ok.
 	// For example, {"spec", "*"} will allow any path under "spec" to change.
+	// For example, {"spec"} will allow "spec" to also be unset.
 	allowedPaths := [][]string{
 		// metadata
 		{"metadata", "*"},
 		// spec.kubeadmConfigSpec.clusterConfiguration
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "imageRepository"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "imageTag"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "extraArgs"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "extraArgs", "*"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "dataDir"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "peerCertSANs"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "serverCertSANs"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "external", "endpoints"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "external", "caFile"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "external", "certFile"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "external", "keyFile"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "dns", "imageRepository"},
-		{spec, kubeadmConfigSpec, clusterConfiguration, "dns", "imageTag"},
+		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local"},
+		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "local", "*"},
+		{spec, kubeadmConfigSpec, clusterConfiguration, "etcd", "external", "*"},
+		{spec, kubeadmConfigSpec, clusterConfiguration, "dns"},
+		{spec, kubeadmConfigSpec, clusterConfiguration, "dns", "*"},
 		{spec, kubeadmConfigSpec, clusterConfiguration, "imageRepository"},
 		{spec, kubeadmConfigSpec, clusterConfiguration, featureGates},
 		{spec, kubeadmConfigSpec, clusterConfiguration, featureGates, "*"},
@@ -552,7 +545,7 @@ func validateClusterConfiguration(oldClusterConfiguration, newClusterConfigurati
 
 	// update validations
 	if oldClusterConfiguration != nil {
-		if newClusterConfiguration.Etcd.External != nil && oldClusterConfiguration.Etcd.Local != nil {
+		if (newClusterConfiguration.Etcd.External != nil && oldClusterConfiguration.Etcd.External == nil) || (newClusterConfiguration.Etcd.External == nil && oldClusterConfiguration.Etcd.External != nil) {
 			allErrs = append(
 				allErrs,
 				field.Forbidden(
@@ -561,16 +554,6 @@ func validateClusterConfiguration(oldClusterConfiguration, newClusterConfigurati
 				),
 			)
 		}
-
-		if newClusterConfiguration.Etcd.Local != nil && oldClusterConfiguration.Etcd.External != nil {
-			allErrs = append(
-				allErrs,
-				field.Forbidden(
-					pathPrefix.Child("etcd", "local"),
-					"cannot change between external and local etcd",
-				),
-			)
-		}
 	}
 
 	return allErrs

controlplane/kubeadm/internal/webhooks/kubeadm_control_plane_test.go

@@ -459,6 +459,8 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 			ImageTag: "v9.1.1",
 		},
 	}
+	etcdLocalImageTagAndDataDir := etcdLocalImageTag.DeepCopy()
+	etcdLocalImageTagAndDataDir.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local.DataDir = "/foo"
 
 	etcdLocalImageBuildTag := before.DeepCopy()
 	etcdLocalImageBuildTag.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = &bootstrapv1.LocalEtcd{
@@ -474,8 +476,8 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 		},
 	}
 
-	unsetEtcd := etcdLocalImageTag.DeepCopy()
-	unsetEtcd.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = nil
+	unsetEtcdLocal := etcdLocalImageTag.DeepCopy()
+	unsetEtcdLocal.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = nil
 
 	networking := before.DeepCopy()
 	networking.Spec.KubeadmConfigSpec.ClusterConfiguration.Networking.DNSDomain = "some dns domain"
@@ -587,6 +589,10 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 	externalEtcd.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External = &bootstrapv1.ExternalEtcd{
 		KeyFile: "some key file",
 	}
+	externalEtcdChanged := before.DeepCopy()
+	externalEtcdChanged.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External = &bootstrapv1.ExternalEtcd{
+		KeyFile: "another key file",
+	}
 
 	localDataDir := before.DeepCopy()
 	localDataDir.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = &bootstrapv1.LocalEtcd{
@@ -856,6 +862,12 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 			before:    dns,
 			kcp:       unsetCoreDNSToVersion,
 		},
+		{
+			name:      "should succeed when DNS is set to nil",
+			expectErr: false,
+			before:    dns,
+			kcp:       unsetCoreDNSToVersion,
+		},
 		{
 			name:      "should succeed when using an valid DNS build",
 			expectErr: false,
@@ -941,14 +953,26 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 		{
 			name:      "should succeed when making a change to the cluster config's external etcd's configuration",
 			expectErr: false,
-			before:    before,
-			kcp:       externalEtcd,
+			before:    externalEtcd,
+			kcp:       externalEtcdChanged,
 		},
 		{
-			name:      "should fail when attempting to unset the etcd local object",
-			expectErr: true,
+			name:      "should succeed when adding the cluster config's local etcd's configuration",
+			expectErr: false,
+			before:    unsetEtcdLocal,
+			kcp:       etcdLocalImageTag,
+		},
+		{
+			name:      "should succeed when making a change to the cluster config's local etcd's configuration",
+			expectErr: false,
+			before:    etcdLocalImageTag,
+			kcp:       etcdLocalImageTagAndDataDir,
+		},
+		{
+			name:      "should succeed when attempting to unset the etcd local object to fallback to the default",
+			expectErr: false,
 			before:    etcdLocalImageTag,
-			kcp:       unsetEtcd,
+			kcp:       unsetEtcdLocal,
 		},
 		{
 			name:      "should fail if both local and external etcd are set",