From 3563bfc3344ec0f5a688f77b31325cee6ac5bc71 Mon Sep 17 00:00:00 2001 From: "Lubomir I. Ivanov" Date: Tue, 9 Apr 2024 14:21:44 +0300 Subject: [PATCH] clusterctl/client/cert_manager: improve shouldUpgrade Make shouldUpgrade() accept the list of objects to be installed too. Compare the installed and to-be-installed objects. If their length is different, assume that an upgrade is always required. Update unit tests. --- cmd/clusterctl/client/cluster/cert_manager.go | 64 +++++++++++++------ .../client/cluster/cert_manager_test.go | 59 ++++++++++++++--- 2 files changed, 96 insertions(+), 27 deletions(-) diff --git a/cmd/clusterctl/client/cluster/cert_manager.go b/cmd/clusterctl/client/cluster/cert_manager.go index a99119ac35a7..dcd551b6e341 100644 --- a/cmd/clusterctl/client/cluster/cert_manager.go +++ b/cmd/clusterctl/client/cluster/cert_manager.go @@ -159,27 +159,26 @@ func (cm *certManagerClient) EnsureInstalled(ctx context.Context) error { return nil } - // Otherwise install cert manager. - // NOTE: this instance of cert-manager will have clusterctl specific annotations that will be used to - // manage the lifecycle of all the components. - return cm.install(ctx) -} - -func (cm *certManagerClient) install(ctx context.Context) error { - log := logf.Log - config, err := cm.configClient.CertManager().Get() if err != nil { return err } - log.Info("Installing cert-manager", "Version", config.Version()) - - // Gets the cert-manager components from the repository. objs, err := cm.getManifestObjs(ctx, config) if err != nil { return err } + // Otherwise install cert manager. + // NOTE: this instance of cert-manager will have clusterctl specific annotations that will be used to + // manage the lifecycle of all the components. + return cm.install(ctx, config.Version(), objs) +} + +func (cm *certManagerClient) install(ctx context.Context, version string, objs []unstructured.Unstructured) error { + log := logf.Log + + log.Info("Installing cert-manager", "Version", version) + // Install all cert-manager manifests createCertManagerBackoff := newWriteBackoff() objs = utilresource.SortForCreate(objs) @@ -214,8 +213,18 @@ func (cm *certManagerClient) PlanUpgrade(ctx context.Context) (CertManagerUpgrad return CertManagerUpgradePlan{ExternallyManaged: true}, nil } - log.Info("Checking cert-manager version...") - currentVersion, targetVersion, shouldUpgrade, err := cm.shouldUpgrade(objs) + // Get the list of objects to install. + config, err := cm.configClient.CertManager().Get() + if err != nil { + return CertManagerUpgradePlan{}, err + } + installObjs, err := cm.getManifestObjs(ctx, config) + if err != nil { + return CertManagerUpgradePlan{}, err + } + + log.Info("Checking if cert-manager needs upgrade...") + currentVersion, targetVersion, shouldUpgrade, err := cm.shouldUpgrade(objs, installObjs) if err != nil { return CertManagerUpgradePlan{}, err } @@ -243,8 +252,18 @@ func (cm *certManagerClient) EnsureLatestVersion(ctx context.Context) error { return nil } - log.Info("Checking cert-manager version...") - currentVersion, _, shouldUpgrade, err := cm.shouldUpgrade(objs) + // Get the list of objects to install. + config, err := cm.configClient.CertManager().Get() + if err != nil { + return err + } + installObjs, err := cm.getManifestObjs(ctx, config) + if err != nil { + return err + } + + log.Info("Checking if cert-manager needs upgrade...") + currentVersion, _, shouldUpgrade, err := cm.shouldUpgrade(objs, installObjs) if err != nil { return err } @@ -269,7 +288,7 @@ func (cm *certManagerClient) EnsureLatestVersion(ctx context.Context) error { } // Install cert-manager. - return cm.install(ctx) + return cm.install(ctx, config.Version(), installObjs) } func (cm *certManagerClient) migrateCRDs(ctx context.Context) error { @@ -322,7 +341,7 @@ func (cm *certManagerClient) deleteObjs(ctx context.Context, objs []unstructured return nil } -func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (string, string, bool, error) { +func (cm *certManagerClient) shouldUpgrade(objs, installObjs []unstructured.Unstructured) (string, string, bool, error) { config, err := cm.configClient.CertManager().Get() if err != nil { return "", "", false, err @@ -334,6 +353,12 @@ func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (st return "", "", false, errors.Wrapf(err, "failed to parse config version [%s] for cert-manager component", desiredVersion) } + // If the number of avaiable objects and objects to install differ, return true, but first fetch the current version. + diffObjLen := false + if len(objs) != len(installObjs) { + diffObjLen = true + } + needUpgrade := false currentVersion := "" for i := range objs { @@ -376,6 +401,9 @@ func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (st break } } + if diffObjLen { + needUpgrade = true + } return currentVersion, desiredVersion, needUpgrade, nil } diff --git a/cmd/clusterctl/client/cluster/cert_manager_test.go b/cmd/clusterctl/client/cluster/cert_manager_test.go index 7d530f836539..027c4e89061e 100644 --- a/cmd/clusterctl/client/cluster/cert_manager_test.go +++ b/cmd/clusterctl/client/cluster/cert_manager_test.go @@ -211,13 +211,14 @@ func Test_shouldUpgrade(t *testing.T) { objs []unstructured.Unstructured } tests := []struct { - name string - configVersion string - args args - wantFromVersion string - wantToVersion string - want bool - wantErr bool + name string + configVersion string + args args + wantFromVersion string + wantToVersion string + hasDiffInstallObjs bool + want bool + wantErr bool }{ { name: "Version is not defined (e.g. cluster created with clusterctl < v0.3.9), should upgrade", @@ -398,6 +399,27 @@ func Test_shouldUpgrade(t *testing.T) { want: false, wantErr: false, }, + { + name: "Version is newer, should not upgrade, but objects to install are a different size", + args: args{ + objs: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "annotations": map[string]interface{}{ + clusterctlv1.CertManagerVersionAnnotation: "v100.0.0", + }, + }, + }, + }, + }, + }, + wantFromVersion: "v100.0.0", + wantToVersion: config.CertManagerDefaultVersion, + hasDiffInstallObjs: true, + want: true, + wantErr: false, + }, { name: "Endpoint are ignored", args: args{ @@ -431,7 +453,16 @@ func Test_shouldUpgrade(t *testing.T) { } cm := newCertManagerClient(fakeConfigClient, nil, proxy, pollImmediateWaiter) - fromVersion, toVersion, got, err := cm.shouldUpgrade(tt.args.objs) + // By default, make the installed and to-be-installed objects the same, but if hasDiffInstallObjs is set, + // just append an empty unstructured object at the end to make them different. + installObjs := tt.args.objs + if tt.hasDiffInstallObjs { + installObjs = make([]unstructured.Unstructured, len(tt.args.objs)) + copy(installObjs, tt.args.objs) + installObjs = append(installObjs, unstructured.Unstructured{}) + } + + fromVersion, toVersion, got, err := cm.shouldUpgrade(tt.args.objs, installObjs) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return @@ -718,7 +749,17 @@ func Test_certManagerClient_PlanUpgrade(t *testing.T) { pollImmediateWaiter := func(context.Context, time.Duration, time.Duration, wait.ConditionWithContextFunc) error { return nil } - cm := newCertManagerClient(fakeConfigClient, nil, proxy, pollImmediateWaiter) + + // Preopare a fake memory repo from which getManifestObjs() called from PlanUpgrade() will fetch to-be-installed objects. + fakeRepositoryClientFactory := func(ctx context.Context, provider config.Provider, configClient config.Client, options ...repository.Option) (repository.Client, error) { + repo := repository.NewMemoryRepository(). + WithPaths("root", "components.yaml"). + WithDefaultVersion(config.CertManagerDefaultVersion). + WithFile(config.CertManagerDefaultVersion, "components.yaml", certManagerDeploymentYaml) + return repository.New(ctx, provider, configClient, repository.InjectRepository(repo)) + } + + cm := newCertManagerClient(fakeConfigClient, fakeRepositoryClientFactory, proxy, pollImmediateWaiter) actualPlan, err := cm.PlanUpgrade(ctx) if tt.expectErr {