diff --git a/chart/newsfragments/34331.bugfix.rst b/chart/newsfragments/34331.bugfix.rst new file mode 100644 index 0000000000000..0485fe3c9c1b8 --- /dev/null +++ b/chart/newsfragments/34331.bugfix.rst @@ -0,0 +1 @@ +Fix missing ``git-sync-ssh-key`` volume in ``scheduler`` pod while using ``dags.persistence.enabled=true`` with ``dags.gitSync.sshKeySecret``. diff --git a/chart/templates/dag-processor/dag-processor-deployment.yaml b/chart/templates/dag-processor/dag-processor-deployment.yaml index 61cd3f56bcb53..430becc6b2d03 100644 --- a/chart/templates/dag-processor/dag-processor-deployment.yaml +++ b/chart/templates/dag-processor/dag-processor-deployment.yaml @@ -239,10 +239,10 @@ spec: {{- else if .Values.dags.gitSync.enabled }} - name: dags emptyDir: {{- toYaml (default (dict) .Values.dags.gitSync.emptyDirConfig) | nindent 12 }} - {{- end }} - {{- if and .Values.dags.gitSync.enabled .Values.dags.gitSync.sshKeySecret }} + {{- if .Values.dags.gitSync.sshKeySecret }} {{- include "git_sync_ssh_key_volume" . | indent 8 }} {{- end }} + {{- end }} {{- if .Values.volumes }} {{- toYaml .Values.volumes | nindent 8 }} {{- end }} diff --git a/chart/templates/scheduler/scheduler-deployment.yaml b/chart/templates/scheduler/scheduler-deployment.yaml index 8e599b4a286f2..283d4171e7cf5 100644 --- a/chart/templates/scheduler/scheduler-deployment.yaml +++ b/chart/templates/scheduler/scheduler-deployment.yaml @@ -166,6 +166,7 @@ spec: {{- end }} {{- end }} {{- if and $localOrDagProcessorDisabled .Values.dags.gitSync.enabled }} + # unlike other pods, git-sync in scheduler is enabled even if dag.persistence is enabled {{- include "git_sync_container" (dict "Values" .Values "is_init" "true" "Template" .Template) | nindent 8 }} {{- end }} {{- if .Values.scheduler.extraInitContainers }} @@ -244,6 +245,7 @@ spec: {{- tpl (toYaml .Values.scheduler.extraVolumeMounts) . | nindent 12 }} {{- end }} {{- if and $localOrDagProcessorDisabled .Values.dags.gitSync.enabled }} + # unlike other pods, git-sync in scheduler is enabled even if dag.persistence is enabled {{- include "git_sync_container" . | indent 8 }} {{- end }} {{- if .Values.scheduler.logGroomerSidecar.enabled }} @@ -299,11 +301,12 @@ spec: {{- else if .Values.dags.gitSync.enabled }} - name: dags emptyDir: {{- toYaml (default (dict) .Values.dags.gitSync.emptyDirConfig) | nindent 12 }} + {{- end }} {{- if .Values.dags.gitSync.sshKeySecret }} + # unlike other pods, git-sync in scheduler is enabled even if dag.persistence is enabled {{- include "git_sync_ssh_key_volume" . | indent 8 }} {{- end }} {{- end }} - {{- end }} {{- if .Values.volumes }} {{- toYaml .Values.volumes | nindent 8 }} {{- end }} diff --git a/helm_tests/other/test_git_sync_dag_processor.py b/helm_tests/other/test_git_sync_dag_processor.py new file mode 100644 index 0000000000000..cbf95d4d8e98f --- /dev/null +++ b/helm_tests/other/test_git_sync_dag_processor.py @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import jmespath + +from tests.charts.helm_template_generator import render_chart + + +class TestGitSyncDagProcessor: + """Test git sync dag processor.""" + + def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): + docs = render_chart( + values={ + "dagProcessor": {"enabled": True}, + "dags": { + "gitSync": { + "enabled": True, + "containerName": "git-sync-test", + "sshKeySecret": "ssh-secret", + "knownHosts": None, + "branch": "test-branch", + }, + "persistence": {"enabled": True}, + }, + }, + show_only=["templates/dag-processor/dag-processor-deployment.yaml"], + ) + assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.containers[].volumeMounts[].name", + docs[0], + ) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.initContainers[].volumeMounts[].name", + docs[0], + ) diff --git a/helm_tests/other/test_git_sync_scheduler.py b/helm_tests/other/test_git_sync_scheduler.py index 91039954f8d6e..41e26246a2d42 100644 --- a/helm_tests/other/test_git_sync_scheduler.py +++ b/helm_tests/other/test_git_sync_scheduler.py @@ -17,6 +17,7 @@ from __future__ import annotations import jmespath +import pytest from tests.charts.helm_template_generator import render_chart @@ -132,7 +133,9 @@ def test_validate_if_ssh_params_are_added(self): "secret": {"secretName": "ssh-secret", "defaultMode": 288}, } in jmespath.search("spec.template.spec.volumes", docs[0]) - def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): + @pytest.mark.parametrize("persistence", [True, False]) + def test_validate_sshkeysecret_added_regardless_of_persistence_value(self, persistence): + # Unlike docs = render_chart( values={ "dags": { @@ -143,12 +146,20 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): "knownHosts": None, "branch": "test-branch", }, - "persistence": {"enabled": True}, + "persistence": {"enabled": persistence}, } }, show_only=["templates/scheduler/scheduler-deployment.yaml"], ) - assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) + assert "git-sync-ssh-key" in jmespath.search("spec.template.spec.volumes[].name", docs[0]) + assert "git-sync-ssh-key" in jmespath.search( + "spec.template.spec.containers[].volumeMounts[].name", + docs[0], + ) + assert "git-sync-ssh-key" in jmespath.search( + "spec.template.spec.initContainers[].volumeMounts[].name", + docs[0], + ) def test_should_set_username_and_pass_env_variables(self): docs = render_chart( diff --git a/helm_tests/other/test_git_sync_triggerer.py b/helm_tests/other/test_git_sync_triggerer.py index 96f7092666ebe..a46d1bfb8b57a 100644 --- a/helm_tests/other/test_git_sync_triggerer.py +++ b/helm_tests/other/test_git_sync_triggerer.py @@ -41,3 +41,11 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): show_only=["templates/triggerer/triggerer-deployment.yaml"], ) assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.containers[].volumeMounts[].name", + docs[0], + ) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.initContainers[].volumeMounts[].name", + docs[0], + ) diff --git a/helm_tests/other/test_git_sync_webserver.py b/helm_tests/other/test_git_sync_webserver.py index 01bf49a65b83b..4c810c4ad6ed0 100644 --- a/helm_tests/other/test_git_sync_webserver.py +++ b/helm_tests/other/test_git_sync_webserver.py @@ -172,3 +172,11 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): show_only=["templates/webserver/webserver-deployment.yaml"], ) assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.containers[].volumeMounts[].name", + docs[0], + ) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.initContainers[].volumeMounts[].name", + docs[0], + ) diff --git a/helm_tests/other/test_git_sync_worker.py b/helm_tests/other/test_git_sync_worker.py index 31589a78f3ba7..fce87190324a4 100644 --- a/helm_tests/other/test_git_sync_worker.py +++ b/helm_tests/other/test_git_sync_worker.py @@ -132,3 +132,11 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): ) assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.containers[].volumeMounts[].name", + docs[0], + ) + assert "git-sync-ssh-key" not in jmespath.search( + "spec.template.spec.initContainers[].volumeMounts[].name", + docs[0], + )