From 0949df0baa0d273743d357d8246882a82f409ec5 Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Wed, 16 Sep 2026 10:32:32 -0400 Subject: [PATCH 1/3] fix: add guard for deletion depending on if CR owns apiservice Signed-off-by: Christopher Coco --- .../controllers/gitopspromoter/apiservice.go | 9 +++++ .../gitopspromoter/apiservice_test.go | 35 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/argocd-operator/controllers/gitopspromoter/apiservice.go b/argocd-operator/controllers/gitopspromoter/apiservice.go index 83035d9589f..5b30108ad73 100644 --- a/argocd-operator/controllers/gitopspromoter/apiservice.go +++ b/argocd-operator/controllers/gitopspromoter/apiservice.go @@ -57,6 +57,15 @@ func ReconcilePromoterAPIServerAPIService(client client.Client, compName string, if exists { if !cr.Spec.Promoter.IsEnabled() || !enabled || !allowed { + // Check to see if referenced service was created by the CR if it is not do not delete + // This safe guards from another ArgoCD CR instance from deleting the APIService even if the promoter is disabled + // If the service field is for some reason nil be safe and skip deleting anyway + ownsAPISvc := apiSvc.Spec.Service != nil && cr.Namespace == apiSvc.Spec.Service.Namespace && generatePromoterResourceName(compName, cr) == apiSvc.Spec.Service.Name + + if !ownsAPISvc { + return apiSvc, nil + } + argoutil.LogResourceDeletion(log, apiSvc, fmt.Sprintf("promoter apiservice for component %s is being deleted due to being disabled", compName)) if err := client.Delete(context.Background(), apiSvc); err != nil { return nil, fmt.Errorf("failed to delete promoter service %s: %v", apiSvc.Name, err) diff --git a/argocd-operator/controllers/gitopspromoter/apiservice_test.go b/argocd-operator/controllers/gitopspromoter/apiservice_test.go index 7db5f17b3f7..067f5cefaa7 100644 --- a/argocd-operator/controllers/gitopspromoter/apiservice_test.go +++ b/argocd-operator/controllers/gitopspromoter/apiservice_test.go @@ -392,3 +392,38 @@ func TestReconcilePromoterAPIServerAPIService_Exists_Update(t *testing.T) { assert.Equal(t, cr.Namespace, retrievedAPIService.Spec.Service.Namespace) assert.Equal(t, ptr.To(int32(APIServerPort)), retrievedAPIService.Spec.Service.Port) } + +func TestReconcilePromoterAPIServerAPIService_MultipleCRsPresent(t *testing.T) { + // Test case: APIService gets reconciled then another CR with the promoter disabled gets reconciled + // Expected behavior: the APIService should not get deleted + + crEnabled := makeTestArgoCD(withPromoterEnabled(true), withPromoterAPIServerEnabled(true)) + crDisabled := makeTestArgoCD(withPromoterEnabled(false), withPromoterAPIServerEnabled(false)) + crDisabled.Name = "disabled-cr" + crDisabled.Namespace = "different-namespace" + + resObjs := []client.Object{crEnabled, crDisabled} + sch := makeTestReconcilerScheme() + client := makeTestReconcilerClient(sch, resObjs) + + apiService, err := ReconcilePromoterAPIServerAPIService(client, testCompName, crEnabled) + assert.NoError(t, err) + assert.NotNil(t, apiService) + + // Make sure APIService as created + retrievedAPIService := &apiregistrationv1.APIService{} + err = client.Get(context.Background(), types.NamespacedName{ + Name: "v1alpha1.view.promoter.argoproj.io", + }, retrievedAPIService) + assert.NoError(t, err) + + apiService, err = ReconcilePromoterAPIServerAPIService(client, testCompName, crDisabled) + assert.NoError(t, err) + assert.NotNil(t, apiService) + + // Get APIService again to make sure it is not deleted + err = client.Get(context.Background(), types.NamespacedName{ + Name: "v1alpha1.view.promoter.argoproj.io", + }, retrievedAPIService) + assert.NoError(t, err) +} From 46741b81edcdc85008b2704499152346bc6e2ab9 Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Wed, 16 Sep 2026 11:35:34 -0400 Subject: [PATCH 2/3] fix: address coderabbit feedback Signed-off-by: Christopher Coco --- argocd-operator/controllers/argocd/util.go | 4 ++-- argocd-operator/controllers/gitopspromoter/apiservice.go | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/argocd-operator/controllers/argocd/util.go b/argocd-operator/controllers/argocd/util.go index 354b36a46c6..e308a7aa8b5 100644 --- a/argocd-operator/controllers/argocd/util.go +++ b/argocd-operator/controllers/argocd/util.go @@ -734,7 +734,8 @@ func (r *ReconcileArgoCD) deleteClusterResources(cr *argoproj.ArgoCD) error { return fmt.Errorf("failed to filter APIServices for %s: %w", cr.Name, err) } - if err := gitopspromoter.DeleteAPIServices(r.Client, apiSvcList); err != nil { + apiServerCompName := string(argoproj.PromoterComponentTypeAPIServer) + if err := gitopspromoter.DeleteAPIServices(r.Client, apiSvcList, apiServerCompName, cr); err != nil { return err } @@ -826,7 +827,6 @@ func removeString(slice []string, s string) []string { // setResourceWatches will register Watches for each of the supported Resources. func (r *ReconcileArgoCD) setResourceWatches(bldr *builder.Builder, clusterResourceMapper, tlsSecretMapper, namespaceResourceMapper, clusterSecretResourceMapper, applicationSetGitlabSCMTLSConfigMapMapper, nmMapper, systemCATrustMapper, imagePullSecretMapper handler.MapFunc) *builder.Builder { - // Add new predicate to delete Notifications Resources. The predicate watches the Argo CD CR for changes to the `.spec.Notifications.Enabled` // field. When a change is detected that results in notifications being disabled, we trigger deletion of notifications resources deleteNotificationsPred := predicate.Funcs{ diff --git a/argocd-operator/controllers/gitopspromoter/apiservice.go b/argocd-operator/controllers/gitopspromoter/apiservice.go index 5b30108ad73..941806408f4 100644 --- a/argocd-operator/controllers/gitopspromoter/apiservice.go +++ b/argocd-operator/controllers/gitopspromoter/apiservice.go @@ -61,7 +61,6 @@ func ReconcilePromoterAPIServerAPIService(client client.Client, compName string, // This safe guards from another ArgoCD CR instance from deleting the APIService even if the promoter is disabled // If the service field is for some reason nil be safe and skip deleting anyway ownsAPISvc := apiSvc.Spec.Service != nil && cr.Namespace == apiSvc.Spec.Service.Namespace && generatePromoterResourceName(compName, cr) == apiSvc.Spec.Service.Name - if !ownsAPISvc { return apiSvc, nil } @@ -158,8 +157,13 @@ func buildAPIServiceSpec(client client.Client, compName string, cr *argoproj.Arg } // DeleteAPIServices deletes a list of API Services -func DeleteAPIServices(c client.Client, apiSvcList *apiregistrationv1.APIServiceList) error { +func DeleteAPIServices(c client.Client, apiSvcList *apiregistrationv1.APIServiceList, compName string, cr *argoproj.ArgoCD) error { for _, apiSvc := range apiSvcList.Items { + ownsAPISvc := apiSvc.Spec.Service != nil && cr.Namespace == apiSvc.Spec.Service.Namespace && generatePromoterResourceName(compName, cr) == apiSvc.Spec.Service.Name + if !ownsAPISvc { + continue + } + argoutil.LogResourceDeletion(log, &apiSvc, "cleaning up cluster resources") if err := c.Delete(context.TODO(), &apiSvc); err != nil { return fmt.Errorf("failed to delete APIService %s during cleanup: %w", apiSvc.Name, err) From a9636699cffa38e96ab566d589e07b908b764d0e Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Wed, 16 Sep 2026 14:26:23 -0400 Subject: [PATCH 3/3] fix: address feedback Signed-off-by: Christopher Coco --- .../controllers/gitopspromoter/apiservice.go | 24 ++++++++++--------- .../gitopspromoter/apiservice_test.go | 2 -- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/argocd-operator/controllers/gitopspromoter/apiservice.go b/argocd-operator/controllers/gitopspromoter/apiservice.go index 941806408f4..bb97849641f 100644 --- a/argocd-operator/controllers/gitopspromoter/apiservice.go +++ b/argocd-operator/controllers/gitopspromoter/apiservice.go @@ -56,18 +56,17 @@ func ReconcilePromoterAPIServerAPIService(client client.Client, compName string, } if exists { - if !cr.Spec.Promoter.IsEnabled() || !enabled || !allowed { - // Check to see if referenced service was created by the CR if it is not do not delete - // This safe guards from another ArgoCD CR instance from deleting the APIService even if the promoter is disabled - // If the service field is for some reason nil be safe and skip deleting anyway - ownsAPISvc := apiSvc.Spec.Service != nil && cr.Namespace == apiSvc.Spec.Service.Namespace && generatePromoterResourceName(compName, cr) == apiSvc.Spec.Service.Name - if !ownsAPISvc { - return apiSvc, nil - } + // Check to see if referenced service was created by the CR if it is not do not do any operations on it + // This safe guards from another ArgoCD CR instance from deleting the APIService even if the promoter is disabled + // If the service field is for some reason nil be safe and skip deleting anyway + if !ownsAPISvc(compName, cr, apiSvc) { + return apiSvc, nil + } + if !cr.Spec.Promoter.IsEnabled() || !enabled || !allowed { argoutil.LogResourceDeletion(log, apiSvc, fmt.Sprintf("promoter apiservice for component %s is being deleted due to being disabled", compName)) if err := client.Delete(context.Background(), apiSvc); err != nil { - return nil, fmt.Errorf("failed to delete promoter service %s: %v", apiSvc.Name, err) + return nil, fmt.Errorf("failed to delete promoter api service %s: %v", apiSvc.Name, err) } return apiSvc, nil } @@ -159,8 +158,7 @@ func buildAPIServiceSpec(client client.Client, compName string, cr *argoproj.Arg // DeleteAPIServices deletes a list of API Services func DeleteAPIServices(c client.Client, apiSvcList *apiregistrationv1.APIServiceList, compName string, cr *argoproj.ArgoCD) error { for _, apiSvc := range apiSvcList.Items { - ownsAPISvc := apiSvc.Spec.Service != nil && cr.Namespace == apiSvc.Spec.Service.Namespace && generatePromoterResourceName(compName, cr) == apiSvc.Spec.Service.Name - if !ownsAPISvc { + if !ownsAPISvc(compName, cr, &apiSvc) { continue } @@ -171,3 +169,7 @@ func DeleteAPIServices(c client.Client, apiSvcList *apiregistrationv1.APIService } return nil } + +func ownsAPISvc(compName string, cr *argoproj.ArgoCD, apiSvc *apiregistrationv1.APIService) bool { + return apiSvc.Spec.Service != nil && cr.Namespace == apiSvc.Spec.Service.Namespace && generatePromoterResourceName(compName, cr) == apiSvc.Spec.Service.Name +} diff --git a/argocd-operator/controllers/gitopspromoter/apiservice_test.go b/argocd-operator/controllers/gitopspromoter/apiservice_test.go index 067f5cefaa7..afeb5461b84 100644 --- a/argocd-operator/controllers/gitopspromoter/apiservice_test.go +++ b/argocd-operator/controllers/gitopspromoter/apiservice_test.go @@ -370,8 +370,6 @@ func TestReconcilePromoterAPIServerAPIService_Exists_Update(t *testing.T) { cr := makeTestArgoCD(withPromoterEnabled(true), withPromoterAPIServerEnabled(true)) existingAPIService := makeExistingAPIService(cr) - existingAPIService.Spec.Service.Name = "not-a-real-service" - existingAPIService.Spec.Service.Namespace = "not-a-real-namespace" existingAPIService.Spec.Service.Port = ptr.To(int32(25565)) resObjs := []client.Object{cr, existingAPIService}