From 5a85fa6a801658b931ea5630ec776b6a6fcd19e3 Mon Sep 17 00:00:00 2001 From: faiq Date: Tue, 29 Oct 2024 13:33:09 -0700 Subject: [PATCH 1/3] fix: considers objects in kube-system for cert-manager to avoid upgrading twice --- cmd/clusterctl/client/cluster/cert_manager.go | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/cmd/clusterctl/client/cluster/cert_manager.go b/cmd/clusterctl/client/cluster/cert_manager.go index 637dbffc5c7e..614e3452305f 100644 --- a/cmd/clusterctl/client/cluster/cert_manager.go +++ b/cmd/clusterctl/client/cluster/cert_manager.go @@ -19,12 +19,14 @@ package cluster import ( "context" _ "embed" + "slices" "time" "github.com/blang/semver/v4" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" @@ -53,6 +55,10 @@ const ( var ( //go:embed assets/cert-manager-test-resources.yaml certManagerTestManifest []byte + // namespaces for all relevant objects in a cert-manager installation. + // It also includes relevant resources in the kube-system namespace, which is used by cert-manager + // for leader election (/~https://github.com/cert-manager/cert-manager/issues/6716). + certManagerNamespaces = []string{certManagerNamespace, metav1.NamespaceSystem} ) // CertManagerUpgradePlan defines the upgrade plan if cert-manager needs to be @@ -202,9 +208,9 @@ func (cm *certManagerClient) install(ctx context.Context, version string, objs [ func (cm *certManagerClient) PlanUpgrade(ctx context.Context) (CertManagerUpgradePlan, error) { log := logf.Log - objs, err := cm.proxy.ListResources(ctx, map[string]string{clusterctlv1.ClusterctlCoreLabel: clusterctlv1.ClusterctlCoreLabelCertManagerValue}, certManagerNamespace) + objs, err := cm.proxy.ListResources(ctx, map[string]string{clusterctlv1.ClusterctlCoreLabel: clusterctlv1.ClusterctlCoreLabelCertManagerValue}, certManagerNamespaces...) if err != nil { - return CertManagerUpgradePlan{}, errors.Wrap(err, "failed get cert manager components") + return CertManagerUpgradePlan{}, errors.Wrap(err, "failed to get cert-manager components") } // If there are no cert manager components with the clusterctl labels, it means that cert-manager is externally managed. @@ -240,12 +246,10 @@ func (cm *certManagerClient) PlanUpgrade(ctx context.Context) (CertManagerUpgrad // older than the version currently suggested by clusterctl, upgrades it. func (cm *certManagerClient) EnsureLatestVersion(ctx context.Context) error { log := logf.Log - - objs, err := cm.proxy.ListResources(ctx, map[string]string{clusterctlv1.ClusterctlCoreLabel: clusterctlv1.ClusterctlCoreLabelCertManagerValue}, certManagerNamespace) + objs, err := cm.proxy.ListResources(ctx, map[string]string{clusterctlv1.ClusterctlCoreLabel: clusterctlv1.ClusterctlCoreLabelCertManagerValue}, certManagerNamespaces...) if err != nil { - return errors.Wrap(err, "failed get cert manager components") + return errors.Wrap(err, "failed to get cert-manager components") } - // If there are no cert manager components with the clusterctl labels, it means that cert-manager is externally managed. if len(objs) == 0 { log.V(5).Info("Skipping cert-manager upgrade because externally managed") @@ -338,14 +342,16 @@ func (cm *certManagerClient) shouldUpgrade(desiredVersion string, objs, installO needUpgrade := false currentVersion := "" + + // removes resources that are generated by the kubernetes API + // this is relevant if the versions are the same, because we compare + // the number of objects when version of objects are equal + objs = slices.DeleteFunc(objs, func(obj unstructured.Unstructured) bool { + return obj.GetKind() == "Endpoints" || obj.GetKind() == "EndpointSlice" + }) for i := range objs { obj := objs[i] - // Endpoints and EndpointSlices are generated by Kubernetes without the version annotation, so we are skipping them - if obj.GetKind() == "Endpoints" || obj.GetKind() == "EndpointSlice" { - continue - } - // if there is no version annotation, this means the obj is cert-manager v0.11.0 (installed with older version of clusterctl) objVersion, ok := obj.GetAnnotations()[clusterctlv1.CertManagerVersionAnnotation] if !ok { From 641bca6fbbbf45504ce5a1eaa90be2d8cb6a6083 Mon Sep 17 00:00:00 2001 From: faiq Date: Mon, 4 Nov 2024 10:31:41 -0800 Subject: [PATCH 2/3] fix: removes use of slices.DeleteFunc because it zeros the elements and doesn't remove those from obj list --- cmd/clusterctl/client/cluster/cert_manager.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cmd/clusterctl/client/cluster/cert_manager.go b/cmd/clusterctl/client/cluster/cert_manager.go index 614e3452305f..83594b73aa13 100644 --- a/cmd/clusterctl/client/cluster/cert_manager.go +++ b/cmd/clusterctl/client/cluster/cert_manager.go @@ -19,7 +19,6 @@ package cluster import ( "context" _ "embed" - "slices" "time" "github.com/blang/semver/v4" @@ -346,9 +345,14 @@ func (cm *certManagerClient) shouldUpgrade(desiredVersion string, objs, installO // removes resources that are generated by the kubernetes API // this is relevant if the versions are the same, because we compare // the number of objects when version of objects are equal - objs = slices.DeleteFunc(objs, func(obj unstructured.Unstructured) bool { - return obj.GetKind() == "Endpoints" || obj.GetKind() == "EndpointSlice" - }) + newObjs := []unstructured.Unstructured{} + for _, o := range objs { + if !(o.GetKind() == "Endpoints" || o.GetKind() == "EndpointSlice") { + newObjs = append(newObjs, o) + } + } + objs = newObjs + for i := range objs { obj := objs[i] From 176b2e7f91c715dea46e1de034fff383fafb634c Mon Sep 17 00:00:00 2001 From: faiq Date: Tue, 5 Nov 2024 11:29:55 -0800 Subject: [PATCH 3/3] fix: do not reassign slice as it is used in different methods --- cmd/clusterctl/client/cluster/cert_manager.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/cmd/clusterctl/client/cluster/cert_manager.go b/cmd/clusterctl/client/cluster/cert_manager.go index 83594b73aa13..3d233817d2c7 100644 --- a/cmd/clusterctl/client/cluster/cert_manager.go +++ b/cmd/clusterctl/client/cluster/cert_manager.go @@ -342,19 +342,18 @@ func (cm *certManagerClient) shouldUpgrade(desiredVersion string, objs, installO needUpgrade := false currentVersion := "" - // removes resources that are generated by the kubernetes API + // creates a new list removing resources that are generated by the kubernetes API // this is relevant if the versions are the same, because we compare // the number of objects when version of objects are equal - newObjs := []unstructured.Unstructured{} + relevantObjs := []unstructured.Unstructured{} for _, o := range objs { if !(o.GetKind() == "Endpoints" || o.GetKind() == "EndpointSlice") { - newObjs = append(newObjs, o) + relevantObjs = append(relevantObjs, o) } } - objs = newObjs - for i := range objs { - obj := objs[i] + for i := range relevantObjs { + obj := relevantObjs[i] // if there is no version annotation, this means the obj is cert-manager v0.11.0 (installed with older version of clusterctl) objVersion, ok := obj.GetAnnotations()[clusterctlv1.CertManagerVersionAnnotation] @@ -384,7 +383,7 @@ func (cm *certManagerClient) shouldUpgrade(desiredVersion string, objs, installO // 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) + needUpgrade = len(relevantObjs) != len(installObjs) case c > 0: // The installed version is greater than the desired version. Upgrade is not required. currentVersion = objVersion