Fix git-sync rotated credentials support in Helm chart#63276
Fix git-sync rotated credentials support in Helm chart#63276sidshas03 wants to merge 4 commits intoapache:mainfrom
Conversation
|
@sidshas03 Airflow currently uses version 4.4.2 of gitsync, according to kubernetes/git-sync#976 the feature required is in 4.6.0? |
Good catch, thanks for pointing this out. You’re right, in git-sync v4.4.2, --password-file exists but it is not re-read on every sync. The auto re-read for rotated credentials came in v4.6.0 via kubernetes/git-sync#976. So to make this fix effective by default, we should run git-sync >= v4.6.0. |
There was a problem hiding this comment.
Overall - good idea, but we cannot break backward compatibility for Git-Sync (we still support version 3 even). I placed some idea how to do it in the comments.
Also, Git-Sync container is used in pod-template-file too (used by KubernetesExecutor). Could you add a change there too?
| {{- if or .Values.dags.gitSync.sshKeySecret .Values.dags.gitSync.sshKey}} | ||
| {{- include "git_sync_ssh_key_volume" . | indent 8 }} | ||
| {{- end }} | ||
| {{- if .Values.dags.gitSync.credentialsSecret }} |
There was a problem hiding this comment.
| {{- if .Values.dags.gitSync.credentialsSecret }} | |
| {{- if and $localOrDagProcessorDisabled .Values.dags.gitSync.enabled .Values.dags.gitSync.credentialsSecret }} |
following git sync container falgs for scheduler
| {{- if or .Values.dags.gitSync.sshKeySecret .Values.dags.gitSync.sshKey}} | ||
| {{- include "git_sync_ssh_key_volume" . | nindent 8 }} | ||
| {{- end }} | ||
| {{- if .Values.dags.gitSync.credentialsSecret }} |
There was a problem hiding this comment.
| {{- if .Values.dags.gitSync.credentialsSecret }} | |
| {{- if and .Values.dags.gitSync.enabled (not .Values.dags.persistence.enabled) }} |
following git sync container falgs for triggerer
| {{- if or .Values.dags.gitSync.sshKeySecret .Values.dags.gitSync.sshKey}} | ||
| {{- include "git_sync_ssh_key_volume" . | indent 8 }} | ||
| {{- end }} | ||
| {{- if .Values.dags.gitSync.credentialsSecret }} |
There was a problem hiding this comment.
| {{- if .Values.dags.gitSync.credentialsSecret }} | |
| {{- if and .Values.dags.gitSync.enabled (not .Values.dags.persistence.enabled) }} |
following git sync container falgs for workers
chart/templates/_helpers.yaml
Outdated
| defaultMode: 288 | ||
| {{- end }} | ||
|
|
||
| {{/* Git credentials volume */}} |
There was a problem hiding this comment.
| {{/* Git credentials volume */}} | |
| {{/* Git credentials volume */}} |
| - name: GIT_SYNC_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.dags.gitSync.credentialsSecret | quote }} | ||
| key: GIT_SYNC_PASSWORD | ||
| - name: GITSYNC_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.dags.gitSync.credentialsSecret | quote }} | ||
| key: GITSYNC_PASSWORD | ||
| - name: GIT_SYNC_PASSWORD_FILE | ||
| value: "/etc/git-secret/credentials/GIT_SYNC_PASSWORD" | ||
| - name: GITSYNC_PASSWORD_FILE | ||
| value: "/etc/git-secret/credentials/GITSYNC_PASSWORD" |
There was a problem hiding this comment.
This change breaks backward-compatibility. You could prevent that by making if condition on these envs where, by default, old behaviour will be in place.
chart/values.schema.json
Outdated
| "description": "The gitSync image tag.", | ||
| "type": "string", | ||
| "default": "v4.4.2" | ||
| "default": "v4.6.0" |
There was a problem hiding this comment.
I believe this should be in a separate PR as it is unrelated change for this one.
chart/values.schema.json
Outdated
| "credentialsSecret": { | ||
| "description": "Name of a Secret containing the repo `GIT_SYNC_USERNAME` and `GIT_SYNC_PASSWORD`.", | ||
| "description": "Name of a Secret containing `GIT_SYNC_USERNAME`, `GITSYNC_USERNAME`, `GIT_SYNC_PASSWORD`, and `GITSYNC_PASSWORD` keys. The password keys are mounted as files and used via `*_PASSWORD_FILE` env vars so rotated credentials can be re-read.", |
There was a problem hiding this comment.
You probably need to create a new field dedicated for the file as this breaks backward-compatibility.
|
Thanks @Miretpl for the detailed review. I got your point on the backward compatibility part, and I will rework this accordingly. I will keep the existing |
|
Note: We are currently planning and discussing a revamt to a chart v2 where we want to re-model some things and also drop some legacy. Would it be OK to "park" this PR, drop all git sync legacy support and use minimum 4.6.0 from then on and have this as a new feature? w/o all the backwards compatatibility complexity? |
|
Thanks for the guidance @jscheffl. I’m parking this PR as discussed. I’m interested to work on the chart v2 follow-up (git-sync >= 4.6.0) as well, please tag me when that issue/epic is created. |
|
@jscheffl Do you have an estimation on the timeframe for the new Helm chart? |
|
@electrical We had initial discussions, are currently collecting topics and will drop a DISCUSS to devlist once we have a (draft) plan. |
Fixes #63253
Hi team, this PR is to handle token rotation properly when
dags.gitSync.credentialsSecretis used.Problem
Right now password/token is passed as env var (
GIT_SYNC_PASSWORD/GITSYNC_PASSWORD).When secret is rotated (for example ESO GitHub App token), running git-sync container does not pick the new value until restart.
What I changed
git-sync-credentials) in:GIT_SYNC_PASSWORD_FILE=/etc/git-secret/credentials/GIT_SYNC_PASSWORDGITSYNC_PASSWORD_FILE=/etc/git-secret/credentials/GITSYNC_PASSWORDGIT_SYNC_USERNAME/GITSYNC_USERNAME).Why this fix
git-sync can re-read password from file path on sync loop, so rotated token is picked without forcing pod restart.
Testing
Ran helm unit tests for the updated git-sync paths and related suites (
HELM_TEST_KUBERNETES_VERSION=1.32.8), and selected tests are passing.Ran focused Helm unit tests for the new credential-file behavior:
test_git_sync_scheduler.py::test_should_set_username_and_password_file_env_variablestest_git_sync_worker.py::test_should_set_password_file_env_variables_when_credentials_secret_is_configuredtest_git_sync_triggerer.py::test_should_set_password_file_env_variables_when_credentials_secret_is_configuredtest_dag_processor.py::test_should_set_password_file_env_variables_when_credentials_secret_is_configuredResult: 4 passed
Ran broader git-sync regression subset:
helm-tests/tests/helm_tests/other/test_git_sync_scheduler.pyhelm-tests/tests/helm_tests/other/test_git_sync_worker.pyhelm-tests/tests/helm_tests/other/test_git_sync_triggerer.pytest_validate_if_ssh_params_are_added_with_git_ssh_keytest_should_set_password_file_env_variables_when_credentials_secret_is_configuredResult: 42 passed
Sanity checks:
jq empty chart/values.schema.json(schema JSON valid)compileall) passedEnvironment note: tests were run with
HELM_TEST_KUBERNETES_VERSION=1.32.8.