Helm chart - move sensitive data to secret, refactor hard coded names #11020

georgekaz · 2022-03-10T11:18:05Z

What

removes sensitive data from being deployed into the ConfigMap via the helm chart and instead puts them in a secret
adds the ability to inject the gcp json creds file contents via helm values and the gcs-log-creds secret. chart README.md is updated.
changes hard-coded references of ConfigMap to use the dynamic release name (this would currently be broken if the release was called anything other than airbyte)
standardises on the use of {{ include "common.names.fullname" } where previously there was a mix of that and {{ include "airbyte.fullname" . }}
fixes a bug in the ingress (ref: Helm Lint error helm/helm#10149)

How

Via updates to the helm chart

🚨 User Impact 🚨

I don't think there are any breaking changes

Pre-merge Checklist

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 by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory 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

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

❯ helm lint .                                                                                                                     
==> Linting .
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

Integration

Helm template works but is too long to post here

Acceptance

Put your acceptance tests output here.

CLAassistant · 2022-03-10T11:18:13Z

All committers have signed the CLA.

georgekaz · 2022-03-10T11:33:29Z

I'm unable to sign this with the CLAassitant, process doesn't seem to be working

georgekaz · 2022-03-10T11:40:52Z

Eventually the CLA worked. There is an open issue here that maybe related: cla-assistant/cla-assistant#829

alafanechere · 2022-03-10T16:43:29Z

Hi @georgekaz thank you for this needed contribution 😄 I'll go for a review asap.
But first question: do you confirm you've been able to successfully deploy a new airbyte deployment from your branch?

georgekaz · 2022-03-10T21:51:58Z

Hi @alafanechere .Yes, I can confirm I've deployed it and I'm using the new feature of passing the json creds as a value too. It's all working fine.

alafanechere

Hi @georgekaz , I went for a first review. Thank you for this awesome work. I made a suggestion that could reduce a bit the clunkiness of the deployment.yaml files.

charts/airbyte/templates/server/deployment.yaml

charts/airbyte/templates/secret.yaml

georgekaz · 2022-03-22T18:01:15Z

@alafanechere I've made the changes we discussed on my last commit and rebased. It helm lints and templates ok and I'm going to test a deployment into my own cluster shortly. Let me know your thoughts on the changes though please.

This chart now requires helm >= 3.7.0 due to the use of the .Subcharts function

{{ template "postgresql.secretName" .Subcharts.postgresql }}

georgekaz · 2022-03-23T11:18:52Z

I can confirm that I've now deployed this chart in a cluster and it's working fine.

As an aside, I have some other thoughts about the helm chart that I thought I'd share. Although I understand the reason for trying to control the order of component deployment, I'm not keen on the helm hook annotations i.e.

    helm.sh/hook: pre-install,pre-upgrade

When these are used, the components they deploy are considered outside the scope of normal helm management, see: https://helm.sh/docs/topics/charts_hooks/#hook-resources-are-not-managed-with-corresponding-releases

We use argocd and this means that if these manifests change, they may not be correctly kept in sync.

What are your thoughts on it? The deployment can work without these annotations.

charts/airbyte/templates/gcs-log-creds-secret.yaml

alafanechere · 2022-03-23T13:49:07Z

Thank you for the refactoring. I think it significantly increased the chart readability!

The deployment can work without these annotations.
We defined these hooks to ensure some resources are deployed before others. e.g., the bootloader needs the Postgres DB to be up to run correctly. The server needs to be started after the bootloader runs, etc. ...
Does Helm have a deployment ordering feature that could bypass hooks? I'm not a helm ninja, so I'm probably missing a best practice.

davinchia · 2022-03-23T18:07:49Z

charts/airbyte/templates/_helpers.tpl

 {{- else }}
    {{- if .Values.externalDatabase.existingSecret -}}
        {{- printf "%s" .Values.externalDatabase.existingSecret -}}
    {{- else -}}
-        {{ printf "%s-%s" .Release.Name "externaldb" }}
+        {{ printf "%s-%s" (include "common.names.fullname" .) "secrets" }}


I'm not too familiar with this template helper - how is this set?

This is being included as a chart dependency https://github.com/airbytehq/airbyte/blob/master/charts/airbyte/Chart.yaml#L27-L30

It's not something I initially added but as it was there I've standardised it's use across this chart when it comes to resource names. You can see its use in other charts' helpers such as bitnami postgres https://github.com/bitnami/charts/blob/master/bitnami/postgresql/templates/_helpers.tpl#L11

Got it. Makes sense. Thanks!

davinchia · 2022-03-23T18:19:34Z

@georgekaz thanks for the contribution!

IIRC the pre-hooks are needed since 1) the bootloader needs to run first 2) this is a one time run as a Pod and Helm does not recognise the 'completed' status on the pod.

Since this isn't a deployment, I think it's fine if it's not governed by Helm. Does that make sense?

georgekaz · 2022-03-24T07:49:28Z

Since this isn't a deployment, I think it's fine if it's not governed by Helm.

I think that's fine, but because the bootloader has a dependency on postgres (if enabled), the service account, the configmap and the secret, those all now also have these hooks, so are also not governed by Helm.

We can leave it for now, I wouldn't want to delay this PR because of it, but maybe we can find a better way of managing the dependency on the bootloader pod in future.

davinchia · 2022-03-24T09:32:09Z

@alafanechere I'll let you merge this in as appropriate.

alafanechere

Thank you again @georgekaz for this improvement + your patience and availability to make it to the finish line! Feel free to open other PR on this chart, especially on the hook topic if you have some suggestions.

georgekaz · 2022-03-24T17:27:28Z

Great, thanks for merging. I'm happy to help out and will add what I can to the project

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

octavia-squidington-iii added the community label Mar 10, 2022

georgekaz marked this pull request as ready for review March 10, 2022 11:31

alafanechere self-assigned this Mar 10, 2022

alafanechere requested changes Mar 18, 2022

View reviewed changes

charts/airbyte/templates/server/deployment.yaml Outdated Show resolved Hide resolved

charts/airbyte/templates/secret.yaml Outdated Show resolved Hide resolved

alafanechere requested a review from davinchia March 18, 2022 09:46

georgekaz added 6 commits March 22, 2022 17:39

move sensitive data to secret. Refactor hard coded names

010cd95

remove separator - ref helm/helm/issues/10149

356bb75

remove new hardcoded names

fcda591

add some quotes

b844a61

bump chart version

13cb786

refactor DB PWD secret definitions

0610b91

georgekaz force-pushed the chart-secrets-refactor branch from a134fd1 to 0610b91 Compare March 22, 2022 17:39

alafanechere reviewed Mar 23, 2022

View reviewed changes

charts/airbyte/templates/gcs-log-creds-secret.yaml Show resolved Hide resolved

davinchia reviewed Mar 23, 2022

View reviewed changes

davinchia approved these changes Mar 24, 2022

View reviewed changes

alafanechere approved these changes Mar 24, 2022

View reviewed changes

alafanechere merged commit 382ce65 into airbytehq:master Mar 24, 2022

octavia-squidington-iii mentioned this pull request Mar 24, 2022

Bump Airbyte version from 0.35.59-alpha to 0.35.60-alpha #11402

Merged

georgekaz deleted the chart-secrets-refactor branch March 24, 2022 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm chart - move sensitive data to secret, refactor hard coded names #11020

Helm chart - move sensitive data to secret, refactor hard coded names #11020

georgekaz commented Mar 10, 2022 •

edited

Loading

CLAassistant commented Mar 10, 2022 •

edited

Loading

georgekaz commented Mar 10, 2022 •

edited

Loading

georgekaz commented Mar 10, 2022

alafanechere commented Mar 10, 2022 •

edited

Loading

georgekaz commented Mar 10, 2022

alafanechere left a comment

georgekaz commented Mar 22, 2022 •

edited

Loading

georgekaz commented Mar 23, 2022

alafanechere commented Mar 23, 2022

davinchia Mar 23, 2022

georgekaz Mar 24, 2022

davinchia Mar 24, 2022

davinchia commented Mar 23, 2022

georgekaz commented Mar 24, 2022

davinchia commented Mar 24, 2022

alafanechere left a comment

georgekaz commented Mar 24, 2022

Helm chart - move sensitive data to secret, refactor hard coded names #11020

Helm chart - move sensitive data to secret, refactor hard coded names #11020

Conversation

georgekaz commented Mar 10, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

CLAassistant commented Mar 10, 2022 • edited Loading

georgekaz commented Mar 10, 2022 • edited Loading

georgekaz commented Mar 10, 2022

alafanechere commented Mar 10, 2022 • edited Loading

georgekaz commented Mar 10, 2022

alafanechere left a comment

Choose a reason for hiding this comment

georgekaz commented Mar 22, 2022 • edited Loading

georgekaz commented Mar 23, 2022

alafanechere commented Mar 23, 2022

davinchia Mar 23, 2022

Choose a reason for hiding this comment

georgekaz Mar 24, 2022

Choose a reason for hiding this comment

davinchia Mar 24, 2022

Choose a reason for hiding this comment

davinchia commented Mar 23, 2022

georgekaz commented Mar 24, 2022

davinchia commented Mar 24, 2022

alafanechere left a comment

Choose a reason for hiding this comment

georgekaz commented Mar 24, 2022

georgekaz commented Mar 10, 2022 •

edited

Loading

CLAassistant commented Mar 10, 2022 •

edited

Loading

georgekaz commented Mar 10, 2022 •

edited

Loading

alafanechere commented Mar 10, 2022 •

edited

Loading

georgekaz commented Mar 22, 2022 •

edited

Loading