Skip to content

helm chart: add Kubernetes Auth options #12314

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

Closed
wants to merge 2 commits into from
Closed

helm chart: add Kubernetes Auth options #12314

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42cfd54
helm chart: add Kubernetes Auth options
ufou Apr 25, 2022
c0fe614
Merge branch 'master' into helm-k8s-auths
ufou May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions charts/airbyte/templates/env-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ data:
JOB_MAIN_CONTAINER_MEMORY_LIMIT: {{ ((.Values.jobs.resources | default dict).limits | default dict).memory | default "" | quote }}
JOB_MAIN_CONTAINER_MEMORY_REQUEST: {{ ((.Values.jobs.resources | default dict).requests | default dict).memory | default "" | quote }}
JOBS_DATABASE_MINIMUM_FLYWAY_MIGRATION_VERSION: "0.29.15.001"
KUBERNETES_AUTH_TRYKUBECONFIG: {{ .Values.kubernetesAuth.tryKubeConfig | quote }}
KUBERNETES_AUTH_TRYSERVICEACCOUNT: {{ .Values.kubernetesAuth.tryServiceAccount | quote }}
LOCAL_ROOT: /tmp/airbyte_local
RUN_DATABASE_MIGRATION_ON_STARTUP: "true"
S3_LOG_BUCKET: {{ .Values.logs.s3.bucket | quote }}
Expand Down
13 changes: 13 additions & 0 deletions charts/airbyte/templates/pod-sweeper/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ spec:
{{- end }}
spec:
serviceAccountName: {{ include "airbyte.serviceAccountName" . }}
{{- if and .Values.serviceAccount.create .Values.kubernetesAuth.tryServiceAccount }}
automountServiceAccountToken: true
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this set to true by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to keep it consistent with the other pod that needs service account token access to k8s https://github.com/airbytehq/airbyte/blob/master/charts/airbyte/templates/worker/deployment.yaml#L22

{{- end }}
{{- if .Values.podSweeper.nodeSelector }}
nodeSelector: {{- include "common.tplvalues.render" (dict "value" .Values.podSweeper.nodeSelector "context" $) | nindent 8 }}
{{- end }}
Expand All @@ -40,6 +43,16 @@ spec:
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: KUBERNETES_AUTH_TRYSERVICEACCOUNT
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add the link to the fabric client documentation here to make it clear for users what is consuming these?

valueFrom:
configMapKeyRef:
name: airbyte-env
key: KUBERNETES_AUTH_TRYSERVICEACCOUNT
- name: KUBERNETES_AUTH_TRYKUBECONFIG
valueFrom:
configMapKeyRef:
name: airbyte-env
key: KUBERNETES_AUTH_TRYKUBECONFIG
{{- if .Values.podSweeper.containerSecurityContext }}
securityContext: {{- toYaml .Values.podSweeper.containerSecurityContext | nindent 10 }}
{{- end }}
Expand Down
10 changes: 10 additions & 0 deletions charts/airbyte/templates/worker/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,16 @@ spec:
configMapKeyRef:
name: {{ include "common.names.fullname" . }}-env
key: INTERNAL_API_HOST
- name: KUBERNETES_AUTH_TRYSERVICEACCOUNT
Copy link
Contributor

Choose a reason for hiding this comment

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

what is using these env vars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are most likely using a k8s client lib to interact with k8s, eg: https://github.com/fabric8io/kubernetes-client/blob/master/README.md#configuring-the-client

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks for the link.

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here on adding the link to make it clear what is consuming this

valueFrom:
configMapKeyRef:
name: airbyte-env
key: KUBERNETES_AUTH_TRYSERVICEACCOUNT
- name: KUBERNETES_AUTH_TRYKUBECONFIG
valueFrom:
configMapKeyRef:
name: airbyte-env
key: KUBERNETES_AUTH_TRYKUBECONFIG
{{- if .Values.worker.extraEnv }}
{{ .Values.worker.extraEnv | toYaml | nindent 8 }}
{{- end }}
Expand Down
9 changes: 9 additions & 0 deletions charts/airbyte/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ serviceAccount:
annotations: {}
name: airbyte-admin

## Kubernetes Authentication
## Authentication method used by airbyte pods requiring cluster access, eg. worker/pod-sweeper
## @param kubernetesAuth.tryKubeConfig if true, will try to use kube config mounted inside the pod (default: true)
## @param kubernetesAuth.tryServiceAccount if true, will try to use serviceAccount credentials from serviceAccount.name (default: false)
##
kubernetesAuth:
tryKubeConfig: true
Copy link
Contributor

Choose a reason for hiding this comment

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

what does tryKubeConfig do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it tells you in the param section a couple of lines up:

## @param kubernetesAuth.tryKubeConfig if true, will try to use kube config mounted inside the pod (default: true)

I defaulted this to true as this is what seemed to be the way airbyte wanted to auth against k8s, given the errors in the logs

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fabric link. I understand this now. What do you think about removing the try kube config option and switching on the service account? I don't think a user would want to want both to be set to true/false so this seems like a clearer configuration option to me. We can have this tryServiceAccount variable be the switch. What do you think?

Can we rename tryServiceAccount to useServiceAccount? That seems clearer to me.

Copy link
Contributor Author

@ufou ufou May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fabric link. I understand this now. What do you think about removing the try kube config option and switching on the service account? I don't think a user would want to want both to be set to true/false so this seems like a clearer configuration option to me. We can have this tryServiceAccount variable be the switch. What do you think?

Yeah, sounds good - I'll make the changes - I could not think of a use-case for having kube config mounted inside a k8s pod - but did not want to deviate too much, but I agree - it doesn't make sense - I also think that useServiceAccount should default to true as, again, I can't see many scenarios where this is not the desired behaviour?

Can we rename tryServiceAccount to useServiceAccount? That seems clearer to me.

Yep, will do

tryServiceAccount: false

## @param version Sets the AIRBYTE_VERSION environment variable. Defaults to Chart.AppVersion.
## If changing the image tags below, you should probably also update this.
version: ""
Expand Down