diff --git a/pkg/controller/goldmane/controller.go b/pkg/controller/goldmane/controller.go index e1e93b1b50..ff8bf02fbc 100644 --- a/pkg/controller/goldmane/controller.go +++ b/pkg/controller/goldmane/controller.go @@ -18,7 +18,9 @@ import ( "context" "fmt" + "github.com/go-logr/logr" v1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -154,13 +156,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( if err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying Goldmane CR", err, reqLogger) return reconcile.Result{}, err - } else if goldmaneCR == nil { - r.status.OnCRNotFound() - return reconcile.Result{}, r.maintainFinalizer(ctx, nil) } - r.status.OnCRFound() - // SetMetaData in the TigeraStatus such as observedGenerations. - defer r.status.SetMetaData(&goldmaneCR.ObjectMeta) variant, installation, err := utils.GetInstallation(ctx, r.cli) if err != nil { @@ -169,6 +165,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, nil } + if goldmaneCR == nil { + r.status.OnCRNotFound() + // Handle deletion: delete deployment before removing finalizer + return r.handleDeletion(ctx, variant, installation, reqLogger) + } + r.status.OnCRFound() + // SetMetaData in the TigeraStatus such as observedGenerations. + defer r.status.SetMetaData(&goldmaneCR.ObjectMeta) + mgmtClusterConnectionCR, err := utils.GetIfExists[operatorv1.ManagementClusterConnection](ctx, utils.DefaultTSEEInstanceKey, r.cli) if err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying ManagementClusterConnection CR", err, reqLogger) @@ -254,6 +259,31 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, nil } +func (r *Reconciler) handleDeletion(ctx context.Context, variant operatorv1.ProductVariant, installation *operatorv1.InstallationSpec, reqLogger logr.Logger) (reconcile.Result, error) { + // When the Goldmane CR is deleted, we need to clean up the deployment before removing the finalizer. + // Delete the goldmane deployment directly + goldmaneDeployment := &v1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: goldmane.GoldmaneDeploymentName, + Namespace: goldmane.GoldmaneNamespace, + }, + } + + err := r.cli.Delete(ctx, goldmaneDeployment) + if err != nil && !errors.IsNotFound(err) { + reqLogger.Error(err, "Error deleting goldmane deployment during cleanup") + return reconcile.Result{}, err + } + + // Now try to maintain the finalizer - it will only be removed if all resources and pods are cleaned up + if err := r.maintainFinalizer(ctx, nil); err != nil { + reqLogger.Error(err, "Error maintaining finalizer during deletion") + return reconcile.Result{}, err + } + + return reconcile.Result{}, nil +} + func (r *Reconciler) maintainFinalizer(ctx context.Context, goldmaneCr client.Object) error { // These objects require graceful termination before the CNI plugin is torn down. goldmaneDeployment := &v1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: common.CalicoNamespace, Name: goldmane.GoldmaneDeploymentName}} diff --git a/pkg/controller/policyrecommendation/policyrecommendation_controller.go b/pkg/controller/policyrecommendation/policyrecommendation_controller.go index 1f0109dfe6..e2455d3238 100644 --- a/pkg/controller/policyrecommendation/policyrecommendation_controller.go +++ b/pkg/controller/policyrecommendation/policyrecommendation_controller.go @@ -471,26 +471,8 @@ func (r *ReconcilePolicyRecommendation) Reconcile(ctx context.Context, request r return reconcile.Result{}, err } - setUp := render.NewSetup(&render.SetUpConfiguration{ - OpenShift: r.provider.IsOpenShift(), - Installation: installation, - PullSecrets: pullSecrets, - Namespace: helper.InstallNamespace(), - PSS: render.PSSRestricted, - CreateNamespace: !tenant.MultiTenant(), - }) - // Prepend PolicyRecommendation before certificate creation components = append([]render.Component{component}, components...) - setupHandler := defaultHandler - if tenant.MultiTenant() { - // In standard installs, the PolicyRecommendation CR owns all the objects. For multi-tenant, pull secrets are owned by the Tenant instance. - setupHandler = utils.NewComponentHandler(log, r.client, r.scheme, tenant) - } - if err := setupHandler.CreateOrUpdateOrDelete(ctx, setUp, r.status); err != nil { - r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error creating / updating resource", err, logc) - return reconcile.Result{}, err - } for _, cmp := range components { if err := defaultHandler.CreateOrUpdateOrDelete(context.Background(), cmp, r.status); err != nil { diff --git a/pkg/controller/policyrecommendation/policyrecommendation_controller_test.go b/pkg/controller/policyrecommendation/policyrecommendation_controller_test.go index 3c2935c86f..2da2525e9e 100644 --- a/pkg/controller/policyrecommendation/policyrecommendation_controller_test.go +++ b/pkg/controller/policyrecommendation/policyrecommendation_controller_test.go @@ -169,45 +169,6 @@ var _ = Describe("PolicyRecommendation controller tests", func() { r.policyRecScopeWatchReady.MarkAsReady() }) - It("should reconcile namespace, role binding and pull secrts", func() { - result, err := r.Reconcile(ctx, reconcile.Request{}) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(0 * time.Second)) - - namespace := corev1.Namespace{ - TypeMeta: metav1.TypeMeta{Kind: "Namespace", APIVersion: "v1"}, - } - Expect(c.Get(ctx, client.ObjectKey{ - Name: render.PolicyRecommendationNamespace, - }, &namespace)).NotTo(HaveOccurred()) - Expect(namespace.Labels["pod-security.kubernetes.io/enforce"]).To(Equal("restricted")) - Expect(namespace.Labels["pod-security.kubernetes.io/enforce-version"]).To(Equal("latest")) - - // Expect operator role binding to be created - rb := rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{}, - } - Expect(c.Get(ctx, client.ObjectKey{ - Name: render.TigeraOperatorSecrets, - Namespace: render.PolicyRecommendationNamespace, - }, &rb)).NotTo(HaveOccurred()) - Expect(rb.OwnerReferences).To(HaveLen(1)) - ownerRoleBinding := rb.OwnerReferences[0] - Expect(ownerRoleBinding.Kind).To(Equal("PolicyRecommendation")) - - // Expect pull secrets to be created - pullSecrets := corev1.Secret{ - TypeMeta: metav1.TypeMeta{Kind: "Secret", APIVersion: "v1"}, - } - Expect(c.Get(ctx, client.ObjectKey{ - Name: "tigera-pull-secret", - Namespace: render.PolicyRecommendationNamespace, - }, &pullSecrets)).NotTo(HaveOccurred()) - Expect(pullSecrets.OwnerReferences).To(HaveLen(1)) - pullSecret := pullSecrets.OwnerReferences[0] - Expect(pullSecret.Kind).To(Equal("PolicyRecommendation")) - }) - Context("image reconciliation", func() { It("should use builtin images", func() { _, err := r.Reconcile(ctx, reconcile.Request{}) @@ -492,64 +453,6 @@ var _ = Describe("PolicyRecommendation controller tests", func() { _, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: tenantANamespace}}) Expect(err).ShouldNot(HaveOccurred()) }) - - It("should reconcile pull secrets and role bindings", func() { - // Create the Tenant resources for tenant-a. - tenantA := &operatorv1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: tenantANamespace, - }, - Spec: operatorv1.TenantSpec{ID: "tenant-a"}, - } - Expect(c.Create(ctx, tenantA)).NotTo(HaveOccurred()) - certificateManagerTenantA, err := certificatemanager.Create(c, nil, "", tenantANamespace, certificatemanager.AllowCACreation(), certificatemanager.WithTenant(tenantA)) - Expect(err).NotTo(HaveOccurred()) - Expect(c.Create(ctx, certificateManagerTenantA.KeyPair().Secret(tenantANamespace))) - Expect(c.Create(ctx, certificateManagerTenantA.CreateTrustedBundle().ConfigMap(tenantANamespace))).NotTo(HaveOccurred()) - - linseedTLSTenantA, err := certificateManagerTenantA.GetOrCreateKeyPair(c, render.TigeraLinseedSecret, tenantANamespace, []string{render.TigeraLinseedSecret}) - Expect(err).NotTo(HaveOccurred()) - Expect(c.Create(ctx, linseedTLSTenantA.Secret(tenantANamespace))).NotTo(HaveOccurred()) - - Expect(c.Create(ctx, &operatorv1.PolicyRecommendation{ - ObjectMeta: metav1.ObjectMeta{ - Name: "tigera-secure", - Namespace: tenantANamespace, - }, - })).NotTo(HaveOccurred()) - - _, err = r.Reconcile(ctx, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: tenantANamespace, - }, - }) - Expect(err).ShouldNot(HaveOccurred()) - - // Expect operator role binding to be created - rb := rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{}, - } - Expect(c.Get(ctx, client.ObjectKey{ - Name: render.TigeraOperatorSecrets, - Namespace: tenantANamespace, - }, &rb)).NotTo(HaveOccurred()) - Expect(rb.OwnerReferences).To(HaveLen(1)) - ownerRoleBinding := rb.OwnerReferences[0] - Expect(ownerRoleBinding.Kind).To(Equal("Tenant")) - - // Expect pull secrets to be created - pullSecrets := corev1.Secret{ - TypeMeta: metav1.TypeMeta{Kind: "Secret", APIVersion: "v1"}, - } - Expect(c.Get(ctx, client.ObjectKey{ - Name: "tigera-pull-secret", - Namespace: tenantANamespace, - }, &pullSecrets)).NotTo(HaveOccurred()) - Expect(pullSecrets.OwnerReferences).To(HaveLen(1)) - pullSecret := pullSecrets.OwnerReferences[0] - Expect(pullSecret.Kind).To(Equal("Tenant")) - }) }) }) }) diff --git a/pkg/controller/whisker/controller.go b/pkg/controller/whisker/controller.go index c26eacbabf..039bba7c30 100644 --- a/pkg/controller/whisker/controller.go +++ b/pkg/controller/whisker/controller.go @@ -18,7 +18,9 @@ import ( "context" "fmt" + "github.com/go-logr/logr" v1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -157,13 +159,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( if err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying Whisker CR", err, reqLogger) return reconcile.Result{}, err - } else if whiskerCR == nil { - r.status.OnCRNotFound() - return reconcile.Result{}, r.maintainFinalizer(ctx, nil) } - r.status.OnCRFound() - // SetMetaData in the TigeraStatus such as observedGenerations. - defer r.status.SetMetaData(&whiskerCR.ObjectMeta) variant, installation, err := utils.GetInstallation(ctx, r.cli) if err != nil { @@ -172,6 +168,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, nil } + if whiskerCR == nil { + r.status.OnCRNotFound() + // Handle deletion: process components to clean up resources before removing finalizer + return r.handleDeletion(ctx, variant, installation, reqLogger) + } + r.status.OnCRFound() + // SetMetaData in the TigeraStatus such as observedGenerations. + defer r.status.SetMetaData(&whiskerCR.ObjectMeta) + if goldmaneCR, err := utils.GetIfExists[operatorv1.Goldmane](ctx, utils.DefaultInstanceKey, r.cli); err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying for Goldmane CR", err, reqLogger) return reconcile.Result{}, err @@ -278,6 +283,31 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( return reconcile.Result{}, nil } +func (r *Reconciler) handleDeletion(ctx context.Context, variant operatorv1.ProductVariant, installation *operatorv1.InstallationSpec, reqLogger logr.Logger) (reconcile.Result, error) { + // When the Whisker CR is deleted, we need to clean up the deployment before removing the finalizer. + // Delete the whisker deployment directly + whiskerDeployment := &v1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: whisker.WhiskerDeploymentName, + Namespace: whisker.WhiskerNamespace, + }, + } + + err := r.cli.Delete(ctx, whiskerDeployment) + if err != nil && !errors.IsNotFound(err) { + reqLogger.Error(err, "Error deleting whisker deployment during cleanup") + return reconcile.Result{}, err + } + + // Now try to maintain the finalizer - it will only be removed if all resources and pods are cleaned up + if err := r.maintainFinalizer(ctx, nil); err != nil { + reqLogger.Error(err, "Error maintaining finalizer during deletion") + return reconcile.Result{}, err + } + + return reconcile.Result{}, nil +} + func updateWhiskerWithDefaults(instance *operatorv1.Whisker) { if instance.Spec.Notifications == nil { instance.Spec.Notifications = ptr.ToPtr(operatorv1.Enabled) diff --git a/test/whisker_test.go b/test/whisker_test.go index 6402c60fdd..622d17aa69 100644 --- a/test/whisker_test.go +++ b/test/whisker_test.go @@ -75,6 +75,9 @@ var _ = Describe("Tests for Whisker installation", func() { }) AfterEach(func() { + By("Cleaning up resources after the test") + cleanupResources(c) + defer func() { cancel() Eventually(func() error { @@ -87,9 +90,6 @@ var _ = Describe("Tests for Whisker installation", func() { }, 60*time.Second).ShouldNot(HaveOccurred()) }() - By("Cleaning up resources after the test") - cleanupResources(c) - // Clean up Calico data that might be left behind. Eventually(func() error { cs := kubernetes.NewForConfigOrDie(mgr.GetConfig()) @@ -143,7 +143,7 @@ var _ = Describe("Tests for Whisker installation", func() { Expect(install.ObjectMeta.Finalizers).To(ContainElement(render.WhiskerFinalizer)) Expect(install.ObjectMeta.Finalizers).To(ContainElement(render.GoldmaneFinalizer)) - By("Verifying that the whisker finalizer is removed in the installation CR") + By("Verifying that the whisker and goldmane finalizers are removed from the installation CR") Expect(c.Delete(context.Background(), whiskerCR)).To(BeNil()) Expect(c.Delete(context.Background(), goldmaneCR)).To(BeNil()) Eventually(func() error { @@ -151,7 +151,7 @@ var _ = Describe("Tests for Whisker installation", func() { fmt.Println("Finalizers: ", install.ObjectMeta.Finalizers) if containsFinalizer(install.ObjectMeta.Finalizers, render.WhiskerFinalizer) || containsFinalizer(install.ObjectMeta.Finalizers, render.GoldmaneFinalizer) { - return fmt.Errorf("expected finalizers to be removed, but found: %v", install.ObjectMeta.Finalizers) + return fmt.Errorf("expected whisker and goldmane finalizers to be removed, but found: %v", install.ObjectMeta.Finalizers) } return nil }, 1*time.Minute, 1*time.Second).Should(BeNil())