-
Notifications
You must be signed in to change notification settings - Fork 505
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
Add support for IAM proxies for S3 #1484
base: main
Are you sure you want to change the base?
Conversation
1) Adding env variable to set mode on/off 2) Using env variable, bypass credential provider creation if env var is set to true. 3) add helm support
1fc8043
to
003c58f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaqel-s Thanks for this! I left a few comments for you, when you get a chance. Also, can you add the required configuration to the config.dev.toml
that correspond to the UseIAMProxy
?
@@ -24,6 +24,9 @@ spec: | |||
checksum/upstream: {{ include (print $.Template.BasePath "/config-upstream.yaml") . | sha256sum }} | |||
checksum/ssh-config: {{ include (print $.Template.BasePath "/config-ssh-git-servers.yaml") . | sha256sum }} | |||
checksum/ssh-secret: {{ include (print $.Template.BasePath "/secret-ssh-git-servers.yaml") . | sha256sum }} | |||
{{- if .Values.storage.s3.iamRole }} | |||
iam.amazonaws.com/role: {{ .Values.storage.s3.iamRole | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaqel-s will a controller/operator need to be present in the cluster to detect this annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, here's an example https://github.com/uswitch/kiam
go.sum
Outdated
@@ -11,15 +11,21 @@ github.com/Azure/azure-pipeline-go v0.2.1 h1:OLBdZJ3yvOn2MezlWvbrBMTEUQC72zAftRZ | |||
github.com/Azure/azure-pipeline-go v0.2.1/go.mod h1:UGSo8XybXnIGZ3epmeBw7Jdz+HiUVpqIlpz/HKHylF4= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaqel-s did you add/remove/change any dependencies that caused the go.sum
to change? If not, I'd like to take out these changes
@aaqel-s just wanted to check in and see if you saw the other changes requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with S3's Go client, but one thing off the bat is we need to update config.dev.toml
since this adds a new property :)
Sorry folks, I got sidetracked with other stuff at work, I'll try to get back to this asap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, one more thing @aaqel-s - can you please change the |
Just doing a quick review since this is in the post 0.13.x milestone:
I'm willing to take this change over if @aaqel-s doesn't have time. I don't really have a way to test it however as this requires a specific Kubernetes setup. |
What is the problem I am trying to address?
Adding support for AWS IAM proxies in Kubernetes
Currently, S3 support in S3s only supports AWS configuration using Hardcoded credentials. Our application of K8s uses KIAM avoid using hardcoded creds for AWS by having AWS requests proxied through a sidecar that injects credentials based on the IAM role of the pod.
How is the fix applied?
Added an environment variable to enable support for IAM proxies and then added an if statement in s3.go to bypass the AWS cred provider (which is not needed when using a proxy.
Added two flags to Helm to configure IAM support and the particular role a user wants the proxy attached to.
Verified with make docker and running in our k8s cluster.
Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.
Fixes #1452