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

feat: Decoupling application sync using impersonation #17403

Merged
merged 19 commits into from
Sep 4, 2024

Conversation

anandf
Copy link
Contributor

@anandf anandf commented Mar 5, 2024

Implementation of proposal #14255
Addresses issue #7689
Related PR in gitops-engine: argoproj/gitops-engine#534

Many engineers from Red Hat worked on this effort. This PR consolidates all their effort to have a single PR/merge commit for the entire feature implementation so that it easy for maintainers to review and merge it.
CLI changes - @ishitasequeira
GUI changes - @raghavi101 @keithchong
E2E Tests - @Mangaal

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Testing this feature

Prerequisites

  • make
  • docker
  • sed
  • kubectl
  • kind

Procedure

  1. Clone the repo and checkout the branch
git clone git@github.com:anandf/argo-cd.git
cd argo-cd
git checkout sync_with_impersonate
  1. Build the docker image and the CLI client. Push the docker image to quay.io for testing.
export QUAY_USER=<username_in_quay.io>
IMAGE_NAMESPACE=quay.io/$QUAY_USER make image build-local
docker push quay.io/$QUAY_USER/argocd:latest
ls -l ./dist/argocd
  1. Create a kind test cluster
kind create cluster --name argocd
  1. Modify the image name to use the image built and pushed in step 2.
export QUAY_USER=<username_in_quay.io> 
sed -i "s/quay.io\/argoproj\/argocd/quay.io\/$QUAY_USER\/argocd/g" manifests/install.yaml
  1. Install ArgoCD
kubectl create ns argocd
kubectl apply -f manifests/install.yaml -n argocd
kubectl config set-context --current --namespace argocd
  1. Enable the Application sync impersonation feature in argocd-cm
kubectl patch cm/argocd-cm -n argocd --type=merge -p='{"data":{"application.sync.impersonation.enabled":"true"}}'
  1. Create an App project
./dist/argocd proj create guestbook-proj -d https://kubernetes.default.svc,guestbook -s https://github.com/argoproj/argocd-example-apps.git --core
  1. Add destination service account configuration for guestbook ns as below
./dist/argocd proj add-destination-service-account guestbook-proj https://kubernetes.default.svc guestbook guestbook-deployer --core
  1. Create an argo application guestbook associated with AppProject guestbook-proj
./dist/argocd app create guestbook --core \
    --repo https://github.com/argoproj/argocd-example-apps \
    --path guestbook \
    --project guestbook-proj \
    --dest-server  https://kubernetes.default.svc \
    --dest-namespace guestbook \
    --directory-recurse \
    --sync-policy automated \
    --sync-option ServerSideApply=true
  1. Check if the application fails to sync as the service account is not created yet.
kubectl get application guestbook -n argocd -o yaml

Sample error message:

message: 'Namespace auto creation failed: namespaces "guestbook" is forbidden:
          User "system:serviceaccount:guestbook:guestbook-deployer" cannot get resource
          "namespaces" in API group "" in the namespace "guestbook"'
  1. Now create the service account guestbook-deployer in guestbook ns with the required access.
kubectl create ns guestbook
kubectl create sa guestbook-deployer -n guestbook
kubectl create rolebinding guestbook-deployer-rb -n guestbook --clusterrole cluster-admin --serviceaccount guestbook:guestbook-deployer
  1. Sync the application and see if the sync operation succeeds now.
./dist/argocd app sync argocd/guestbook --core
./dist/argocd app list --core
  1. Check the negative scenario when the sync operation fails with error when no matching SA is present.
./dist/argocd proj add-destination  guestbook-proj https://kubernetes.default.svc guestbook-dev --core
./dist/argocd app create guestbook-dev --core \
    --repo https://github.com/argoproj/argocd-example-apps \
    --path guestbook \
    --project guestbook-proj \
    --dest-server  https://kubernetes.default.svc \
    --dest-namespace guestbook-dev \
    --directory-recurse \
    --sync-policy automated \
    --sync-option ServerSideApply=true
  1. Check if the application fails to sync as the service account is not created yet.
kubectl get application guestbook-dev -n argocd -o yaml

Sample error message:

failed to find a matching service account to impersonate: no matching
      service account found for destination server https://kubernetes.default.svc
      and namespace guestbook-dev. (retried 5 times)

@anandf anandf requested review from a team as code owners March 5, 2024 13:55
@anandf anandf marked this pull request as draft March 5, 2024 13:55
@anandf anandf changed the title Sync with impersonate Decoupling application sync using impersonation Mar 6, 2024
@anandf anandf changed the title Decoupling application sync using impersonation [WIP] Decoupling application sync using impersonation Mar 6, 2024
@anandf anandf changed the title [WIP] Decoupling application sync using impersonation feat: [WIP] Decoupling application sync using impersonation Mar 6, 2024
@anandf anandf force-pushed the sync_with_impersonate branch 3 times, most recently from 25a9971 to 67e5ad8 Compare March 8, 2024 09:26
@anandf anandf force-pushed the sync_with_impersonate branch 3 times, most recently from c818883 to 56dd3a0 Compare March 20, 2024 08:57
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 15.78947% with 128 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@f071fdc). Learn more about missing BASE report.
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
cmd/argocd/commands/project.go 0.00% 96 Missing ⚠️
cmd/util/project.go 0.00% 15 Missing ⚠️
pkg/apis/application/v1alpha1/app_project_types.go 10.00% 8 Missing and 1 partial ⚠️
controller/sync.go 79.16% 2 Missing and 3 partials ⚠️
util/settings/settings.go 66.66% 1 Missing and 1 partial ⚠️
server/settings/settings.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #17403   +/-   ##
=========================================
  Coverage          ?   55.80%           
=========================================
  Files             ?      320           
  Lines             ?    44206           
  Branches          ?        0           
=========================================
  Hits              ?    24669           
  Misses            ?    16977           
  Partials          ?     2560           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anandf anandf force-pushed the sync_with_impersonate branch 3 times, most recently from ab70455 to 06f2c6b Compare March 26, 2024 08:52
@anandf anandf force-pushed the sync_with_impersonate branch 8 times, most recently from a5f55a8 to 8e24832 Compare April 26, 2024 02:52
@anandf anandf changed the title feat: [WIP] Decoupling application sync using impersonation feat: Decoupling application sync using impersonation Apr 30, 2024
@anandf anandf marked this pull request as ready for review April 30, 2024 08:46
@anandf anandf force-pushed the sync_with_impersonate branch 2 times, most recently from 9f1694c to db73370 Compare September 4, 2024 12:54
anandf and others added 19 commits September 4, 2024 22:23
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com>
Signed-off-by: Anand Francis Joseph <anandfrancis.joseph@gmail.com>
Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com>
Signed-off-by: Anand Francis Joseph <anandfrancis.joseph@gmail.com>
…ing logic

Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
…operation

Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
Signed-off-by: anandf <anjoseph@redhat.com>
…sa is found

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM

@jannfis jannfis merged commit 1028808 into argoproj:master Sep 4, 2024
27 of 28 checks passed
controller/sync.go Show resolved Hide resolved
controller/sync.go Show resolved Hide resolved
pkg/apis/application/v1alpha1/types.go Show resolved Hide resolved
util/settings/settings.go Show resolved Hide resolved
test/e2e/sync_with_impersonate_test.go Show resolved Hide resolved
test/e2e/sync_with_impersonate_test.go Show resolved Hide resolved
cmd/argocd/commands/project.go Show resolved Hide resolved
cmd/argocd/commands/project.go Show resolved Hide resolved
docs/operator-manual/argocd-cm.yaml Show resolved Hide resolved
@alpozcan
Copy link

alpozcan commented Oct 1, 2024

Keen to see this feature stabilise!

FYI, I receive the following error following
argocd proj add-destination-service-account guestbook-proj https://kubernetes.default.svc guestbook guestbook-deployer --core on Argo CD 2.13.0 RC2:

W1001 11:13:09.905522   44304 warnings.go:70] unknown field "spec.destinationServiceAccounts"

which I'm guessing is because #19966 is still to be merged.

Also, is having the SA association at Application level planned? It seems to me that having this at Project level would force users to have one Project per Application if each app has its own deployer SA.

@anandf
Copy link
Contributor Author

anandf commented Oct 2, 2024

Thanks @alpozcan for trying out this feature and providing a feedback. The error you are seeing, looks like the server/client may not be running the version with this code change. Could you try with the latest changes.
Please check the server and client versions using the following commands to confirm if you are using the right versions.

argocd version --server
argocd version --client

Also, is having the SA association at Application level planned?
There is a plan to implement it, if we receive sufficient interests. Since, it affects the security posture, we want to have the admins to control it. You can still add multiple destinations to the same project and refer them in multiple applications.
For eg:

argocd proj add-destination-service-account guestbook-proj https://dev-cluster.example.com:6443 guestbook-dev guestbook-deployer --core
argocd proj add-destination-service-account guestbook-proj https://staging-cluster.example.com:6443 guestbook-stage guestbook-deployer --core
argocd proj add-destination-service-account guestbook-proj https://prod-cluster.example.com:6443 guestbook-prod guestbook-deployer --core

Different applications having different destinations can then be targeted to the same guestbook-proj.
Let me know if the above solution can work for your case, or is it something else that I misunderstood.

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.

8 participants