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

@@ -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
 }
 

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 {