diff --git a/cmd/clusterctl/client/cluster/cert_manager.go b/cmd/clusterctl/client/cluster/cert_manager.go index a99119ac35a7..637dbffc5c7e 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,15 +213,25 @@ 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, shouldUpgrade, err := cm.shouldUpgrade(config.Version(), objs, installObjs) if err != nil { return CertManagerUpgradePlan{}, err } return CertManagerUpgradePlan{ From: currentVersion, - To: targetVersion, + To: config.Version(), ShouldUpgrade: shouldUpgrade, }, nil } @@ -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(config.Version(), objs, installObjs) if err != nil { return err } @@ -256,7 +275,7 @@ func (cm *certManagerClient) EnsureLatestVersion(ctx context.Context) error { // Migrate CRs to latest CRD storage version, if necessary. // Note: We have to do this before cert-manager is deleted so conversion webhooks still work. - if err := cm.migrateCRDs(ctx); err != nil { + if err := cm.migrateCRDs(ctx, installObjs); err != nil { return err } @@ -269,27 +288,16 @@ 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 { - config, err := cm.configClient.CertManager().Get() - if err != nil { - return err - } - - // Gets the new cert-manager components from the repository. - objs, err := cm.getManifestObjs(ctx, config) - if err != nil { - return err - } - +func (cm *certManagerClient) migrateCRDs(ctx context.Context, installObj []unstructured.Unstructured) error { c, err := cm.proxy.NewClient(ctx) if err != nil { return err } - return NewCRDMigrator(c).Run(ctx, objs) + return NewCRDMigrator(c).Run(ctx, installObj) } func (cm *certManagerClient) deleteObjs(ctx context.Context, objs []unstructured.Unstructured) error { @@ -322,16 +330,10 @@ func (cm *certManagerClient) deleteObjs(ctx context.Context, objs []unstructured return nil } -func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (string, string, bool, error) { - config, err := cm.configClient.CertManager().Get() - if err != nil { - return "", "", false, err - } - - desiredVersion := config.Version() +func (cm *certManagerClient) shouldUpgrade(desiredVersion string, objs, installObjs []unstructured.Unstructured) (string, bool, error) { desiredSemVersion, err := semver.ParseTolerant(desiredVersion) if err != nil { - return "", "", false, errors.Wrapf(err, "failed to parse config version [%s] for cert-manager component", desiredVersion) + return "", false, errors.Wrapf(err, "failed to parse config version [%s] for cert-manager component", desiredVersion) } needUpgrade := false @@ -358,17 +360,23 @@ func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (st objSemVersion, err := semver.ParseTolerant(objVersion) if err != nil { - return "", "", false, errors.Wrapf(err, "failed to parse version for cert-manager component %s/%s", obj.GetKind(), obj.GetName()) + return "", false, errors.Wrapf(err, "failed to parse version for cert-manager component %s/%s", obj.GetKind(), obj.GetName()) } c := version.Compare(objSemVersion, desiredSemVersion, version.WithBuildTags()) switch { case c < 0 || c == 2: - // if version < current or same version and different non-numeric build metadata, then upgrade + // The installed version is lower than the desired version or they are equal, but their metadata + // is different non-numerically (see version.WithBuildTags()). Upgrade is required. currentVersion = objVersion needUpgrade = true - case c >= 0: - // the installed version is greater than or equal to one required by clusterctl, so we are ok + case c == 0: + // The installed version is equal to the desired version. Upgrade is required only if the number + // of available objects and objects to install differ. This would act as a re-install. + currentVersion = objVersion + needUpgrade = len(objs) != len(installObjs) + case c > 0: + // The installed version is greater than the desired version. Upgrade is not required. currentVersion = objVersion } @@ -376,7 +384,7 @@ func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (st break } } - return currentVersion, desiredVersion, needUpgrade, nil + return currentVersion, needUpgrade, nil } func (cm *certManagerClient) getWaitTimeout() time.Duration { diff --git a/cmd/clusterctl/client/cluster/cert_manager_test.go b/cmd/clusterctl/client/cluster/cert_manager_test.go index 7d530f836539..cab681d02acb 100644 --- a/cmd/clusterctl/client/cluster/cert_manager_test.go +++ b/cmd/clusterctl/client/cluster/cert_manager_test.go @@ -211,16 +211,17 @@ 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 + hasDiffInstallObjs bool + want bool + wantErr bool }{ { - name: "Version is not defined (e.g. cluster created with clusterctl < v0.3.9), should upgrade", + name: "Version is not defined (e.g. cluster created with clusterctl < v0.3.9), should upgrade", + configVersion: config.CertManagerDefaultVersion, args: args{ objs: []unstructured.Unstructured{ { @@ -229,12 +230,12 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v0.11.0", - wantToVersion: config.CertManagerDefaultVersion, want: true, wantErr: false, }, { - name: "Version is equal, should not upgrade", + name: "Version is equal, should not upgrade", + configVersion: config.CertManagerDefaultVersion, args: args{ objs: []unstructured.Unstructured{ { @@ -249,7 +250,6 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: config.CertManagerDefaultVersion, - wantToVersion: config.CertManagerDefaultVersion, want: false, wantErr: false, }, @@ -270,7 +270,6 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v1.5.3", - wantToVersion: "v1.5.3+h4fd4", want: true, wantErr: false, }, @@ -291,7 +290,6 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v1.5.3+h4fd5", - wantToVersion: "v1.5.3+h4fd4", want: true, wantErr: false, }, @@ -312,7 +310,6 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v1.5.3+h4fd5", - wantToVersion: "v1.5.3+h4fd5", want: false, wantErr: false, }, @@ -333,7 +330,6 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v1.5.3+build.2", - wantToVersion: "v1.5.3+build.1", want: false, wantErr: false, }, @@ -354,12 +350,33 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v1.5.3+build.2", - wantToVersion: "v1.5.3+build.3", want: true, wantErr: false, }, { - name: "Version is older, should upgrade", + name: "Version is equal, but should upgrade because objects to install are a different size", + configVersion: config.CertManagerDefaultVersion, + args: args{ + objs: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "annotations": map[string]interface{}{ + clusterctlv1.CertManagerVersionAnnotation: config.CertManagerDefaultVersion, + }, + }, + }, + }, + }, + }, + wantFromVersion: config.CertManagerDefaultVersion, + hasDiffInstallObjs: true, + want: true, + wantErr: false, + }, + { + name: "Version is older, should upgrade", + configVersion: config.CertManagerDefaultVersion, args: args{ objs: []unstructured.Unstructured{ { @@ -374,12 +391,12 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v0.11.0", - wantToVersion: config.CertManagerDefaultVersion, want: true, wantErr: false, }, { - name: "Version is newer, should not upgrade", + name: "Version is newer, should not upgrade", + configVersion: config.CertManagerDefaultVersion, args: args{ objs: []unstructured.Unstructured{ { @@ -394,12 +411,13 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "v100.0.0", - wantToVersion: config.CertManagerDefaultVersion, want: false, wantErr: false, }, + { - name: "Endpoint are ignored", + name: "Endpoint are ignored", + configVersion: config.CertManagerDefaultVersion, args: args{ objs: []unstructured.Unstructured{ { @@ -415,7 +433,6 @@ func Test_shouldUpgrade(t *testing.T) { }, }, wantFromVersion: "", - wantToVersion: config.CertManagerDefaultVersion, want: false, wantErr: false, }, @@ -431,7 +448,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, got, err := cm.shouldUpgrade(tt.configVersion, tt.args.objs, installObjs) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return @@ -440,7 +466,6 @@ func Test_shouldUpgrade(t *testing.T) { g.Expect(got).To(Equal(tt.want)) g.Expect(fromVersion).To(Equal(tt.wantFromVersion)) - g.Expect(toVersion).To(Equal(tt.wantToVersion)) }) } } @@ -718,7 +743,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) + + // Prepare 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, _ ...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 {