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

Added support for IRSA, fixes - #86 #169

Merged
merged 8 commits into from
Apr 13, 2021
Merged

Added support for IRSA, fixes - #86 #169

merged 8 commits into from
Apr 13, 2021

Conversation

infa-mhadiman
Copy link
Contributor

Adding support for IAM roles for service accounts (IRSA) on EKS. This PR takes care of injecting the volume mounts and env variables when a deployment & SA is configured to use pod identity (IRSA).

Looks up an app container for aws-iam-token volume mount and if found, uses the same and mounts it to vault init & sidecar containers.

**Fixes - #86 **

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 13, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@tvoran tvoran self-requested a review August 14, 2020 23:59
@infa-mhadiman
Copy link
Contributor Author

Hi @tvoran ,
I've added changes to resolve the conflicts. Can you please review ? and let me know if there are any further changes.

Thanks

@tvoran
Copy link
Member

tvoran commented Dec 14, 2020

Hi @infa-mhadiman, apologies for the delay, I should be able to try this out this week.

tvoran
tvoran reviewed Mar 10, 2021
View reviewed changes
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Good stuff here! I think it could be really useful, especially now that #213 has been merged.

I left some suggestions, mostly around style. And in general I think the AWS-specific items like this should only be triggered if AuthType=="aws" if possible. Let me know what you think!

agent-inject/agent/container_init_sidecar.go Outdated Show resolved Hide resolved
agent-inject/agent/container_sidecar.go Outdated Show resolved Hide resolved
agent-inject/agent/container_sidecar_test.go Outdated Show resolved Hide resolved
agent-inject/agent/agent.go Outdated Show resolved Hide resolved
agent-inject/agent/container_env.go Outdated Show resolved Hide resolved
agent-inject/agent/container_env.go Outdated Show resolved Hide resolved
agent-inject/agent/agent.go Outdated Show resolved Hide resolved
agent-inject/agent/agent.go Outdated Show resolved Hide resolved
@infa-mhadiman
Copy link
Contributor Author

infa-mhadiman commented Mar 12, 2021 •
edited
Loading

Agreed, I'll make the suggested changes. thanks for the review.

@infa-mhadiman infa-mhadiman requested a review from tvoran March 12, 2021 11:08
@infa-mhadiman
Copy link
Contributor Author

Hey @tvoran,
I've updated the PR with the suggested changes. Can you please review and let me know if there is any change req ?

Thanks

tvoran
tvoran reviewed Apr 3, 2021
View reviewed changes
agent-inject/agent/container_env.go Outdated Show resolved Hide resolved
agent-inject/agent/container_env.go Outdated Show resolved Hide resolved
@infa-mhadiman
Copy link
Contributor Author

This should be good now, can you please review again ? @tvoran

@infa-mhadiman infa-mhadiman requested a review from tvoran April 5, 2021 07:34
@tvoran
Copy link
Member

tvoran commented Apr 6, 2021

Hi @infa-mhadiman, I was experimenting with IRSA on EKS, and I don't know if something changed recently there, but it looks like the IRSA env vars and token are being added to the vault-agent init and sidecar containers without any special handling required by vault-k8s. I'm wondering if you're still seeing the need for this PR? If so, can you detail your setup/use case?

@infa-mhadiman
Copy link
Contributor Author

infa-mhadiman commented Apr 6, 2021 •
edited
Loading

hmm, interesting. @tvoran, what is the EKS version ur testing on ?

The pod identity webhook will inject the IRSA env vars and token for all the available containers in a POD. so, wondering how this is possible if the agent injector webhook hasn't mutated the object yet.

During my testing, I observed the mutating webooks invocation to be in the order they exist (k get mutatingwebhookconfigurations.admissionregistration.k8s.io), the pod identity webhook is being invoked even before, vault injector webhook. So, do you've a vault injector webhook configuration with a different name ? such that it's being invoked before pod identity webhook ?

@infa-mhadiman
Copy link
Contributor Author

Coming to our setup.
we use a old vault 1.3.x on EKS 1.15, 1.16, which does not have OIDC support for vault, the vault sidecar being injected invokes a small program which performs an assume webidentity call, gets the creds and makes it available for vault agent to perform an auth.

@marknet15
Copy link

marknet15 commented Apr 6, 2021 •
edited
Loading

@infa-mhadiman @tvoran If it helps at my company, we were recently testing with EKS 1.18 & 1.19 and Vault v1.6.1 (hosted on ec2 utilising IAM auth) with the latest version of the Vault sidecar agent to get it up and running with the the web identity IAM service account integration, and we were only seeing the EKS node IAM being used by the Vault sidecar instead of the web identity token. So I think this is definitely still needed.

Our setup makes use of the OIDC IAM provider that maps an IAM role through to the pods service account, which is able to be used fine on the actual pod app container to access AWS resources etc. However we weren't able to authenticate on the vault agent with this same auth as it fell back to the node IAM role.

@tvoran
Copy link
Member

tvoran commented Apr 8, 2021

@infa-mhadiman Yeah, I'm just testing with new EKS clusters, so they're on 1.18, and it certainly wouldn't surprise me if this behavior changed between EKS versions. Nothing special with naming the injector webhook, just the defaults for the chart.

I do see that the pod-identity-webhook in my EKS cluster has reinvocationPolicy: IfNeeded, so it'll process a pod again if another webhook modified the pod after the initial mutation. And if I set that as reinvocationPolicy: Never I get the behavior you've described. Though with this PR the env variables are set on the vault-agent init container, but the volume mount isn't added. I should be able to do more testing tomorrow.

tvoran
tvoran reviewed Apr 10, 2021
View reviewed changes
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

The trouble I was having before was because of the check for AnnotationVaultAuthPath instead of AnnotationVaultAuthType when looking for the IAM token volume. With that change it started working for me. Also a few other test comments.

agent-inject/agent/agent.go Outdated Show resolved Hide resolved
agent-inject/agent/container_sidecar_test.go Outdated Show resolved Hide resolved
agent-inject/agent/container_sidecar_test.go Show resolved Hide resolved
agent-inject/agent/container_sidecar_test.go Show resolved Hide resolved
agent-inject/agent/container_sidecar_test.go Outdated Show resolved Hide resolved
agent-inject/agent/container_sidecar_test.go Outdated Show resolved Hide resolved
@infa-mhadiman infa-mhadiman requested a review from tvoran April 10, 2021 06:41
@infa-mhadiman
Copy link
Contributor Author

infa-mhadiman commented Apr 10, 2021 •
edited
Loading

@tvoran
Thanks for the feedback. I've addressed the review comments, please review.

tvoran
tvoran reviewed Apr 12, 2021
View reviewed changes
agent-inject/agent/agent.go Outdated Show resolved Hide resolved
@infa-mhadiman infa-mhadiman requested a review from tvoran April 13, 2021 17:29
@tvoran
Copy link
Member

tvoran commented Apr 13, 2021

Thanks for your patience with this one. We'll get it in to the next release!

@tvoran tvoran merged commit e651f63 into hashicorp:master Apr 13, 2021
@tvoran tvoran mentioned this pull request Apr 20, 2021
Closed
RemcoBuddelmeijer pushed a commit to RemcoBuddelmeijer/vault-k8s that referenced this pull request Feb 22, 2022
@infa-mhadiman
Added support for IRSA, fixes - hashicorp#86 (hashicorp#169)
daee8a2 
* fix failing tests

* removed check for length of envmap

* perform strict compare for env names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvoran tvoran tvoran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@infa-mhadiman @hashicorp-cla @tvoran @marknet15