Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release-1.7] 🌱 clusterctl/client/cert_manager: improve shouldUpgrade #10497

Merged
merged 1 commit into from
Apr 23, 2024
Merged

[release-1.7] 🌱 clusterctl/client/cert_manager: improve shouldUpgrade #10497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 53 additions & 45 deletions cmd/clusterctl/client/cluster/cert_manager.go
View file
Open in desktop
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
Open in desktop
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