fix: upgrade clustermanager/klusterlet when source repo changes (#75)

* fix: upgrade clustermanager/klusterlet when source repo changes

Signed-off-by: Artur Shad Nik <[email protected]>

* feat: dont nuke annotations

Signed-off-by: Artur Shad Nik <[email protected]>

* chore: handle all hashing algos

Signed-off-by: Artur Shad Nik <[email protected]>

---------

Signed-off-by: Artur Shad Nik <[email protected]>

Author: arturshadnik

fleetconfig-controller/charts/fleetconfig-controller/templates/ocm/fcc-addon/addon-template.yaml

@@ -96,7 +96,7 @@ spec:
           verbs: ["get", "list", "watch"]
         - apiGroups: ["apps"]
           resources: ["deployments"]
-          verbs: ["get", "list", "watch"]
+          verbs: ["get", "list", "watch", "patch"]
         - apiGroups: ["rbac.authorization.k8s.io"]
           resources: ["clusterrolebindings", "clusterroles"]
           verbs: ["get", "list", "watch"]

fleetconfig-controller/internal/controller/v1beta1/hub_controller.go

@@ -482,6 +482,7 @@ func (r *HubReconciler) hubNeedsUpgrade(ctx context.Context, hub *v1beta1.Hub, o
 	if cm.Spec.WorkImagePullSpec != "" {
 		bundleSpecs = append(bundleSpecs, cm.Spec.WorkImagePullSpec)
 	}
+	// bundle version changed
 	activeBundleVersion, err := version.LowestBundleVersion(ctx, bundleSpecs)
 	if err != nil {
 		return false, fmt.Errorf("failed to detect bundleVersion from clustermanager spec: %w", err)
@@ -490,12 +491,24 @@ func (r *HubReconciler) hubNeedsUpgrade(ctx context.Context, hub *v1beta1.Hub, o
 	if err != nil {
 		return false, err
 	}
+	versionChanged := activeBundleVersion != desiredBundleVersion
+
+	// bundle source changed
+	activeBundleSource, err := version.GetBundleSource(bundleSpecs)
+	if err != nil {
+		return false, fmt.Errorf("failed to get bundle source: %w", err)
+	}
+	desiredBundleSource := hub.Spec.ClusterManager.Source.Registry
+	sourceChanged := activeBundleSource != desiredBundleSource
 
 	logger.V(0).Info("found clustermanager bundleVersions",
 		"activeBundleVersion", activeBundleVersion,
 		"desiredBundleVersion", desiredBundleVersion,
+		"activeBundleSource", activeBundleSource,
+		"desiredBundleSource", desiredBundleSource,
 	)
-	return activeBundleVersion != desiredBundleVersion, nil
+
+	return versionChanged || sourceChanged, nil
 }
 
 // upgradeHub upgrades the Hub cluster's clustermanager to the specified version

fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go

@@ -8,8 +8,10 @@ import (
 	"maps"
 	"os"
 	"os/exec"
+	"reflect"
 	"slices"
 	"strconv"
+	"strings"
 
 	"dario.cat/mergo"
 	certificatesv1 "k8s.io/api/certificates/v1"
@@ -77,13 +79,25 @@ func (r *SpokeReconciler) handleSpoke(ctx context.Context, spoke *v1beta1.Spoke,
 		return err
 	}
 
+	// to avoid conflicts between sources, always use OCMSource as the source of truth for registry and version
+	if klusterletValues != nil {
+		if hubMeta.hub != nil && hubMeta.hub.Spec.ClusterManager != nil {
+			if klusterletValues.Images.Registry != "" {
+				klusterletValues.Images.Registry = hubMeta.hub.Spec.ClusterManager.Source.Registry
+			}
+			if klusterletValues.Images.Tag != "" {
+				klusterletValues.Images.Tag = hubMeta.hub.Spec.ClusterManager.Source.BundleVersion
+			}
+		}
+	}
+
 	switch r.InstanceType {
 	case v1beta1.InstanceTypeManager:
 		err = r.doHubWork(ctx, spoke, hubMeta, klusterletValues)
 		if err != nil {
 			return err
 		}
-		if spoke.IsHubAsSpoke() { // hub-as-spoke
+		if spoke.IsHubAsSpoke() {
 			err = r.doSpokeWork(ctx, spoke, hubMeta.hub, klusterletValues)
 			if err != nil {
 				spoke.SetConditions(true, v1beta1.NewCondition(
@@ -121,6 +135,57 @@ func (r *SpokeReconciler) handleSpoke(ctx context.Context, spoke *v1beta1.Spoke,
 	}
 }
 
+// merges annotation from Spoke spec and Klusterlet values overrides. For consistency with clusteradm, priority is given to klusterlet overrides.
+// since the behaviour w.r.t prefixes of the `--klusterlet-annotation` flag, and the annotations specified in `--klusterlet-values-file` are different,
+// this function will add the prefix to both before merging.
+// the output of this function is the complete and finalized set of annotations that will be applied to the ManagedCluster
+func mergeKlusterletAnnotations(base, override map[string]string) map[string]string {
+	formattedBase := make(map[string]string, len(base))
+	for k, v := range base {
+		if !strings.HasPrefix(k, operatorv1.ClusterAnnotationsKeyPrefix) {
+			k = fmt.Sprintf("%s/%s", operatorv1.ClusterAnnotationsKeyPrefix, k)
+		}
+		formattedBase[k] = v
+	}
+	formattedOverride := make(map[string]string, len(override))
+	for k, v := range override {
+		if !strings.HasPrefix(k, operatorv1.ClusterAnnotationsKeyPrefix) {
+			k = fmt.Sprintf("%s/%s", operatorv1.ClusterAnnotationsKeyPrefix, k)
+		}
+		formattedOverride[k] = v
+	}
+	out := make(map[string]string, 0)
+	maps.Copy(out, formattedBase)
+	maps.Copy(out, formattedOverride)
+	return out
+}
+
+// syncManagedClusterAnnotations merges requested klusterlet annotations into the ManagedCluster's
+// existing annotations, preserving all non-klusterlet annotations while adding/updating/removing
+// only those with the klusterlet prefix.
+func syncManagedClusterAnnotations(current, requested map[string]string) map[string]string {
+	if current == nil {
+		current = map[string]string{}
+	}
+
+	result := maps.Clone(current)
+	prefix := operatorv1.ClusterAnnotationsKeyPrefix + "/"
+
+	// Remove klusterlet annotations that are no longer requested
+	for key := range current {
+		if strings.HasPrefix(key, prefix) {
+			if _, stillWanted := requested[key]; !stillWanted {
+				delete(result, key)
+			}
+		}
+	}
+
+	// Add or update all requested klusterlet annotations
+	maps.Copy(result, requested)
+
+	return result
+}
+
 // doHubWork handles hub-side work such as joins and addons
 func (r *SpokeReconciler) doHubWork(ctx context.Context, spoke *v1beta1.Spoke, hubMeta hubMeta, klusterletValues *v1beta1.KlusterletChartConfig) error {
 	logger := log.FromContext(ctx)
@@ -172,6 +237,23 @@ func (r *SpokeReconciler) doHubWork(ctx context.Context, spoke *v1beta1.Spoke, h
 		}
 	}
 
+	// TODO - handle this via `klusterlet upgrade` once https://github.com/open-cluster-management-io/ocm/issues/1210 is resolved
+	if managedCluster != nil {
+		klusterletValuesAnnotations := map[string]string{}
+		if klusterletValues != nil {
+			klusterletValuesAnnotations = klusterletValues.Klusterlet.RegistrationConfiguration.ClusterAnnotations
+		}
+		requestedAnnotations := mergeKlusterletAnnotations(spoke.Spec.Klusterlet.Annotations, klusterletValuesAnnotations)
+		updatedAnnotations := syncManagedClusterAnnotations(managedCluster.GetAnnotations(), requestedAnnotations)
+		if !reflect.DeepEqual(updatedAnnotations, managedCluster.GetAnnotations()) {
+			managedCluster.SetAnnotations(updatedAnnotations)
+			if err = common.UpdateManagedCluster(ctx, clusterClient, managedCluster); err != nil {
+				return err
+			}
+			logger.V(1).Info("synced annotations to ManagedCluster")
+		}
+	}
+
 	// precreate the namespace that the agent will be installed into
 	// this prevents it from being automatically garbage collected when the spoke is deregistered
 	err = r.createAgentNamespace(ctx, spoke)
@@ -346,6 +428,7 @@ func (r *SpokeReconciler) doSpokeWork(ctx context.Context, spoke *v1beta1.Spoke,
 	if err != nil {
 		return fmt.Errorf("failed to load kubeconfig from inCluster: %v", err)
 	}
+
 	// attempt an upgrade whenever the klusterlet's bundleVersion or values change
 	currKlusterletHash, err := hash.ComputeHash(klusterletValues)
 	if err != nil {
@@ -663,7 +746,6 @@ func (r *SpokeReconciler) joinSpoke(ctx context.Context, spoke *v1beta1.Spoke, h
 	for k, v := range spoke.Spec.Klusterlet.Annotations {
 		joinArgs = append(joinArgs, fmt.Sprintf("--klusterlet-annotation=%s=%s", k, v))
 	}
-
 	// resources args
 	joinArgs = append(joinArgs, arg_utils.PrepareResources(spoke.Spec.Klusterlet.Resources)...)
 
@@ -813,6 +895,7 @@ func (r *SpokeReconciler) spokeNeedsUpgrade(ctx context.Context, spoke *v1beta1.
 	logger := log.FromContext(ctx)
 	logger.V(0).Info("spokeNeedsUpgrade", "spokeClusterName", spoke.Name)
 
+	// klusterlet values hash changed
 	prevHash := spoke.Status.KlusterletHash
 	hashChanged := prevHash != currKlusterletHash && prevHash != ""
 	logger.V(2).Info("comparing klusterlet values hash",
@@ -855,6 +938,8 @@ func (r *SpokeReconciler) spokeNeedsUpgrade(ctx context.Context, spoke *v1beta1.
 	if k.Spec.WorkImagePullSpec != "" {
 		bundleSpecs = append(bundleSpecs, k.Spec.WorkImagePullSpec)
 	}
+
+	// bundle version changed
 	activeBundleVersion, err := version.LowestBundleVersion(ctx, bundleSpecs)
 	if err != nil {
 		return false, fmt.Errorf("failed to detect bundleVersion from klusterlet spec: %w", err)
@@ -863,12 +948,24 @@ func (r *SpokeReconciler) spokeNeedsUpgrade(ctx context.Context, spoke *v1beta1.
 	if err != nil {
 		return false, err
 	}
+	versionChanged := activeBundleVersion != desiredBundleVersion
+
+	// bundle source changed
+	activeBundleSource, err := version.GetBundleSource(bundleSpecs)
+	if err != nil {
+		return false, fmt.Errorf("failed to get bundle source: %w", err)
+	}
+	desiredBundleSource := source.Registry
+	sourceChanged := activeBundleSource != desiredBundleSource
 
 	logger.V(0).Info("found klusterlet bundleVersions",
 		"activeBundleVersion", activeBundleVersion,
 		"desiredBundleVersion", desiredBundleVersion,
+		"activeBundleSource", activeBundleSource,
+		"desiredBundleSource", desiredBundleSource,
 	)
-	return activeBundleVersion != desiredBundleVersion, nil
+
+	return versionChanged || sourceChanged, nil
 }
 
 // upgradeSpoke upgrades the Spoke cluster's klusterlet
@@ -1075,7 +1172,6 @@ func (r *SpokeReconciler) mergeKlusterletValues(ctx context.Context, spoke *v1be
 	}
 
 	return merged, nil
-
 }
 
 // prepareKlusterletValuesFile creates a temporary file with klusterlet values and returns

fleetconfig-controller/internal/controller/v1beta1/spoke_handler_test.go

@@ -0,0 +1,165 @@
+package v1beta1
+
+import (
+	"testing"
+
+	operatorv1 "open-cluster-management.io/api/operator/v1"
+)
+
+func TestSyncManagedClusterAnnotations(t *testing.T) {
+	const prefix = operatorv1.ClusterAnnotationsKeyPrefix + "/"
+
+	tests := []struct {
+		name      string
+		current   map[string]string
+		requested map[string]string
+		want      map[string]string
+	}{
+		{
+			name: "preserve non-klusterlet annotations",
+			current: map[string]string{
+				"other.io/annotation":     "keep-me",
+				"another.io/annotation":   "also-keep",
+				prefix + "klusterlet-key": "old-value",
+			},
+			requested: map[string]string{
+				prefix + "klusterlet-key": "new-value",
+			},
+			want: map[string]string{
+				"other.io/annotation":     "keep-me",
+				"another.io/annotation":   "also-keep",
+				prefix + "klusterlet-key": "new-value",
+			},
+		},
+		{
+			name: "add new klusterlet annotations",
+			current: map[string]string{
+				"other.io/annotation":   "keep-me",
+				prefix + "existing-key": "existing-value",
+			},
+			requested: map[string]string{
+				prefix + "existing-key": "existing-value",
+				prefix + "new-key":      "new-value",
+			},
+			want: map[string]string{
+				"other.io/annotation":   "keep-me",
+				prefix + "existing-key": "existing-value",
+				prefix + "new-key":      "new-value",
+			},
+		},
+		{
+			name: "update existing klusterlet annotations",
+			current: map[string]string{
+				"other.io/annotation": "keep-me",
+				prefix + "key1":       "old-value-1",
+				prefix + "key2":       "old-value-2",
+			},
+			requested: map[string]string{
+				prefix + "key1": "new-value-1",
+				prefix + "key2": "new-value-2",
+			},
+			want: map[string]string{
+				"other.io/annotation": "keep-me",
+				prefix + "key1":       "new-value-1",
+				prefix + "key2":       "new-value-2",
+			},
+		},
+		{
+			name: "remove klusterlet annotations that are no longer requested",
+			current: map[string]string{
+				"other.io/annotation":  "keep-me",
+				prefix + "keep-key":    "keep-value",
+				prefix + "remove-key1": "will-be-removed",
+				prefix + "remove-key2": "also-removed",
+			},
+			requested: map[string]string{
+				prefix + "keep-key": "keep-value",
+			},
+			want: map[string]string{
+				"other.io/annotation": "keep-me",
+				prefix + "keep-key":   "keep-value",
+			},
+		},
+		{
+			name:    "handle nil current annotations",
+			current: nil,
+			requested: map[string]string{
+				prefix + "new-key": "new-value",
+			},
+			want: map[string]string{
+				prefix + "new-key": "new-value",
+			},
+		},
+		{
+			name:    "handle empty current annotations",
+			current: map[string]string{},
+			requested: map[string]string{
+				prefix + "new-key": "new-value",
+			},
+			want: map[string]string{
+				prefix + "new-key": "new-value",
+			},
+		},
+		{
+			name: "handle empty requested annotations",
+			current: map[string]string{
+				"other.io/annotation":  "keep-me",
+				prefix + "remove-key1": "will-be-removed",
+				prefix + "remove-key2": "also-removed",
+			},
+			requested: map[string]string{},
+			want: map[string]string{
+				"other.io/annotation": "keep-me",
+			},
+		},
+		{
+			name: "complex scenario with add, update, remove, and preserve",
+			current: map[string]string{
+				"other.io/annotation":       "keep-me",
+				"third-party.io/annotation": "also-keep",
+				prefix + "update-me":        "old-value",
+				prefix + "keep-me":          "keep-value",
+				prefix + "remove-me":        "will-be-removed",
+			},
+			requested: map[string]string{
+				prefix + "update-me": "new-value",
+				prefix + "keep-me":   "keep-value",
+				prefix + "add-me":    "new-annotation",
+			},
+			want: map[string]string{
+				"other.io/annotation":       "keep-me",
+				"third-party.io/annotation": "also-keep",
+				prefix + "update-me":        "new-value",
+				prefix + "keep-me":          "keep-value",
+				prefix + "add-me":           "new-annotation",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := syncManagedClusterAnnotations(tt.current, tt.requested)
+
+			if len(got) != len(tt.want) {
+				t.Errorf("syncManagedClusterAnnotations() returned %d annotations, want %d", len(got), len(tt.want))
+			}
+
+			for key, wantValue := range tt.want {
+				gotValue, ok := got[key]
+				if !ok {
+					t.Errorf("syncManagedClusterAnnotations() missing key %q", key)
+					continue
+				}
+				if gotValue != wantValue {
+					t.Errorf("syncManagedClusterAnnotations() key %q = %q, want %q", key, gotValue, wantValue)
+				}
+			}
+
+			for key := range got {
+				if _, ok := tt.want[key]; !ok {
+					t.Errorf("syncManagedClusterAnnotations() has unexpected key %q with value %q", key, got[key])
+				}
+			}
+		})
+	}
+}