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

Adding chart for AWS' new EKS Pod Identity Webhook #28

Closed
wants to merge 11 commits into from
Closed

Adding chart for AWS' new EKS Pod Identity Webhook #28

wants to merge 11 commits into from

Conversation

max-rocket-internet
Copy link
Contributor

Note the chart won't run yet as we are still waiting on official docker images: aws/amazon-eks-pod-identity-webhook#5

Related issues:
aws/amazon-eks-pod-identity-webhook#4
aws/containers-roadmap#23

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@nckturner
Copy link
Contributor

Thanks @max-rocket-internet! Working on getting images published for this :D

@@ -0,0 +1,20 @@
apiVersion: admissionregistration.k8s.io/v1beta1
kind: MutatingWebhookConfiguration
metadata:

Choose a reason for hiding this comment

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

Could support be added here to configure annotations? We've pulled this to use it, but want to use it with self-signed certificates from cert-manager instead.

- name: webhook
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
command:

Choose a reason for hiding this comment

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

Could this be updated to also templatize the port being used?

--port={{ .Values.port }}

@amitlt
Copy link

amitlt commented Jan 23, 2020

thanks a lot for this @max-rocket-internet ! If i could ask for one more change, could it be possible to add a namespaceOverride? There are some CD tools (i.e Spinnaker) that only use helm for it's templating capabilities and thus don't change .Release.Namespace, which means it will always be installed in the default namespace. A namespaceOverride will solve this very niche issue :) I'd be happy to add this myself if you want

apiVersion: admissionregistration.k8s.io/v1beta1
kind: MutatingWebhookConfiguration
metadata:
name: {{ include "aws-pod-identity-webhook.fullname" . }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micahhausler I think this must be pod-identity-webhook? I just checked all our clusters and this already exists in each with a webhook called iam-for-pods.amazonaws.com.

What is supposed to happen here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Judging by what the name here I think we are supposed to overwrite the pod-identity-webhook one in the cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK now I realise this chart is not to be installed into an EKS cluster because EKS provides this service by default. This chart would be for none-EKS clusters.

@max-rocket-internet
Copy link
Contributor Author

If anyone wants extra changes, just create a PR after this is merged.

@nckturner I think we merge this and just make users build their own docker images. Then if AWS releases docker images, the chart can be updated?

@jinglejengel
Copy link

Just personally speaking, I don't know if having a chart published that doesn't just "work" out of the box is the best approach. While I'm eagerly awaiting this awesome work, it doesn't introduce an ideal workflow for consuming something in Helm if there's a multi-step approach to have the chart actually apply. Regardless, thanks for the awesome commitment of work on this @max-rocket-internet

stable/aws-pod-identity-webhook/Chart.yaml Outdated Show resolved Hide resolved
stable/aws-pod-identity-webhook/Chart.yaml Outdated Show resolved Hide resolved
stable/aws-pod-identity-webhook/Chart.yaml Outdated Show resolved Hide resolved
stable/aws-pod-identity-webhook/README.md Outdated Show resolved Hide resolved
stable/aws-pod-identity-webhook/README.md Show resolved Hide resolved
stable/aws-pod-identity-webhook/values.yaml Outdated Show resolved Hide resolved

tolerations: []

affinity: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include the following (and add to README values listing):


serviceAccount:
  # serviceAccount.create: Whether to create a service account or not
  create: true
  # serviceAccount.name: The name of the service account to create or use
  name: ""

rbac:
  # rbac.create: `true` if rbac resources should be created
  create: true
  # rbac.pspEnabled: `true` if PodSecurityPolicy resources should be created
  pspEnabled: false

This matches other charts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, for other charts, we've just used a generic podAnnotations value:

https://github.com/aws/eks-charts/blob/master/stable/aws-vpc-cni/values.yaml#L29

annotations:
{{- range $key, $value := .Values.podAnnotations }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}

Maybe do that for this chart as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK serviceAccount thing is done.

podAnnotations thing is done.

RBAC is not optional any more I believe and I don't know how to write a PSP so perhaps this can follow in a later PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, PSP stuff can ceertainly be done in followup

stable/aws-pod-identity-webhook/values.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
tls_secret_name: pod-identity-webhook
annotation_prefix: eks.amazonaws.com
Copy link
Contributor

Choose a reason for hiding this comment

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

As you alluded to, EKS already installs the pod identity webhook by default, so this EKS-specific annotation prefix is probably not appropriate as the default for this chart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change it but I don't know what for?

prometheus.io/port: "443"
prometheus.io/scheme: "https"
prometheus.io/scrape: "true"
spec:
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to templatize the Service's type?

Example:

type: {{ .Values.service.type }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's add this in a later PR

@jaypipes
Copy link
Contributor

jaypipes commented Feb 4, 2020

Thanks very much for this submission @max-rocket-internet! Left a few suggestions and comments to address inline but overall, great start.

@max-rocket-internet
Copy link
Contributor Author

@jaypipes OK done.


image:
repository: 602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pod-identity-webhook
tag: latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tag should be a a git SHA or sem ver. With latest, the same replica set could run different "versions" of latest. How would a user suppose to upgrade the chart? Helm will see no changes in the image tag and running helm upgrade will not start a rolling update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I set it to 0.2.0, the latest release. It's cosmetic anyway as the docker images don't exist yet.

@k
Copy link

k commented Apr 8, 2020

Any update on this?

Comment on lines +9 to +10
prometheus.io/port: "443"
prometheus.io/scheme: "https"

Choose a reason for hiding this comment

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

These annotations are incorrect.
The metrics endpoint for these pods is using http on port 9999

Suggested change
prometheus.io/port: "443"
prometheus.io/scheme: "https"
prometheus.io/port: "9999"
prometheus.io/scheme: "http"

@max-rocket-internet
Copy link
Contributor Author

I will close due to review process being too slow or stalled.

@jinglejengel
Copy link

@max-rocket-internet thow back, but it looks like they have a docker image now: https://hub.docker.com/r/amazon/amazon-eks-pod-identity-webhook

@max-rocket-internet
Copy link
Contributor Author

OK I rebased my branch and opened a new PR (can't reopen this one): #286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants