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

patch pod spec even if it's already been patched #62

Merged
merged 3 commits into from
Oct 21, 2020

Conversation

yangkev
Copy link
Contributor

@yangkev yangkev commented Jun 3, 2020

Before, the updatePodSpec function would return immediately if it detected that the
aws-iam-token volume was already present.

This patch modifies the behavior so that the function continues to
inject environment variables and volume mounts as needed.

Specifically, the behavior of injecting env vars, volume mounts, and
volumes is now:

  1. For each env var in "AWS_ROLE_ARN, AWS_WEB_IDENTITY_TOKEN_FILE,
    AWS_REGION, AWS_DEFAULT_REGION", inject if not already present in
    container spec

  2. Inject volume mount if not already present in container spec

  3. Inject volume if not already present in pod spec

Fixes #61

@yangkev yangkev requested a review from a team as a code owner June 3, 2020 22:10
@yangkev yangkev marked this pull request as draft June 3, 2020 22:12
@yangkev yangkev marked this pull request as ready for review June 3, 2020 22:14
before, the updatePodSpec function would return immediately if it detected that the
aws-iam-token volume was already present.

This patch modifies the behavior so that the function continues to
inject environment variables and volume mounts as needed.

Specifically, the behavior of injecting env vars, volume mounts, and
volumes is now:

1. For each env var in "AWS_ROLE_ARN, AWS_WEB_IDENTITY_TOKEN_FILE,
AWS_REGION, AWS_DEFAULT_REGION", inject if not already present in
container spec

2. Inject volume mount if not already present in container spec

3. Inject volume if not already present in pod spec
@yangkev
Copy link
Contributor Author

yangkev commented Sep 16, 2020

@micahhausler I think folks who want to adopt pod-identity-webhook and use the sidecar pattern will find this useful. Could someone take a look?

Copy link
Contributor

@josselin-c josselin-c left a comment

Choose a reason for hiding this comment

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

I think the test needs to be improved a bit to better test what this patch is doing. Otherwise looks good to me.

Copy link
Contributor Author

@yangkev yangkev left a comment

Choose a reason for hiding this comment

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

Also added a test that initContainer's get modified as well

pkg/handler/handler_test.go Show resolved Hide resolved
Copy link
Contributor

@josselin-c josselin-c left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@josselin-c josselin-c merged commit d808b78 into aws:master Oct 21, 2020
@luciano-nbs
Copy link

Hi guys, i think i'm hitting this issue in our EKS cluster, how do we know if this fix is in our EKS cluster? I couldn't find any information on releases. thanks

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.

re-invoking webhook exits early
4 participants