Merge pull request #2581 from vincepri/add-tests-etcd-kcp

KCP etcd upgrade: handle conditional update & add tests

Author: k8s-ci-robot

controlplane/kubeadm/internal/kubeadm_config_map.go

@@ -83,31 +83,55 @@ func (k *kubeadmConfig) UpdateKubernetesVersion(version string) error {
 }
 
 // UpdateEtcdMeta sets the local etcd's configuration's image repository and image tag
-func (k *kubeadmConfig) UpdateEtcdMeta(imageRepository, imageTag string) error {
+func (k *kubeadmConfig) UpdateEtcdMeta(imageRepository, imageTag string) (bool, error) {
 	data, ok := k.ConfigMap.Data[clusterConfigurationKey]
 	if !ok {
-		return errors.Errorf("could not find key %q in kubeadm config", clusterConfigurationKey)
+		return false, errors.Errorf("could not find key %q in kubeadm config", clusterConfigurationKey)
 	}
 	configuration, err := yamlToUnstructured([]byte(data))
 	if err != nil {
-		return errors.Wrap(err, "unable to convert YAML to unstructured")
+		return false, errors.Wrap(err, "unable to convert YAML to unstructured")
 	}
-	if imageRepository != "" {
-		if err := unstructured.SetNestedField(configuration.UnstructuredContent(), imageRepository, "etcd", "local", "imageRepository"); err != nil {
-			return errors.Wrap(err, "unable to update image repository on kubeadm configmap")
+
+	var changed bool
+
+	// Handle etcd.local.imageRepository.
+	imageRepositoryPath := []string{"etcd", "local", "imageRepository"}
+	currentImageRepository, _, err := unstructured.NestedString(configuration.UnstructuredContent(), imageRepositoryPath...)
+	if err != nil {
+		return false, errors.Wrap(err, "unable to retrieve current image repository from kubeadm configmap")
+	}
+	if currentImageRepository != imageRepository {
+		if err := unstructured.SetNestedField(configuration.UnstructuredContent(), imageRepository, imageRepositoryPath...); err != nil {
+			return false, errors.Wrap(err, "unable to update etcd.local.imageRepository on kubeadm configmap")
 		}
+		changed = true
+	}
+
+	// Handle etcd.local.imageTag.
+	imageTagPath := []string{"etcd", "local", "imageTag"}
+	currentImageTag, _, err := unstructured.NestedString(configuration.UnstructuredContent(), imageTagPath...)
+	if err != nil {
+		return false, errors.Wrap(err, "unable to retrieve current image repository from kubeadm configmap")
 	}
-	if imageTag != "" {
-		if err := unstructured.SetNestedField(configuration.UnstructuredContent(), imageTag, "etcd", "local", "imageTag"); err != nil {
-			return errors.Wrap(err, "unable to update image repository on kubeadm configmap")
+	if currentImageTag != imageTag {
+		if err := unstructured.SetNestedField(configuration.UnstructuredContent(), imageTag, imageTagPath...); err != nil {
+			return false, errors.Wrap(err, "unable to update etcd.local.imageTag on kubeadm configmap")
 		}
+		changed = true
+	}
+
+	// Return early if no changes have been performed.
+	if !changed {
+		return changed, nil
 	}
+
 	updated, err := yaml.Marshal(configuration)
 	if err != nil {
-		return errors.Wrap(err, "error encoding kubeadm cluster configuration object")
+		return false, errors.Wrap(err, "error encoding kubeadm cluster configuration object")
 	}
 	k.ConfigMap.Data[clusterConfigurationKey] = string(updated)
-	return nil
+	return changed, nil
 }
 
 // yamlToUnstructured looks inside a config map for a specific key and extracts the embedded YAML into an

controlplane/kubeadm/internal/kubeadm_config_map_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package internal
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/onsi/gomega"
@@ -87,3 +88,139 @@ kind: ClusterStatus`,
 		)),
 	))
 }
+
+func TestUpdateEtcdMeta(t *testing.T) {
+
+	tests := []struct {
+		name                      string
+		clusterConfigurationValue string
+		imageRepository           string
+		imageTag                  string
+		expectChanged             bool
+		expectErr                 error
+	}{
+		{
+			name: "it should set the values, if they were empty",
+			clusterConfigurationValue: `
+apiVersion: kubeadm.k8s.io/v1beta2
+kind: ClusterConfiguration
+etcd:
+  local:
+    dataDir: /var/lib/etcd
+`,
+			imageRepository: "gcr.io/k8s/etcd",
+			imageTag:        "0.10.9",
+			expectChanged:   true,
+		},
+		{
+			name: "it should return false with no error, if there are no changes",
+			clusterConfigurationValue: `
+apiVersion: kubeadm.k8s.io/v1beta2
+kind: ClusterConfiguration
+etcd:
+  local:
+    dataDir: /var/lib/etcd
+    imageRepository: "gcr.io/k8s/etcd"
+    imageTag: "0.10.9"
+`,
+			imageRepository: "gcr.io/k8s/etcd",
+			imageTag:        "0.10.9",
+			expectChanged:   false,
+		},
+		{
+			name: "it shouldn't write empty strings",
+			clusterConfigurationValue: `
+apiVersion: kubeadm.k8s.io/v1beta2
+kind: ClusterConfiguration
+etcd:
+  local:
+    dataDir: /var/lib/etcd
+`,
+			imageRepository: "",
+			imageTag:        "",
+			expectChanged:   false,
+		},
+		{
+			name: "it should overwrite imageTag",
+			clusterConfigurationValue: `
+apiVersion: kubeadm.k8s.io/v1beta2
+kind: ClusterConfiguration
+etcd:
+  local:
+    imageTag: 0.10.8
+    dataDir: /var/lib/etcd
+`,
+			imageTag:      "0.10.9",
+			expectChanged: true,
+		},
+		{
+			name: "it should overwrite imageRepository",
+			clusterConfigurationValue: `
+apiVersion: kubeadm.k8s.io/v1beta2
+kind: ClusterConfiguration
+etcd:
+  local:
+    imageRepository: another-custom-repo
+    dataDir: /var/lib/etcd
+`,
+			imageRepository: "gcr.io/k8s/etcd",
+			expectChanged:   true,
+		},
+		{
+			name: "it should error if it's not a valid k8s object",
+			clusterConfigurationValue: `
+etcd:
+  local:
+    imageRepository: another-custom-repo
+    dataDir: /var/lib/etcd
+`,
+			expectErr: errors.New("Object 'Kind' is missing"),
+		},
+		{
+			name: "it should error if the current value is a type we don't expect",
+			clusterConfigurationValue: `
+apiVersion: kubeadm.k8s.io/v1beta2
+kind: ClusterConfiguration
+etcd:
+  local:
+    imageRepository: true
+    dataDir: /var/lib/etcd
+`,
+			expectErr: errors.New(".etcd.local.imageRepository accessor error"),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			g := gomega.NewWithT(t)
+
+			kconfig := &kubeadmConfig{
+				ConfigMap: &corev1.ConfigMap{
+					Data: map[string]string{
+						clusterConfigurationKey: test.clusterConfigurationValue,
+					},
+				},
+			}
+
+			changed, err := kconfig.UpdateEtcdMeta(test.imageRepository, test.imageTag)
+			if test.expectErr == nil {
+				g.Expect(err).ToNot(HaveOccurred())
+			} else {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(gomega.ContainSubstring(test.expectErr.Error()))
+			}
+
+			g.Expect(changed).To(gomega.Equal(test.expectChanged))
+			if changed {
+				if test.imageRepository != "" {
+					g.Expect(kconfig.ConfigMap.Data[clusterConfigurationKey]).To(gomega.ContainSubstring(test.imageRepository))
+				}
+				if test.imageTag != "" {
+					g.Expect(kconfig.ConfigMap.Data[clusterConfigurationKey]).To(gomega.ContainSubstring(test.imageTag))
+				}
+			}
+
+		})
+	}
+
+}

controlplane/kubeadm/internal/workload_cluster.go

@@ -266,7 +266,8 @@ func (w *Workload) UpdateEtcdVersionInKubeadmConfigMap(ctx context.Context, imag
 		return err
 	}
 	config := &kubeadmConfig{ConfigMap: kubeadmConfigMap}
-	if err := config.UpdateEtcdMeta(imageRepository, imageTag); err != nil {
+	changed, err := config.UpdateEtcdMeta(imageRepository, imageTag)
+	if err != nil || !changed {
 		return err
 	}
 	if err := w.Client.Update(ctx, config.ConfigMap); err != nil {