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

refactor: replace aws CLI with argocd-k8s-auth #8032

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented Dec 23, 2021

Signed-off-by: Alexander Matyushentsev AMatyushentsev@gmail.com

PR is inspired by #7947 . PR removes aws and replaces it with the new argocd-k8s-auth binary . The argocd-k8s-auth provides argocd-k8s-auth aws command that generates authentication token.

This change provides several following advantages: removes ~120+ mb from the image, gives us the opportunity to support GKE authentication #3027

The disadvantage is that we have to support more code. I also could not find any easy way to test it, since we need real EKS cluster.

Let me know what you think please

@alexmt
Copy link
Collaborator Author

alexmt commented Dec 23, 2021

@jannfis , @jessesuen what do you think? Does it worth it?

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #8032 (b6f391a) into master (6de5516) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8032      +/-   ##
==========================================
- Coverage   45.51%   45.47%   -0.05%     
==========================================
  Files         217      217              
  Lines       25663    25663              
==========================================
- Hits        11680    11669      -11     
- Misses      12355    12369      +14     
+ Partials     1628     1625       -3     
Impacted Files Coverage Δ
pkg/apis/application/v1alpha1/types.go 54.63% <0.00%> (ø)
applicationset/services/scm_provider/github.go 63.52% <0.00%> (-17.65%) ⬇️
applicationset/services/scm_provider/utils.go 88.50% <0.00%> (+4.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6de5516...b6f391a. Read the comment docs.

Copy link
Member

@pasha-codefresh pasha-codefresh 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
Copy link
Member

jannfis commented Dec 31, 2021

I do like this change very much - instead of pulling in several (large) vendor specific binaries, having a single binary using the vendor's libraries is the right way to go. I share the concern for code support tho, because maintaining this piece of code would require access to a vendor specific cluster. I for my part have zero access to any AWS or Google platforms.

I have a couple of cosmetic nits for this one:

  1. I think it would be better to get rid of the k8s part in the binary name, in case we support other auth targets than Kubernetes in the future. What do you think about argocd-ext-auth or argocd-external-auth?

  2. I'm not very familiar with all things AWS, but I think the command aws should rather be eks or aws-eks if it targets EKS only and not one of the other AWS offerings.

@alexmt
Copy link
Collaborator Author

alexmt commented Jan 6, 2022

Thank you for reviewing @jannfis ! I agree - argocd-external-auth sounds better. Regarding aws vs eks: I think aws still makes sense. IAM authentication works for both EKS and manually provisioned clusters, as long as the cluster is in AWS.

@alexmt
Copy link
Collaborator Author

alexmt commented Jan 6, 2022

@jessesuen pointed out that We could use https://github.com/kubernetes-sigs/aws-iam-authenticator . In the past, we've moved from aws-iam-authenticator to aws cli to get support IRSA (#3010) . Currently aws-iam-authenticator supports IRSA as well.

The aws-iam-authenticator binary is only 30 MB so if we use it then saving 30MB is probably not worth introducing a new binary. However, GKE authentication is definitely worth it. I'm working on adding GKE support in the same PR

@alexmt
Copy link
Collaborator Author

alexmt commented Jan 18, 2022

@jannfis Thanks for mentioning #6441 ! One more reason to finish this PR

@zhang-xuebin
Copy link

Any ETA on this PR? I'm interested to see if we can enhance to support GCP auth as a followup.

@alexmt alexmt marked this pull request as draft February 16, 2022 21:14
@owenhaynes
Copy link

@zhang-xuebin by GCP support you mean workload identity?

@zhang-xuebin
Copy link

@zhang-xuebin by GCP support you mean workload identity?

I mean to allow ArgoCD's application controller (and server) to be able to obtain a GCP Oauth token so it can call Google APIs and manage more platforms provided by Google (including Private GKE, GKE-on-VMware, etc).
So Workload identity is part of it. If the ArgoCD's host cluster supports WI, then good. If not, we can use other ways like putting a GCP SA key file as a workaround.

I have figured a short-term solution to use Workload Identity and let ArgoCD manage Private GKE and GKE-on-VMware clusters. But if Argo can support this natively, it will be definitely fantastic.

@lacarvalho91
Copy link
Contributor

@zhang-xuebin

I have figured a short-term solution to use Workload Identity and let ArgoCD manage Private GKE and GKE-on-VMware clusters.

how did you accomplish this?

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

For posterity the options were:

  1. aws cli - too big, brings in python. we only used it because it was the only one that supported IRSA originally
  2. aws-iam-authenticator - one more binary to vendor
  3. argocd-k8s-auth aws - our own command which replicates functionality of aws-iam-authenticator

After discussion, we agreed to go with option 3 because:

  1. it produces the smallest image size
  2. code maintenance will be easier than expected (only needs two AWS API calls)
  3. dependabot can help us keep up-to-date (vs. upstream CLI)

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
@alexmt alexmt merged commit 655be25 into argoproj:master Apr 15, 2022
@alexmt alexmt deleted the aws-auth branch April 15, 2022 00:25
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
leoluz added a commit to leoluz/argo-cd that referenced this pull request Aug 16, 2022
This reverts commit 655be25.

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
leoluz added a commit to leoluz/argo-cd that referenced this pull request Aug 16, 2022
This reverts commit 655be25.

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.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.

None yet

7 participants