From 44bfb43bb60c51e7b13a01ff438311933d088d10 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Fri, 6 Mar 2020 14:45:59 -0800 Subject: [PATCH] KCP etcd upgrade: handle conditional update & add tests Signed-off-by: Vince Prignano --- .../kubeadm/internal/kubeadm_config_map.go | 46 ++++-- .../internal/kubeadm_config_map_test.go | 137 ++++++++++++++++++ .../kubeadm/internal/workload_cluster.go | 3 +- 3 files changed, 174 insertions(+), 12 deletions(-) diff --git a/controlplane/kubeadm/internal/kubeadm_config_map.go b/controlplane/kubeadm/internal/kubeadm_config_map.go index 78678f2d60c0..19e7bd8c73db 100644 --- a/controlplane/kubeadm/internal/kubeadm_config_map.go +++ b/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 diff --git a/controlplane/kubeadm/internal/kubeadm_config_map_test.go b/controlplane/kubeadm/internal/kubeadm_config_map_test.go index 3a81a5e70451..4cedb9546c4f 100644 --- a/controlplane/kubeadm/internal/kubeadm_config_map_test.go +++ b/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)) + } + } + + }) + } + +} diff --git a/controlplane/kubeadm/internal/workload_cluster.go b/controlplane/kubeadm/internal/workload_cluster.go index 9d91c112033d..600526bb23de 100644 --- a/controlplane/kubeadm/internal/workload_cluster.go +++ b/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 {