✨ Implement Reconcile mode for ClusterResourceSet

docs/book/src/tasks/experimental-features/cluster-resource-set.md

@@ -8,3 +8,8 @@ The `ClusterResourceSet` feature is introduced to provide a way to automatically
 
 More details on `ClusterResourceSet` and an example to test it can be found at:
 [ClusterResourceSet CAEP](https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20200220-cluster-resource-set.md)
+
+## Update from `ApplyOnce` to `Reconcile`
+
+The `strategy` field is immutable so existing CRS can't be updated directly. However, CAPI won't delete the managed resources in the target cluster when the CRS is deleted.
+So if you want to start using the `Reconcile` strategy, delete your existing CRS and create it again with the updated `strategy`.

exp/addons/api/v1beta1/clusterresourceset_types.go

@@ -46,7 +46,7 @@ type ClusterResourceSetSpec struct {
 	Resources []ResourceRef `json:"resources,omitempty"`
 
 	// Strategy is the strategy to be used during applying resources. Defaults to ApplyOnce. This field is immutable.
-	// +kubebuilder:validation:Enum=ApplyOnce
+	// +kubebuilder:validation:Enum=ApplyOnce;Reconcile
 	// +optional
 	Strategy string `json:"strategy,omitempty"`
 }
@@ -80,6 +80,9 @@ const (
 	// ClusterResourceSetStrategyApplyOnce is the default strategy a ClusterResourceSet strategy is assigned by
 	// ClusterResourceSet controller after being created if not specified by user.
 	ClusterResourceSetStrategyApplyOnce ClusterResourceSetStrategy = "ApplyOnce"
+	// ClusterResourceSetStrategyReconcile reapplies the resources managed by a ClusterResourceSet
+	// if their normalized hash changes.
+	ClusterResourceSetStrategyReconcile ClusterResourceSetStrategy = "Reconcile"
 )
 
 // SetTypedStrategy sets the Strategy field to the string representation of ClusterResourceSetStrategy.
@@ -112,6 +115,11 @@ func (m *ClusterResourceSet) SetConditions(conditions clusterv1.Conditions) {
 	m.Status.Conditions = conditions
 }
 
+// IsStrategy compares the ClusterResourceSet strategy to the given ClusterResourceSetStrategy.
+func (m *ClusterResourceSet) IsStrategy(s ClusterResourceSetStrategy) bool {
+	return m.Spec.Strategy == string(s)
+}
+
 // +kubebuilder:object:root=true
 // +kubebuilder:resource:path=clusterresourcesets,scope=Namespaced,categories=cluster-api
 // +kubebuilder:subresource:status

exp/addons/api/v1beta1/clusterresourceset_types_test.go

@@ -0,0 +1,65 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1beta1
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestClusterResourceSetIsStrategy(t *testing.T) {
+	tests := []struct {
+		name     string
+		crs      *ClusterResourceSet
+		strategy ClusterResourceSetStrategy
+		want     bool
+	}{
+		{
+			name:     "no strategy set",
+			crs:      &ClusterResourceSet{},
+			strategy: ClusterResourceSetStrategyApplyOnce,
+			want:     false,
+		},
+		{
+			name: "diff strategy",
+			crs: &ClusterResourceSet{
+				Spec: ClusterResourceSetSpec{
+					Strategy: string(ClusterResourceSetStrategyReconcile),
+				},
+			},
+			strategy: ClusterResourceSetStrategyApplyOnce,
+			want:     false,
+		},
+		{
+			name: "same strategy",
+			crs: &ClusterResourceSet{
+				Spec: ClusterResourceSetSpec{
+					Strategy: string(ClusterResourceSetStrategyReconcile),
+				},
+			},
+			strategy: ClusterResourceSetStrategyReconcile,
+			want:     true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)
+			gs.Expect(tt.crs.IsStrategy(tt.strategy)).To(Equal(tt.want))
+		})
+	}
+}

exp/addons/api/v1beta1/clusterresourcesetbinding_types.go

@@ -58,14 +58,22 @@ type ResourceSetBinding struct {
 
 // IsApplied returns true if the resource is applied to the cluster by checking the cluster's binding.
 func (r *ResourceSetBinding) IsApplied(resourceRef ResourceRef) bool {
+	resourceBinding := r.GetResourceBinding(resourceRef)
+	if resourceBinding == nil {
+		return false
+	}
+
+	return resourceBinding.Applied
+}
+
+// GetResourceBinding returns a ResourceBinding for a resource ref if present.
+func (r *ResourceSetBinding) GetResourceBinding(resourceRef ResourceRef) *ResourceBinding {
 	for _, resource := range r.Resources {
 		if reflect.DeepEqual(resource.ResourceRef, resourceRef) {
-			if resource.Applied {
-				return true
-			}
+			return &resource
 		}
 	}
-	return false
+	return nil
 }
 
 // SetBinding sets resourceBinding for a resource in resourceSetbinding either by updating the existing one or

exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go

@@ -90,6 +90,66 @@ func TestIsResourceApplied(t *testing.T) {
 	}
 }
 
+func TestResourceSetBindingGetResourceBinding(t *testing.T) {
+	resourceRefApplyFailed := ResourceRef{
+		Name: "applyFailed",
+		Kind: "Secret",
+	}
+	resourceRefApplySucceeded := ResourceRef{
+		Name: "ApplySucceeded",
+		Kind: "Secret",
+	}
+	resourceRefNotExist := ResourceRef{
+		Name: "notExist",
+		Kind: "Secret",
+	}
+
+	resourceRefApplyFailedBinding := ResourceBinding{
+		ResourceRef:     resourceRefApplyFailed,
+		Applied:         false,
+		Hash:            "",
+		LastAppliedTime: &metav1.Time{Time: time.Now().UTC()},
+	}
+	crsBinding := &ResourceSetBinding{
+		ClusterResourceSetName: "test-clusterResourceSet",
+		Resources: []ResourceBinding{
+			{
+				ResourceRef:     resourceRefApplySucceeded,
+				Applied:         true,
+				Hash:            "xyz",
+				LastAppliedTime: &metav1.Time{Time: time.Now().UTC()},
+			},
+			resourceRefApplyFailedBinding,
+		},
+	}
+
+	tests := []struct {
+		name               string
+		resourceSetBinding *ResourceSetBinding
+		resourceRef        ResourceRef
+		want               *ResourceBinding
+	}{
+		{
+			name:               "does't exist",
+			resourceSetBinding: crsBinding,
+			resourceRef:        resourceRefNotExist,
+			want:               nil,
+		},
+		{
+			name:               "does't exist",
+			resourceSetBinding: crsBinding,
+			resourceRef:        resourceRefApplyFailed,
+			want:               &resourceRefApplyFailedBinding,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)
+			gs.Expect(tt.resourceSetBinding.GetResourceBinding(tt.resourceRef)).To(Equal(tt.want))
+		})
+	}
+}
+
 func TestSetResourceBinding(t *testing.T) {
 	resourceRefApplyFailed := ResourceRef{
 		Name: "applyFailed",

exp/addons/internal/controllers/clusterresourceset_controller.go

@@ -18,9 +18,7 @@ package controllers
 
 import (
 	"context"
-	"encoding/base64"
 	"fmt"
-	"sort"
 	"time"
 
 	"github.com/pkg/errors"
@@ -51,10 +49,8 @@ import (
 	"sigs.k8s.io/cluster-api/util/predicates"
 )
 
-var (
-	// ErrSecretTypeNotSupported signals that a Secret is not supported.
-	ErrSecretTypeNotSupported = errors.New("unsupported secret type")
-)
+// ErrSecretTypeNotSupported signals that a Secret is not supported.
+var ErrSecretTypeNotSupported = errors.New("unsupported secret type")
 
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;patch
 // +kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;list;watch;patch
@@ -82,21 +78,20 @@ func (r *ClusterResourceSetReconciler) SetupWithManager(ctx context.Context, mgr
 			handler.EnqueueRequestsFromMapFunc(r.resourceToClusterResourceSet),
 			builder.OnlyMetadata,
 			builder.WithPredicates(
-				resourcepredicates.ResourceCreate(ctrl.LoggerFrom(ctx)),
+				resourcepredicates.ResourceCreateOrUpdate(ctrl.LoggerFrom(ctx)),
 			),
 		).
 		Watches(
 			&source.Kind{Type: &corev1.Secret{}},
 			handler.EnqueueRequestsFromMapFunc(r.resourceToClusterResourceSet),
 			builder.OnlyMetadata,
 			builder.WithPredicates(
-				resourcepredicates.ResourceCreate(ctrl.LoggerFrom(ctx)),
+				resourcepredicates.ResourceCreateOrUpdate(ctrl.LoggerFrom(ctx)),
 			),
 		).
 		WithOptions(options).
 		WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue)).
 		Complete(r)
-
 	if err != nil {
 		return errors.Wrap(err, "failed setting up with a controller manager")
 	}
@@ -241,6 +236,8 @@ func (r *ClusterResourceSetReconciler) getClustersByClusterResourceSetSelector(c
 // cluster's ClusterResourceSetBinding.
 // In ApplyOnce strategy, resources are applied only once to a particular cluster. ClusterResourceSetBinding is used to check if a resource is applied before.
 // It applies resources best effort and continue on scenarios like: unsupported resource types, failure during creation, missing resources.
+// In Reconcile strategy, resources are re-applied to a particular cluster when their definition changes. The hash in ClusterResourceSetBinding is used to check
+// if a resource has changed or not.
 // TODO: If a resource already exists in the cluster but not applied by ClusterResourceSet, the resource will be updated ?
 func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Context, cluster *clusterv1.Cluster, clusterResourceSet *addonsv1.ClusterResourceSet) error {
 	log := ctrl.LoggerFrom(ctx, "Cluster", klog.KObj(cluster))
@@ -301,8 +298,8 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte
 			errList = append(errList, err)
 		}
 
-		// If resource is already applied successfully and clusterResourceSet mode is "ApplyOnce", continue. (No need to check hash changes here)
-		if resourceSetBinding.IsApplied(resource) {
+		resourceScope, scopeError := reconcileScopeForResource(clusterResourceSet, resource, resourceSetBinding, unstructuredObj)
+		if scopeError == nil && !resourceScope.needsApply() {
 			continue
 		}
 
@@ -314,54 +311,25 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte
 			Applied:         false,
 			LastAppliedTime: &metav1.Time{Time: time.Now().UTC()},
 		})
-		// Since maps are not ordered, we need to order them to get the same hash at each reconcile.
-		keys := make([]string, 0)
-		data, ok := unstructuredObj.UnstructuredContent()["data"]
-		if !ok {
-			errList = append(errList, errors.New("failed to get data field from the resource"))
-			continue
-		}
-
-		unstructuredData := data.(map[string]interface{})
-		for key := range unstructuredData {
-			keys = append(keys, key)
-		}
-		sort.Strings(keys)
-
-		dataList := make([][]byte, 0)
-		for _, key := range keys {
-			val, ok, err := unstructured.NestedString(unstructuredData, key)
-			if !ok || err != nil {
-				errList = append(errList, errors.New("failed to get value field from the resource"))
-				continue
-			}
-
-			byteArr := []byte(val)
-			// If the resource is a Secret, data needs to be decoded.
-			if unstructuredObj.GetKind() == string(addonsv1.SecretClusterResourceSetResourceKind) {
-				byteArr, _ = base64.StdEncoding.DecodeString(val)
-			}
 
-			dataList = append(dataList, byteArr)
+		if scopeError != nil {
+			errList = append(errList, scopeError)
+			continue
 		}
 
 		// Apply all values in the key-value pair of the resource to the cluster.
 		// As there can be multiple key-value pairs in a resource, each value may have multiple objects in it.
 		isSuccessful := true
-		for i := range dataList {
-			data := dataList[i]
-
-			if err := apply(ctx, remoteClient, data); err != nil {
-				isSuccessful = false
-				log.Error(err, "failed to apply ClusterResourceSet resource", "Resource kind", resource.Kind, "Resource name", resource.Name)
-				conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.ApplyFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
-				errList = append(errList, err)
-			}
+		if err := apply(ctx, remoteClient, resourceScope); err != nil {
+			isSuccessful = false
+			log.Error(err, "failed to apply ClusterResourceSet resource", "Resource kind", resource.Kind, "Resource name", resource.Name)
+			conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.ApplyFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
+			errList = append(errList, err)
 		}
 
 		resourceSetBinding.SetBinding(addonsv1.ResourceBinding{
 			ResourceRef:     resource,
-			Hash:            computeHash(dataList),
+			Hash:            resourceScope.hash(),
 			Applied:         isSuccessful,
 			LastAppliedTime: &metav1.Time{Time: time.Now().UTC()},
 		})