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.

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: Dex resource cleanup when .spec.dex & DISABLE_DEX is set #922

Merged
merged 3 commits into from
Jun 2, 2023

Conversation

svghadi
Copy link
Collaborator

@svghadi svghadi commented May 31, 2023

What type of PR is this?
/kind bug

What does this PR do / why we need it:
The secret reconciliation code doesn't respect DISABLE_DEX env var causing the entire resource reconciliation loop to sometimes break due to which the dex resources are not cleaned up properly. This happens mainly when a dex deployment exists(configured using .spec.dex) and is disabled using DISABLE_DEX=true. The sso reconciliation code doesn’t handle deletion in such case,

// no SSO configured, nothing to do here
if !UseDex(cr) {
return nil
}
due to which deletion is deferred to main resource reconciliation
log.Info("reconciling SSO")
if err := r.reconcileSSO(cr); err != nil {
return err
}
log.Info("reconciling status")
if err := r.reconcileStatus(cr); err != nil {
return err
}
log.Info("reconciling roles")
if err := r.reconcileRoles(cr); err != nil {
log.Info(err.Error())
return err
}
log.Info("reconciling rolebindings")
if err := r.reconcileRoleBindings(cr); err != nil {
log.Info(err.Error())
return err
}
log.Info("reconciling service accounts")
if err := r.reconcileServiceAccounts(cr); err != nil {
log.Info(err.Error())
return err
}
log.Info("reconciling certificate authority")
if err := r.reconcileCertificateAuthority(cr); err != nil {
return err
}
log.Info("reconciling secrets")
if err := r.reconcileSecrets(cr); err != nil {
return err
}
useTLSForRedis := r.redisShouldUseTLS(cr)
log.Info("reconciling config maps")
if err := r.reconcileConfigMaps(cr, useTLSForRedis); err != nil {
return err
}
log.Info("reconciling services")
if err := r.reconcileServices(cr); err != nil {
return err
}
log.Info("reconciling deployments")
if err := r.reconcileDeployments(cr, useTLSForRedis); err != nil {
return err
}
loop which starts deletion of dex role & service account before deployment & secret. By the time main reconciliation loops starts reconciling secrets and if dex service account is deleted, the secret reconciliation breaks with "error": "ServiceAccount \"argocd-argocd-dex-server\" not found" causing entire loop to break.

This PR fixes the issue by ensuring secret reconciliation considers DISABLE_DEX env var while reconciling secrets so that reconciliation loop doesn't break due to already deleted dex service account.

Which issue(s) this PR fixes:

Fixes #920

How to test changes / Special notes to the reviewer:
Build operator image

export IMG=quay.io/svghadi/argocd-operator:dex-fix         # override the name 
make docker-build
make docker-push

Testing

  1. Create a k3d cluster
    k3d cluster create test --servers 3
  2. Deploy operator
    make deploy IMG=quay.io/svghadi/argocd-operator:dex-fix
  3. Wait for operator resources to come up
    kubectl -n argocd-operator-system get all
  4. Create a argocd
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: argocd
spec:
  dex:
    openShiftOAuth: true
  1. Wait for dex resources to come up
  2. Disable dex by setting DISABLE_DEX env var
    Directly patch operator controller to simulate update made by operator subscription
    kubectl -n argocd-operator-system set env deployment.apps/argocd-operator-controller-manager DISABLE_DEX="true"
  3. Check operator controller logs
    Should not contain ServiceAccount \"argocd-argocd-dex-server\" not found errors.
    $ kubectl -n argocd-operator-system logs deployment.apps/argocd-operator-controller-manager -c manager
    1.6855994395547652e+09	INFO	controller_argocd	reconciling SSO
    1.6855994395646634e+09	INFO	controller_argocd	reconciling status
    1.685599439594475e+09	INFO	controller_argocd	reconciling roles
    1.6855994395962162e+09	INFO	controller_argocd	deleting the existing Dex role because dex is not configured
    1.6855994396098444e+09	INFO	controller_argocd	reconciling roles for source namespaces
    1.6855994396100032e+09	INFO	controller_argocd	performing cleanup for source namespaces
    1.6855994396100667e+09	INFO	controller_argocd	reconciling rolebindings
    1.6855994397123575e+09	INFO	controller_argocd	deleting the existing Dex service account because dex uninstallation requested
    1.6855994397173743e+09	INFO	controller_argocd	deleting the existing Dex roleBinding because dex uninstallation is requested
    1.6855994397319517e+09	INFO	controller_argocd	reconciling service accounts
    1.6855994397379866e+09	INFO	controller_argocd	reconciling certificate authority
    1.6855994397380471e+09	INFO	controller_argocd	reconciling CA secret
    1.6855994397384193e+09	INFO	controller_argocd	reconciling CA config map
    1.6855994397387211e+09	INFO	controller_argocd	reconciling secrets
    1.6855994398576467e+09	INFO	controller_argocd	reconciling config maps
    1.6855994398582098e+09	INFO	controller_argocd	Using default resource tracking method 'label'
    1.68559943987194e+09	INFO	controller_argocd	reconciling services
    1.6855994398729813e+09	INFO	controller_argocd	deleting the existing Dex service because dex uninstallation has been requested
    1.685599439887624e+09	INFO	controller_argocd	reconciling deployments
    1.6855994398886366e+09	INFO	controller_argocd	deleting the existing dex deployment because dex uninstallation has been requested
    1.6855994399254842e+09	INFO	controller_argocd	reconciling statefulsets
    
  4. All dex resources should be deleted
    kubectl get deployments,services,sa,roles,rolebindings

The secret reconciliation code doesn't respect DISABLE_DEX env var
causing the entire resource reconciliation loop to break sometimes
due to which the dex resources are not cleaned up properly.

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
@svghadi svghadi closed this Jun 1, 2023
@svghadi svghadi reopened this Jun 1, 2023
@svghadi svghadi marked this pull request as ready for review June 1, 2023 07:33
Copy link
Collaborator

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

/lgtm

@jaideepr97
Copy link
Collaborator

Great catch @svghadi !

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
@svghadi svghadi requested a review from jaideepr97 June 2, 2023 05:49
@jaideepr97 jaideepr97 merged commit 9b15d52 into argoproj-labs:master Jun 2, 2023
7 checks passed
@svghadi svghadi deleted the sso-test-fix branch June 2, 2023 13:42
svghadi added a commit to svghadi/argocd-operator that referenced this pull request Jun 12, 2023
…oj-labs#922)

* Fix reconciliation when deprecated DISABLE_DEX & .spec.dex are set

The secret reconciliation code doesn't respect DISABLE_DEX env var
causing the entire resource reconciliation loop to break sometimes
due to which the dex resources are not cleaned up properly.


---------

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
svghadi added a commit to svghadi/argocd-operator that referenced this pull request Jun 12, 2023
…oj-labs#922)

* Fix reconciliation when deprecated DISABLE_DEX & .spec.dex are set

The secret reconciliation code doesn't respect DISABLE_DEX env var
causing the entire resource reconciliation loop to break sometimes
due to which the dex resources are not cleaned up properly.


---------

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
jaideepr97 pushed a commit that referenced this pull request Jun 13, 2023
…931)

* Fix reconciliation when deprecated DISABLE_DEX & .spec.dex are set

The secret reconciliation code doesn't respect DISABLE_DEX env var
causing the entire resource reconciliation loop to break sometimes
due to which the dex resources are not cleaned up properly.


---------

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
jaideepr97 pushed a commit that referenced this pull request Jun 13, 2023
…932)

* Fix reconciliation when deprecated DISABLE_DEX & .spec.dex are set

The secret reconciliation code doesn't respect DISABLE_DEX env var
causing the entire resource reconciliation loop to break sometimes
due to which the dex resources are not cleaned up properly.


---------

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dex resources not deleted when .spec.dex & DISABLE_DEX is set
3 participants