diff --git a/pkg/hub/addon/discovery_controller.go b/pkg/hub/addon/discovery_controller.go index bee52bf01..a422f47e6 100644 --- a/pkg/hub/addon/discovery_controller.go +++ b/pkg/hub/addon/discovery_controller.go @@ -2,18 +2,20 @@ package addon import ( "context" + "encoding/json" "fmt" + "reflect" "strings" "time" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" @@ -110,48 +112,65 @@ func (c *addOnFeatureDiscoveryController) sync(ctx context.Context, syncCtx fact func (c *addOnFeatureDiscoveryController) syncAddOn(ctx context.Context, clusterName, addOnName string) error { klog.V(4).Infof("Reconciling addOn %q", addOnName) - labels := map[string]string{} + cluster, err := c.clusterLister.Get(clusterName) + if errors.IsNotFound(err) { + // no cluster, it could be deleted + return nil + } + if err != nil { + return fmt.Errorf("unable to find cluster with name %q: %w", clusterName, err) + } + // no work if cluster is deleting + if !cluster.DeletionTimestamp.IsZero() { + return nil + } + + labels := cluster.Labels + if len(labels) == 0 { + labels = map[string]string{} + } + + // make a copy to check if the cluster labels are changed + labelsCopy := map[string]string{} + for key, value := range labels { + labelsCopy[key] = value + } + addOn, err := c.addOnLister.ManagedClusterAddOns(clusterName).Get(addOnName) switch { case errors.IsNotFound(err): // addon is deleted - key := fmt.Sprintf("%s%s-", addOnFeaturePrefix, addOnName) - labels[key] = "" + key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName) + delete(labels, key) + labels[fmt.Sprintf("%s-", key)] = "" case err != nil: return err case !addOn.DeletionTimestamp.IsZero(): - key := fmt.Sprintf("%s%s-", addOnFeaturePrefix, addOnName) - labels[key] = "" + key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName) + delete(labels, key) + labels[fmt.Sprintf("%s-", key)] = "" default: key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOn.Name) labels[key] = getAddOnLabelValue(addOn) } - cluster, err := c.clusterLister.Get(clusterName) - if errors.IsNotFound(err) { - // no cluster, it could be deleted + // no work if the labels are not changed + if reflect.DeepEqual(labelsCopy, labels) { return nil } + + patchBytes, err := json.Marshal(map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": labels, + }, + }) if err != nil { - return fmt.Errorf("unable to find cluster with name %q: %w", clusterName, err) + return fmt.Errorf("failed to create patch for cluster %s: %w", cluster.Name, err) } - // no work if cluster is deleting - if !cluster.DeletionTimestamp.IsZero() { - return nil - } - - // merge labels - modified := false - cluster = cluster.DeepCopy() - resourcemerge.MergeMap(&modified, &cluster.Labels, labels) - // no work if the cluster labels have no change - if !modified { - return nil - } + // patch the cluster labels + _, err = c.clusterClient.ClusterV1().ManagedClusters().Patch(ctx, cluster.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}) - // otherwise, update cluster - _, err = c.clusterClient.ClusterV1().ManagedClusters().Update(ctx, cluster, metav1.UpdateOptions{}) return err } @@ -172,43 +191,72 @@ func (c *addOnFeatureDiscoveryController) syncCluster(ctx context.Context, clust } // build labels for existing addons - addOnLabels := map[string]string{} + addOnLabels := cluster.Labels + if len(addOnLabels) == 0 { + addOnLabels = map[string]string{} + } + + // make a copy to check if the cluster labels are changed + addOnLabelsCopy := map[string]string{} + for key, value := range addOnLabels { + addOnLabelsCopy[key] = value + } + + newAddonLabels := map[string]string{} addOns, err := c.addOnLister.ManagedClusterAddOns(clusterName).List(labels.Everything()) if err != nil { return fmt.Errorf("unable to list addOns of cluster %q: %w", clusterName, err) } + + // no work if no addons update + if len(addOnLabels) == 0 && len(addOns) == 0 { + return nil + } + for _, addOn := range addOns { + key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOn.Name) + // addon is deleting if !addOn.DeletionTimestamp.IsZero() { + delete(addOnLabels, key) + addOnLabels[fmt.Sprintf("%s-", key)] = "" continue } - key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOn.Name) + addOnLabels[key] = getAddOnLabelValue(addOn) + newAddonLabels[key] = getAddOnLabelValue(addOn) } // remove addon lable if its corresponding addon no longer exists - for key := range cluster.Labels { + for key := range addOnLabels { if !strings.HasPrefix(key, addOnFeaturePrefix) { continue } - if _, ok := addOnLabels[key]; !ok { + if _, ok := newAddonLabels[key]; !ok { + delete(addOnLabels, key) addOnLabels[fmt.Sprintf("%s-", key)] = "" } } - // merge labels - modified := false - cluster = cluster.DeepCopy() - resourcemerge.MergeMap(&modified, &cluster.Labels, addOnLabels) - - // no work if the cluster labels have no change - if !modified { + // no work if the labels are not changed + if reflect.DeepEqual(addOnLabelsCopy, addOnLabels) { return nil } - // otherwise, update cluster - _, err = c.clusterClient.ClusterV1().ManagedClusters().Update(ctx, cluster, metav1.UpdateOptions{}) + // build cluster labels patch + patchBytes, err := json.Marshal(map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": addOnLabels, + }, + }) + if err != nil { + return fmt.Errorf("failed to create patch for cluster %s: %w", cluster.Name, err) + } + + // patch the cluster labels + _, err = c.clusterClient.ClusterV1().ManagedClusters().Patch(ctx, cluster.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}) + return err } diff --git a/pkg/hub/addon/discovery_controller_test.go b/pkg/hub/addon/discovery_controller_test.go index 76e5dbcd1..7457c4a1f 100644 --- a/pkg/hub/addon/discovery_controller_test.go +++ b/pkg/hub/addon/discovery_controller_test.go @@ -2,6 +2,7 @@ package addon import ( "context" + "encoding/json" "fmt" "testing" "time" @@ -100,9 +101,9 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) { }, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { - testinghelpers.AssertActions(t, actions, "update") - actual := actions[0].(clienttesting.UpdateActionImpl).Object - assertNoAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1") + testinghelpers.AssertActions(t, actions, "patch") + actualPatchAction := actions[0].(clienttesting.PatchActionImpl) + assertPatchNoAddonLabel(t, actualPatchAction, "addon1") }, }, { @@ -116,10 +117,15 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) { addOn: &addonv1alpha1.ManagedClusterAddOn{ ObjectMeta: metav1.ObjectMeta{ Name: "addon1", + Namespace: clusterName, DeletionTimestamp: &deleteTime, }, }, - validateActions: testinghelpers.AssertNoActions, + validateActions: func(t *testing.T, actions []clienttesting.Action) { + testinghelpers.AssertActions(t, actions, "patch") + actualPatchAction := actions[0].(clienttesting.PatchActionImpl) + assertPatchNoAddonLabel(t, actualPatchAction, "addon1") + }, }, { name: "new addon is added", @@ -136,9 +142,9 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) { }, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { - testinghelpers.AssertActions(t, actions, "update") - actual := actions[0].(clienttesting.UpdateActionImpl).Object - assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable) + testinghelpers.AssertActions(t, actions, "patch") + actualPatchAction := actions[0].(clienttesting.PatchActionImpl) + assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable) }, }, { @@ -159,9 +165,9 @@ func TestDiscoveryController_SyncAddOn(t *testing.T) { }, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { - testinghelpers.AssertActions(t, actions, "update") - actual := actions[0].(clienttesting.UpdateActionImpl).Object - assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable) + testinghelpers.AssertActions(t, actions, "patch") + actualPatchAction := actions[0].(clienttesting.PatchActionImpl) + assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable) }, }, { @@ -257,9 +263,9 @@ func TestDiscoveryController_Sync(t *testing.T) { }, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { - testinghelpers.AssertActions(t, actions, "update") - actual := actions[0].(clienttesting.UpdateActionImpl).Object - assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable) + testinghelpers.AssertActions(t, actions, "patch") + actualPatchAction := actions[0].(clienttesting.PatchActionImpl) + assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable) }, }, { @@ -329,11 +335,11 @@ func TestDiscoveryController_Sync(t *testing.T) { }, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { - testinghelpers.AssertActions(t, actions, "update") - actual := actions[0].(clienttesting.UpdateActionImpl).Object - assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon1", addOnStatusUnreachable) - assertAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon3", addOnStatusAvailable) - assertNoAddonLabel(t, actual.(*clusterv1.ManagedCluster), "addon4") + testinghelpers.AssertActions(t, actions, "patch") + actualPatchAction := actions[0].(clienttesting.PatchActionImpl) + assertPatchAddonLabel(t, actualPatchAction, "addon1", addOnStatusUnreachable) + assertPatchAddonLabel(t, actualPatchAction, "addon3", addOnStatusAvailable) + assertPatchNoAddonLabel(t, actualPatchAction, "addon4") }, }, } @@ -402,3 +408,48 @@ func assertNoAddonLabel(t *testing.T, cluster *clusterv1.ManagedCluster, addOnNa t.Errorf("label %q found", key) } } + +func assertPatchAddonLabel(t *testing.T, actionPatch clienttesting.PatchActionImpl, addOnName, addOnStatus string) { + var patchObj map[string]map[string]map[string]string + if err := json.Unmarshal(actionPatch.Patch, &patchObj); err != nil { + t.Errorf("failed to unmarshal patch %s: %v", patchObj, err) + } + metadata, ok := patchObj["metadata"] + if !ok { + t.Errorf("patch %s doesn't contain metadata field", patchObj) + } + labels, ok := metadata["labels"] + if !ok { + t.Errorf("patch %s doesn't contain metadata.labels field", patchObj) + } + + key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName) + value, ok := labels[key] + if !ok { + t.Errorf("label %q not found", key) + } + + if value != addOnStatus { + t.Errorf("expect label value %q but found %q", addOnStatus, value) + } +} + +func assertPatchNoAddonLabel(t *testing.T, actionPatch clienttesting.PatchActionImpl, addOnName string) { + var patchObj map[string]map[string]map[string]string + if err := json.Unmarshal(actionPatch.Patch, &patchObj); err != nil { + t.Errorf("failed to unmarshal patch %s: %v", patchObj, err) + } + metadata, ok := patchObj["metadata"] + if !ok { + t.Errorf("patch %s doesn't contain metadata field", patchObj) + } + labels, ok := metadata["labels"] + if !ok { + t.Errorf("patch %s doesn't contain metadata.labels field", patchObj) + } + + key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName) + if _, ok := labels[key]; ok { + t.Errorf("label %q found", key) + } +}