Skip to content

Commit

Permalink
clusterctl/client/cert_manager: improve shouldUpgrade
Browse files Browse the repository at this point in the history 
Make shouldUpgrade() accept the list of objects to be installed too.
Compare the installed and to-be-installed objects in case the
installed and to-be-installed versions are the same. If their length is
different, assume that an upgrade is required.

Update unit tests.
  • Loading branch information
@neolit123
neolit123 committed Apr 11, 2024
1 parent 4bfc838 commit 47dcdda
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 70 deletions.
98 changes: 53 additions & 45 deletions cmd/clusterctl/client/cluster/cert_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
85 changes: 60 additions & 25 deletions cmd/clusterctl/client/cluster/cert_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
{
Expand All @@ -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{
{
Expand All @@ -249,7 +250,6 @@ func Test_shouldUpgrade(t *testing.T) {
},
},
wantFromVersion: config.CertManagerDefaultVersion,
wantToVersion: config.CertManagerDefaultVersion,
want: false,
wantErr: false,
},
Expand All @@ -270,7 +270,6 @@ func Test_shouldUpgrade(t *testing.T) {
},
},
wantFromVersion: "v1.5.3",
wantToVersion: "v1.5.3+h4fd4",
want: true,
wantErr: false,
},
Expand All @@ -291,7 +290,6 @@ func Test_shouldUpgrade(t *testing.T) {
},
},
wantFromVersion: "v1.5.3+h4fd5",
wantToVersion: "v1.5.3+h4fd4",
want: true,
wantErr: false,
},
Expand All @@ -312,7 +310,6 @@ func Test_shouldUpgrade(t *testing.T) {
},
},
wantFromVersion: "v1.5.3+h4fd5",
wantToVersion: "v1.5.3+h4fd5",
want: false,
wantErr: false,
},
Expand All @@ -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,
},
Expand All @@ -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{
{
Expand All @@ -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{
{
Expand All @@ -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{
{
Expand All @@ -415,7 +433,6 @@ func Test_shouldUpgrade(t *testing.T) {
},
},
wantFromVersion: "",
wantToVersion: config.CertManagerDefaultVersion,
want: false,
wantErr: false,
},
Expand All @@ -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
Expand All @@ -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))
})
}
}
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 47dcdda

Please sign in to comment.