diff --git a/controllers/argocd/argocd_controller_test.go b/controllers/argocd/argocd_controller_test.go index 18a655d97..6700d3b03 100644 --- a/controllers/argocd/argocd_controller_test.go +++ b/controllers/argocd/argocd_controller_test.go @@ -75,7 +75,9 @@ func TestReconcileArgoCD_Reconcile_with_deleted(t *testing.T) { func TestReconcileArgoCD_Reconcile(t *testing.T) { logf.SetLogger(ZapLogger(true)) - a := makeTestArgoCD() + a := makeTestArgoCD(func(a *argov1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = true + }) r := makeTestReconciler(t, a) assert.NoError(t, createNamespace(r, a.Namespace, "")) diff --git a/controllers/argocd/notifications.go b/controllers/argocd/notifications.go index 79a4766ed..d4e29d505 100644 --- a/controllers/argocd/notifications.go +++ b/controllers/argocd/notifications.go @@ -77,34 +77,33 @@ func (r *ReconcileArgoCD) deleteNotificationsResources(cr *argoprojv1a1.ArgoCD) } } - log.Info("reconciling notifications deployment") - if err := r.reconcileNotificationsDeployment(cr, sa); err != nil { + log.Info("delete notifications deployment") + if err := r.deleteNotificationsDeployment(cr); err != nil { return err } - log.Info("reconciling notifications secret") - if err := r.reconcileNotificationsSecret(cr); err != nil { + log.Info("delete notifications secret") + if err := r.deleteNotificationsSecret(cr); err != nil { return err } - log.Info("reconciling notifications configmap") - if err := r.reconcileNotificationsConfigMap(cr); err != nil { + log.Info("delete notifications configmap") + if err := r.deleteNotificationsConfigMap(cr); err != nil { return err } - log.Info("reconciling notifications role binding") - if err := r.reconcileNotificationsRoleBinding(cr, role, sa); err != nil { + log.Info("delete notifications role binding") + if err := r.deleteNotificationsRoleBinding(cr); err != nil { return err } - log.Info("reconciling notifications role") - _, err := r.reconcileNotificationsRole(cr) - if err != nil { + log.Info("delete notifications role") + if err := r.deleteNotificationsRole(cr); err != nil { return err } - log.Info("reconciling notifications serviceaccount") - _, err = r.reconcileNotificationsServiceAccount(cr) + log.Info("delete notifications serviceaccount") + err := r.deleteNotificationsServiceAccount(cr, sa) if err != nil { return err } @@ -113,37 +112,25 @@ func (r *ReconcileArgoCD) deleteNotificationsResources(cr *argoprojv1a1.ArgoCD) } func (r *ReconcileArgoCD) reconcileNotificationsServiceAccount(cr *argoprojv1a1.ArgoCD) (*corev1.ServiceAccount, error) { - sa := newServiceAccountWithName(common.ArgoCDNotificationsControllerComponent, cr) + err := argoutil.FetchObject(r.Client, cr.Namespace, sa.Name, sa) - if err := argoutil.FetchObject(r.Client, cr.Namespace, sa.Name, sa); err != nil { - if !errors.IsNotFound(err) { - return nil, fmt.Errorf("failed to get the serviceAccount associated with %s : %s", sa.Name, err) - } - - // SA doesn't exist and shouldn't, nothing to do here - if !cr.Spec.Notifications.Enabled { - return nil, nil - } - - // SA doesn't exist but should, so it should be created - if err := controllerutil.SetControllerReference(cr, sa, r.Scheme); err != nil { - return nil, err - } - - log.Info(fmt.Sprintf("Creating serviceaccount %s", sa.Name)) - err := r.Client.Create(context.TODO(), sa) - if err != nil { - return nil, err + if err != nil { + // handle case when service account doesn't exist + if errors.IsNotFound(err) { + if err := controllerutil.SetControllerReference(cr, sa, r.Scheme); err != nil { + return nil, err + } + log.Info(fmt.Sprintf("Creating serviceaccount %s", sa.Name)) + if err := r.Client.Create(context.TODO(), sa); err != nil { + return nil, err + } + } else { + // handle other errors + return nil, fmt.Errorf("failed to get the serviceAccount associated with %s: %s", sa.Name, err) } } - // SA exists but shouldn't, so it should be deleted - if !cr.Spec.Notifications.Enabled { - log.Info(fmt.Sprintf("Deleting serviceaccount %s as notifications is disabled", sa.Name)) - return nil, r.Client.Delete(context.TODO(), sa) - } - return sa, nil } @@ -153,16 +140,12 @@ func (r *ReconcileArgoCD) reconcileNotificationsRole(cr *argoprojv1a1.ArgoCD) (* desiredRole := newRole(common.ArgoCDNotificationsControllerComponent, policyRules, cr) existingRole := &rbacv1.Role{} - if err := argoutil.FetchObject(r.Client, cr.Namespace, desiredRole.Name, existingRole); err != nil { + err := argoutil.FetchObject(r.Client, cr.Namespace, desiredRole.Name, existingRole) + if err != nil { if !errors.IsNotFound(err) { return nil, fmt.Errorf("failed to get the role associated with %s : %s", desiredRole.Name, err) } - // role does not exist and shouldn't, nothing to do here - if !cr.Spec.Notifications.Enabled { - return nil, nil - } - // role does not exist but should, so it should be created if err := controllerutil.SetControllerReference(cr, desiredRole, r.Scheme); err != nil { return nil, err @@ -176,12 +159,6 @@ func (r *ReconcileArgoCD) reconcileNotificationsRole(cr *argoprojv1a1.ArgoCD) (* return desiredRole, nil } - // role exists but shouldn't, so it should be deleted - if !cr.Spec.Notifications.Enabled { - log.Info(fmt.Sprintf("Deleting role %s as notifications is disabled", existingRole.Name)) - return nil, r.Client.Delete(context.TODO(), existingRole) - } - // role exists and should. Reconcile role if changed if !reflect.DeepEqual(existingRole.Rules, desiredRole.Rules) { existingRole.Rules = desiredRole.Rules @@ -218,11 +195,6 @@ func (r *ReconcileArgoCD) reconcileNotificationsRoleBinding(cr *argoprojv1a1.Arg return fmt.Errorf("failed to get the rolebinding associated with %s : %s", desiredRoleBinding.Name, err) } - // roleBinding does not exist and shouldn't, nothing to do here - if !cr.Spec.Notifications.Enabled { - return nil - } - // roleBinding does not exist but should, so it should be created if err := controllerutil.SetControllerReference(cr, desiredRoleBinding, r.Scheme); err != nil { return err @@ -232,12 +204,6 @@ func (r *ReconcileArgoCD) reconcileNotificationsRoleBinding(cr *argoprojv1a1.Arg return r.Client.Create(context.TODO(), desiredRoleBinding) } - // roleBinding exists but shouldn't, so it should be deleted - if !cr.Spec.Notifications.Enabled { - log.Info(fmt.Sprintf("Deleting roleBinding %s as notifications is disabled", existingRoleBinding.Name)) - return r.Client.Delete(context.TODO(), existingRoleBinding) - } - // roleBinding exists and should. Reconcile roleBinding if changed if !reflect.DeepEqual(existingRoleBinding.RoleRef, desiredRoleBinding.RoleRef) { // if the RoleRef changes, delete the existing role binding and create a new one @@ -256,9 +222,7 @@ func (r *ReconcileArgoCD) reconcileNotificationsRoleBinding(cr *argoprojv1a1.Arg } func (r *ReconcileArgoCD) reconcileNotificationsDeployment(cr *argoprojv1a1.ArgoCD, sa *corev1.ServiceAccount) error { - desiredDeployment := newDeploymentWithSuffix("notifications-controller", "controller", cr) - desiredDeployment.Spec.Strategy = appsv1.DeploymentStrategy{ Type: appsv1.RecreateDeploymentStrategyType, } @@ -337,19 +301,14 @@ func (r *ReconcileArgoCD) reconcileNotificationsDeployment(cr *argoprojv1a1.Argo }} // fetch existing deployment by name - deploymentChanged := false existingDeployment := &appsv1.Deployment{} - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: desiredDeployment.Name, Namespace: cr.Namespace}, existingDeployment); err != nil { - if !errors.IsNotFound(err) { - return fmt.Errorf("failed to get the deployment associated with %s : %s", existingDeployment.Name, err) - } - - // deployment does not exist and shouldn't, nothing to do here - if !cr.Spec.Notifications.Enabled { - return nil - } + err := r.Client.Get(context.TODO(), types.NamespacedName{Name: desiredDeployment.Name, Namespace: cr.Namespace}, existingDeployment) + if err != nil && !errors.IsNotFound(err) { + return err + } - // deployment does not exist but should, so it should be created + if err != nil && errors.IsNotFound(err) { + // deployment does not exist but should, so create it if err := controllerutil.SetControllerReference(cr, desiredDeployment, r.Scheme); err != nil { return err } @@ -358,15 +317,21 @@ func (r *ReconcileArgoCD) reconcileNotificationsDeployment(cr *argoprojv1a1.Argo return r.Client.Create(context.TODO(), desiredDeployment) } - // deployment exists but shouldn't, so it should be deleted - if !cr.Spec.Notifications.Enabled { - log.Info(fmt.Sprintf("Deleting deployment %s as notifications is disabled", existingDeployment.Name)) - return r.Client.Delete(context.TODO(), existingDeployment) - } - // deployment exists and should. Reconcile deployment if changed + deploymentChanged := updateDeployment(existingDeployment, desiredDeployment) updateNodePlacement(existingDeployment, desiredDeployment, &deploymentChanged) + if deploymentChanged { + return r.Client.Update(context.TODO(), existingDeployment) + } + + return nil + +} + +func updateDeployment(existingDeployment, desiredDeployment *appsv1.Deployment) bool { + deploymentChanged := false + if existingDeployment.Spec.Template.Spec.Containers[0].Image != desiredDeployment.Spec.Template.Spec.Containers[0].Image { existingDeployment.Spec.Template.Spec.Containers[0].Image = desiredDeployment.Spec.Template.Spec.Containers[0].Image existingDeployment.Spec.Template.ObjectMeta.Labels["image.upgraded"] = time.Now().UTC().Format("01022006-150406-MST") @@ -424,64 +389,40 @@ func (r *ReconcileArgoCD) reconcileNotificationsDeployment(cr *argoprojv1a1.Argo deploymentChanged = true } - if deploymentChanged { - return r.Client.Update(context.TODO(), existingDeployment) - } - - return nil - + return deploymentChanged } // reconcileNotificationsConfigMap only creates/deletes the argocd-notifications-cm based on whether notifications is enabled/disabled in the CR // It does not reconcile/overwrite any fields or information in the configmap itself func (r *ReconcileArgoCD) reconcileNotificationsConfigMap(cr *argoprojv1a1.ArgoCD) error { - desiredConfigMap := newConfigMapWithName("argocd-notifications-cm", cr) desiredConfigMap.Data = getDefaultNotificationsConfig() - cmExists := true existingConfigMap := &corev1.ConfigMap{} - if err := argoutil.FetchObject(r.Client, cr.Namespace, desiredConfigMap.Name, existingConfigMap); err != nil { + err := argoutil.FetchObject(r.Client, cr.Namespace, desiredConfigMap.Name, existingConfigMap) + if err != nil { if !errors.IsNotFound(err) { return fmt.Errorf("failed to get the configmap associated with %s : %s", desiredConfigMap.Name, err) } - cmExists = false - } - if cmExists { - // CM exists but shouldn't, so it should be deleted - if !cr.Spec.Notifications.Enabled { - log.Info(fmt.Sprintf("Deleting configmap %s as notifications is disabled", existingConfigMap.Name)) - return r.Client.Delete(context.TODO(), existingConfigMap) + // ConfigMap doesn't exist, so it should be created + if err := controllerutil.SetControllerReference(cr, desiredConfigMap, r.Scheme); err != nil { + return err } - // CM exists and should, nothing to do here - return nil - } - - // CM doesn't exist and shouldn't, nothing to do here - if !cr.Spec.Notifications.Enabled { - return nil - } - - // CM doesn't exist but should, so it should be created - if err := controllerutil.SetControllerReference(cr, desiredConfigMap, r.Scheme); err != nil { - return err - } - - log.Info(fmt.Sprintf("Creating configmap %s", desiredConfigMap.Name)) - err := r.Client.Create(context.TODO(), desiredConfigMap) - if err != nil { - return err + log.Info(fmt.Sprintf("Creating configmap %s", desiredConfigMap.Name)) + if err := r.Client.Create(context.TODO(), desiredConfigMap); err != nil { + return err + } } + // If the ConfigMap already exists, do nothing. return nil } // reconcileNotificationsSecret only creates/deletes the argocd-notifications-secret based on whether notifications is enabled/disabled in the CR // It does not reconcile/overwrite any fields or information in the secret itself func (r *ReconcileArgoCD) reconcileNotificationsSecret(cr *argoprojv1a1.ArgoCD) error { - desiredSecret := argoutil.NewSecretWithName(cr, "argocd-notifications-secret") secretExists := true @@ -494,22 +435,10 @@ func (r *ReconcileArgoCD) reconcileNotificationsSecret(cr *argoprojv1a1.ArgoCD) } if secretExists { - // secret exists but shouldn't, so it should be deleted - if !cr.Spec.Notifications.Enabled { - log.Info(fmt.Sprintf("Deleting secret %s as notifications is disabled", existingSecret.Name)) - return r.Client.Delete(context.TODO(), existingSecret) - } - // secret exists and should, nothing to do here return nil } - // secret doesn't exist and shouldn't, nothing to do here - if !cr.Spec.Notifications.Enabled { - return nil - } - - // secret doesn't exist but should, so it should be created if err := controllerutil.SetControllerReference(cr, desiredSecret, r.Scheme); err != nil { return err } @@ -545,3 +474,125 @@ func getNotificationsResources(cr *argoprojv1a1.ArgoCD) corev1.ResourceRequireme return resources } + +func (r *ReconcileArgoCD) deleteNotificationsConfigMap(cr *argoprojv1a1.ArgoCD) error { + existingConfigMap := &corev1.ConfigMap{} + desiredConfigMap := newConfigMapWithName("argocd-notifications-cm", cr) + + if err := argoutil.FetchObject(r.Client, cr.Namespace, desiredConfigMap.Name, existingConfigMap); err != nil { + if !errors.IsNotFound(err) { + return err + } + + // ConfigMap does not exist and shouldn't, nothing to do here + return nil + } + + // ConfigMap exists but shouldn't, so it should be deleted + log.Info(fmt.Sprintf("Deleting configmap %s as notifications is disabled", existingConfigMap.Name)) + return r.Client.Delete(context.TODO(), existingConfigMap) +} + +func (r *ReconcileArgoCD) deleteNotificationsRole(cr *argoprojv1a1.ArgoCD) error { + desiredRole := newRole(common.ArgoCDNotificationsControllerComponent, nil, cr) + existingRole := &rbacv1.Role{} + + if err := argoutil.FetchObject(r.Client, cr.Namespace, desiredRole.Name, existingRole); err != nil { + // If an error other than 'NotFound' occurred during the fetch, return it + if !errors.IsNotFound(err) { + return err + } + // If the role doesn't exist, no further actions are necessary + return nil + } + + // At this point, the role exists, so proceed with its deletion + log.Info(fmt.Sprintf("Deleting role %s as notifications are disabled", existingRole.Name)) + return r.Client.Delete(context.TODO(), existingRole) +} + +func (r *ReconcileArgoCD) deleteNotificationsRoleBinding(cr *argoprojv1a1.ArgoCD) error { + existingRoleBinding := &rbacv1.RoleBinding{} + desiredRoleBinding := newRoleBindingWithname(common.ArgoCDNotificationsControllerComponent, cr) + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: desiredRoleBinding.Name, Namespace: cr.Namespace}, existingRoleBinding); err != nil { + if !errors.IsNotFound(err) { + return err + } + // roleBinding does not exist and shouldn't, nothing to do here + return nil + } + + // roleBinding exists but shouldn't, so it should be deleted + log.Info(fmt.Sprintf("Deleting roleBinding %s as notifications is disabled", existingRoleBinding.Name)) + return r.Client.Delete(context.TODO(), existingRoleBinding) +} + +// deleteNotificationsSecret deletes the argocd-notifications-secret if notifications is disabled in the CR +func (r *ReconcileArgoCD) deleteNotificationsSecret(cr *argoprojv1a1.ArgoCD) error { + desiredSecret := argoutil.NewSecretWithName(cr, "argocd-notifications-secret") + existingSecret := &corev1.Secret{} + if err := argoutil.FetchObject(r.Client, cr.Namespace, desiredSecret.Name, existingSecret); err != nil { + if !errors.IsNotFound(err) { + return err + } + // secret doesn't exist and shouldn't, nothing to do here + return nil + } + + log.Info(fmt.Sprintf("Deleting secret %s as notifications is disabled", existingSecret.Name)) + return r.Client.Delete(context.TODO(), existingSecret) +} + +// deleteNotificationsDeployment deletes the notifications-controller Deployment if notifications are disabled. +func (r *ReconcileArgoCD) deleteNotificationsDeployment(cr *argoprojv1a1.ArgoCD) error { + desiredDeployment := newDeploymentWithSuffix("notifications-controller", "controller", cr) + existingDeployment := &appsv1.Deployment{} + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: desiredDeployment.Name, Namespace: cr.Namespace}, existingDeployment); err != nil { + if !errors.IsNotFound(err) { + return err + } + // deployment does not exist and shouldn't, nothing to do here + return nil + } + + // Set controller reference before deleting the deployment + if err := controllerutil.SetControllerReference(cr, existingDeployment, r.Scheme); err != nil { + return err + } + + log.Info(fmt.Sprintf("Deleting deployment %s as notifications is disabled", existingDeployment.Name)) + return r.Client.Delete(context.TODO(), existingDeployment) +} + +func (r *ReconcileArgoCD) deleteNotificationsServiceAccount(cr *argoprojv1a1.ArgoCD, sa *corev1.ServiceAccount) error { + deployment := &appsv1.Deployment{} + roleBinding := &rbacv1.RoleBinding{} + + deploymentName := fmt.Sprintf("%s-%s", cr.Name, "notifications-controller") + roleBindingName := fmt.Sprintf("%s-%s", cr.Name, common.ArgoCDNotificationsControllerComponent) + + depErr := r.Client.Get(context.TODO(), types.NamespacedName{Name: deploymentName, Namespace: sa.Namespace}, deployment) + rbErr := r.Client.Get(context.TODO(), types.NamespacedName{Name: roleBindingName, Namespace: sa.Namespace}, roleBinding) + + // Check if either the Deployment or RoleBinding still exist + if errors.IsNotFound(depErr) && errors.IsNotFound(rbErr) { + // If both do not exist, delete the ServiceAccount + log.Info(fmt.Sprintf("Deleting ServiceAccount %s as notifications are disabled", sa.Name)) + return r.Client.Delete(context.TODO(), sa) + } else if depErr != nil && !errors.IsNotFound(depErr) { + // If Deployment still exists, don't delete the ServiceAccount and requeue the reconcile request + log.Info(fmt.Sprintf("Not deleting ServiceAccount %s because it is still used by Deployment: %s", sa.Name, deploymentName)) + + // Deployment still exists, requeue the reconcile request + return fmt.Errorf("deployment %s still exists, requeue the reconcile request", deploymentName) + } else if rbErr != nil && !errors.IsNotFound(rbErr) { + // If RoleBinding still exists, don't delete the ServiceAccount and requeue the reconcile request + log.Info(fmt.Sprintf("Not deleting ServiceAccount %s because it is still used by RoleBinding: %s", sa.Name, roleBindingName)) + + // RoleBinding still exists, requeue the reconcile request + return fmt.Errorf("roleBinding %s still exists, requeue the reconcile request", roleBindingName) + } + + // The ServiceAccount is not used by any resources, no action needed + return nil +} diff --git a/controllers/argocd/notifications_test.go b/controllers/argocd/notifications_test.go index 2648f5a0c..8a998b6bf 100644 --- a/controllers/argocd/notifications_test.go +++ b/controllers/argocd/notifications_test.go @@ -41,10 +41,55 @@ func TestReconcileNotifications_CreateRoles(t *testing.T) { assert.Equal(t, desiredPolicyRules, testRole.Rules) - a.Spec.Notifications.Enabled = false + // Modify the rules of the existing role + testRole.Rules = append(testRole.Rules, rbacv1.PolicyRule{ + Verbs: []string{"get", "list"}, + APIGroups: []string{"", "apps", "extensions", "argoproj.io"}, + Resources: []string{"configmaps", "secrets"}, + }) + + // Update the role with the modified rules + assert.NoError(t, r.Client.Update(context.TODO(), testRole)) + + // Reconcile again to correct the state _, err = r.reconcileNotificationsRole(a) assert.NoError(t, err) + // Fetch the updated role + updatedRole := &rbacv1.Role{} + assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{ + Name: generateResourceName(common.ArgoCDNotificationsControllerComponent, a), + Namespace: a.Namespace, + }, updatedRole)) + + // Assert that the rules have been updated back to match the desired state + assert.Equal(t, desiredPolicyRules, updatedRole.Rules) +} + +func TestReconcileNotifications_DeleteRoles(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = true + }) + + r := makeTestReconciler(t, a) + + // Initially create the Role + _, err := r.reconcileNotificationsRole(a) + assert.NoError(t, err) + + testRole := &rbacv1.Role{} + assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{ + Name: generateResourceName(common.ArgoCDNotificationsControllerComponent, a), + Namespace: a.Namespace, + }, testRole)) + + // Disable notifications, which should trigger deletion of the Role + a.Spec.Notifications.Enabled = false + err = r.deleteNotificationsRole(a) + assert.NoError(t, err) + + // Check that the Role no longer exists err = r.Client.Get(context.TODO(), types.NamespacedName{ Name: generateResourceName(common.ArgoCDNotificationsControllerComponent, a), Namespace: a.Namespace, @@ -70,17 +115,39 @@ func TestReconcileNotifications_CreateServiceAccount(t *testing.T) { }, testSa)) assert.Equal(t, testSa.Name, desiredSa.Name) +} - a.Spec.Notifications.Enabled = false - _, err = r.reconcileNotificationsServiceAccount(a) +func TestReconcileNotifications_DeleteServiceAccount(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = false + }) + + r := makeTestReconciler(t, a) + + sa := newServiceAccountWithName(common.ArgoCDNotificationsControllerComponent, a) + + // Create the ServiceAccount + assert.NoError(t, r.Client.Create(context.TODO(), sa)) + + // Ensure the ServiceAccount is created + testSa := &corev1.ServiceAccount{} + assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{ + Name: generateResourceName(common.ArgoCDNotificationsControllerComponent, a), + Namespace: a.Namespace, + }, testSa)) + assert.Equal(t, testSa.Name, sa.Name) + + // Call the deleteNotificationsServiceAccount function + err := r.deleteNotificationsServiceAccount(a, sa) assert.NoError(t, err) + // Ensure the ServiceAccount is deleted err = r.Client.Get(context.TODO(), types.NamespacedName{ Name: generateResourceName(common.ArgoCDNotificationsControllerComponent, a), Namespace: a.Namespace, }, testSa) assert.True(t, errors.IsNotFound(err)) - } func TestReconcileNotifications_CreateRoleBinding(t *testing.T) { @@ -107,11 +174,40 @@ func TestReconcileNotifications_CreateRoleBinding(t *testing.T) { assert.Equal(t, roleBinding.RoleRef.Name, role.Name) assert.Equal(t, roleBinding.Subjects[0].Name, sa.Name) +} - a.Spec.Notifications.Enabled = false - err = r.reconcileNotificationsRoleBinding(a, role, sa) +func TestReconcileNotifications_DeleteRoleBinding(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = true + }) + r := makeTestReconciler(t, a) + + role := &rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: "role-name"}} + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "sa-name"}} + + // Assuming the role binding has been reconciled and exists + err := r.reconcileNotificationsRoleBinding(a, role, sa) assert.NoError(t, err) + roleBinding := &rbacv1.RoleBinding{} + assert.NoError(t, r.Client.Get( + context.TODO(), + types.NamespacedName{ + Name: generateResourceName(common.ArgoCDNotificationsControllerComponent, a), + Namespace: a.Namespace, + }, + roleBinding)) + + // Confirm that the role binding exists + assert.Equal(t, roleBinding.RoleRef.Name, role.Name) + assert.Equal(t, roleBinding.Subjects[0].Name, sa.Name) + + // Now delete the role binding + err = r.deleteNotificationsRoleBinding(a) + assert.NoError(t, err) + + // Confirm the role binding has been deleted err = r.Client.Get(context.TODO(), types.NamespacedName{ Name: generateResourceName(common.ArgoCDNotificationsControllerComponent, a), Namespace: a.Namespace, @@ -218,9 +314,32 @@ func TestReconcileNotifications_CreateDeployments(t *testing.T) { if diff := cmp.Diff(expectedSelector, deployment.Spec.Selector); diff != "" { t.Fatalf("failed to reconcile notifications-controller label selector:\n%s", diff) } +} +func TestReconcileNotifications_DeleteDeployments(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = true + }) + r := makeTestReconciler(t, a) + sa := corev1.ServiceAccount{} + assert.NoError(t, r.reconcileNotificationsDeployment(a, &sa)) + + deployment := &appsv1.Deployment{} + assert.NoError(t, r.Client.Get( + context.TODO(), + types.NamespacedName{ + Name: a.Name + "-notifications-controller", + Namespace: a.Namespace, + }, + deployment)) + + // Ensure the created Deployment has the expected properties + assert.Equal(t, deployment.Spec.Template.Spec.ServiceAccountName, sa.ObjectMeta.Name) + + // Ensure the deployment is deleted a.Spec.Notifications.Enabled = false - err := r.reconcileNotificationsDeployment(a, &sa) + err := r.deleteNotificationsDeployment(a) assert.NoError(t, err) err = r.Client.Get(context.TODO(), types.NamespacedName{ @@ -232,6 +351,7 @@ func TestReconcileNotifications_CreateDeployments(t *testing.T) { func TestReconcileNotifications_CreateSecret(t *testing.T) { logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) { a.Spec.Notifications.Enabled = true }) @@ -246,12 +366,25 @@ func TestReconcileNotifications_CreateSecret(t *testing.T) { Name: "argocd-notifications-secret", Namespace: a.Namespace, }, testSecret)) +} - a.Spec.Notifications.Enabled = false - err = r.reconcileNotificationsSecret(a) +func TestDeleteNotifications_DeleteSecret(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + + a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = false + }) + + r := makeTestReconciler(t, a) + + err := r.deleteNotificationsSecret(a) assert.NoError(t, err) + secret := &corev1.Secret{} - err = r.Client.Get(context.TODO(), types.NamespacedName{Name: "argocd-notifications-secret", Namespace: a.Namespace}, secret) + err = r.Client.Get(context.TODO(), types.NamespacedName{ + Name: "argocd-notifications-secret", + Namespace: a.Namespace, + }, secret) assertNotFound(t, err) } @@ -273,13 +406,35 @@ func TestReconcileNotifications_CreateConfigMap(t *testing.T) { }, testCm)) assert.True(t, len(testCm.Data) > 0) +} - a.Spec.Notifications.Enabled = false - err = r.reconcileNotificationsConfigMap(a) +func TestReconcileNotifications_DeleteConfigMap(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = false + }) + + r := makeTestReconciler(t, a) + + // Create the existing ConfigMap + existingCm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argocd-notifications-cm", + Namespace: a.Namespace, + }, + } + assert.NoError(t, r.Client.Create(context.TODO(), existingCm)) + + err := r.deleteNotificationsConfigMap(a) assert.NoError(t, err) - testCm = &corev1.ConfigMap{} - err = r.Client.Get(context.TODO(), types.NamespacedName{Name: "argocd-notifications-cm", Namespace: a.Namespace}, testCm) - assertNotFound(t, err) + + // Check if the ConfigMap was deleted + cm := &corev1.ConfigMap{} + err = r.Client.Get(context.TODO(), types.NamespacedName{ + Name: "argocd-notifications-cm", + Namespace: a.Namespace, + }, cm) + assert.True(t, errors.IsNotFound(err)) } func TestReconcileNotifications_testEnvVars(t *testing.T) { diff --git a/controllers/argocd/route_test.go b/controllers/argocd/route_test.go index 2272335ee..3fae546ff 100644 --- a/controllers/argocd/route_test.go +++ b/controllers/argocd/route_test.go @@ -28,6 +28,7 @@ func TestReconcileRouteSetLabels(t *testing.T) { ctx := context.Background() logf.SetLogger(ZapLogger(true)) argoCD := makeArgoCD(func(a *argov1alpha1.ArgoCD) { + a.Spec.Notifications.Enabled = true a.Spec.Server.Route.Enabled = true labels := make(map[string]string) labels["my-key"] = "my-value" @@ -64,6 +65,7 @@ func TestReconcileRouteSetsInsecure(t *testing.T) { logf.SetLogger(ZapLogger(true)) argoCD := makeArgoCD(func(a *argov1alpha1.ArgoCD) { a.Spec.Server.Route.Enabled = true + a.Spec.Notifications.Enabled = true }) objs := []runtime.Object{ argoCD, @@ -136,6 +138,7 @@ func TestReconcileRouteUnsetsInsecure(t *testing.T) { argoCD := makeArgoCD(func(a *argov1alpha1.ArgoCD) { a.Spec.Server.Route.Enabled = true a.Spec.Server.Insecure = true + a.Spec.Notifications.Enabled = true }) objs := []runtime.Object{ argoCD, diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index dfef38c35..119f23b05 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -823,6 +823,11 @@ func (r *ReconcileArgoCD) reconcileResources(cr *argoprojv1a1.ArgoCD) error { if err := r.reconcileNotificationsController(cr); err != nil { return err } + } else { + log.Info("deleting Notifications controller resources") + if err := r.deleteNotificationsResources(cr); err != nil { + return err + } } if err := r.reconcileRepoServerTLSSecret(cr); err != nil { @@ -1023,14 +1028,7 @@ func (r *ReconcileArgoCD) setResourceWatches(bldr *builder.Builder, clusterResou if !ok { return false } - if oldCR.Spec.Notifications.Enabled && !newCR.Spec.Notifications.Enabled { - err := r.deleteNotificationsResources(newCR) - if err != nil { - log.Error(err, fmt.Sprintf("Failed to delete notifications controller resources for ArgoCD %s in namespace %s", - newCR.Name, newCR.Namespace)) - } - } - return true + return oldCR.Spec.Notifications.Enabled && !newCR.Spec.Notifications.Enabled }, } diff --git a/tests/ha/1-020_validate_redis_ha_nonha/01-assert.yaml b/tests/ha/1-020_validate_redis_ha_nonha/01-assert.yaml index 77e2f772f..3fdf539dc 100644 --- a/tests/ha/1-020_validate_redis_ha_nonha/01-assert.yaml +++ b/tests/ha/1-020_validate_redis_ha_nonha/01-assert.yaml @@ -1,7 +1,3 @@ -apiVersion: kuttl.dev/v1beta1 -kind: TestAssert -timeout: 720 ---- apiVersion: v1 kind: Service metadata: diff --git a/tests/ha/1-020_validate_redis_ha_nonha/04-assert.yaml b/tests/ha/1-020_validate_redis_ha_nonha/04-assert.yaml index 2d48ff872..c48877c14 100644 --- a/tests/ha/1-020_validate_redis_ha_nonha/04-assert.yaml +++ b/tests/ha/1-020_validate_redis_ha_nonha/04-assert.yaml @@ -1,7 +1,3 @@ -apiVersion: kuttl.dev/v1beta1 -kind: TestAssert -timeout: 720 ---- apiVersion: argoproj.io/v1alpha1 kind: ArgoCD metadata: diff --git a/tests/k8s/1-014_validate_parallelism_limit/02-check-deployment.yaml b/tests/k8s/1-014_validate_parallelism_limit/02-check-deployment.yaml index b60954eb0..d2b23a0bb 100644 --- a/tests/k8s/1-014_validate_parallelism_limit/02-check-deployment.yaml +++ b/tests/k8s/1-014_validate_parallelism_limit/02-check-deployment.yaml @@ -1,13 +1,17 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: -- script: sleep 30 - script: | set -e - expected=10 - wlCommand=$(kubectl get -n $NAMESPACE statefulset/argocd-application-controller -o jsonpath='{.spec.template.spec.containers[0].command}'| jq -r '.[]' ) - if ! echo "$wlCommand" | grep -qPz -- "--kubectl-parallelism-limit\\n${expected}(\$|\\n)"; then + expected="10" + wlCommand=$(kubectl get -n $NAMESPACE statefulset/argocd-application-controller -o jsonpath='{.spec.template.spec.containers[0].command}'| jq -r '.[]') + if ! echo "$wlCommand" | grep -Fxq -- "--kubectl-parallelism-limit"; then echo "Incorrect or missing --kubectl-parallelism-limit detected." echo "$wlCommand" exit 1 + fi + if ! echo "$wlCommand" | grep -Fxq -- "$expected"; then + echo "Incorrect value for --kubectl-parallelism-limit detected." + echo "$wlCommand" + exit 1 fi \ No newline at end of file diff --git a/tests/k8s/1-014_validate_parallelism_limit/04-check-deployment.yaml b/tests/k8s/1-014_validate_parallelism_limit/04-check-deployment.yaml index edbdbe5eb..8c710be9b 100644 --- a/tests/k8s/1-014_validate_parallelism_limit/04-check-deployment.yaml +++ b/tests/k8s/1-014_validate_parallelism_limit/04-check-deployment.yaml @@ -1,13 +1,17 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: -- script: sleep 30 - script: | set -e - expected=20 - wlCommand=$(kubectl get -n $NAMESPACE statefulset/argocd-application-controller -o jsonpath='{.spec.template.spec.containers[0].command}'| jq -r '.[]' ) - if ! echo "$wlCommand" | grep -qPz -- "--kubectl-parallelism-limit\\n${expected}(\$|\\n)"; then + expected="20" + wlCommand=$(kubectl get -n $NAMESPACE statefulset/argocd-application-controller -o jsonpath='{.spec.template.spec.containers[0].command}'| jq -r '.[]') + if ! echo "$wlCommand" | grep -Fxq -- "--kubectl-parallelism-limit"; then echo "Incorrect or missing --kubectl-parallelism-limit detected." echo "$wlCommand" exit 1 + fi + if ! echo "$wlCommand" | grep -Fxq -- "$expected"; then + echo "Incorrect value for --kubectl-parallelism-limit detected." + echo "$wlCommand" + exit 1 fi \ No newline at end of file diff --git a/tests/k8s/1-014_validate_parallelism_limit/06-check-deployment.yaml b/tests/k8s/1-014_validate_parallelism_limit/06-check-deployment.yaml index 753984460..f5eed3237 100644 --- a/tests/k8s/1-014_validate_parallelism_limit/06-check-deployment.yaml +++ b/tests/k8s/1-014_validate_parallelism_limit/06-check-deployment.yaml @@ -1,16 +1,22 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep -commands: -- script: | - set -e +wait: +- condition: command-exit-code + timeout: 30s + command: | kubectl patch -n $NAMESPACE argocds/argocd --type=json --patch '[{"op": "remove", "path": "/spec/controller/parallelismLimit"}]' -- script: sleep 30 + successCodes: [0] - script: | set -e - expected=10 - wlCommand=$(kubectl get -n $NAMESPACE statefulset/argocd-application-controller -o jsonpath='{.spec.template.spec.containers[0].command}'| jq -r '.[]' ) - if ! echo "$wlCommand" | grep -qPz -- "--kubectl-parallelism-limit\\n${expected}(\$|\\n)"; then + expected="10" + wlCommand=$(kubectl get -n $NAMESPACE statefulset/argocd-application-controller -o jsonpath='{.spec.template.spec.containers[0].command}'| jq -r '.[]') + if ! echo "$wlCommand" | grep -Fxq -- "--kubectl-parallelism-limit"; then echo "Incorrect or missing --kubectl-parallelism-limit detected." echo "$wlCommand" exit 1 - fi \ No newline at end of file + fi + if ! echo "$wlCommand" | grep -Fxq -- "$expected"; then + echo "Incorrect value for --kubectl-parallelism-limit detected." + echo "$wlCommand" + exit 1 + fi diff --git a/tests/k8s/1-022_validate_notifications/01-assert.yaml b/tests/k8s/1-022_validate_notifications/01-assert.yaml new file mode 100644 index 000000000..6cb0e0da1 --- /dev/null +++ b/tests/k8s/1-022_validate_notifications/01-assert.yaml @@ -0,0 +1,43 @@ +apiVersion: argoproj.io/v1alpha1 +kind: ArgoCD +metadata: + name: argocd +status: + phase: Available + notificationsController: Running +--- +kind: Deployment +apiVersion: apps/v1 +metadata: + name: argocd-notifications-controller +status: + conditions: + - type: Available + status: 'True' + - type: Progressing + status: 'True' +--- +kind: Secret +apiVersion: v1 +metadata: + name: argocd-notifications-secret +--- +kind: ConfigMap +apiVersion: v1 +metadata: + name: argocd-notifications-cm +--- +kind: ServiceAccount +apiVersion: v1 +metadata: + name: argocd-argocd-notifications-controller +--- +kind: Role +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: argocd-argocd-notifications-controller +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: argocd-argocd-notifications-controller diff --git a/tests/k8s/1-022_validate_notifications/02-assert.yaml b/tests/k8s/1-022_validate_notifications/02-assert.yaml new file mode 100644 index 000000000..46d87bdf3 --- /dev/null +++ b/tests/k8s/1-022_validate_notifications/02-assert.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: argocd-notifications-cm +data: + service.email.gmail: "{host: smtp4dev, port: 2525, from: fake@email.com}" diff --git a/tests/k8s/1-022_validate_notifications/02-update-notifications-cm.yaml b/tests/k8s/1-022_validate_notifications/02-update-notifications-cm.yaml index 1b7c948b9..47b2083af 100644 --- a/tests/k8s/1-022_validate_notifications/02-update-notifications-cm.yaml +++ b/tests/k8s/1-022_validate_notifications/02-update-notifications-cm.yaml @@ -1,10 +1,7 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: -- script: sleep 15 - script: | set -e - kubectl patch cm argocd-notifications-cm -n $NAMESPACE --type merge -p '{"data": {"service.email.gmail": "{host: smtp4dev, port: 2525, from: fake@email.com }" }}' -- script: sleep 5 - + kubectl patch cm argocd-notifications-cm -n $NAMESPACE --type merge -p '{"data": {"service.email.gmail": "{host: smtp4dev, port: 2525, from: fake@email.com}" }}' \ No newline at end of file diff --git a/tests/k8s/1-022_validate_notifications/03-assert.yaml b/tests/k8s/1-022_validate_notifications/03-assert.yaml new file mode 100644 index 000000000..a71debccf --- /dev/null +++ b/tests/k8s/1-022_validate_notifications/03-assert.yaml @@ -0,0 +1,14 @@ +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + name: my-app-3 + annotations: + "notifications.argoproj.io/subscribe.on-created.gmail": "jdfake@email.com" +spec: + destination: + server: https://kubernetes.default.svc + project: default + source: + repoURL: https://github.com/redhat-developer/gitops-operator + path: test/examples/nginx + targetRevision: HEAD \ No newline at end of file diff --git a/tests/k8s/1-022_validate_notifications/03-create-app.yaml b/tests/k8s/1-022_validate_notifications/03-create-app.yaml index d9a436f7c..d942591ef 100644 --- a/tests/k8s/1-022_validate_notifications/03-create-app.yaml +++ b/tests/k8s/1-022_validate_notifications/03-create-app.yaml @@ -21,5 +21,4 @@ commands: repoURL: https://github.com/redhat-developer/gitops-operator path: test/examples/nginx targetRevision: HEAD - EOF -- script: sleep 5 \ No newline at end of file + EOF \ No newline at end of file diff --git a/tests/k8s/1-022_validate_notifications/04-delete-app.yaml b/tests/k8s/1-022_validate_notifications/04-delete-app.yaml index 47052970a..c7b6c781a 100644 --- a/tests/k8s/1-022_validate_notifications/04-delete-app.yaml +++ b/tests/k8s/1-022_validate_notifications/04-delete-app.yaml @@ -5,4 +5,10 @@ commands: set -e kubectl delete -n $NAMESPACE application.argoproj.io my-app-3 -- script: sleep 5 \ No newline at end of file +- script: sleep 5 +- script: | + output=$(kubectl get -n $NAMESPACE application.argoproj.io my-app-3 --no-headers 2>/dev/null) + if [ -n "$output" ]; then + echo "my-app-3 still exists" + exit 1 + fi \ No newline at end of file diff --git a/tests/k8s/1-022_validate_notifications/05-verify-email.yaml b/tests/k8s/1-022_validate_notifications/05-verify-email.yaml index 239621b07..97f5c7381 100644 --- a/tests/k8s/1-022_validate_notifications/05-verify-email.yaml +++ b/tests/k8s/1-022_validate_notifications/05-verify-email.yaml @@ -2,16 +2,7 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: - script: | - set -e - smtp4dev_pod=$(kubectl get pod -l=app=smtp4dev -o NAME -n $NAMESPACE) - exit_code=$(kubectl -n $NAMESPACE exec -i --stdin "${smtp4dev_pod}" -- /bin/bash \ - -c 'if [[ $(grep -rnw /tmp -e "Subject: Application my-app-3 has been created.") ]]; then - exit 0; else - exit 1; - fi') + set -euo pipefail - if [ $exit_code=0 ]; then - exit 0 - else - exit 1 - fi \ No newline at end of file + smtp4dev_pod=$(kubectl get pod -l=app=smtp4dev -o jsonpath='{.items[0].metadata.name}' -n $NAMESPACE) + kubectl -n $NAMESPACE exec -i "${smtp4dev_pod}" -- /bin/bash -c 'grep -rnw /tmp -e "Subject: Application my-app-3 has been created."' diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml index 8f02301dc..fe41808c5 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml @@ -1,4 +1,3 @@ ---- apiVersion: kuttl.dev/v1beta1 kind: TestStep delete: @@ -6,6 +5,11 @@ delete: kind: ArgoCD name: example-argocd namespace: central-argocd -commands: - # Sleep to allow resources to be completely deleted - - command: sleep 30s \ No newline at end of file +wait: +- condition: delete + timeout: 30s + selector: + apiVersion: argoproj.io/v1alpha1 + kind: ArgoCD + name: example-argocd + namespace: central-argocd diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml index 0d87c9379..b0dd05839 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml @@ -11,5 +11,3 @@ delete: - apiVersion: v1 kind: Namespace name: central-argocd -commands: - - command: sleep 30s \ No newline at end of file diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/99-errors.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/99-errors.yaml new file mode 100644 index 000000000..05de49f4c --- /dev/null +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/99-errors.yaml @@ -0,0 +1,14 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: test-1-24-custom +--- +apiVersion: v1 +kind: Namespace +metadata: + name: test-2-24-custom +--- +apiVersion: v1 +kind: Namespace +metadata: + name: central-argocd \ No newline at end of file