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.

Sign up for GitHub

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

Jump to bottom

Add support for IAM proxies for S3 #1484

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add support for IAM proxies for S3 #1484

wants to merge 10 commits into from

Conversation

aaqel-s
Copy link

@aaqel-s aaqel-s commented Dec 5, 2019

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

@aaqel-s aaqel-s requested a review from a team as a code owner December 5, 2019 21:59
@aaqel-s aaqel-s changed the title Add support for IAM proxies for S3, like Kube2IAM and KIAM by: Add support for IAM proxies for S3 Dec 5, 2019
@aaqel-s
Add support for IAM proxies for S3, like Kube2IAM and KIAM by:
dcde2ab 
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
@aaqel-s
Fix go sum
a5e3c2f
@aaqel-s
Update chart version
12bdf26
@aaqel-s
Add missing end in helm template
003c58f
arschles
arschles requested changes Dec 6, 2019
View reviewed changes
Copy link
Member

@arschles arschles left a 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?

charts/athens-proxy/templates/deployment.yaml Outdated
@@ -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 }}
Copy link
Member

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?

Copy link
Author

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=
Copy link
Member

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

@ghost
Copy link

ghost commented Jan 20, 2020

@aaqel-s just wanted to check in and see if you saw the other changes requested.

marwan-at-work
marwan-at-work requested changes Jan 20, 2020
View reviewed changes
Copy link
Contributor

@marwan-at-work marwan-at-work left a 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 :)

@aaqel-s
Copy link
Author

aaqel-s commented Jan 21, 2020

Sorry folks, I got sidetracked with other stuff at work, I'll try to get back to this asap.

arschles
arschles approved these changes Feb 19, 2020
View reviewed changes
Copy link
Member

@arschles arschles left a comment

Choose a reason for hiding this comment

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

@aaqel-s this looks good, and thanks for your patience! There were 3 files with conflicts that I resolved in ca8ec0a. Can you take a look and make sure it looks ok to you? If so, then I'll merge this :)

🎉 :shipit: 🚀

@arschles
Copy link
Member

Oh, one more thing @aaqel-s - can you please change the version field in Chart.yaml to 0.4.4?

@arschles arschles changed the base branch from master to main June 15, 2020 19:21
@DrPsychick DrPsychick added this to the 0.12.x thereafter milestone Mar 28, 2023
@matt0x6F
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marwan-at-work marwan-at-work marwan-at-work requested changes

@arschles arschles arschles approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.x thereafter
Development

Successfully merging this pull request may close these issues.

Support for KIAM and kube2iam for S3 storage
5 participants
@aaqel-s @arschles @matt0x6F @marwan-at-work @DrPsychick