Repository: argoproj-labs/argocd-operator
Pull request #797 (2022-10-13 18:19:03)

fix: Emit sso deprecation event only once for each instance that needs it
**What type of PR is this?**
> Uncomment only one ` /kind` line, and delete the rest.
> For example, `> /kind bug` would simply become: `/kind bug`

 /kind bug
> /kind chore
> /kind cleanup
> /kind failing-test
> /kind enhancement
> /kind documentation
> /kind code-refactoring


**What does this PR do / why we need it**:
This PR fixes a flaw in an earlier fix meant to stop the spamming of SSO related deprecation notice events.

**Original issue:** operator would spam the cluster with deprecation warning events when it identifies deprecated fields in use
**Original fix:** stopped the spamming but made it so that only the first problematic instance would receive deprecation notice events, operator would stop emitting them completely after that point and subsequent instances would not be notified at all.
**Current fix:** ensures that every problematic instance sees exactly one event generated per deprecated field - such that all instance owners are sufficiently notified and no one is spammed with events.


**Have you updated the necessary documentation?**

* [ ] Documentation update is required by this PR.
* [ ] Documentation has been updated.

**Which issue(s) this PR fixes**:

Fixes #?

**How to test changes / Special notes to the reviewer**:
1. Run operator
2. Create multiple argo-cd instances in different namespaces, and ensure they contain either .spec.dex or .spec.sso.verifyTLS or both (for e.g)
3. Ensure that there is only 1 deprecation notice event created for each deprecated field, in each namespace
4. Delete argo-cd instance in one of the namespaces and re-create it with the same faults, another set of events should be generated
5. Delete one of the namespaces containing argo-cd instance and recreate it with the same name and new argo-cd instance with the same faults. Confirm that another set of events is generated.


Patch:
diff --git a/controllers/argocd/argocd_controller.go b/controllers/argocd/argocd_controller.go
index 5f282d87..ffe7952d 100644
--- a/controllers/argocd/argocd_controller.go
+++ b/controllers/argocd/argocd_controller.go
@@ -103,6 +103,12 @@ func (r *ReconcileArgoCD) Reconcile(ctx context.Context, request ctrl.Request) (
 			if err := r.removeDeletionFinalizer(argocd); err != nil {
 				return reconcile.Result{}, err
 			}
+
+			// remove namespace of deleted Argo CD instance from deprecationEventEmissionTracker (if exists) so that if another instance
+			// is created in the same namespace in the future, that instance is appropriately tracked
+			if _, ok := DeprecationEventEmissionTracker[argocd.Namespace]; ok {
+				delete(DeprecationEventEmissionTracker, argocd.Namespace)
+			}
 		}
 		return reconcile.Result{}, nil
 	}
diff --git a/controllers/argocd/sso.go b/controllers/argocd/sso.go
index 709882d6..b3026191 100644
--- a/controllers/argocd/sso.go
+++ b/controllers/argocd/sso.go
@@ -17,6 +17,7 @@ package argocd
 import (
 	"errors"
 	"fmt"
+	"os"
 	"reflect"
 
 	template "github.com/openshift/api/template/v1"
@@ -35,15 +36,23 @@ const (
 	multipleSSOConfiguration string = "multiple SSO configuration: "
 )
 
-var (
-	SSOSpecDeprecationWarningEmitted    bool = false
-	DexSpecDeprecationWarningEmitted    bool = false
-	DisableDexDeprecationWarningEmitted bool = false
+// DeprecationEventEmissionStatus is meant to track which deprecation events have been emitted already. This is temporary and can be removed in v0.0.6 once we have provided enough deprecation notice
+type DeprecationEventEmissionStatus struct {
+	SSOSpecDeprecationWarningEmitted    bool
+	DexSpecDeprecationWarningEmitted    bool
+	DisableDexDeprecationWarningEmitted bool
+}
 
+var (
 	templateAPIFound     = false
 	ssoConfigLegalStatus string
 )
 
+// DeprecationEventEmissionTracker map stores the namespace containing ArgoCD instance as key and DeprecationEventEmissionStatus as value,
+// where DeprecationEventEmissionStatus tracks the events that have been emitted for the instance in the particular namespace.
+// This is temporary and can be removed in v0.0.6 when we remove the deprecated fields.
+var DeprecationEventEmissionTracker = make(map[string]DeprecationEventEmissionStatus)
+
 // IsTemplateAPIAvailable returns true if the template API is present.
 func IsTemplateAPIAvailable() bool {
 	return templateAPIFound
@@ -69,24 +78,41 @@ func (r *ReconcileArgoCD) reconcileSSO(cr *argoprojv1a1.ArgoCD) error {
 	// reset ssoConfigLegalStatus at the beginning of each SSO reconciliation round
 	ssoConfigLegalStatus = ssoLegalUnknown
 
-	// Emit event providing users with deprecation notice for `DISABLE_DEX` if not emitted already
-	if !DisableDexDeprecationWarningEmitted {
-		err := argoutil.CreateEvent(r.Client, "Warning", "Deprecated", "`DISABLE_DEX` is deprecated, and support will be removed in Argo CD Operator v0.6.0/OpenShift GitOps v1.9.0. Dex can be enabled/disabled through `.spec.sso`", "DeprecationNotice", cr.ObjectMeta, cr.TypeMeta)
-		DisableDexDeprecationWarningEmitted = true
-		if err != nil {
-			return err
+	// Emit events warning users about deprecation notice for soon-to-be-removed fields in the CR if being used
+
+	if env := os.Getenv("DISABLE_DEX"); env != "" {
+		// Emit event for each instance providing users with deprecation notice for `DISABLE_DEX` if not emitted already
+		if currentInstanceEventEmissionStatus, ok := DeprecationEventEmissionTracker[cr.Namespace]; !ok || !currentInstanceEventEmissionStatus.DisableDexDeprecationWarningEmitted {
+			err := argoutil.CreateEvent(r.Client, "Warning", "Deprecated", "`DISABLE_DEX` is deprecated, and support will be removed in Argo CD Operator v0.6.0/OpenShift GitOps v1.9.0. Dex can be enabled/disabled through `.spec.sso`", "DeprecationNotice", cr.ObjectMeta, cr.TypeMeta)
+			if err != nil {
+				return err
+			}
+
+			if !ok {
+				currentInstanceEventEmissionStatus = DeprecationEventEmissionStatus{DisableDexDeprecationWarningEmitted: true}
+			} else {
+				currentInstanceEventEmissionStatus.DisableDexDeprecationWarningEmitted = true
+			}
+			DeprecationEventEmissionTracker[cr.Namespace] = currentInstanceEventEmissionStatus
 		}
+
 	}
 
 	if cr.Spec.Dex != nil && !reflect.DeepEqual(cr.Spec.Dex, &v1alpha1.ArgoCDDexSpec{}) {
 
-		// Emit event providing users with deprecation notice for `.spec.dex` if not emitted already
-		if !DexSpecDeprecationWarningEmitted {
+		// Emit event for each instance providing users with deprecation notice for `.spec.dex` if not emitted already
+		if currentInstanceEventEmissionStatus, ok := DeprecationEventEmissionTracker[cr.Namespace]; !ok || !currentInstanceEventEmissionStatus.DexSpecDeprecationWarningEmitted {
 			err := argoutil.CreateEvent(r.Client, "Warning", "Deprecated", "`.spec.dex` is deprecated, and support will be removed in Argo CD Operator v0.6.0/OpenShift GitOps v1.9.0. Dex configuration can be managed through `.spec.sso.dex`", "DeprecationNotice", cr.ObjectMeta, cr.TypeMeta)
-			DexSpecDeprecationWarningEmitted = true
 			if err != nil {
 				return err
 			}
+
+			if !ok {
+				currentInstanceEventEmissionStatus = DeprecationEventEmissionStatus{DexSpecDeprecationWarningEmitted: true}
+			} else {
+				currentInstanceEventEmissionStatus.DexSpecDeprecationWarningEmitted = true
+			}
+			DeprecationEventEmissionTracker[cr.Namespace] = currentInstanceEventEmissionStatus
 		}
 
 	}
@@ -94,15 +120,20 @@ func (r *ReconcileArgoCD) reconcileSSO(cr *argoprojv1a1.ArgoCD) error {
 	if cr.Spec.SSO != nil && (cr.Spec.SSO.Image != "" || cr.Spec.SSO.Version != "" ||
 		cr.Spec.SSO.VerifyTLS != nil || cr.Spec.SSO.Resources != nil) {
 
-		// Emit event providing users with deprecation notice for `.spec.SSO` subfields if not emitted already
-		if !SSOSpecDeprecationWarningEmitted {
+		// Emit event for each instance providing users with deprecation notice for `.spec.SSO` subfields if not emitted already
+		if currentInstanceEventEmissionStatus, ok := DeprecationEventEmissionTracker[cr.Namespace]; !ok || !currentInstanceEventEmissionStatus.SSOSpecDeprecationWarningEmitted {
 			err := argoutil.CreateEvent(r.Client, "Warning", "Deprecated", "`.spec.SSO.Image`, `.spec.SSO.Version`, `.spec.SSO.Resources` and `.spec.SSO.VerifyTLS` are deprecated, and support will be removed in Argo CD Operator v0.6.0/OpenShift GitOps v1.9.0. Keycloak configuration can be managed through `.spec.sso.keycloak`", "DeprecationNotice", cr.ObjectMeta, cr.TypeMeta)
-			SSOSpecDeprecationWarningEmitted = true
 			if err != nil {
 				return err
 			}
-		}
 
+			if !ok {
+				currentInstanceEventEmissionStatus = DeprecationEventEmissionStatus{SSOSpecDeprecationWarningEmitted: true}
+			} else {
+				currentInstanceEventEmissionStatus.SSOSpecDeprecationWarningEmitted = true
+			}
+			DeprecationEventEmissionTracker[cr.Namespace] = currentInstanceEventEmissionStatus
+		}
 	}
 
 	// case 1
diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go
index 044be379..b25a04b4 100644
--- a/controllers/argocd/util.go
+++ b/controllers/argocd/util.go
@@ -1219,6 +1219,13 @@ func namespaceFilterPredicate() predicate.Predicate {
 					log.Info(fmt.Sprintf("Successfully deleted namespace %s from cluster secret", e.Object.GetName()))
 				}
 			}
+
+			// if a namespace is deleted, remove it from deprecationEventEmissionTracker (if exists) so that if a namespace with the same name
+			// is created in the future and contains an Argo CD instance, it will be tracked appropriately
+			if _, ok := DeprecationEventEmissionTracker[e.Object.GetName()]; ok {
+				delete(DeprecationEventEmissionTracker, e.Object.GetName())
+			}
+
 			return false
 		},
 	}


Test patch:
diff --git a/controllers/argocd/sso_test.go b/controllers/argocd/sso_test.go
index 4f8fdf0e..324a332b 100644
--- a/controllers/argocd/sso_test.go
+++ b/controllers/argocd/sso_test.go
@@ -349,9 +349,7 @@ func TestReconcile_illegalSSOConfiguration(t *testing.T) {
 func TestReconcile_emitEventOnDetectingDeprecatedFields(t *testing.T) {
 	logf.SetLogger(ZapLogger(true))
 
-	SSOSpecDeprecationWarningEmitted = false
-	DexSpecDeprecationWarningEmitted = false
-	DisableDexDeprecationWarningEmitted = false
+	DeprecationEventEmissionTracker = make(map[string]DeprecationEventEmissionStatus)
 
 	disableDexEvent := &corev1.Event{
 		Reason:  "DeprecationNotice",