[stable/aws-pod-identity-webhook] Adding chart for AWS' new EKS Pod Identity Webhook #17099

max-rocket-internet · 2019-09-12T15:13:35Z

What this PR does / why we need it:

This chart will install the Amazon EKS Pod Identity Webhook. This tool allows you to specify IAM Roles for Kubernetes Service Accounts. This allows a pod to assume a IAM role.

Further details can be found here: https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts-technical-overview.html

Related issues:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md
Title of the PR starts with chart name (e.g. [stable/chart])

Signed-off-by: Max Williams <[email protected]>

k8s-ci-robot · 2019-09-12T15:13:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: max-rocket-internet
To complete the pull request process, please assign jlegrone
You can assign the PR to them by writing /assign @jlegrone in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-09-12T15:13:50Z

Hi @max-rocket-internet. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Max Williams <[email protected]>

micahhausler · 2019-09-12T17:42:56Z

I'll review this PR later today, but for the record I'm happy to be a maintainer/OWNER and join the helm GitHub org. Looks like membership isn't yet automated

nckturner · 2019-09-12T17:48:34Z

Also happy to be maintainer/member of the helm org.

jqmichael · 2019-09-12T17:53:26Z

Thanks for the PR!
Happy to be the maintainer/member of the helm org.

micahhausler

This is a great start! Thanks for the PR

micahhausler · 2019-09-12T17:57:04Z

stable/aws-pod-identity-webhook/templates/deployment.yaml

+    {{- end }}
+      serviceAccountName: {{ include "aws-pod-identity-webhook.fullname" . }}
+      containers:
+      - name: {{ .Chart.Name }}


I think the container name can probably be a stable name. Any objections?

This name is stable, it's the name of the chart, i.e. aws-pod-identity-webhook. But I agree with you, I would rather simply app or something else totally generic but this is the default name that comes from helm create. I've had objections in other PRs where things deviated from these defaults. Up to you, just say what you want it to be.

I'd lean toward something stable, that way you could compare apples to apples when looking at Prometheus metrics by container name

OK, I'll make it webhook. Cool?

micahhausler · 2019-09-12T17:57:41Z

stable/aws-pod-identity-webhook/templates/deployment.yaml

+        - --in-cluster
+        - --namespace=default
+        - --service-name={{ include "aws-pod-identity-webhook.fullname" . }}
+        - --tls-secret=pod-identity-webhook


Can you make this a configurable value that defaults to pod-identity-webhook?

Done.

Would it be better to use {{ include "aws-pod-identity-webhook.fullname" . }} though? Would this ever been installed multiple times in the same cluster?

micahhausler · 2019-09-12T17:57:53Z

stable/aws-pod-identity-webhook/templates/deployment.yaml

+        command:
+        - /webhook
+        - --in-cluster
+        - --namespace=default


This should be the release namespace

micahhausler · 2019-09-12T17:58:01Z

stable/aws-pod-identity-webhook/templates/deployment.yaml

+        - --namespace=default
+        - --service-name={{ include "aws-pod-identity-webhook.fullname" . }}
+        - --tls-secret=pod-identity-webhook
+        - --annotation-prefix=eks.amazonaws.com


This should be configurable in the values

micahhausler · 2019-09-12T17:58:09Z

stable/aws-pod-identity-webhook/templates/deployment.yaml

+        - --service-name={{ include "aws-pod-identity-webhook.fullname" . }}
+        - --tls-secret=pod-identity-webhook
+        - --annotation-prefix=eks.amazonaws.com
+        - --token-audience=sts.amazonaws.com


This should be configurable in the values

micahhausler · 2019-09-12T18:02:39Z

stable/aws-pod-identity-webhook/templates/deployment.yaml

+  labels:
+{{ include "aws-pod-identity-webhook.labels" . | indent 4 }}
+spec:
+  replicas: 1


We should set the default replicas to 2 for HA and make this tunable in the values

Cool. I wasn't sure if it stored stateful information or not so wasn't sure if we could run more than 1 pod.

I've set it to 3 like CoreDNS. OK?

micahhausler · 2019-09-12T18:02:53Z

stable/aws-pod-identity-webhook/templates/deployment.yaml

+    metadata:
+      labels:
+        app.kubernetes.io/name: {{ include "aws-pod-identity-webhook.name" . }}
+        app.kubernetes.io/instance: {{ .Release.Name }}


The {{ include "aws-pod-identity-webhook.labels" . | indent 4 }} should also be added

helm create doesn't add this here. Are you sure?

I'm surprised this isn't the default. I'm take it or leave it on adding it here

micahhausler · 2019-09-12T18:03:55Z

stable/aws-pod-identity-webhook/templates/role.yaml

+  - update
+  - patch
+  resourceNames:
+  - "{{ include "aws-pod-identity-webhook.fullname" . }}"


This should be the same value as the --tls-secret flag on the webhook

micahhausler · 2019-09-12T18:08:07Z

stable/aws-pod-identity-webhook/README.md

+$ helm install --name my-release stable/aws-pod-identity-webhook --set caBundle="${CA_BUNDLE}"
+```
+
+After installation you need to approve the certificate. Follow the chart notes after installation for this step.


Can you note that the webhook will request a new CSR prior to its expiry in 1 year, and it will need to be approved either by an operator or by some other automated process?

Cool. I added a note.

Would be great to have something better here in the chart itself. Perhaps a cronjob. Or something in the webhook itself?

I'd lean toward a cronjob that can only approve rather than something in the webhook. Its pretty scary to grant something the ability to create and approve certs

I'd lean toward a cronjob that can only approve

Makes sense. I'll leave this for a later PR though.

micahhausler · 2019-09-12T18:10:31Z

stable/aws-pod-identity-webhook/README.md

+
+## Prerequisites
+
+- Kubernetes 1.13+


Technically this works with 1.12+. It might also be worth saying that if you're not using EKS, you will need your cluster configured according to the SELF_HOSTED_SETUP.md

Nice. Added.

- fix namespace in command - add options for annotation_prefix / token_audience - updating doc and note about k8s version - fix quote - fix indentation - fix resourcename in role - add note about CSR expiry Signed-off-by: Max Williams <[email protected]>

max-rocket-internet · 2019-09-13T10:43:43Z

@lachie83, @unguiculus, @cpanato or @maorfr: these cats from AWS (@jqmichael, @nckturner, @micahhausler) want to be owners/maintainers of this chart. What's the process here? Can you add them to the Helm github org?

cpanato · 2019-09-13T14:20:27Z

@max-rocket-internet they need to follow this: https://github.com/helm/charts#trusted-collaborator

max-rocket-internet · 2019-09-13T14:48:26Z

/verify-owners

maorfr · 2019-09-15T06:13:38Z

/ok-to-test
/verify-owners

nckturner · 2019-09-15T10:35:35Z

stable/aws-pod-identity-webhook/Chart.yaml

+home: https://github.com/aws/amazon-eks-pod-identity-webhook
+sources:
+- https://github.com/aws/amazon-eks-pod-identity-webhook
+maintainers:


Can you add me as well here?

I had you there in the beginning but read the message here: #17099 (comment)

So I think we just have @micahhausler there for now as they are already a member of the kubernetes org.

And then you guys can create a PR later to add yourself when you become a trusted-collaborator

Signed-off-by: Max Williams <[email protected]>

max-rocket-internet · 2019-09-16T08:49:24Z

@micahhausler the pull-charts-e2e test is failing because of aws/amazon-eks-pod-identity-webhook#5

max-rocket-internet · 2019-09-16T09:28:27Z

@cpanato

If we want to merge this sooner rather than later, and sort the org membership thing in the future, how do we remove this do-not-merge/invalid-owners-file label? Just delete OWNERS? Then who can be in Chart.yaml as maintainer? Anyone?

yorinasub17 · 2019-09-17T17:59:26Z

Thanks for getting this in place so fast! I certainly don't want to delay this chart from being added, but I do have a small feature request: would it be possible to make the RBAC resources optional and have the service account passed in? E.g the external-dns chart has an input value rbac.create which controls whether or not to create the RBAC roles and rbac.serviceAccountName to set the service account on the pod.

Use case: We are still using Helm 2 with Tiller and our Tiller does not have the permissions to create RBAC resources. We rely on a separate system to create the RBAC roles/service accounts which we then pass into the charts.

EDITED: I can also wait until this version is merged, and open the PR afterwards. Also happy to open a PR against your fork, assuming it might take more time before this PR is ready.

max-rocket-internet · 2019-09-18T09:34:05Z

EDITED: I can also wait until this version is merged, and open the PR afterwards

I think we get this merged first, then make changes like you mention. The process for merging PRs after a chart has been added can be quite fast 🙂

micahhausler · 2019-09-18T16:25:09Z

stable/aws-pod-identity-webhook/values.yaml

@@ -0,0 +1,24 @@
+tls_secret_name: pod-identity-webhook


Can you document these added fields in the REAME?

Ah yes, I forgot to add these.

Signed-off-by: Max Williams <[email protected]>

k8s-ci-robot · 2019-09-25T10:29:57Z

The following users are mentioned in OWNERS file(s) but are not members of the helm org.

Once all users have been added as members of the org, you can trigger verification by writing /verify-owners in a comment.

micahhausler
- stable/aws-pod-identity-webhook/OWNERS

max-rocket-internet · 2019-09-25T10:30:02Z

@micahhausler still waiting on a docker image to be available 😄

k8s-ci-robot · 2019-09-25T10:45:10Z

@max-rocket-internet: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-charts-e2e	`233a1c0`	link	`/test pull-charts-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

stale · 2019-10-25T10:56:22Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

max-rocket-internet · 2019-10-25T11:55:34Z

@micahhausler any update here?

Waiting on you or someone in your team to:

Join the Helm org
Make docker images public

nckturner · 2019-10-28T08:28:03Z

@max-rocket-internet I can take a look at the images. Also, I believe I was added to trusted collaborators in #17126, if you can add me to maintainer of this chart.

Join the Helm org

Can you explain a little more--do you mean someone needs to join the helm github organization? The one I see doesn't have very many people in it.

max-rocket-internet · 2019-10-28T10:45:13Z

I can take a look at the images

Nice! The e2e job will fail until this is resolved.

Also, I believe I was added to trusted collaborators

🎉

Can you explain a little more--do you mean someone needs to join the helm

It's just in reference so this comment by k8s-ci-robot

max-rocket-internet · 2019-10-28T14:42:17Z

Or we move this PR to https://github.com/aws/eks-charts ?

nckturner · 2019-10-29T02:09:42Z

Yeah I was going to suggest that as an alternative. I think it will be easier to manage and easier for users to find everything eks related in one place.

…

On Mon, Oct 28, 2019 at 7:42 AM Max Williams ***@***.***> wrote: Or we move this PR to https://github.com/aws/eks-charts ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17099?email_source=notifications&email_token=AAJGJEJTE7TV57PH5Y3ZWADQQ326BA5CNFSM4IWF5EYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECNDSPQ#issuecomment-546978110>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJGJEIBK2OQC3ZFDWZ2SJTQQ326BANCNFSM4IWF5EYA> .

max-rocket-internet · 2019-10-29T15:25:42Z

OK cool. Please review my new PR: aws/eks-charts#28

initial commit

608f243

Signed-off-by: Max Williams <[email protected]>

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 12, 2019

k8s-ci-robot requested review from davidkarlsen and lachie83 September 12, 2019 15:13

helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 12, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Sep 12, 2019

remove owners

918eaa8

Signed-off-by: Max Williams <[email protected]>

max-rocket-internet mentioned this pull request Sep 12, 2019

Helm Chart for Deployments aws/amazon-eks-pod-identity-webhook#4

Open

fix maintainer

d1e7a0c

Signed-off-by: Max Williams <[email protected]>

micahhausler suggested changes Sep 12, 2019

View reviewed changes

helm-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 13, 2019

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 13, 2019

nckturner mentioned this pull request Sep 13, 2019

Add @nckturner as Trusted Collaborator #17126

Closed

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 15, 2019

nckturner reviewed Sep 15, 2019

View reviewed changes

Adding Micah as owner

7645d67

Signed-off-by: Max Williams <[email protected]>

micahhausler mentioned this pull request Sep 18, 2019

Add @micahhausler as trusted colaborator #17228

Closed

micahhausler reviewed Sep 18, 2019

View reviewed changes

change container name and update values in README

233a1c0

Signed-off-by: Max Williams <[email protected]>

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2019

stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2019

max-rocket-internet closed this Oct 29, 2019

max-rocket-internet deleted the aws-pod-identity-webhook branch October 30, 2019 17:14

[stable/aws-pod-identity-webhook] Adding chart for AWS' new EKS Pod Identity Webhook #17099

[stable/aws-pod-identity-webhook] Adding chart for AWS' new EKS Pod Identity Webhook #17099

Conversation

max-rocket-internet commented Sep 12, 2019 • edited Loading

What this PR does / why we need it:

Checklist

k8s-ci-robot commented Sep 12, 2019

k8s-ci-robot commented Sep 12, 2019

micahhausler commented Sep 12, 2019

nckturner commented Sep 12, 2019

jqmichael commented Sep 12, 2019

micahhausler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-rocket-internet commented Sep 13, 2019

cpanato commented Sep 13, 2019

max-rocket-internet commented Sep 13, 2019

maorfr commented Sep 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-rocket-internet commented Sep 16, 2019

max-rocket-internet commented Sep 16, 2019

yorinasub17 commented Sep 17, 2019 • edited Loading

max-rocket-internet commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 25, 2019

max-rocket-internet commented Sep 25, 2019

k8s-ci-robot commented Sep 25, 2019

stale bot commented Oct 25, 2019

max-rocket-internet commented Oct 25, 2019

nckturner commented Oct 28, 2019

max-rocket-internet commented Oct 28, 2019

max-rocket-internet commented Oct 28, 2019

nckturner commented Oct 29, 2019 via email

max-rocket-internet commented Oct 29, 2019

max-rocket-internet commented Sep 12, 2019 •

edited

Loading

yorinasub17 commented Sep 17, 2019 •

edited

Loading