From 25204571f39386be5f601b8dc50a2dd7f15a98ba Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 22 Nov 2024 16:47:00 +0100 Subject: [PATCH 1/3] crs: implement ResourcesApplied v1beta2 condition --- .../api/v1beta1/clusterresourceset_types.go | 26 ++++++++++++++ .../clusterresourceset_controller.go | 36 +++++++++++++++++++ .../clusterresourceset_controller_test.go | 7 ++++ 3 files changed, 69 insertions(+) diff --git a/exp/addons/api/v1beta1/clusterresourceset_types.go b/exp/addons/api/v1beta1/clusterresourceset_types.go index 780bd28c5627..8fe52c054503 100644 --- a/exp/addons/api/v1beta1/clusterresourceset_types.go +++ b/exp/addons/api/v1beta1/clusterresourceset_types.go @@ -23,6 +23,32 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) +// ClusterResourceSet's ResourcesApplied condition and corresponding reasons that will be used in v1Beta2 API version. +const ( + // ResourcesAppliedV1beta2Condition surfaces wether the resources in the ClusterResourceSet are applied to all matching clusters. + // This indicates all resources exist, and no errors during applying them to all clusters. + ResourcesAppliedV1beta2Condition = "ResourcesApplied" + + // ResourcesAppliedV1beta2Reason is the reason used when all resources in the ClusterResourceSet object got applied + // to all matching clusters. + ResourcesAppliedV1beta2Reason = "Applied" + + // ResourcesAppliedApplyFailedV1beta2Reason is the reason used when applying at least one of the resources to one of the matching clusters failed. + ResourcesAppliedApplyFailedV1beta2Reason = "ApplyFailed" + + // ResourcesAppliedRemoteClusterClientFailedV1beta2Reason is the reason used on failures during getting the remote cluster client. + ResourcesAppliedRemoteClusterClientFailedV1beta2Reason = "RemoteClusterClientFailed" + + // ResourcesAppliedClusterMatchFailedV1beta2Reason is the reason used on failure getting clusters that match the clusterSelector. + ResourcesAppliedClusterMatchFailedV1beta2Reason = "ClusterMatchFailed" + + // ResourcesAppliedRetrievingResourceFailedV1beta2Reason is the reason used when resources are not successfully retrieved. + ResourcesAppliedRetrievingResourceFailedV1beta2Reason = "RetrievingResourceFailed" + + // ResourcesAppliedWrongSecretTypeV1beta2Reason is the reason used when the the Secret's type in the resource list is not supported. + ResourcesAppliedWrongSecretTypeV1beta2Reason = "WrongSecretType" +) + const ( // ClusterResourceSetSecretType is the only accepted type of secret in resources. ClusterResourceSetSecretType corev1.SecretType = "addons.cluster.x-k8s.io/resource-set" //nolint:gosec diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 9136f3118151..99028a83982c 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -46,6 +46,7 @@ import ( resourcepredicates "sigs.k8s.io/cluster-api/exp/addons/internal/controllers/predicates" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" + v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2" "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/paused" @@ -158,6 +159,12 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R if err != nil { log.Error(err, "Failed fetching clusters that matches ClusterResourceSet labels", "ClusterResourceSet", klog.KObj(clusterResourceSet)) conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.ClusterMatchFailedReason, clusterv1.ConditionSeverityWarning, err.Error()) + v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ + Type: addonsv1.ResourcesAppliedV1beta2Condition, + Status: metav1.ConditionFalse, + Reason: addonsv1.ResourcesAppliedClusterMatchFailedV1beta2Reason, + Message: "Failed to get Clusters by ClusterSelector", + }) return ctrl.Result{}, err } @@ -309,8 +316,20 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte if err != nil { if err == ErrSecretTypeNotSupported { conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.WrongSecretTypeReason, clusterv1.ConditionSeverityWarning, err.Error()) + v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ + Type: addonsv1.ResourcesAppliedV1beta2Condition, + Status: metav1.ConditionFalse, + Reason: addonsv1.ResourcesAppliedWrongSecretTypeV1beta2Reason, + Message: fmt.Sprintf("Secret type of resource %s is not supported", resource.Name), + }) } else { conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RetrievingResourceFailedReason, clusterv1.ConditionSeverityWarning, err.Error()) + v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ + Type: addonsv1.ResourcesAppliedV1beta2Condition, + Status: metav1.ConditionFalse, + Reason: addonsv1.ResourcesAppliedRetrievingResourceFailedV1beta2Reason, + Message: "Failed to get resource", + }) // Continue without adding the error to the aggregate if we can't find the resource. if apierrors.IsNotFound(err) { @@ -363,6 +382,12 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte remoteClient, err := r.ClusterCache.GetClient(ctx, util.ObjectKey(cluster)) if err != nil { conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RemoteClusterClientFailedReason, clusterv1.ConditionSeverityError, err.Error()) + v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ + Type: addonsv1.ResourcesAppliedV1beta2Condition, + Status: metav1.ConditionFalse, + Reason: addonsv1.ResourcesAppliedRemoteClusterClientFailedV1beta2Reason, + Message: "Please check controller logs for errors", + }) return err } @@ -414,6 +439,12 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte isSuccessful = false log.Error(err, "Failed to apply ClusterResourceSet resource", resource.Kind, klog.KRef(clusterResourceSet.Namespace, resource.Name)) conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.ApplyFailedReason, clusterv1.ConditionSeverityWarning, err.Error()) + v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ + Type: addonsv1.ResourcesAppliedV1beta2Condition, + Status: metav1.ConditionFalse, + Reason: addonsv1.ResourcesAppliedApplyFailedV1beta2Reason, + Message: "Failed to apply ClusterResourceSet resources to Cluster", + }) errList = append(errList, err) } @@ -429,6 +460,11 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte } conditions.MarkTrue(clusterResourceSet, addonsv1.ResourcesAppliedCondition) + v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ + Type: addonsv1.ResourcesAppliedV1beta2Condition, + Status: metav1.ConditionTrue, + Reason: addonsv1.ResourcesAppliedV1beta2Reason, + }) return nil } diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index e5663d1ba976..804c3e6577cd 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -36,6 +36,7 @@ import ( "sigs.k8s.io/cluster-api/internal/test/envtest" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" + v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2" ) const ( @@ -897,6 +898,12 @@ metadata: 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)) + + appliedConditionV1Beta2 := v1beta2conditions.Get(crs, addonsv1.ResourcesAppliedV1beta2Condition) + g.Expect(appliedConditionV1Beta2).NotTo(BeNil()) + g.Expect(appliedConditionV1Beta2.Status).To(BeEquivalentTo(corev1.ConditionFalse)) + g.Expect(appliedConditionV1Beta2.Reason).To(Equal(addonsv1.ApplyFailedReason)) + g.Expect(appliedConditionV1Beta2.Message).To(Equal("Failed to apply ClusterResourceSet resources to Cluster")) }, timeout).Should(Succeed()) t.Log("Creating missing namespace") From dbb8d93231bf581657c33707ddb6cbb5d8bffe10 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 25 Nov 2024 11:10:46 +0100 Subject: [PATCH 2/3] review fixes --- .../api/v1beta1/clusterresourceset_types.go | 22 ++++++---------- .../clusterresourceset_controller.go | 26 +++++++++---------- .../clusterresourceset_controller_test.go | 2 +- 3 files changed, 22 insertions(+), 28 deletions(-) diff --git a/exp/addons/api/v1beta1/clusterresourceset_types.go b/exp/addons/api/v1beta1/clusterresourceset_types.go index 8fe52c054503..bc9dae48fcab 100644 --- a/exp/addons/api/v1beta1/clusterresourceset_types.go +++ b/exp/addons/api/v1beta1/clusterresourceset_types.go @@ -25,28 +25,22 @@ import ( // ClusterResourceSet's ResourcesApplied condition and corresponding reasons that will be used in v1Beta2 API version. const ( - // ResourcesAppliedV1beta2Condition surfaces wether the resources in the ClusterResourceSet are applied to all matching clusters. + // ResourcesAppliedV1Beta2Condition surfaces wether the resources in the ClusterResourceSet are applied to all matching clusters. // This indicates all resources exist, and no errors during applying them to all clusters. - ResourcesAppliedV1beta2Condition = "ResourcesApplied" + ResourcesAppliedV1Beta2Condition = "ResourcesApplied" // ResourcesAppliedV1beta2Reason is the reason used when all resources in the ClusterResourceSet object got applied // to all matching clusters. ResourcesAppliedV1beta2Reason = "Applied" - // ResourcesAppliedApplyFailedV1beta2Reason is the reason used when applying at least one of the resources to one of the matching clusters failed. - ResourcesAppliedApplyFailedV1beta2Reason = "ApplyFailed" + // ResourcesNotAppliedV1Beta2Reason is the reason used when applying at least one of the resources to one of the matching clusters failed. + ResourcesNotAppliedV1Beta2Reason = "NotApplied" - // ResourcesAppliedRemoteClusterClientFailedV1beta2Reason is the reason used on failures during getting the remote cluster client. - ResourcesAppliedRemoteClusterClientFailedV1beta2Reason = "RemoteClusterClientFailed" + // ResourcesAppliedWrongSecretTypeV1Beta2Reason is the reason used when the Secret's type in the resource list is not supported. + ResourcesAppliedWrongSecretTypeV1Beta2Reason = "WrongSecretType" - // ResourcesAppliedClusterMatchFailedV1beta2Reason is the reason used on failure getting clusters that match the clusterSelector. - ResourcesAppliedClusterMatchFailedV1beta2Reason = "ClusterMatchFailed" - - // ResourcesAppliedRetrievingResourceFailedV1beta2Reason is the reason used when resources are not successfully retrieved. - ResourcesAppliedRetrievingResourceFailedV1beta2Reason = "RetrievingResourceFailed" - - // ResourcesAppliedWrongSecretTypeV1beta2Reason is the reason used when the the Secret's type in the resource list is not supported. - ResourcesAppliedWrongSecretTypeV1beta2Reason = "WrongSecretType" + // ResourcesAppliedInternalErrorV1Beta2Reason surfaces unexpected failures when reconciling a ClusterResourceSet. + ResourcesAppliedInternalErrorV1Beta2Reason = clusterv1.InternalErrorV1Beta2Reason ) const ( diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 99028a83982c..fb9c6bb54072 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -160,10 +160,10 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R log.Error(err, "Failed fetching clusters that matches ClusterResourceSet labels", "ClusterResourceSet", klog.KObj(clusterResourceSet)) conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.ClusterMatchFailedReason, clusterv1.ConditionSeverityWarning, err.Error()) v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ - Type: addonsv1.ResourcesAppliedV1beta2Condition, + Type: addonsv1.ResourcesAppliedV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: addonsv1.ResourcesAppliedClusterMatchFailedV1beta2Reason, - Message: "Failed to get Clusters by ClusterSelector", + Reason: addonsv1.ResourcesAppliedInternalErrorV1Beta2Reason, + Message: "Please check controller logs for errors", }) return ctrl.Result{}, err } @@ -317,18 +317,18 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte if err == ErrSecretTypeNotSupported { conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.WrongSecretTypeReason, clusterv1.ConditionSeverityWarning, err.Error()) v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ - Type: addonsv1.ResourcesAppliedV1beta2Condition, + Type: addonsv1.ResourcesAppliedV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: addonsv1.ResourcesAppliedWrongSecretTypeV1beta2Reason, + Reason: addonsv1.ResourcesAppliedWrongSecretTypeV1Beta2Reason, Message: fmt.Sprintf("Secret type of resource %s is not supported", resource.Name), }) } else { conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RetrievingResourceFailedReason, clusterv1.ConditionSeverityWarning, err.Error()) v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ - Type: addonsv1.ResourcesAppliedV1beta2Condition, + Type: addonsv1.ResourcesAppliedV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: addonsv1.ResourcesAppliedRetrievingResourceFailedV1beta2Reason, - Message: "Failed to get resource", + Reason: addonsv1.ResourcesAppliedInternalErrorV1Beta2Reason, + Message: "Please check controller logs for errors", }) // Continue without adding the error to the aggregate if we can't find the resource. @@ -383,9 +383,9 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte if err != nil { conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.RemoteClusterClientFailedReason, clusterv1.ConditionSeverityError, err.Error()) v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ - Type: addonsv1.ResourcesAppliedV1beta2Condition, + Type: addonsv1.ResourcesAppliedV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: addonsv1.ResourcesAppliedRemoteClusterClientFailedV1beta2Reason, + Reason: clusterv1.InternalErrorV1Beta2Reason, Message: "Please check controller logs for errors", }) return err @@ -440,9 +440,9 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte log.Error(err, "Failed to apply ClusterResourceSet resource", resource.Kind, klog.KRef(clusterResourceSet.Namespace, resource.Name)) conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedCondition, addonsv1.ApplyFailedReason, clusterv1.ConditionSeverityWarning, err.Error()) v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ - Type: addonsv1.ResourcesAppliedV1beta2Condition, + Type: addonsv1.ResourcesAppliedV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: addonsv1.ResourcesAppliedApplyFailedV1beta2Reason, + Reason: addonsv1.ResourcesNotAppliedV1Beta2Reason, Message: "Failed to apply ClusterResourceSet resources to Cluster", }) errList = append(errList, err) @@ -461,7 +461,7 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte conditions.MarkTrue(clusterResourceSet, addonsv1.ResourcesAppliedCondition) v1beta2conditions.Set(clusterResourceSet, metav1.Condition{ - Type: addonsv1.ResourcesAppliedV1beta2Condition, + Type: addonsv1.ResourcesAppliedV1Beta2Condition, Status: metav1.ConditionTrue, Reason: addonsv1.ResourcesAppliedV1beta2Reason, }) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index 804c3e6577cd..fd65347af44b 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -899,7 +899,7 @@ metadata: g.Expect(appliedCondition.Reason).To(Equal(addonsv1.ApplyFailedReason)) g.Expect(appliedCondition.Message).To(ContainSubstring("creating object /v1, Kind=ConfigMap %s/cm-missing-namespace", missingNamespace)) - appliedConditionV1Beta2 := v1beta2conditions.Get(crs, addonsv1.ResourcesAppliedV1beta2Condition) + appliedConditionV1Beta2 := v1beta2conditions.Get(crs, addonsv1.ResourcesAppliedV1Beta2Condition) g.Expect(appliedConditionV1Beta2).NotTo(BeNil()) g.Expect(appliedConditionV1Beta2.Status).To(BeEquivalentTo(corev1.ConditionFalse)) g.Expect(appliedConditionV1Beta2.Reason).To(Equal(addonsv1.ApplyFailedReason)) From 7195441d4f48ec9967dba614be020321a658a77d Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 25 Nov 2024 11:45:14 +0100 Subject: [PATCH 3/3] fix test --- .../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 fd65347af44b..a0e834cf383b 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -902,7 +902,7 @@ metadata: appliedConditionV1Beta2 := v1beta2conditions.Get(crs, addonsv1.ResourcesAppliedV1Beta2Condition) g.Expect(appliedConditionV1Beta2).NotTo(BeNil()) g.Expect(appliedConditionV1Beta2.Status).To(BeEquivalentTo(corev1.ConditionFalse)) - g.Expect(appliedConditionV1Beta2.Reason).To(Equal(addonsv1.ApplyFailedReason)) + g.Expect(appliedConditionV1Beta2.Reason).To(Equal(addonsv1.ResourcesNotAppliedV1Beta2Reason)) g.Expect(appliedConditionV1Beta2.Message).To(Equal("Failed to apply ClusterResourceSet resources to Cluster")) }, timeout).Should(Succeed())