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

[release-1.7] 🐛 fix: considers objects in kube-system for cert-manager to avoid upgrading twice #11455

Merged
merged 3 commits into from
Nov 21, 2024
Merged

[release-1.7] 🐛 fix: considers objects in kube-system for cert-manager to avoid upgrading twice #11455

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a85fa6
fix: considers objects in kube-system for cert-manager to avoid upgra…
faiq Oct 29, 2024
641bca6
fix: removes use of slices.DeleteFunc because it zeros the elements a…
faiq Nov 4, 2024
176b2e7
fix: do not reassign slice as it is used in different methods
faiq Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 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 @@ -25,6 +25,7 @@ import (
"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"

Expand Down Expand Up @@ -53,6 +54,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
Expand Down Expand Up @@ -202,9 +207,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.
Expand Down Expand Up @@ -240,12 +245,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")
Expand Down Expand Up @@ -338,13 +341,19 @@ func (cm *certManagerClient) shouldUpgrade(desiredVersion string, objs, installO

needUpgrade := false
currentVersion := ""
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
// 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
relevantObjs := []unstructured.Unstructured{}
for _, o := range objs {
if !(o.GetKind() == "Endpoints" || o.GetKind() == "EndpointSlice") {
relevantObjs = append(relevantObjs, o)
}
}

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]
Expand Down Expand Up @@ -374,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
Expand Down