nabsul / k8s-ecr-login-renew

Renews Docker login credentials for an AWS ECR container registry.
MIT License
205 stars 49 forks source link

Please implement ability to leverage iRSA #45

Open jwitko opened 1 year ago

jwitko commented 1 year ago

Is there any chance you could implement the ability for this app/chart to have the ability to leverage the aws-pod-identity-webhook which implements iRSA ? It should be pretty straight forward if you're using a relatively modern AWS SDK version. You would also have to remove the hard requirements for the AWS credential env vars.

This would get us the ability to have dynamic ecr credentials and not have to store AWS user credentials in the cluster. It would be a huge win and even better than the new kubernetes native image credential helper which requires AWS credentials on the nodes.

nabsul commented 1 year ago

This sounds completely doable... however, I wonder if it's really that relevant to this particular tool.

The reason I ask is that the idea behind this tool is to make accessing ECR easy from non-AWS Kubernetes clusters. If you're running in EKS, don't you have better ways to get access to ECR? (do you even need to use k8s-ecr-login-renew?)

jwitko commented 1 year ago

If you're running in EKS

Not running in EKS. Using a hybrid infrastructure with kubernetes running in an on-premise datacenter but leveraging the aws-pod-identity-webhook to provide iRSA capabilities to the service accounts within the on-premise cluster.

nabsul commented 1 year ago

Silly question, but have you tried using this tool without providing the environment variables? I'm using NewSession() to generate credentials, so it's already not hard-coded to only use environment variables.

https://docs.aws.amazon.com/sdk-for-go/api/aws/session/

jwitko commented 1 year ago

Yes an up to date version of the standard SDK credentials chain would definitely support iRSA through the instancePrincipals method. However the issue is your helm chart https://github.com/nabsul/k8s-ecr-login-renew/blob/main/chart/templates/005-CronJob.yaml#L48-L54

I did not make an attempt to modify and use a local version of your helm chart

nabsul commented 1 year ago

I think I can make a fairly safe tweak to the chart to allow you to test this: https://github.com/nabsul/k8s-ecr-login-renew/pull/53/files

nabsul commented 1 year ago

helm chart is published.

nabsul commented 1 year ago

Actually, you might also want to change the docker image to the latest one here: https://hub.docker.com/layers/nabsul/k8s-ecr-login-renew/sha-adba342/images/sha256-b55d183b333eeebaff8affdb446601b63d7c76829974f0ee487aab2a843b35db?context=explore

It has all the latest dependency updates. I'm not sure if the current official version has a new enough AWS SDK.

The downside of this image is that I haven't tested it yet. There could be a bug, especially around the namespace detection logic 🤷

Hokwang commented 9 months ago

I think that we need to add annotation in serviceaccount. It is needed for IRSA.