helm chart: add Kubernetes Auth options #12314

ufou · 2022-04-25T16:57:09Z

What

Helm chart deployed pod-sweeper and worker pods both require k8s authentication to query the local cluster - this more often requires service account + tokens rather than mounting the kube config fixes #11323

Adds kubernetes authentication options:
- use kube config inside pod
- use service account/token inside pod

How

this PR offers the helm chart user flexibility to use either kube config or more probably service account + token

Add 2 new ENV vars:

KUBERNETES_AUTH_TRYKUBECONFIG
KUBERNETES_AUTH_TRYSERVICEACCOUNT
If either is set to true then the pod will try to use the authentication method against kubernetes, afaik this is only worker and pod_sweeper

🚨 User Impact 🚨

Should be no breaking changes as the current behaviour is left as default

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

alafanechere · 2022-04-26T22:22:55Z

Hi @ufou, thank you for opening this PR. I'm going to review it soon, I also asked for @davinchia's eye 👀 😄

davinchia · 2022-04-27T10:49:14Z

charts/airbyte/templates/pod-sweeper/deployment.yaml

@@ -22,6 +22,9 @@ spec:
      {{- end }}
    spec:
      serviceAccountName: {{ include "airbyte.serviceAccountName" . }}
+      {{- if and .Values.serviceAccount.create .Values.kubernetesAuth.tryServiceAccount }}
+      automountServiceAccountToken: true


isn't this set to true by default?

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

davinchia · 2022-04-27T10:49:34Z

charts/airbyte/values.yaml

+## @param kubernetesAuth.tryServiceAccount if true, will try to use serviceAccount credentials from serviceAccount.name (default: false)
+##
+kubernetesAuth:
+  tryKubeConfig: true


what does tryKubeConfig do?

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

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.

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

davinchia · 2022-04-27T10:49:46Z

charts/airbyte/templates/worker/deployment.yaml

@@ -239,6 +239,16 @@ spec:
            configMapKeyRef:
              name: {{ include "common.names.fullname" . }}-env
              key: INTERNAL_API_HOST
+        - name: KUBERNETES_AUTH_TRYSERVICEACCOUNT


what is using these env vars?

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

Got it. Thanks for the link.

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

davinchia · 2022-04-27T10:50:44Z

@ufou I took a quick look. I don't understand how this fixes the stated issue. Can you explain more? Have you tested this on the infrastructure you want to support?

ufou · 2022-04-27T12:32:21Z

@ufou I took a quick look. I don't understand how this fixes the stated issue. Can you explain more? Have you tested this on the infrastructure you want to support?

Yeah, we've been running it like this for ~1 month - by default I think the client lib airbyte uses to interact with k8s, expects kube config to be available inside the pod, but for k8s on bare metal (and maybe cloud providers too?) this is not desirable, so instead the option to use service account token is available, the syntax I've used matches that of this client lib, which may or may not be the one airbyte uses: https://github.com/fabric8io/kubernetes-client/blob/master/README.md#configuring-the-client

CLAassistant · 2022-05-05T13:09:44Z

All committers have signed the CLA.

davinchia · 2022-05-06T08:47:50Z

charts/airbyte/templates/pod-sweeper/deployment.yaml

@@ -40,6 +43,16 @@ spec:
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace
+        - name: KUBERNETES_AUTH_TRYSERVICEACCOUNT


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

davinchia · 2022-05-06T08:52:31Z

@ufou thanks for the explanation! I didn't realise you were running this on bare metal Kubernetes. I left some comments around simplifying the configuration. Lmk if this makes sense.

ufou · 2022-05-06T12:43:43Z

After merging upstream into my branch and testing again - these changes no longer seem to be required - I'm not sure if the underlying k8s auth mechanism has been updated or not but I no longer need to explicitly declare to use the service account token - and since the service account token is mounted by default this whole PR is moot - maybe the only thing of contention is whether the automountServiceAccountToken should be explicitly set to true in the pod sweeper deployment or not as it is in the worker deployment, but this works, so closing - sorry for the noise!

alafanechere · 2022-05-06T13:14:24Z

@ufou I recently merge this helm chart PR, the auth was maybe required for creating PVC?

davinchia · 2022-05-06T13:46:29Z

@ufou you are good! I'm not sure what changes would have solved this. Cannot think of any off the top of my head. Thank you for the contribution and sorry it took me a while to get back to you. Feel free to reopen if it's still causing you grief.

helm chart: add Kubernetes Auth options

42cfd54

github-actions bot added area/platform issues related to the platform kubernetes labels Apr 25, 2022

octavia-squidington-iii added the community label Apr 25, 2022

ufou mentioned this pull request Apr 25, 2022

Helm chart: add Kubernetes Auth options, fix k8s job tolerations #12236

Closed

38 tasks

alafanechere self-assigned this Apr 26, 2022

alafanechere requested a review from davinchia April 26, 2022 22:21

davinchia reviewed Apr 27, 2022

View reviewed changes

davinchia reviewed May 6, 2022

View reviewed changes

Merge branch 'master' into helm-k8s-auths

c0fe614

ufou closed this May 6, 2022

ufou deleted the helm-k8s-auths branch May 6, 2022 12:43

davinchia mentioned this pull request Jul 5, 2022

helm chart: fix /root/.kube/config (No such file or directory) #11475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm chart: add Kubernetes Auth options #12314

helm chart: add Kubernetes Auth options #12314

ufou commented Apr 25, 2022 •

edited

Loading

alafanechere commented Apr 26, 2022

davinchia Apr 27, 2022

ufou Apr 27, 2022

davinchia Apr 27, 2022

ufou Apr 27, 2022

davinchia May 6, 2022

ufou May 6, 2022 •

edited

Loading

davinchia Apr 27, 2022

ufou Apr 27, 2022

davinchia May 6, 2022

davinchia May 6, 2022

davinchia commented Apr 27, 2022

ufou commented Apr 27, 2022 •

edited

Loading

CLAassistant commented May 5, 2022 •

edited

Loading

davinchia May 6, 2022

davinchia commented May 6, 2022

ufou commented May 6, 2022

alafanechere commented May 6, 2022

davinchia commented May 6, 2022

helm chart: add Kubernetes Auth options #12314

helm chart: add Kubernetes Auth options #12314

Conversation

ufou commented Apr 25, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

alafanechere commented Apr 26, 2022

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

ufou May 6, 2022 • edited Loading

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

davinchia commented Apr 27, 2022

ufou commented Apr 27, 2022 • edited Loading

CLAassistant commented May 5, 2022 • edited Loading

Choose a reason for hiding this comment

davinchia commented May 6, 2022

ufou commented May 6, 2022

alafanechere commented May 6, 2022

davinchia commented May 6, 2022

ufou commented Apr 25, 2022 •

edited

Loading

ufou May 6, 2022 •

edited

Loading

ufou commented Apr 27, 2022 •

edited

Loading

CLAassistant commented May 5, 2022 •

edited

Loading