From 6136f9375b1a8f29f55b83d43c5635629c562259 Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Tue, 14 Jul 2026 13:46:17 +0200 Subject: [PATCH] fix: Report failureMessage when deleting a ClusterSummary fails --- api/v1beta1/zz_generated.deepcopy.go | 5 +- controllers/clustersummary_deployer.go | 25 +++++++--- controllers/clustersummary_deployer_test.go | 54 +++++++++++++++++++++ 3 files changed, 74 insertions(+), 10 deletions(-) diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index dd08190d..9e31aec6 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -21,11 +21,12 @@ limitations under the License. package v1beta1 import ( - apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + + apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/controllers/clustersummary_deployer.go b/controllers/clustersummary_deployer.go index b52d8485..3e994bea 100644 --- a/controllers/clustersummary_deployer.go +++ b/controllers/clustersummary_deployer.go @@ -512,12 +512,12 @@ func (r *ClusterSummaryReconciler) processUndeployResult(ctx context.Context, cl return fmt.Errorf("feature is still being removed") } - // Failure to undeploy because of missing permission is ignored. + // Failure to undeploy because of missing permission is not treated as terminal: this Forbidden + // error also covers admission webhooks denying the delete, so the reason is still surfaced via + // FailureMessage even though the feature keeps retrying. if apierrors.IsForbidden(result.Err) { logger.V(logs.LogInfo).Info("undeploying failing because of missing permission.") - tmpStatus := libsveltosv1beta1.FeatureStatusRemoving - status = &tmpStatus - r.updateFeatureStatus(clusterSummaryScope, f.id, status, nil, result.Err, logger) + r.markUndeployFailureRetriable(clusterSummaryScope, f.id, result.Err, logger) return nil } @@ -535,14 +535,12 @@ func (r *ClusterSummaryReconciler) processUndeployResult(ctx context.Context, cl var nonRetriableError *configv1beta1.NonRetriableError if errors.As(result.Err, &nonRetriableError) { - removing := libsveltosv1beta1.FeatureStatusRemoving - r.updateFeatureStatus(clusterSummaryScope, f.id, &removing, nil, result.Err, logger) + r.markUndeployFailureRetriable(clusterSummaryScope, f.id, result.Err, logger) return result.Err } var templateError *configv1beta1.TemplateInstantiationError if errors.As(result.Err, &templateError) { - removing := libsveltosv1beta1.FeatureStatusRemoving - r.updateFeatureStatus(clusterSummaryScope, f.id, &removing, nil, result.Err, logger) + r.markUndeployFailureRetriable(clusterSummaryScope, f.id, result.Err, logger) return result.Err } @@ -587,6 +585,17 @@ func (r *ClusterSummaryReconciler) processUndeployResult(ctx context.Context, cl return fmt.Errorf("cleanup request is queued") } +// markUndeployFailureRetriable sets the feature status back to Removing (undeploy will be retried) +// while still recording failureErr as the FailureMessage, so the reason is not silently dropped. +func (r *ClusterSummaryReconciler) markUndeployFailureRetriable(clusterSummaryScope *scope.ClusterSummaryScope, + featureID libsveltosv1beta1.FeatureID, failureErr error, logger logr.Logger) { + + removing := libsveltosv1beta1.FeatureStatusRemoving + r.updateFeatureStatus(clusterSummaryScope, featureID, &removing, nil, failureErr, logger) + errorMsg := failureErr.Error() + clusterSummaryScope.SetFailureMessage(featureID, &errorMsg) +} + func cleanPreviousActiveTracking(ctx context.Context, clusterSummary *configv1beta1.ClusterSummary, f feature, logger logr.Logger) error { diff --git a/controllers/clustersummary_deployer_test.go b/controllers/clustersummary_deployer_test.go index 2650d8e7..508192f5 100644 --- a/controllers/clustersummary_deployer_test.go +++ b/controllers/clustersummary_deployer_test.go @@ -27,7 +27,9 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2/textlogger" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" @@ -529,6 +531,58 @@ var _ = Describe("ClustersummaryDeployer", func() { Expect(dep.IsKeyInProgress(key)).To(BeTrue()) }) + It("undeployFeature sets FailureMessage when undeploy fails with a Forbidden error", func() { + clusterSummary.Spec.ClusterProfileSpec.PolicyRefs = []configv1beta1.PolicyRef{ + { + Namespace: randomString(), + Name: randomString(), + Kind: string(libsveltosv1beta1.ConfigMapReferencedResourceKind), + }, + } + clusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{ + {FeatureID: libsveltosv1beta1.FeatureResources, Status: libsveltosv1beta1.FeatureStatusRemoving}, + } + + initObjects := []client.Object{ + clusterSummary, + clusterProfile, + cluster, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...).WithObjects(initObjects...).Build() + + clusterSummaryScope := getClusterSummaryScope(c, logger, clusterProfile, clusterSummary) + + // An admission webhook denying the delete request surfaces to the client as a Forbidden + // error, same as a genuine RBAC permission error. Verify the underlying message is not lost. + webhookDenialErr := apierrors.NewForbidden(schema.GroupResource{Group: "network.kmetal.io", Resource: "eipclaims"}, + "external", fmt.Errorf("admission webhook \"veipclaim.kb.io\" denied the request: cannot delete EipClaim")) + + dep := fakedeployer.GetClient(context.TODO(), textlogger.NewLogger(textlogger.NewConfig()), c) + dep.StoreResult(clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, + clusterSummary.Name, string(libsveltosv1beta1.FeatureResources), libsveltosv1beta1.ClusterTypeCapi, true, + webhookDenialErr) + + reconciler := getClusterSummaryReconciler(c, dep) + + f := controllers.GetHandlersForFeature(libsveltosv1beta1.FeatureResources) + + err := controllers.UndeployFeature(reconciler, context.TODO(), clusterSummaryScope, f, textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + var featureSummary *configv1beta1.FeatureSummary + for i := range clusterSummaryScope.ClusterSummary.Status.FeatureSummaries { + fs := &clusterSummaryScope.ClusterSummary.Status.FeatureSummaries[i] + if fs.FeatureID == libsveltosv1beta1.FeatureResources { + featureSummary = fs + } + } + Expect(featureSummary).ToNot(BeNil()) + Expect(featureSummary.Status).To(Equal(libsveltosv1beta1.FeatureStatusRemoving)) + Expect(featureSummary.FailureMessage).ToNot(BeNil()) + Expect(*featureSummary.FailureMessage).To(Equal(webhookDenialErr.Error())) + }) + //nolint: dupl // better readibility of test It("deployFeature return an error if cleaning up is in progress", func() { clusterSummary.Spec.ClusterProfileSpec.PolicyRefs = []configv1beta1.PolicyRef{