Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the logic for applicationset resources reconcilation when spec.applicationset.enabled is false #1089

Merged
merged 14 commits into from
Dec 1, 2023
Merged

fix the logic for applicationset resources reconcilation when spec.applicationset.enabled is false #1089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
eacd348
fix the logic for applicationset resources reconciliation when spec.a…
ishitasequeira Nov 29, 2023
0db9889
fix tests
ishitasequeira Nov 29, 2023
20c4b87
delete repo server when repo.enabled is set to false
ishitasequeira Nov 29, 2023
8b87c9d
Update status.Phase based on component enabled flag
ishitasequeira Nov 29, 2023
e5c70e5
Added kuttl tests
raghavi101 Nov 29, 2023
fd1869d
Added namespace creation step
raghavi101 Nov 29, 2023
9206c34
delete services created for resources
ishitasequeira Nov 29, 2023
b170d5d
delete server deployment when enabled flag set to false
ishitasequeira Nov 29, 2023
49873b2
fix e2e test
ishitasequeira Nov 30, 2023
a079ae2
fix log message
ishitasequeira Nov 30, 2023
c2396ab
revert kuttl test timeout
ishitasequeira Nov 30, 2023
6471fb5
Added test for reverse scenario
raghavi101 Nov 30, 2023
862b5a9
Dir rename
raghavi101 Nov 30, 2023
d1c4b9a
Added e2e test for ha mode
raghavi101 Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions controllers/argocd/applicationset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,10 @@ func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD,

podSpec := &deploy.Spec.Template.Spec

podSpec.ServiceAccountName = sa.ObjectMeta.Name

// sa would be nil when spec.applicationset.enabled = false
if sa != nil {
podSpec.ServiceAccountName = sa.ObjectMeta.Name
}
podSpec.Volumes = []corev1.Volume{
{
Name: "ssh-known-hosts",
Expand Down Expand Up @@ -181,7 +183,7 @@ func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD,

if existing := newDeploymentWithSuffix("applicationset-controller", "controller", cr); argoutil.IsObjectFound(r.Client, cr.Namespace, existing.Name, existing) {

if !cr.Spec.ApplicationSet.IsEnabled() {
if cr.Spec.ApplicationSet != nil && !cr.Spec.ApplicationSet.IsEnabled() {
err := r.Client.Delete(context.TODO(), existing)
return err
}
Expand Down Expand Up @@ -425,19 +427,16 @@ func (r *ReconcileArgoCD) reconcileApplicationSetRole(cr *argoproj.ArgoCD) (*v1.
if !errors.IsNotFound(err) {
return nil, fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", role.Name, err)
}
if errors.IsNotFound(err) && cr.Spec.ApplicationSet != nil && !cr.Spec.ApplicationSet.IsEnabled() {
return nil, nil
}
if err = controllerutil.SetControllerReference(cr, role, r.Scheme); err != nil {
return nil, err
}
if cr.Spec.ApplicationSet != nil && !cr.Spec.ApplicationSet.IsEnabled() {
err1 := r.Client.Delete(context.TODO(), role)
return nil, err1
}
return role, r.Client.Create(context.TODO(), role)
}

if cr.Spec.ApplicationSet != nil && !cr.Spec.ApplicationSet.IsEnabled() {
err := r.Client.Delete(context.TODO(), role)
return nil, err
return nil, r.Client.Delete(context.TODO(), role)
}

role.Rules = policyRules
Expand All @@ -458,17 +457,16 @@ func (r *ReconcileArgoCD) reconcileApplicationSetRoleBinding(cr *argoproj.ArgoCD
roleBindingExists := true
if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: roleBinding.Name, Namespace: cr.Namespace}, roleBinding); err != nil {
if !errors.IsNotFound(err) {
if cr.Spec.ApplicationSet != nil && !cr.Spec.ApplicationSet.IsEnabled() {
return nil
}
return fmt.Errorf("failed to get the rolebinding associated with %s : %s", name, err)
}
if errors.IsNotFound(err) && cr.Spec.ApplicationSet != nil && !cr.Spec.ApplicationSet.IsEnabled() {
return nil
}
roleBindingExists = false
}

if cr.Spec.ApplicationSet != nil && !cr.Spec.ApplicationSet.IsEnabled() {
err := r.Client.Delete(context.TODO(), roleBinding)
return err
return r.Client.Delete(context.TODO(), roleBinding)
}

setAppSetLabels(&roleBinding.ObjectMeta)
Expand Down Expand Up @@ -563,6 +561,7 @@ func (r *ReconcileArgoCD) reconcileApplicationSetService(cr *argoproj.ArgoCD) er
return err
}
}
return nil
} else {
if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
return nil // Service found, do nothing
Expand Down
1 change: 1 addition & 0 deletions controllers/argocd/applicationset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,7 @@ func setProxyEnvVars(t *testing.T) {
func TestReconcileApplicationSet_Service(t *testing.T) {
logf.SetLogger(ZapLogger(true))
a := makeTestArgoCD()
a.Spec.ApplicationSet = &argoproj.ArgoCDApplicationSet{}

resObjs := []client.Object{a}
subresObjs := []client.Object{a}
Expand Down
8 changes: 4 additions & 4 deletions controllers/argocd/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,7 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor
if !cr.Spec.Repo.IsEnabled() {
log.Info("Existing ArgoCD Repo Server found but should be disabled. Deleting Repo Server")
// Delete existing deployment for ArgoCD Repo Server, if any ..
return nil
return r.Client.Delete(context.TODO(), existing)
}

changed := false
Expand Down Expand Up @@ -1328,7 +1328,7 @@ func (r *ReconcileArgoCD) reconcileServerDeployment(cr *argoproj.ArgoCD, useTLSF
if !cr.Spec.Server.IsEnabled() {
log.Info("Existing ArgoCD Server found but should be disabled. Deleting ArgoCD Server")
// Delete existing deployment for ArgoCD Server, if any ..
return nil
return r.Client.Delete(context.TODO(), existing)
}
actualImage := existing.Spec.Template.Spec.Containers[0].Image
desiredImage := getArgoContainerImage(cr)
Expand Down Expand Up @@ -1375,8 +1375,8 @@ func (r *ReconcileArgoCD) reconcileServerDeployment(cr *argoproj.ArgoCD, useTLSF
return nil // Deployment found with nothing to do, move along...
}

if !cr.Spec.Controller.IsEnabled() {
log.Info("ArgoCD Repo Server disabled. Skipping starting repo server.")
if !cr.Spec.Server.IsEnabled() {
log.Info("ArgoCD Server disabled. Skipping starting argocd server.")
return nil
}

Expand Down
31 changes: 24 additions & 7 deletions controllers/argocd/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,13 @@ func (r *ReconcileArgoCD) reconcileRedisHAAnnounceServices(cr *argoproj.ArgoCD)
for i := int32(0); i < common.ArgoCDDefaultRedisHAReplicas; i++ {
svc := newServiceWithSuffix(fmt.Sprintf("redis-ha-announce-%d", i), "redis", cr)
if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
if !cr.Spec.HA.Enabled {
if !cr.Spec.HA.Enabled || !cr.Spec.Redis.IsEnabled() {
anandf marked this conversation as resolved.
Show resolved Hide resolved
return r.Client.Delete(context.TODO(), svc)
}
return nil // Service found, do nothing
}

if !cr.Spec.HA.Enabled {
if !cr.Spec.HA.Enabled || !cr.Spec.Redis.IsEnabled() {
return nil //return as Ha is not enabled do nothing
}

Expand Down Expand Up @@ -183,13 +183,13 @@ func (r *ReconcileArgoCD) reconcileRedisHAAnnounceServices(cr *argoproj.ArgoCD)
func (r *ReconcileArgoCD) reconcileRedisHAMasterService(cr *argoproj.ArgoCD) error {
svc := newServiceWithSuffix("redis-ha", "redis", cr)
if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
if !cr.Spec.HA.Enabled {
if !cr.Spec.HA.Enabled || !cr.Spec.Redis.IsEnabled() {
return r.Client.Delete(context.TODO(), svc)
}
return nil // Service found, do nothing
}

if !cr.Spec.HA.Enabled {
if !cr.Spec.HA.Enabled || !cr.Spec.Redis.IsEnabled() {
return nil //return as Ha is not enabled do nothing
}

Expand Down Expand Up @@ -222,7 +222,7 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyService(cr *argoproj.ArgoCD) erro
svc := newServiceWithSuffix("redis-ha-haproxy", "redis", cr)
if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {

if !cr.Spec.HA.Enabled {
if !cr.Spec.HA.Enabled || !cr.Spec.Redis.IsEnabled() {
return r.Client.Delete(context.TODO(), svc)
}

Expand All @@ -232,7 +232,7 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyService(cr *argoproj.ArgoCD) erro
return nil // Service found, do nothing
}

if !cr.Spec.HA.Enabled {
if !cr.Spec.HA.Enabled || !cr.Spec.Redis.IsEnabled() {
return nil //return as Ha is not enabled do nothing
}

Expand Down Expand Up @@ -279,6 +279,9 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
svc := newServiceWithSuffix("redis", "redis", cr)

if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
if !cr.Spec.Redis.IsEnabled() {
return r.Client.Delete(context.TODO(), svc)
}
if ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS()) {
return r.Client.Update(context.TODO(), svc)
}
Expand All @@ -288,7 +291,7 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
return nil // Service found, do nothing
}

if cr.Spec.HA.Enabled {
if cr.Spec.HA.Enabled || !cr.Spec.Redis.IsEnabled() {
return nil //return as Ha is enabled do nothing
}

Expand Down Expand Up @@ -358,12 +361,19 @@ func (r *ReconcileArgoCD) reconcileRepoService(cr *argoproj.ArgoCD) error {
svc := newServiceWithSuffix("repo-server", "repo-server", cr)

if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
if !cr.Spec.Repo.IsEnabled() {
return r.Client.Delete(context.TODO(), svc)
}
if ensureAutoTLSAnnotation(svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS()) {
return r.Client.Update(context.TODO(), svc)
}
return nil // Service found, do nothing
}

if !cr.Spec.Repo.IsEnabled() {
return nil
}

ensureAutoTLSAnnotation(svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS())

svc.Spec.Selector = map[string]string{
Expand Down Expand Up @@ -420,12 +430,19 @@ func (r *ReconcileArgoCD) reconcileServerMetricsService(cr *argoproj.ArgoCD) err
func (r *ReconcileArgoCD) reconcileServerService(cr *argoproj.ArgoCD) error {
svc := newServiceWithSuffix("server", "server", cr)
if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
if !cr.Spec.Server.IsEnabled() {
return r.Client.Delete(context.TODO(), svc)
}
if ensureAutoTLSAnnotation(svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS()) {
return r.Client.Update(context.TODO(), svc)
}
return nil // Service found, do nothing
}

if !cr.Spec.Repo.IsEnabled() {
return nil
}

ensureAutoTLSAnnotation(svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS())

svc.Spec.Ports = []corev1.ServicePort{
Expand Down
5 changes: 4 additions & 1 deletion controllers/argocd/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,10 @@ func (r *ReconcileArgoCD) reconcileStatusSSO(cr *argoproj.ArgoCD) error {
func (r *ReconcileArgoCD) reconcileStatusPhase(cr *argoproj.ArgoCD) error {
var phase string

if cr.Status.ApplicationController == "Running" && cr.Status.Redis == "Running" && cr.Status.Repo == "Running" && cr.Status.Server == "Running" {
if ((!cr.Spec.Controller.IsEnabled() && cr.Status.ApplicationController == "Unknown") || cr.Status.ApplicationController == "Running") &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a status of Unknown can we have a state called Disabled. It would be more intutive for the user to understand that the component is explicitly disabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not maintain a Disabled state today and add Unknown as the state for missing components like ApplicationSet if spec.applicationset = nil. It would be like adding a new behavior and a new Status as part of bugfix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not as part of this PR, we can create a separate PR for adding the Disabled state. That being a string and not part of the API spec, it can be introduced with minimal impact to end users.

((!cr.Spec.Redis.IsEnabled() && cr.Status.Redis == "Unknown") || cr.Status.Redis == "Running") &&
((!cr.Spec.Repo.IsEnabled() && cr.Status.Repo == "Unknown") || cr.Status.Repo == "Running") &&
((!cr.Spec.Server.IsEnabled() && cr.Status.Server == "Unknown") || cr.Status.Server == "Running") {
phase = "Available"
} else {
phase = "Pending"
Expand Down
63 changes: 63 additions & 0 deletions tests/k8s/1-034_validate_applicationset_reconcile_enabled_set_false/01-errors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
apiVersion: argoproj.io/v1beta1
kind: ArgoCD
metadata:
name: argocd-test
namespace: test
status:
phase: Available
ishitasequeira marked this conversation as resolved.
Show resolved Hide resolved
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: argocd-test-redis
namespace: test

---
apiVersion: apps/v1
kind: Deployment
metadata:
name: argocd-test-repo-server
namespace: test

---
apiVersion: apps/v1
kind: Deployment
metadata:
name: argocd-test-server
namespace: test

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: argocd-test-server
namespace: test

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: argocd-test-application-controller
namespace: test

---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: argocd-test-argocd-application-controller
namespace: test

---
kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: argocd-test-argocd-server
namespace: test

---
kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: argocd-test-argocd-redis-ha
namespace: test

22 changes: 22 additions & 0 deletions tests/k8s/1-034_validate_applicationset_reconcile_enabled_set_false/01-install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
apiVersion: v1
kind: Namespace
metadata:
name: test
---

apiVersion: argoproj.io/v1beta1
kind: ArgoCD
metadata:
name: argocd-test
namespace: test
spec:
controller:
enabled: false
redis:
enabled: false
repo:
enabled: false
server:
enabled: false
applicationSet:
enabled: false
25 changes: 25 additions & 0 deletions tests/k8s/1-034_validate_applicationset_reconcile_enabled_set_false/02-assert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
apiVersion: argoproj.io/v1beta1
kind: ArgoCD
metadata:
name: argocd-test
namespace: test
status:
phase: Available
---
apiVersion: apps/v1
kind: StatefulSet
metadata:
name: argocd-test-application-controller
namespace: test
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: argocd-test-argocd-application-controller
namespace: test
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: argocd-test-argocd-application-controller
namespace: test
34 changes: 34 additions & 0 deletions tests/k8s/1-034_validate_applicationset_reconcile_enabled_set_false/02-errors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: argocd-test-redis
namespace: test

---
apiVersion: apps/v1
kind: Deployment
metadata:
name: argocd-test-repo-server
namespace: test

---
apiVersion: apps/v1
kind: Deployment
metadata:
name: argocd-test-server
namespace: test

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: argocd-test-server
namespace: test

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: argocd-test-repo-server
namespace: test
Loading
Loading