Merge pull request kubernetes-sigs#10407 from neolit123/1.7-improve-clusterctl-cert-manager-install

🌱 clusterctl/client/cert_manager: improve shouldUpgrade

Author: k8s-ci-robot

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,25 +360,31 @@ 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
 		}
 
 		if needUpgrade {
 			break
 		}
 	}
-	return currentVersion, desiredVersion, needUpgrade, nil
+	return currentVersion, needUpgrade, nil
 }
 
 func (cm *certManagerClient) getWaitTimeout() time.Duration {

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 {