From a6ef8dc62f530bd984bea8a241b014451f172010 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Fri, 4 Nov 2022 13:55:39 +0000 Subject: [PATCH 01/22] Implement Reconcile mode for ClusterResourceSet --- ....cluster.x-k8s.io_clusterresourcesets.yaml | 1 + .../api/v1beta1/clusterresourceset_types.go | 10 +- .../v1beta1/clusterresourceset_types_test.go | 65 ++++ .../clusterresourcesetbinding_types.go | 16 +- .../clusterresourcesetbinding_types_test.go | 60 ++++ .../clusterresourceset_controller.go | 77 ++--- .../clusterresourceset_controller_test.go | 291 +++++++++++++++--- .../controllers/clusterresourceset_helpers.go | 131 +++++--- .../controllers/clusterresourceset_scope.go | 173 +++++++++++ .../clusterresourceset_scope_test.go | 187 +++++++++++ 10 files changed, 868 insertions(+), 143 deletions(-) create mode 100644 exp/addons/api/v1beta1/clusterresourceset_types_test.go create mode 100644 exp/addons/internal/controllers/clusterresourceset_scope.go create mode 100644 exp/addons/internal/controllers/clusterresourceset_scope_test.go diff --git a/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesets.yaml b/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesets.yaml index d495f3aa2849..8080f7d8f18f 100644 --- a/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesets.yaml +++ b/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesets.yaml @@ -438,6 +438,7 @@ spec: Defaults to ApplyOnce. This field is immutable. enum: - ApplyOnce + - Reconcile type: string required: - clusterSelector diff --git a/exp/addons/api/v1beta1/clusterresourceset_types.go b/exp/addons/api/v1beta1/clusterresourceset_types.go index 77fd7dcc5eaf..4b2c774ea0af 100644 --- a/exp/addons/api/v1beta1/clusterresourceset_types.go +++ b/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 normalize 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 diff --git a/exp/addons/api/v1beta1/clusterresourceset_types_test.go b/exp/addons/api/v1beta1/clusterresourceset_types_test.go new file mode 100644 index 000000000000..c46ce58bbc64 --- /dev/null +++ b/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)) + }) + } +} diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go index 8a88cff993c6..b7839b15b54c 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go +++ b/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 diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go index 5c59e40f5127..2e71c3b7b8f2 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go +++ b/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", diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 513609224fad..387a47fc272c 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -18,9 +18,7 @@ package controllers import ( "context" - "encoding/base64" "fmt" - "sort" "time" "github.com/pkg/errors" @@ -38,23 +36,22 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/source" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/remote" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" - resourcepredicates "sigs.k8s.io/cluster-api/exp/addons/internal/controllers/predicates" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" "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,7 +79,12 @@ func (r *ClusterResourceSetReconciler) SetupWithManager(ctx context.Context, mgr handler.EnqueueRequestsFromMapFunc(r.resourceToClusterResourceSet), builder.OnlyMetadata, builder.WithPredicates( - resourcepredicates.ResourceCreate(ctrl.LoggerFrom(ctx)), + predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { return true }, + UpdateFunc: func(e event.UpdateEvent) bool { return true }, + DeleteFunc: func(e event.DeleteEvent) bool { return false }, + GenericFunc: func(e event.GenericEvent) bool { return false }, + }, ), ). Watches( @@ -90,13 +92,17 @@ func (r *ClusterResourceSetReconciler) SetupWithManager(ctx context.Context, mgr handler.EnqueueRequestsFromMapFunc(r.resourceToClusterResourceSet), builder.OnlyMetadata, builder.WithPredicates( - resourcepredicates.ResourceCreate(ctrl.LoggerFrom(ctx)), + predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { return true }, + UpdateFunc: func(e event.UpdateEvent) bool { return true }, + DeleteFunc: func(e event.DeleteEvent) bool { return false }, + GenericFunc: func(e event.GenericEvent) bool { return false }, + }, ), ). 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") } @@ -301,8 +307,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 +320,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()}, }) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index 3a14392ae841..c50f9c9cf097 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -18,6 +18,7 @@ package controllers import ( "fmt" + "reflect" "testing" "time" @@ -27,9 +28,11 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/yaml" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/test/envtest" "sigs.k8s.io/cluster-api/util" ) @@ -39,13 +42,16 @@ const ( func TestClusterResourceSetReconciler(t *testing.T) { var ( - clusterResourceSetName string - testCluster *clusterv1.Cluster - clusterName string - labels map[string]string - configmapName = "test-configmap" - secretName = "test-secret" - namespacePrefix = "test-cluster-resource-set" + clusterResourceSetName string + testCluster *clusterv1.Cluster + clusterName string + labels map[string]string + configmapName = "test-configmap" + secretName = "test-secret" + namespacePrefix = "test-cluster-resource-set" + resourceConfigMap1Name = "resource-configmap" + resourceConfigMap2Name = "resource-configmap-2" + resourceConfigMapsNamespace = "default" ) setup := func(t *testing.T, g *WithT) *corev1.Namespace { @@ -64,19 +70,29 @@ func TestClusterResourceSetReconciler(t *testing.T) { g.Expect(env.Create(ctx, testCluster)).To(Succeed()) t.Log("Creating the remote Cluster kubeconfig") g.Expect(env.CreateKubeconfigSecret(ctx, testCluster)).To(Succeed()) + + resourceConfigMap1Content := fmt.Sprintf(`metadata: + name: %s + namespace: %s +kind: ConfigMap +apiVersion: v1`, resourceConfigMap1Name, resourceConfigMapsNamespace) + testConfigmap := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: configmapName, Namespace: ns.Name, }, Data: map[string]string{ - "cm": `metadata: - name: resource-configmap - namespace: default -kind: ConfigMap -apiVersion: v1`, + "cm": resourceConfigMap1Content, }, } + + resourceConfigMap2Content := fmt.Sprintf(`metadata: +kind: ConfigMap +apiVersion: v1 +metadata: + name: %s + namespace: %s`, resourceConfigMap2Name, resourceConfigMapsNamespace) testSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, @@ -84,12 +100,7 @@ apiVersion: v1`, }, Type: "addons.cluster.x-k8s.io/resource-set", StringData: map[string]string{ - "cm": `metadata: -kind: ConfigMap -apiVersion: v1 -metadata: - name: resource-configmap - namespace: default`, + "cm": resourceConfigMap2Content, }, } t.Log("Creating a Secret and a ConfigMap with ConfigMap in their data field") @@ -141,7 +152,32 @@ metadata: Name: secretName, Namespace: ns.Name, }})).To(Succeed()) + + cm1 := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: resourceConfigMap1Name, + Namespace: resourceConfigMapsNamespace, + }, + } + if err = env.Get(ctx, client.ObjectKeyFromObject(cm1), cm1); err == nil { + g.Expect(env.Delete(ctx, cm1)).To(Succeed()) + } + cm2 := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: resourceConfigMap2Name, + Namespace: resourceConfigMapsNamespace, + }, + } + if err = env.Get(ctx, client.ObjectKeyFromObject(cm2), cm2); err == nil { + g.Expect(env.Delete(ctx, cm2)).To(Succeed()) + } + g.Expect(env.Delete(ctx, ns)).To(Succeed()) + + clusterKey := client.ObjectKey{Namespace: testCluster.Namespace, Name: testCluster.Name} + if err = env.Get(ctx, clusterKey, testCluster); err == nil { + g.Expect(env.Delete(ctx, testCluster)).To(Succeed()) + } } t.Run("Should reconcile a ClusterResourceSet with multiple resources when a cluster with matching label exists", func(t *testing.T) { @@ -170,35 +206,7 @@ metadata: g.Expect(env.Create(ctx, clusterResourceSetInstance)).To(Succeed()) t.Log("Verifying ClusterResourceSetBinding is created with cluster owner reference") - g.Eventually(func() bool { - binding := &addonsv1.ClusterResourceSetBinding{} - clusterResourceSetBindingKey := client.ObjectKey{ - Namespace: testCluster.Namespace, - Name: testCluster.Name, - } - err := env.Get(ctx, clusterResourceSetBindingKey, binding) - if err != nil { - return false - } - - if len(binding.Spec.Bindings) != 1 { - return false - } - if len(binding.Spec.Bindings[0].Resources) != 2 { - return false - } - - if !binding.Spec.Bindings[0].Resources[0].Applied || !binding.Spec.Bindings[0].Resources[1].Applied { - return false - } - - return util.HasOwnerRef(binding.GetOwnerReferences(), metav1.OwnerReference{ - APIVersion: clusterv1.GroupVersion.String(), - Kind: "Cluster", - Name: testCluster.Name, - UID: testCluster.UID, - }) - }, timeout).Should(BeTrue()) + g.Eventually(clusterResourceSetBindingReady(env, testCluster), timeout).Should(BeTrue()) t.Log("Deleting the Cluster") g.Expect(env.Delete(ctx, testCluster)).To(Succeed()) }) @@ -588,4 +596,193 @@ metadata: return false }, timeout).Should(BeTrue()) }) + + t.Run("Should reconcile a ClusterResourceSet with Reconcile strategy after the resources have already been created", func(t *testing.T) { + g := NewWithT(t) + ns := setup(t, g) + defer teardown(t, g, ns) + + t.Log("Updating the cluster with labels") + testCluster.SetLabels(labels) + g.Expect(env.Update(ctx, testCluster)).To(Succeed()) + + t.Log("Creating a ClusterResourceSet instance that has same labels as selector") + clusterResourceSet := &addonsv1.ClusterResourceSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterResourceSetName, + Namespace: ns.Name, + }, + Spec: addonsv1.ClusterResourceSetSpec{ + Strategy: string(addonsv1.ClusterResourceSetStrategyReconcile), + ClusterSelector: metav1.LabelSelector{ + MatchLabels: labels, + }, + Resources: []addonsv1.ResourceRef{{Name: configmapName, Kind: "ConfigMap"}, {Name: secretName, Kind: "Secret"}}, + }, + } + + g.Expect(env.Create(ctx, clusterResourceSet)).To(Succeed()) + + t.Log("Verifying ClusterResourceSetBinding is created with cluster owner reference") + clusterResourceSetBindingKey := client.ObjectKey{ + Namespace: testCluster.Namespace, + Name: testCluster.Name, + } + g.Eventually(clusterResourceSetBindingReady(env, testCluster), timeout).Should(BeTrue()) + + binding := &addonsv1.ClusterResourceSetBinding{} + err := env.Get(ctx, clusterResourceSetBindingKey, binding) + g.Expect(err).NotTo(HaveOccurred()) + resourceHashes := map[string]string{} + for _, r := range binding.Spec.Bindings[0].Resources { + resourceHashes[r.Name] = r.Hash + } + + t.Log("Verifying resource ConfigMap 1 has been created") + resourceConfigMap1Key := client.ObjectKey{ + Namespace: resourceConfigMapsNamespace, + Name: resourceConfigMap1Name, + } + g.Eventually(func() error { + cm := &corev1.ConfigMap{} + return env.Get(ctx, resourceConfigMap1Key, cm) + }, timeout).Should(Succeed()) + + t.Log("Verifying resource ConfigMap 2 has been created") + resourceConfigMap2Key := client.ObjectKey{ + Namespace: resourceConfigMapsNamespace, + Name: resourceConfigMap2Name, + } + g.Eventually(func() error { + cm := &corev1.ConfigMap{} + return env.Get(ctx, resourceConfigMap2Key, cm) + }, timeout).Should(Succeed()) + + resourceConfigMap1 := configMap( + resourceConfigMap1Name, + resourceConfigMapsNamespace, + map[string]string{ + "my_new_config": "some_value", + }, + ) + + resourceConfigMap1Content, err := yaml.Marshal(resourceConfigMap1) + g.Expect(err).NotTo(HaveOccurred()) + + testConfigmap := configMap( + configmapName, + ns.Name, + map[string]string{ + "cm": string(resourceConfigMap1Content), + }, + ) + + resourceConfigMap2 := configMap( + resourceConfigMap2Name, + resourceConfigMapsNamespace, + map[string]string{ + "my_new_secret_config": "some_secret_value", + }, + ) + + resourceConfigMap2Content, err := yaml.Marshal(resourceConfigMap2) + g.Expect(err).NotTo(HaveOccurred()) + + testSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: ns.Name, + }, + Type: "addons.cluster.x-k8s.io/resource-set", + StringData: map[string]string{ + "cm": string(resourceConfigMap2Content), + }, + } + t.Log("Updating the Secret and a ConfigMap with updated ConfigMaps in their data field") + g.Expect(env.Update(ctx, testConfigmap)).To(Succeed()) + g.Expect(env.Update(ctx, testSecret)).To(Succeed()) + + t.Log("Verifying ClusterResourceSetBinding has been updated with new hashes") + g.Eventually(func() error { + binding := &addonsv1.ClusterResourceSetBinding{} + if err := env.Get(ctx, clusterResourceSetBindingKey, binding); err != nil { + return err + } + + for _, r := range binding.Spec.Bindings[0].Resources { + if resourceHashes[r.Name] == r.Hash { + return errors.Errorf("resource binding for %s hasn't been updated with new hash", r.Name) + } + } + + return nil + }, timeout).Should(Succeed()) + + t.Log("Checking resource ConfigMap 1 has been updated") + g.Eventually(configMapHasBeenUpdated(env, resourceConfigMap1Key, resourceConfigMap1), timeout).Should(Succeed()) + + t.Log("Checking resource ConfigMap 2 has been updated") + g.Eventually(configMapHasBeenUpdated(env, resourceConfigMap2Key, resourceConfigMap2), timeout).Should(Succeed()) + }) +} + +func clusterResourceSetBindingReady(env *envtest.Environment, cluster *clusterv1.Cluster) func() bool { + return func() bool { + clusterResourceSetBindingKey := client.ObjectKey{ + Namespace: cluster.Namespace, + Name: cluster.Name, + } + binding := &addonsv1.ClusterResourceSetBinding{} + err := env.Get(ctx, clusterResourceSetBindingKey, binding) + if err != nil { + return false + } + + if len(binding.Spec.Bindings) != 1 { + return false + } + if len(binding.Spec.Bindings[0].Resources) != 2 { + return false + } + + if !binding.Spec.Bindings[0].Resources[0].Applied || !binding.Spec.Bindings[0].Resources[1].Applied { + return false + } + + return util.HasOwnerRef(binding.GetOwnerReferences(), metav1.OwnerReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: cluster.Name, + UID: cluster.UID, + }) + } +} + +func configMapHasBeenUpdated(env *envtest.Environment, key client.ObjectKey, newState *corev1.ConfigMap) func() error { + return func() error { + cm := &corev1.ConfigMap{} + if err := env.Get(ctx, key, cm); err != nil { + return err + } + + if !reflect.DeepEqual(cm.Data, newState.Data) { + return errors.Errorf("configMap %s hasn't been updated yet", key.Name) + } + + return nil + } +} + +func configMap(name, namespace string, data map[string]string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Data: data, + } } diff --git a/exp/addons/internal/controllers/clusterresourceset_helpers.go b/exp/addons/internal/controllers/clusterresourceset_helpers.go index d077816d06a5..d8da5716b290 100644 --- a/exp/addons/internal/controllers/clusterresourceset_helpers.go +++ b/exp/addons/internal/controllers/clusterresourceset_helpers.go @@ -21,8 +21,10 @@ import ( "bytes" "context" "crypto/sha256" + "encoding/base64" "encoding/json" "fmt" + "sort" "unicode" "github.com/pkg/errors" @@ -43,6 +45,42 @@ import ( var jsonListPrefix = []byte("[") +// objsFromYamlData parses a collection of yaml documents into Unstructured objects. +// The returned objects are sorted for creation priority within the objects defined +// in the same document. The flattening of the documents preserves the original order. +func objsFromYamlData(yamlDocs [][]byte) ([]unstructured.Unstructured, error) { + allObjs := []unstructured.Unstructured{} + for _, data := range yamlDocs { + isJSONList, err := isJSONList(data) + if err != nil { + return nil, err + } + objs := []unstructured.Unstructured{} + // If it is a json list, convert each list element to an unstructured object. + if isJSONList { + var results []map[string]interface{} + // Unmarshal the JSON to the interface. + if err = json.Unmarshal(data, &results); err == nil { + for i := range results { + var u unstructured.Unstructured + u.SetUnstructuredContent(results[i]) + objs = append(objs, u) + } + } + } else { + // If it is not a json list, data is either json or yaml format. + objs, err = utilyaml.ToUnstructured(data) + if err != nil { + return nil, errors.Wrapf(err, "failed converting data to unstructured objects") + } + } + + allObjs = append(allObjs, utilresource.SortForCreate(objs)...) + } + + return allObjs, nil +} + // isJSONList returns whether the data is in JSON list format. func isJSONList(data []byte) (bool, error) { const peekSize = 32 @@ -55,56 +93,30 @@ func isJSONList(data []byte) (bool, error) { return bytes.HasPrefix(trim, jsonListPrefix), nil } -func apply(ctx context.Context, c client.Client, data []byte) error { - isJSONList, err := isJSONList(data) - if err != nil { - return err - } - objs := []unstructured.Unstructured{} - // If it is a json list, convert each list element to an unstructured object. - if isJSONList { - var results []map[string]interface{} - // Unmarshal the JSON to the interface. - if err = json.Unmarshal(data, &results); err == nil { - for i := range results { - var u unstructured.Unstructured - u.SetUnstructuredContent(results[i]) - objs = append(objs, u) - } - } - } else { - // If it is not a json list, data is either json or yaml format. - objs, err = utilyaml.ToUnstructured(data) - if err != nil { - return errors.Wrapf(err, "failed converting data to unstructured objects") - } - } - +// apply reconciles all objects defined by a resource from a ClusterResourceSet +// delegating on the reconcileScope for the applying strategy. +func apply(ctx context.Context, c client.Client, scope resourceReconcileScope) error { errList := []error{} - sortedObjs := utilresource.SortForCreate(objs) - for i := range sortedObjs { - if err := applyUnstructured(ctx, c, &sortedObjs[i]); err != nil { + objs := scope.objs() + for i := range objs { + if err := scope.apply(ctx, c, &objs[i]); err != nil { errList = append(errList, err) } } return kerrors.NewAggregate(errList) } -func applyUnstructured(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { - // Create the object on the API server. - // TODO: Errors are only logged. If needed, exponential backoff or requeuing could be used here for remedying connection glitches etc. +func createUnstructured(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { if err := c.Create(ctx, obj); err != nil { - // The create call is idempotent, so if the object already exists - // then do not consider it to be an error. - if !apierrors.IsAlreadyExists(err) { - return errors.Wrapf( - err, - "failed to create object %s %s/%s", - obj.GroupVersionKind(), - obj.GetNamespace(), - obj.GetName()) - } + return errors.Wrapf( + err, + "creating object %s %s/%s", + obj.GroupVersionKind(), + obj.GetNamespace(), + obj.GetName(), + ) } + return nil } @@ -196,3 +208,40 @@ func computeHash(dataArr [][]byte) string { } return fmt.Sprintf("sha256:%x", hash.Sum(nil)) } + +// normalizeData reads content of the resource (configmap or secret) and returns +// them serialized with constant order. Secret's data is base64 decoded. +// This is useful to achieve consistent data between runs, since the content +// of the data field is a mapp and its order is non deterministic. +func normalizeData(resource *unstructured.Unstructured) ([][]byte, error) { + // Since maps are not ordered, we need to order them to get the same hash at each reconcile. + keys := make([]string, 0) + data, ok := resource.UnstructuredContent()["data"] + if !ok { + return nil, errors.New("failed to get data field from the resource") + } + + 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 { + return nil, errors.New("failed to get value field from the resource") + } + + byteArr := []byte(val) + // If the resource is a Secret, data needs to be decoded. + if resource.GetKind() == string(addonsv1.SecretClusterResourceSetResourceKind) { + byteArr, _ = base64.StdEncoding.DecodeString(val) + } + + dataList = append(dataList, byteArr) + } + + return dataList, nil +} diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go new file mode 100644 index 000000000000..3cf15d1516c8 --- /dev/null +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -0,0 +1,173 @@ +/* +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 controllers + +import ( + "context" + + "github.com/pkg/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" + + addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" +) + +// resourceReconcileScope contains the scope for a CRS's resource +// reconciliation request. +type resourceReconcileScope interface { + // needsApply determines if a resource needs to be applied to the target cluster + // based on the strategy. + needsApply() bool + // apply reconciles the resource to the target cluster following a different process + // depending on the strategy. + apply(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error + // objs returns all the Kubernetes objects defined in the resource. + objs() []unstructured.Unstructured + // hash returns a computed hash of the defined objects in the resource. It is consistent + // between runs. + hash() string +} + +func reconcileScopeForResource( + crs *addonsv1.ClusterResourceSet, + resourceRef addonsv1.ResourceRef, + resourceSetBinding *addonsv1.ResourceSetBinding, + resource *unstructured.Unstructured, +) (resourceReconcileScope, error) { + normalizedData, err := normalizeData(resource) + if err != nil { + return nil, err + } + + objs, err := objsFromYamlData(normalizedData) + if err != nil { + return nil, err + } + + return newResourceReconcileScope(crs, resourceRef, resourceSetBinding, normalizedData, objs), nil +} + +func newResourceReconcileScope( + clusterResourceSet *addonsv1.ClusterResourceSet, + resourceRef addonsv1.ResourceRef, + resourceSetBinding *addonsv1.ResourceSetBinding, + normalizedData [][]byte, + objs []unstructured.Unstructured, +) resourceReconcileScope { + if clusterResourceSet.IsStrategy(addonsv1.ClusterResourceSetStrategyApplyOnce) { + return &reconcileApplyOnceScope{ + baseResourceReconcileScope{ + clusterResourceSet: clusterResourceSet, + resourceRef: resourceRef, + resourceSetBinding: resourceSetBinding, + data: normalizedData, + normalizedObjs: objs, + computedHash: computeHash(normalizedData), + }, + } + } else if clusterResourceSet.IsStrategy(addonsv1.ClusterResourceSetStrategyReconcile) { + return &reconcileStrategyScope{ + baseResourceReconcileScope{ + clusterResourceSet: clusterResourceSet, + resourceRef: resourceRef, + resourceSetBinding: resourceSetBinding, + data: normalizedData, + normalizedObjs: objs, + computedHash: computeHash(normalizedData), + }, + } + } + + return nil +} + +type baseResourceReconcileScope struct { + clusterResourceSet *addonsv1.ClusterResourceSet + resourceRef addonsv1.ResourceRef + resourceSetBinding *addonsv1.ResourceSetBinding + normalizedObjs []unstructured.Unstructured + data [][]byte + computedHash string +} + +func (b baseResourceReconcileScope) objs() []unstructured.Unstructured { + return b.normalizedObjs +} + +func (b baseResourceReconcileScope) hash() string { + return b.computedHash +} + +type reconcileStrategyScope struct { + baseResourceReconcileScope +} + +func (r *reconcileStrategyScope) needsApply() bool { + resourceBinding := r.resourceSetBinding.GetResourceBinding(r.resourceRef) + + return resourceBinding == nil || !resourceBinding.Applied || resourceBinding.Hash != r.computedHash +} + +func (r *reconcileStrategyScope) apply(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { + currentObj := &unstructured.Unstructured{} + currentObj.SetAPIVersion(obj.GetAPIVersion()) + currentObj.SetKind(obj.GetKind()) + err := c.Get(ctx, client.ObjectKeyFromObject(obj), currentObj) + if apierrors.IsNotFound(err) { + return createUnstructured(ctx, c, obj) + } + if err != nil { + return errors.Wrapf( + err, + "reading object %s %s/%s", + obj.GroupVersionKind(), + obj.GetNamespace(), + obj.GetName(), + ) + } + + patch := client.MergeFrom(currentObj.DeepCopy()) + if err = c.Patch(ctx, obj, patch); err != nil { + return errors.Wrapf( + err, + "patching object %s %s/%s", + obj.GroupVersionKind(), + obj.GetNamespace(), + obj.GetName(), + ) + } + + return nil +} + +type reconcileApplyOnceScope struct { + baseResourceReconcileScope +} + +func (r *reconcileApplyOnceScope) needsApply() bool { + return !r.resourceSetBinding.IsApplied(r.resourceRef) +} + +func (r *reconcileApplyOnceScope) apply(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { + // The create call is idempotent, so if the object already exists + // then do not consider it to be an error. + if err := createUnstructured(ctx, c, obj); !apierrors.IsAlreadyExists(err) { + return err + } + return nil +} diff --git a/exp/addons/internal/controllers/clusterresourceset_scope_test.go b/exp/addons/internal/controllers/clusterresourceset_scope_test.go new file mode 100644 index 000000000000..5f495cf6dd90 --- /dev/null +++ b/exp/addons/internal/controllers/clusterresourceset_scope_test.go @@ -0,0 +1,187 @@ +/* +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 controllers + +import ( + "testing" + + . "github.com/onsi/gomega" + + addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" +) + +func TestReconcileStrategyScopeNeedsApply(t *testing.T) { + tests := []struct { + name string + scope *reconcileStrategyScope + want bool + }{ + { + name: "not binding", + scope: &reconcileStrategyScope{ + baseResourceReconcileScope: baseResourceReconcileScope{ + resourceSetBinding: &addonsv1.ResourceSetBinding{}, + resourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + }, + }, + want: true, + }, + { + name: "not applied binding", + scope: &reconcileStrategyScope{ + baseResourceReconcileScope: baseResourceReconcileScope{ + resourceSetBinding: &addonsv1.ResourceSetBinding{ + Resources: []addonsv1.ResourceBinding{ + { + ResourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + Applied: false, + }, + }, + }, + resourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + }, + }, + want: true, + }, + { + name: "applied binding and different hash", + scope: &reconcileStrategyScope{ + baseResourceReconcileScope: baseResourceReconcileScope{ + resourceSetBinding: &addonsv1.ResourceSetBinding{ + Resources: []addonsv1.ResourceBinding{ + { + ResourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + Applied: true, + Hash: "111", + }, + }, + }, + resourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + computedHash: "222", + }, + }, + want: true, + }, + { + name: "applied binding and same hash", + scope: &reconcileStrategyScope{ + baseResourceReconcileScope: baseResourceReconcileScope{ + resourceSetBinding: &addonsv1.ResourceSetBinding{ + Resources: []addonsv1.ResourceBinding{ + { + ResourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + Applied: true, + Hash: "111", + }, + }, + }, + resourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + computedHash: "111", + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gs := NewWithT(t) + gs.Expect(tt.scope.needsApply()).To(Equal(tt.want)) + }) + } +} + +func TestReconcileApplyOnceScopeNeedsApply(t *testing.T) { + tests := []struct { + name string + scope *reconcileApplyOnceScope + want bool + }{ + { + name: "not applied binding", + scope: &reconcileApplyOnceScope{ + baseResourceReconcileScope: baseResourceReconcileScope{ + resourceSetBinding: &addonsv1.ResourceSetBinding{ + Resources: []addonsv1.ResourceBinding{ + { + ResourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + Applied: false, + }, + }, + }, + resourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + }, + }, + want: true, + }, + { + name: "applied binding", + scope: &reconcileApplyOnceScope{ + baseResourceReconcileScope: baseResourceReconcileScope{ + resourceSetBinding: &addonsv1.ResourceSetBinding{ + Resources: []addonsv1.ResourceBinding{ + { + ResourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + Applied: true, + }, + }, + }, + resourceRef: addonsv1.ResourceRef{ + Name: "cp", + Kind: "ConfigMap", + }, + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gs := NewWithT(t) + gs.Expect(tt.scope.needsApply()).To(Equal(tt.want)) + }) + } +} From 4e02c8f523408d774a66ef279c06e60ddbad8d89 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Tue, 8 Nov 2022 22:53:41 +0000 Subject: [PATCH 02/22] Update ApplyClusterResourceSet doc comment with Reconcile strategy --- .../internal/controllers/clusterresourceset_controller.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 387a47fc272c..eb5e2c5f6111 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -247,6 +247,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)) From 5f62bc7a778d976e28f095005b8c764dd2535d73 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Tue, 15 Nov 2022 20:04:13 +0000 Subject: [PATCH 03/22] Added strategy update instructions to docs --- .../src/tasks/experimental-features/cluster-resource-set.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/book/src/tasks/experimental-features/cluster-resource-set.md b/docs/book/src/tasks/experimental-features/cluster-resource-set.md index 6b40739c377f..ec73215676ba 100644 --- a/docs/book/src/tasks/experimental-features/cluster-resource-set.md +++ b/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`. From b70f64929e0af424dd2174b4a98029509b1f01ac Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Wed, 16 Nov 2022 20:41:45 -0600 Subject: [PATCH 04/22] Fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> --- exp/addons/api/v1beta1/clusterresourceset_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exp/addons/api/v1beta1/clusterresourceset_types.go b/exp/addons/api/v1beta1/clusterresourceset_types.go index 4b2c774ea0af..c5d41ba476bd 100644 --- a/exp/addons/api/v1beta1/clusterresourceset_types.go +++ b/exp/addons/api/v1beta1/clusterresourceset_types.go @@ -81,7 +81,7 @@ const ( // ClusterResourceSet controller after being created if not specified by user. ClusterResourceSetStrategyApplyOnce ClusterResourceSetStrategy = "ApplyOnce" // ClusterResourceSetStrategyReconcile reapplies the resources managed by a ClusterResourceSet - // if their normalize hash changes. + // if their normalized hash changes. ClusterResourceSetStrategyReconcile ClusterResourceSetStrategy = "Reconcile" ) From 0c5ee40b4cabed04955677af8abbfcff44c0c473 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Thu, 17 Nov 2022 03:28:24 +0000 Subject: [PATCH 05/22] Use ResourceCreateOrUpdate to dry naked predicates --- .../clusterresourceset_controller.go | 17 +++-------------- .../predicates/resource_predicates.go | 6 +++--- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index eb5e2c5f6111..49cf6080bff2 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -36,14 +36,13 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/source" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/remote" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" + resourcepredicates "sigs.k8s.io/cluster-api/exp/addons/internal/controllers/predicates" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" @@ -79,12 +78,7 @@ func (r *ClusterResourceSetReconciler) SetupWithManager(ctx context.Context, mgr handler.EnqueueRequestsFromMapFunc(r.resourceToClusterResourceSet), builder.OnlyMetadata, builder.WithPredicates( - predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { return true }, - UpdateFunc: func(e event.UpdateEvent) bool { return true }, - DeleteFunc: func(e event.DeleteEvent) bool { return false }, - GenericFunc: func(e event.GenericEvent) bool { return false }, - }, + resourcepredicates.ResourceCreateOrUpdate(ctrl.LoggerFrom(ctx)), ), ). Watches( @@ -92,12 +86,7 @@ func (r *ClusterResourceSetReconciler) SetupWithManager(ctx context.Context, mgr handler.EnqueueRequestsFromMapFunc(r.resourceToClusterResourceSet), builder.OnlyMetadata, builder.WithPredicates( - predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { return true }, - UpdateFunc: func(e event.UpdateEvent) bool { return true }, - DeleteFunc: func(e event.DeleteEvent) bool { return false }, - GenericFunc: func(e event.GenericEvent) bool { return false }, - }, + resourcepredicates.ResourceCreateOrUpdate(ctrl.LoggerFrom(ctx)), ), ). WithOptions(options). diff --git a/exp/addons/internal/controllers/predicates/resource_predicates.go b/exp/addons/internal/controllers/predicates/resource_predicates.go index 7db6a76b77a5..bc12a340a409 100644 --- a/exp/addons/internal/controllers/predicates/resource_predicates.go +++ b/exp/addons/internal/controllers/predicates/resource_predicates.go @@ -23,11 +23,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" ) -// ResourceCreate returns a predicate that returns true for a create event. -func ResourceCreate(_ logr.Logger) predicate.Funcs { +// ResourceCreateOrUpdate returns a predicate that returns true for create and update events. +func ResourceCreateOrUpdate(_ logr.Logger) predicate.Funcs { return predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { return true }, - UpdateFunc: func(e event.UpdateEvent) bool { return false }, + UpdateFunc: func(e event.UpdateEvent) bool { return true }, DeleteFunc: func(e event.DeleteEvent) bool { return false }, GenericFunc: func(e event.GenericEvent) bool { return false }, } From 9586cb53c15edfa39e37736945811948274ff89a Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 5 Dec 2022 17:35:07 -0600 Subject: [PATCH 06/22] Update exp/addons/internal/controllers/clusterresourceset_controller.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> --- .../internal/controllers/clusterresourceset_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 49cf6080bff2..1ac95879c742 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -320,7 +320,7 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte // 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 - if err = apply(ctx, remoteClient, resourceScope); err != nil { + 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()) From b0c73370c7d0a3269e595f3595813e35321eeacb Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 5 Dec 2022 23:49:36 +0000 Subject: [PATCH 07/22] Addressed review comment: minor cleanup --- .../controllers/clusterresourceset_scope.go | 30 +++++++------------ .../clusterresourceset_scope_test.go | 12 ++++---- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index 3cf15d1516c8..eea8d23d7079 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -69,28 +69,18 @@ func newResourceReconcileScope( normalizedData [][]byte, objs []unstructured.Unstructured, ) resourceReconcileScope { + base := baseResourceReconcileScope{ + clusterResourceSet: clusterResourceSet, + resourceRef: resourceRef, + resourceSetBinding: resourceSetBinding, + data: normalizedData, + normalizedObjs: objs, + computedHash: computeHash(normalizedData), + } if clusterResourceSet.IsStrategy(addonsv1.ClusterResourceSetStrategyApplyOnce) { - return &reconcileApplyOnceScope{ - baseResourceReconcileScope{ - clusterResourceSet: clusterResourceSet, - resourceRef: resourceRef, - resourceSetBinding: resourceSetBinding, - data: normalizedData, - normalizedObjs: objs, - computedHash: computeHash(normalizedData), - }, - } + return &reconcileApplyOnceScope{base} } else if clusterResourceSet.IsStrategy(addonsv1.ClusterResourceSetStrategyReconcile) { - return &reconcileStrategyScope{ - baseResourceReconcileScope{ - clusterResourceSet: clusterResourceSet, - resourceRef: resourceRef, - resourceSetBinding: resourceSetBinding, - data: normalizedData, - normalizedObjs: objs, - computedHash: computeHash(normalizedData), - }, - } + return &reconcileStrategyScope{base} } return nil diff --git a/exp/addons/internal/controllers/clusterresourceset_scope_test.go b/exp/addons/internal/controllers/clusterresourceset_scope_test.go index 5f495cf6dd90..081bc93fdb59 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope_test.go @@ -31,7 +31,7 @@ func TestReconcileStrategyScopeNeedsApply(t *testing.T) { want bool }{ { - name: "not binding", + name: "no ResourceBinding", scope: &reconcileStrategyScope{ baseResourceReconcileScope: baseResourceReconcileScope{ resourceSetBinding: &addonsv1.ResourceSetBinding{}, @@ -44,7 +44,7 @@ func TestReconcileStrategyScopeNeedsApply(t *testing.T) { want: true, }, { - name: "not applied binding", + name: "not applied ResourceBinding", scope: &reconcileStrategyScope{ baseResourceReconcileScope: baseResourceReconcileScope{ resourceSetBinding: &addonsv1.ResourceSetBinding{ @@ -67,7 +67,7 @@ func TestReconcileStrategyScopeNeedsApply(t *testing.T) { want: true, }, { - name: "applied binding and different hash", + name: "applied ResourceBinding and different hash", scope: &reconcileStrategyScope{ baseResourceReconcileScope: baseResourceReconcileScope{ resourceSetBinding: &addonsv1.ResourceSetBinding{ @@ -92,7 +92,7 @@ func TestReconcileStrategyScopeNeedsApply(t *testing.T) { want: true, }, { - name: "applied binding and same hash", + name: "applied ResourceBinding and same hash", scope: &reconcileStrategyScope{ baseResourceReconcileScope: baseResourceReconcileScope{ resourceSetBinding: &addonsv1.ResourceSetBinding{ @@ -132,7 +132,7 @@ func TestReconcileApplyOnceScopeNeedsApply(t *testing.T) { want bool }{ { - name: "not applied binding", + name: "not applied ResourceBinding", scope: &reconcileApplyOnceScope{ baseResourceReconcileScope: baseResourceReconcileScope{ resourceSetBinding: &addonsv1.ResourceSetBinding{ @@ -155,7 +155,7 @@ func TestReconcileApplyOnceScopeNeedsApply(t *testing.T) { want: true, }, { - name: "applied binding", + name: "applied ResourceBinding", scope: &reconcileApplyOnceScope{ baseResourceReconcileScope: baseResourceReconcileScope{ resourceSetBinding: &addonsv1.ResourceSetBinding{ From 18261481c3d08d47f272b515649b57fae1809fb8 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Tue, 20 Dec 2022 18:45:32 +0000 Subject: [PATCH 08/22] Set resource version before patch for reconcile mode --- exp/addons/internal/controllers/clusterresourceset_scope.go | 1 + 1 file changed, 1 insertion(+) diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index eea8d23d7079..89835044071d 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -132,6 +132,7 @@ func (r *reconcileStrategyScope) apply(ctx context.Context, c client.Client, obj } patch := client.MergeFrom(currentObj.DeepCopy()) + obj.SetResourceVersion(currentObj.GetResourceVersion()) if err = c.Patch(ctx, obj, patch); err != nil { return errors.Wrapf( err, From fe1613e8ea340d5c21a346f074f85b42a3f5a3cf Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 9 Jan 2023 14:08:51 +0000 Subject: [PATCH 09/22] Address review coments: rename GetResourceBinding --- exp/addons/api/v1beta1/clusterresourcesetbinding_types.go | 6 +++--- .../api/v1beta1/clusterresourcesetbinding_types_test.go | 2 +- exp/addons/internal/controllers/clusterresourceset_scope.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go index b7839b15b54c..ce0b965e31b5 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go @@ -58,7 +58,7 @@ 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) + resourceBinding := r.GetResource(resourceRef) if resourceBinding == nil { return false } @@ -66,8 +66,8 @@ func (r *ResourceSetBinding) IsApplied(resourceRef ResourceRef) bool { return resourceBinding.Applied } -// GetResourceBinding returns a ResourceBinding for a resource ref if present. -func (r *ResourceSetBinding) GetResourceBinding(resourceRef ResourceRef) *ResourceBinding { +// GetResource returns a ResourceBinding for a resource ref if present. +func (r *ResourceSetBinding) GetResource(resourceRef ResourceRef) *ResourceBinding { for _, resource := range r.Resources { if reflect.DeepEqual(resource.ResourceRef, resourceRef) { return &resource diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go index 2e71c3b7b8f2..8e6111c44572 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go @@ -145,7 +145,7 @@ func TestResourceSetBindingGetResourceBinding(t *testing.T) { 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)) + gs.Expect(tt.resourceSetBinding.GetResource(tt.resourceRef)).To(Equal(tt.want)) }) } } diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index 89835044071d..b184ffe1976e 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -108,7 +108,7 @@ type reconcileStrategyScope struct { } func (r *reconcileStrategyScope) needsApply() bool { - resourceBinding := r.resourceSetBinding.GetResourceBinding(r.resourceRef) + resourceBinding := r.resourceSetBinding.GetResource(r.resourceRef) return resourceBinding == nil || !resourceBinding.Applied || resourceBinding.Hash != r.computedHash } From 34725cd9a4e94d4a59b18b6c09c1f3a73d71169e Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 9 Jan 2023 15:09:57 +0100 Subject: [PATCH 10/22] Simplify IsApplied Co-authored-by: Fabrizio Pandini --- exp/addons/api/v1beta1/clusterresourcesetbinding_types.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go index ce0b965e31b5..47f8762ec3e6 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go @@ -59,11 +59,7 @@ 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.GetResource(resourceRef) - if resourceBinding == nil { - return false - } - - return resourceBinding.Applied + return resourceBinding != nil && resourceBinding.Applied } // GetResource returns a ResourceBinding for a resource ref if present. From 86193c91067da3327e358c30ed32a9aeded42524 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 9 Jan 2023 14:17:40 +0000 Subject: [PATCH 11/22] Address review coments: use switch and drop IsStrategy --- .../api/v1beta1/clusterresourceset_types.go | 5 -- .../v1beta1/clusterresourceset_types_test.go | 65 ------------------- .../controllers/clusterresourceset_scope.go | 10 +-- 3 files changed, 6 insertions(+), 74 deletions(-) delete mode 100644 exp/addons/api/v1beta1/clusterresourceset_types_test.go diff --git a/exp/addons/api/v1beta1/clusterresourceset_types.go b/exp/addons/api/v1beta1/clusterresourceset_types.go index c5d41ba476bd..e64dccd29714 100644 --- a/exp/addons/api/v1beta1/clusterresourceset_types.go +++ b/exp/addons/api/v1beta1/clusterresourceset_types.go @@ -115,11 +115,6 @@ 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 diff --git a/exp/addons/api/v1beta1/clusterresourceset_types_test.go b/exp/addons/api/v1beta1/clusterresourceset_types_test.go deleted file mode 100644 index c46ce58bbc64..000000000000 --- a/exp/addons/api/v1beta1/clusterresourceset_types_test.go +++ /dev/null @@ -1,65 +0,0 @@ -/* -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)) - }) - } -} diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index b184ffe1976e..443cb4c56cc6 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -77,13 +77,15 @@ func newResourceReconcileScope( normalizedObjs: objs, computedHash: computeHash(normalizedData), } - if clusterResourceSet.IsStrategy(addonsv1.ClusterResourceSetStrategyApplyOnce) { + + switch addonsv1.ClusterResourceSetStrategy(clusterResourceSet.Spec.Strategy) { + case addonsv1.ClusterResourceSetStrategyApplyOnce: return &reconcileApplyOnceScope{base} - } else if clusterResourceSet.IsStrategy(addonsv1.ClusterResourceSetStrategyReconcile) { + case addonsv1.ClusterResourceSetStrategyReconcile: return &reconcileStrategyScope{base} + default: + return nil } - - return nil } type baseResourceReconcileScope struct { From 3a4bb5fc077b78888211bc61cfe29b9f2dccee81 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 9 Jan 2023 15:19:09 +0100 Subject: [PATCH 12/22] Improve client error wraps Co-authored-by: Fabrizio Pandini --- .../internal/controllers/clusterresourceset_helpers.go | 5 ++--- exp/addons/internal/controllers/clusterresourceset_scope.go | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_helpers.go b/exp/addons/internal/controllers/clusterresourceset_helpers.go index d8da5716b290..305204f4b21e 100644 --- a/exp/addons/internal/controllers/clusterresourceset_helpers.go +++ b/exp/addons/internal/controllers/clusterresourceset_helpers.go @@ -110,10 +110,9 @@ func createUnstructured(ctx context.Context, c client.Client, obj *unstructured. if err := c.Create(ctx, obj); err != nil { return errors.Wrapf( err, - "creating object %s %s/%s", + "creating object %s %s", obj.GroupVersionKind(), - obj.GetNamespace(), - obj.GetName(), + klog.Obj(obj), ) } diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index 443cb4c56cc6..49d8ef5bbd1c 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -126,10 +126,9 @@ func (r *reconcileStrategyScope) apply(ctx context.Context, c client.Client, obj if err != nil { return errors.Wrapf( err, - "reading object %s %s/%s", + "reading object %s %s", obj.GroupVersionKind(), - obj.GetNamespace(), - obj.GetName(), + klog.Obj(obj), ) } From c033f00e0fafa1476597c34b188e95979efe72f5 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 9 Jan 2023 14:57:05 +0000 Subject: [PATCH 13/22] Add unit test to check for apply once logic re-entry --- .../clusterresourceset_controller.go | 8 +-- .../controllers/clusterresourceset_helpers.go | 3 +- .../controllers/clusterresourceset_scope.go | 3 +- .../clusterresourceset_scope_test.go | 65 ++++++++++++++++++- 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 1ac95879c742..474abe60d4bf 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -298,8 +298,8 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte errList = append(errList, err) } - resourceScope, scopeError := reconcileScopeForResource(clusterResourceSet, resource, resourceSetBinding, unstructuredObj) - if scopeError == nil && !resourceScope.needsApply() { + resourceScope, err := reconcileScopeForResource(clusterResourceSet, resource, resourceSetBinding, unstructuredObj) + if err == nil && !resourceScope.needsApply() { continue } @@ -312,8 +312,8 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte LastAppliedTime: &metav1.Time{Time: time.Now().UTC()}, }) - if scopeError != nil { - errList = append(errList, scopeError) + if err != nil { + errList = append(errList, err) continue } diff --git a/exp/addons/internal/controllers/clusterresourceset_helpers.go b/exp/addons/internal/controllers/clusterresourceset_helpers.go index 305204f4b21e..2e5f1eeb948f 100644 --- a/exp/addons/internal/controllers/clusterresourceset_helpers.go +++ b/exp/addons/internal/controllers/clusterresourceset_helpers.go @@ -34,6 +34,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -112,7 +113,7 @@ func createUnstructured(ctx context.Context, c client.Client, obj *unstructured. err, "creating object %s %s", obj.GroupVersionKind(), - klog.Obj(obj), + klog.KObj(obj), ) } diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index 49d8ef5bbd1c..d20b61811a0e 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -22,6 +22,7 @@ import ( "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" @@ -128,7 +129,7 @@ func (r *reconcileStrategyScope) apply(ctx context.Context, c client.Client, obj err, "reading object %s %s", obj.GroupVersionKind(), - klog.Obj(obj), + klog.KObj(obj), ) } diff --git a/exp/addons/internal/controllers/clusterresourceset_scope_test.go b/exp/addons/internal/controllers/clusterresourceset_scope_test.go index 081bc93fdb59..febc3e5471d1 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope_test.go @@ -17,11 +17,16 @@ limitations under the License. package controllers import ( + "context" "testing" . "github.com/onsi/gomega" - + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) func TestReconcileStrategyScopeNeedsApply(t *testing.T) { @@ -185,3 +190,61 @@ func TestReconcileApplyOnceScopeNeedsApply(t *testing.T) { }) } } + +func TestReconcileApplyOnceScopeApply(t *testing.T) { + tests := []struct { + name string + existingObjs []client.Object + obj *unstructured.Unstructured + wantErr string + }{ + { + name: "object doesn't exist", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "my-cm", + "namespace": "that-ns", + }, + }, + }, + }, + { + name: "object exists", + existingObjs: []client.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cm", + Namespace: "that-ns", + }, + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "my-cm", + "namespace": "that-ns", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gs := NewWithT(t) + ctx := context.Background() + client := fake.NewClientBuilder().WithObjects(tt.existingObjs...).Build() + scope := &reconcileApplyOnceScope{} + err := scope.apply(ctx, client, tt.obj) + if tt.wantErr == "" { + gs.Expect(err).NotTo(HaveOccurred()) + } else { + gs.Expect(err).To(MatchError(ContainSubstring(tt.wantErr))) + } + }) + } +} From 6c656f80a5f35546b3dcb28b577fc7dfdfca9928 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 9 Jan 2023 15:30:40 +0000 Subject: [PATCH 14/22] Implement top loevel apply methoid in resource scopes --- .../clusterresourceset_controller.go | 2 +- .../controllers/clusterresourceset_helpers.go | 14 -------- .../controllers/clusterresourceset_scope.go | 33 +++++++++++++++---- .../clusterresourceset_scope_test.go | 7 ++-- 4 files changed, 31 insertions(+), 25 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 474abe60d4bf..2d3cd72a19da 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -320,7 +320,7 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte // 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 - if err := apply(ctx, remoteClient, resourceScope); err != nil { + if err := resourceScope.apply(ctx, remoteClient); 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()) diff --git a/exp/addons/internal/controllers/clusterresourceset_helpers.go b/exp/addons/internal/controllers/clusterresourceset_helpers.go index 2e5f1eeb948f..c0b8fa1a78e6 100644 --- a/exp/addons/internal/controllers/clusterresourceset_helpers.go +++ b/exp/addons/internal/controllers/clusterresourceset_helpers.go @@ -33,7 +33,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" - kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -94,19 +93,6 @@ func isJSONList(data []byte) (bool, error) { return bytes.HasPrefix(trim, jsonListPrefix), nil } -// apply reconciles all objects defined by a resource from a ClusterResourceSet -// delegating on the reconcileScope for the applying strategy. -func apply(ctx context.Context, c client.Client, scope resourceReconcileScope) error { - errList := []error{} - objs := scope.objs() - for i := range objs { - if err := scope.apply(ctx, c, &objs[i]); err != nil { - errList = append(errList, err) - } - } - return kerrors.NewAggregate(errList) -} - func createUnstructured(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { if err := c.Create(ctx, obj); err != nil { return errors.Wrapf( diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index d20b61811a0e..e4af7ff3d2c5 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -22,6 +22,7 @@ import ( "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,11 +35,8 @@ type resourceReconcileScope interface { // needsApply determines if a resource needs to be applied to the target cluster // based on the strategy. needsApply() bool - // apply reconciles the resource to the target cluster following a different process - // depending on the strategy. - apply(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error - // objs returns all the Kubernetes objects defined in the resource. - objs() []unstructured.Unstructured + // apply reconciles all objects defined by the resource following the proper strategy for the CRS. + apply(ctx context.Context, c client.Client) error // hash returns a computed hash of the defined objects in the resource. It is consistent // between runs. hash() string @@ -116,7 +114,11 @@ func (r *reconcileStrategyScope) needsApply() bool { return resourceBinding == nil || !resourceBinding.Applied || resourceBinding.Hash != r.computedHash } -func (r *reconcileStrategyScope) apply(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { +func (r *reconcileStrategyScope) apply(ctx context.Context, c client.Client) error { + return apply(ctx, c, r.applyObj, r.objs()) +} + +func (r *reconcileStrategyScope) applyObj(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { currentObj := &unstructured.Unstructured{} currentObj.SetAPIVersion(obj.GetAPIVersion()) currentObj.SetKind(obj.GetKind()) @@ -156,7 +158,11 @@ func (r *reconcileApplyOnceScope) needsApply() bool { return !r.resourceSetBinding.IsApplied(r.resourceRef) } -func (r *reconcileApplyOnceScope) apply(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { +func (r *reconcileApplyOnceScope) apply(ctx context.Context, c client.Client) error { + return apply(ctx, c, r.applyObj, r.objs()) +} + +func (r *reconcileApplyOnceScope) applyObj(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { // The create call is idempotent, so if the object already exists // then do not consider it to be an error. if err := createUnstructured(ctx, c, obj); !apierrors.IsAlreadyExists(err) { @@ -164,3 +170,16 @@ func (r *reconcileApplyOnceScope) apply(ctx context.Context, c client.Client, ob } return nil } + +type applyObj func(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error + +// apply reconciles unstructured objects using applyObj and aggreates the error if present. +func apply(ctx context.Context, c client.Client, applyObj applyObj, objs []unstructured.Unstructured) error { + errList := []error{} + for i := range objs { + if err := applyObj(ctx, c, &objs[i]); err != nil { + errList = append(errList, err) + } + } + return kerrors.NewAggregate(errList) +} diff --git a/exp/addons/internal/controllers/clusterresourceset_scope_test.go b/exp/addons/internal/controllers/clusterresourceset_scope_test.go index febc3e5471d1..624798294420 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope_test.go @@ -24,9 +24,10 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + + addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" ) func TestReconcileStrategyScopeNeedsApply(t *testing.T) { @@ -191,7 +192,7 @@ func TestReconcileApplyOnceScopeNeedsApply(t *testing.T) { } } -func TestReconcileApplyOnceScopeApply(t *testing.T) { +func TestReconcileApplyOnceScopeApplyObj(t *testing.T) { tests := []struct { name string existingObjs []client.Object @@ -239,7 +240,7 @@ func TestReconcileApplyOnceScopeApply(t *testing.T) { ctx := context.Background() client := fake.NewClientBuilder().WithObjects(tt.existingObjs...).Build() scope := &reconcileApplyOnceScope{} - err := scope.apply(ctx, client, tt.obj) + err := scope.applyObj(ctx, client, tt.obj) if tt.wantErr == "" { gs.Expect(err).NotTo(HaveOccurred()) } else { From 199fcceb505e62ee4b998bd8a7906d133fe5aa34 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Wed, 25 Jan 2023 16:45:56 +0100 Subject: [PATCH 15/22] Improve test case names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> --- .../api/v1beta1/clusterresourcesetbinding_types_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go index 8e6111c44572..d4ec081625f1 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_types_test.go @@ -130,13 +130,13 @@ func TestResourceSetBindingGetResourceBinding(t *testing.T) { want *ResourceBinding }{ { - name: "does't exist", + name: "ResourceRef doesn't exist", resourceSetBinding: crsBinding, resourceRef: resourceRefNotExist, want: nil, }, { - name: "does't exist", + name: "ResourceRef exists", resourceSetBinding: crsBinding, resourceRef: resourceRefApplyFailed, want: &resourceRefApplyFailedBinding, From a69ab2e30678478732e46508b7224571cab16ee4 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Wed, 25 Jan 2023 15:52:41 +0000 Subject: [PATCH 16/22] Check scope error before setting binding to improve legibility --- .../clusterresourceset_controller.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 2d3cd72a19da..170bb2e48ed6 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -299,7 +299,19 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte } resourceScope, err := reconcileScopeForResource(clusterResourceSet, resource, resourceSetBinding, unstructuredObj) - if err == nil && !resourceScope.needsApply() { + if err != nil { + resourceSetBinding.SetBinding(addonsv1.ResourceBinding{ + ResourceRef: resource, + Hash: "", + Applied: false, + LastAppliedTime: &metav1.Time{Time: time.Now().UTC()}, + }) + + errList = append(errList, err) + continue + } + + if !resourceScope.needsApply() { continue } @@ -312,11 +324,6 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte LastAppliedTime: &metav1.Time{Time: time.Now().UTC()}, }) - if err != nil { - errList = append(errList, err) - 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 From d7d1ffad24a528306548dad44e78191d90ef73fa Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Wed, 25 Jan 2023 16:55:28 +0100 Subject: [PATCH 17/22] Fix typo Co-authored-by: Fabrizio Pandini --- exp/addons/internal/controllers/clusterresourceset_helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_helpers.go b/exp/addons/internal/controllers/clusterresourceset_helpers.go index c0b8fa1a78e6..5925c47e5178 100644 --- a/exp/addons/internal/controllers/clusterresourceset_helpers.go +++ b/exp/addons/internal/controllers/clusterresourceset_helpers.go @@ -198,7 +198,7 @@ func computeHash(dataArr [][]byte) string { // normalizeData reads content of the resource (configmap or secret) and returns // them serialized with constant order. Secret's data is base64 decoded. // This is useful to achieve consistent data between runs, since the content -// of the data field is a mapp and its order is non deterministic. +// of the data field is a map and its order is non-deterministic. func normalizeData(resource *unstructured.Unstructured) ([][]byte, error) { // Since maps are not ordered, we need to order them to get the same hash at each reconcile. keys := make([]string, 0) From 2891a9cecd629f75fdb96a7921e956f2cb2313c2 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Wed, 25 Jan 2023 16:35:26 +0000 Subject: [PATCH 18/22] Improve errors in crs normalize data and scope --- .../internal/controllers/clusterresourceset_helpers.go | 9 ++++++--- .../internal/controllers/clusterresourceset_scope.go | 7 +++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_helpers.go b/exp/addons/internal/controllers/clusterresourceset_helpers.go index 5925c47e5178..2b25ceab3ef8 100644 --- a/exp/addons/internal/controllers/clusterresourceset_helpers.go +++ b/exp/addons/internal/controllers/clusterresourceset_helpers.go @@ -204,7 +204,7 @@ func normalizeData(resource *unstructured.Unstructured) ([][]byte, error) { keys := make([]string, 0) data, ok := resource.UnstructuredContent()["data"] if !ok { - return nil, errors.New("failed to get data field from the resource") + return nil, errors.Errorf("failed to get data field from resource %s", klog.KObj(resource)) } unstructuredData := data.(map[string]interface{}) @@ -216,8 +216,11 @@ func normalizeData(resource *unstructured.Unstructured) ([][]byte, error) { dataList := make([][]byte, 0) for _, key := range keys { val, ok, err := unstructured.NestedString(unstructuredData, key) - if !ok || err != nil { - return nil, errors.New("failed to get value field from the resource") + if err != nil { + return nil, errors.Wrapf(err, "getting value for field %s in data from resource %s", key, klog.KObj(resource)) + } + if !ok { + return nil, errors.Errorf("value for field %s not present in data from resource %s", key, klog.KObj(resource)) } byteArr := []byte(val) diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index e4af7ff3d2c5..a37e0bb5b1f9 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -140,10 +140,9 @@ func (r *reconcileStrategyScope) applyObj(ctx context.Context, c client.Client, if err = c.Patch(ctx, obj, patch); err != nil { return errors.Wrapf( err, - "patching object %s %s/%s", + "patching object %s %s", obj.GroupVersionKind(), - obj.GetNamespace(), - obj.GetName(), + klog.KObj(obj), ) } @@ -165,7 +164,7 @@ func (r *reconcileApplyOnceScope) apply(ctx context.Context, c client.Client) er func (r *reconcileApplyOnceScope) applyObj(ctx context.Context, c client.Client, obj *unstructured.Unstructured) error { // The create call is idempotent, so if the object already exists // then do not consider it to be an error. - if err := createUnstructured(ctx, c, obj); !apierrors.IsAlreadyExists(err) { + if err := createUnstructured(ctx, c, obj); err != nil && !apierrors.IsAlreadyExists(err) { return err } return nil From 9f84d7e6bbac70316cc87debeb2918705d4fcd8d Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Thu, 26 Jan 2023 11:36:02 +0000 Subject: [PATCH 19/22] Add tests to cover ApplyOnce reentry and idempotency --- .../clusterresourceset_controller_test.go | 198 ++++++++++++++++++ .../controllers/clusterresourceset_scope.go | 2 + 2 files changed, 200 insertions(+) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index c50f9c9cf097..a7f79dc3a455 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -17,6 +17,7 @@ limitations under the License. package controllers import ( + "crypto/sha1" "fmt" "reflect" "testing" @@ -34,6 +35,7 @@ import ( addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" "sigs.k8s.io/cluster-api/internal/test/envtest" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/conditions" ) const ( @@ -724,6 +726,195 @@ metadata: t.Log("Checking resource ConfigMap 2 has been updated") g.Eventually(configMapHasBeenUpdated(env, resourceConfigMap2Key, resourceConfigMap2), timeout).Should(Succeed()) }) + + t.Run("Should reconcile a ClusterResourceSet with ApplyOnce strategy even when one of the resources already exist", func(t *testing.T) { + g := NewWithT(t) + ns := setup(t, g) + defer teardown(t, g, ns) + + t.Log("Updating the cluster with labels") + testCluster.SetLabels(labels) + g.Expect(env.Update(ctx, testCluster)).To(Succeed()) + + t.Log("Creating resource CM before creating CRS") + // This CM is defined in the data of "configmapName", which is included in the + // CRS we will create in this test + resourceConfigMap1 := configMap( + resourceConfigMap1Name, + resourceConfigMapsNamespace, + map[string]string{ + "created": "before CRS", + }, + ) + g.Expect(env.Create(ctx, resourceConfigMap1)).To(Succeed()) + + t.Log("Creating a ClusterResourceSet instance that has same labels as selector") + clusterResourceSet := &addonsv1.ClusterResourceSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterResourceSetName, + Namespace: ns.Name, + }, + Spec: addonsv1.ClusterResourceSetSpec{ + Strategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce), + ClusterSelector: metav1.LabelSelector{ + MatchLabels: labels, + }, + Resources: []addonsv1.ResourceRef{{Name: configmapName, Kind: "ConfigMap"}, {Name: secretName, Kind: "Secret"}}, + }, + } + + g.Expect(env.Create(ctx, clusterResourceSet)).To(Succeed()) + + t.Log("Checking resource ConfigMap 1 hasn't been updated") + resourceConfigMap1Key := client.ObjectKey{ + Namespace: resourceConfigMapsNamespace, + Name: resourceConfigMap1Name, + } + g.Eventually(configMapHasBeenUpdated(env, resourceConfigMap1Key, resourceConfigMap1), timeout).Should(Succeed()) + + t.Log("Verifying resource ConfigMap 2 has been created") + resourceConfigMap2Key := client.ObjectKey{ + Namespace: resourceConfigMapsNamespace, + Name: resourceConfigMap2Name, + } + g.Eventually(func() error { + cm := &corev1.ConfigMap{} + return env.Get(ctx, resourceConfigMap2Key, cm) + }, timeout).Should(Succeed()) + }) + + t.Run("Should reconcile a ClusterResourceSet with ApplyOnce strategy even when there is an error, after the error has been corrected", func(t *testing.T) { + // To trigger an error in the middle of the reconciliation, we'll define a an object in a namespace that doesn't yet exist. + // We'll expect the CRS to reconcile all other objects except that one and bubble up the error. + // Once that happens, we'll go ahead a create the namespace. Then we'll expect the CRS to, eventually, create that remaining object. + + g := NewWithT(t) + ns := setup(t, g) + defer teardown(t, g, ns) + + t.Log("Updating the cluster with labels") + testCluster.SetLabels(labels) + g.Expect(env.Update(ctx, testCluster)).To(Succeed()) + + t.Log("Updating the test config map with the missing namespace resource") + missingNamespace := randomNamespaceForTest(t) + + resourceConfigMap1 := configMap( + resourceConfigMap1Name, + resourceConfigMapsNamespace, + map[string]string{ + "my_new_config": "some_value", + }, + ) + + resourceConfigMap1Content, err := yaml.Marshal(resourceConfigMap1) + g.Expect(err).NotTo(HaveOccurred()) + + resourceConfigMapWithMissingNamespace := configMap( + "cm-missing-namespace", + missingNamespace, + map[string]string{ + "my_new_config": "this is all new", + }, + ) + + resourceConfigMapMissingNamespaceContent, err := yaml.Marshal(resourceConfigMapWithMissingNamespace) + g.Expect(err).NotTo(HaveOccurred()) + + testConfigmap := configMap( + configmapName, + ns.Name, + map[string]string{ + "cm": string(resourceConfigMap1Content), + "problematic_cm": string(resourceConfigMapMissingNamespaceContent), + }, + ) + + g.Expect(env.Update(ctx, testConfigmap)).To(Succeed()) + + t.Log("Creating a ClusterResourceSet instance that has same labels as selector") + clusterResourceSet := &addonsv1.ClusterResourceSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterResourceSetName, + Namespace: ns.Name, + }, + Spec: addonsv1.ClusterResourceSetSpec{ + Strategy: string(addonsv1.ClusterResourceSetStrategyApplyOnce), + ClusterSelector: metav1.LabelSelector{ + MatchLabels: labels, + }, + Resources: []addonsv1.ResourceRef{{Name: testConfigmap.Name, Kind: "ConfigMap"}, {Name: secretName, Kind: "Secret"}}, + }, + } + + g.Expect(env.Create(ctx, clusterResourceSet)).To(Succeed()) + + t.Log("Verifying resource ConfigMap 1 has been created") + resourceConfigMap1Key := client.ObjectKeyFromObject(resourceConfigMap1) + g.Eventually(configMapHasBeenUpdated(env, resourceConfigMap1Key, resourceConfigMap1), timeout).Should(Succeed()) + + t.Log("Verifying resource ConfigMap 2 has been created") + resourceConfigMap2Key := client.ObjectKey{ + Namespace: resourceConfigMapsNamespace, + Name: resourceConfigMap2Name, + } + g.Eventually(func() error { + cm := &corev1.ConfigMap{} + return env.Get(ctx, resourceConfigMap2Key, cm) + }, timeout).Should(Succeed()) + + t.Log("Verifying CRS Binding failed marked the resource as not applied") + g.Eventually(func(g Gomega) { + clusterResourceSetBindingKey := client.ObjectKey{ + Namespace: testCluster.Namespace, + Name: testCluster.Name, + } + binding := &addonsv1.ClusterResourceSetBinding{} + g.Expect(env.Get(ctx, clusterResourceSetBindingKey, binding)).To(Succeed()) + + g.Expect(binding.Spec.Bindings).To(HaveLen(1)) + g.Expect(binding.Spec.Bindings[0].Resources).To(HaveLen(2)) + + for _, r := range binding.Spec.Bindings[0].Resources { + switch r.ResourceRef.Name { + case testConfigmap.Name: + g.Expect(r.Applied).To(BeFalse(), "test-configmap should be not applied bc of missing namespace") + case secretName: + g.Expect(r.Applied).To(BeTrue(), "test-secret should be applied") + } + } + }, timeout).Should(Succeed()) + + t.Log("Verifying CRS has a false ResourcesApplied condition") + g.Eventually(func(g Gomega) { + clusterResourceSetKey := client.ObjectKeyFromObject(clusterResourceSet) + crs := &addonsv1.ClusterResourceSet{} + g.Expect(env.Get(ctx, clusterResourceSetKey, crs)).To(Succeed()) + + appliedCondition := conditions.Get(crs, addonsv1.ResourcesAppliedCondition) + g.Expect(appliedCondition).NotTo(BeNil()) + g.Expect(appliedCondition.Status).To(Equal(corev1.ConditionFalse)) + g.Expect(appliedCondition.Reason).To(Equal(addonsv1.ApplyFailedReason)) + g.Expect(appliedCondition.Message).To(ContainSubstring("creating object /v1, Kind=ConfigMap %s/cm-missing-namespace", missingNamespace)) + }, timeout).Should(Succeed()) + + t.Log("Creating missing namespace") + missingNs := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: missingNamespace, + }, + } + g.Expect(env.Create(ctx, missingNs)).To(Succeed()) + + t.Log("Verifying CRS Binding has all resources applied") + g.Eventually(clusterResourceSetBindingReady(env, testCluster), timeout).Should(BeTrue()) + + t.Log("Verifying resource ConfigMap with previsouly missing namespace has been created") + g.Eventually(configMapHasBeenUpdated(env, client.ObjectKeyFromObject(resourceConfigMapWithMissingNamespace), resourceConfigMapWithMissingNamespace), timeout).Should(Succeed()) + + g.Expect(env.Delete(ctx, resourceConfigMapWithMissingNamespace)).To(Succeed()) + g.Expect(env.Delete(ctx, missingNs)).To(Succeed()) + }) } func clusterResourceSetBindingReady(env *envtest.Environment, cluster *clusterv1.Cluster) func() bool { @@ -786,3 +977,10 @@ func configMap(name, namespace string, data map[string]string) *corev1.ConfigMap Data: data, } } + +func randomNamespaceForTest(t testing.TB) string { + h := sha1.New() + h.Write([]byte(t.Name())) + testNameHash := fmt.Sprintf("%x", h.Sum(nil)) + return "ns-" + testNameHash[:7] + "-" + util.RandomString(6) +} diff --git a/exp/addons/internal/controllers/clusterresourceset_scope.go b/exp/addons/internal/controllers/clusterresourceset_scope.go index a37e0bb5b1f9..921a752e0d54 100644 --- a/exp/addons/internal/controllers/clusterresourceset_scope.go +++ b/exp/addons/internal/controllers/clusterresourceset_scope.go @@ -167,6 +167,7 @@ func (r *reconcileApplyOnceScope) applyObj(ctx context.Context, c client.Client, if err := createUnstructured(ctx, c, obj); err != nil && !apierrors.IsAlreadyExists(err) { return err } + return nil } @@ -180,5 +181,6 @@ func apply(ctx context.Context, c client.Client, applyObj applyObj, objs []unstr errList = append(errList, err) } } + return kerrors.NewAggregate(errList) } From aed9907acc61449a3c7a2384d1ca1bbfc0159dab Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 13 Feb 2023 12:21:10 -0600 Subject: [PATCH 20/22] Fix typos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> --- .../controllers/clusterresourceset_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index a7f79dc3a455..552c3df60bab 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -784,9 +784,9 @@ metadata: }) t.Run("Should reconcile a ClusterResourceSet with ApplyOnce strategy even when there is an error, after the error has been corrected", func(t *testing.T) { - // To trigger an error in the middle of the reconciliation, we'll define a an object in a namespace that doesn't yet exist. + // To trigger an error in the middle of the reconciliation, we'll define an object in a namespace that doesn't yet exist. // We'll expect the CRS to reconcile all other objects except that one and bubble up the error. - // Once that happens, we'll go ahead a create the namespace. Then we'll expect the CRS to, eventually, create that remaining object. + // Once that happens, we'll go ahead and create the namespace. Then we'll expect the CRS to, eventually, create that remaining object. g := NewWithT(t) ns := setup(t, g) From d4635bbd25d61658befc52f0bb43728261548202 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 13 Feb 2023 12:21:53 -0600 Subject: [PATCH 21/22] Fix exp/addons/internal/controllers/clusterresourceset_controller_test.go typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> --- .../internal/controllers/clusterresourceset_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index 552c3df60bab..0679f9ae1df4 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -909,7 +909,7 @@ metadata: t.Log("Verifying CRS Binding has all resources applied") g.Eventually(clusterResourceSetBindingReady(env, testCluster), timeout).Should(BeTrue()) - t.Log("Verifying resource ConfigMap with previsouly missing namespace has been created") + t.Log("Verifying resource ConfigMap with previously missing namespace has been created") g.Eventually(configMapHasBeenUpdated(env, client.ObjectKeyFromObject(resourceConfigMapWithMissingNamespace), resourceConfigMapWithMissingNamespace), timeout).Should(Succeed()) g.Expect(env.Delete(ctx, resourceConfigMapWithMissingNamespace)).To(Succeed()) From c32cf261d3a6123ffd057337f40a1d7688068750 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Mon, 13 Feb 2023 19:33:46 +0000 Subject: [PATCH 22/22] Improve generate namespace function --- .../controllers/clusterresourceset_controller_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index 0679f9ae1df4..60c53b2702f3 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -17,7 +17,7 @@ limitations under the License. package controllers import ( - "crypto/sha1" + "crypto/sha1" //nolint: gosec "fmt" "reflect" "testing" @@ -978,9 +978,12 @@ func configMap(name, namespace string, data map[string]string) *corev1.ConfigMap } } -func randomNamespaceForTest(t testing.TB) string { - h := sha1.New() - h.Write([]byte(t.Name())) +func randomNamespaceForTest(tb testing.TB) string { + tb.Helper() + // This is just to get a short form of the test name + // sha1 is totally fine + h := sha1.New() //nolint: gosec + h.Write([]byte(tb.Name())) testNameHash := fmt.Sprintf("%x", h.Sum(nil)) return "ns-" + testNameHash[:7] + "-" + util.RandomString(6) }