-
Notifications
You must be signed in to change notification settings - Fork 971
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
Add App Mesh controller and injector charts #1
Conversation
Lint with Helm CLI and validate with kubeval Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Enable Prometheus scraping Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: stefanprodan <[email protected]>
@nckturner this is ready for review |
Signed-off-by: stefanprodan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! This is going to be a huge improvement for installation.
```sh | ||
helm upgrade -i appmesh-inject eks/appmesh-inject \ | ||
--namespace appmesh-system \ | ||
--set mesh.create=true \ | ||
--set mesh.name=global | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below this block, maybe we can have a note about upgrading the existing installation before the helm chart existed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
apiVersion: apiextensions.k8s.io/v1beta1 | ||
kind: CustomResourceDefinition | ||
metadata: | ||
name: meshes.appmesh.k8s.aws |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if there was a way to not have to define these both in the controller package and the helm chart, but I feel like we should keep them in both places for now, for non helm users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CRDs in the controller repo should be for development purpose only. The App Mesh users should use the CRDs from this repo as it doesn't require Helm to install them.
{{- if .Chart.AppVersion }} | ||
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} | ||
{{- end }} | ||
app.kubernetes.io/managed-by: {{ .Release.Service }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the helm docs:
Release.Service: The name of the releasing service (always Tiller).
Is it different in helm V3? Should we only set this if .Release.Service
itself is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the Helm v3 template when creating these charts, the .Release.Service
is handled by v3 like this app.kubernetes.io/managed-by: Helm
.
{{- if .Values.serviceAccount.create -}} | ||
{{ default (include "appmesh-controller.fullname" .) .Values.serviceAccount.name }} | ||
{{- else -}} | ||
{{ default "default" .Values.serviceAccount.name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that if .Values.serviceAccount.create
is specified, then we fallback on the appmesh-controller.fullname
, but otherwise we fallback on the default service account? So .Values.serviceAccount.name
can either refer to a service account that helm creates, or an existing service account, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this allows a user to specify an existing service account and combined with rbac.create=false
you can have full control over what the App Mesh controllers can do.
Install the App Mesh CRD controller: | ||
|
||
```sh | ||
helm upgrade -i appmesh-controller eks/appmesh-controller --namespace appmesh-system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is namespace expected to always be passed? Can we add a default namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default namespace is default
same as with kubectl. Helm v3 doesn't support namespace definitions and that's a good thing :) it uses the kubectl context for that.
Signed-off-by: stefanprodan <[email protected]>
Signed-off-by: Philip Dubois <[email protected]>
This PR adds the App Mesh controller and injector charts and CircleCI config (lint and validate charts).
Tested on EKS v1.13 with Helm v2 and v3:
Fix: #3
Fix: #2
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.