From 09d434047e713ec8e18e09eec877ed5b15f33fae Mon Sep 17 00:00:00 2001 From: Ephraim Anierobi Date: Mon, 11 Apr 2022 17:03:59 +0100 Subject: [PATCH 1/5] Do not declare a volume for sshKeySecret if dag persistence is enabled In scheduler and triggerer components, git-sync-ssh-key volume was created even when persistence is enabled. This PR fixes that and added tests in other components to avoid regression --- .../scheduler/scheduler-deployment.yaml | 1 - .../triggerer/triggerer-deployment.yaml | 2 +- tests/charts/test_git_sync_scheduler.py | 18 ++++++++ tests/charts/test_git_sync_triggerer.py | 42 +++++++++++++++++++ tests/charts/test_git_sync_webserver.py | 18 ++++++++ tests/charts/test_git_sync_worker.py | 19 +++++++++ 6 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 tests/charts/test_git_sync_triggerer.py diff --git a/chart/templates/scheduler/scheduler-deployment.yaml b/chart/templates/scheduler/scheduler-deployment.yaml index 8aaa24e2036e9..00c1b042d548a 100644 --- a/chart/templates/scheduler/scheduler-deployment.yaml +++ b/chart/templates/scheduler/scheduler-deployment.yaml @@ -253,7 +253,6 @@ spec: {{- else if .Values.dags.gitSync.enabled }} - name: dags emptyDir: {} - {{- end }} {{- if and .Values.dags.gitSync.enabled .Values.dags.gitSync.sshKeySecret }} {{- include "git_sync_ssh_key_volume" . | indent 8 }} {{- end }} diff --git a/chart/templates/triggerer/triggerer-deployment.yaml b/chart/templates/triggerer/triggerer-deployment.yaml index 7842e76de885f..4739d7620477a 100644 --- a/chart/templates/triggerer/triggerer-deployment.yaml +++ b/chart/templates/triggerer/triggerer-deployment.yaml @@ -199,10 +199,10 @@ spec: {{- else if .Values.dags.gitSync.enabled }} - name: dags emptyDir: {} - {{- end }} {{- if and .Values.dags.gitSync.enabled .Values.dags.gitSync.sshKeySecret }} {{- include "git_sync_ssh_key_volume" . | indent 8 }} {{- end }} + {{- end }} {{- if .Values.triggerer.extraVolumes }} {{- toYaml .Values.triggerer.extraVolumes | nindent 8 }} {{- end }} diff --git a/tests/charts/test_git_sync_scheduler.py b/tests/charts/test_git_sync_scheduler.py index ba4ca833b7a3a..caa37c1426269 100644 --- a/tests/charts/test_git_sync_scheduler.py +++ b/tests/charts/test_git_sync_scheduler.py @@ -131,6 +131,24 @@ 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): + docs = render_chart( + values={ + "dags": { + "gitSync": { + "enabled": True, + "containerName": "git-sync-test", + "sshKeySecret": "ssh-secret", + "knownHosts": None, + "branch": "test-branch", + }, + "persistence": {"enabled": True}, + } + }, + show_only=["templates/scheduler/scheduler-deployment.yaml"], + ) + assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) + def test_should_set_username_and_pass_env_variables(self): docs = render_chart( values={ diff --git a/tests/charts/test_git_sync_triggerer.py b/tests/charts/test_git_sync_triggerer.py new file mode 100644 index 0000000000000..5942d0c17e62b --- /dev/null +++ b/tests/charts/test_git_sync_triggerer.py @@ -0,0 +1,42 @@ +# 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. + +import unittest + +import jmespath + +from tests.charts.helm_template_generator import render_chart + + +class GitSyncTriggererTest(unittest.TestCase): + def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): + docs = render_chart( + values={ + "dags": { + "gitSync": { + "enabled": True, + "containerName": "git-sync-test", + "sshKeySecret": "ssh-secret", + "knownHosts": None, + "branch": "test-branch", + }, + "persistence": {"enabled": True}, + } + }, + show_only=["templates/triggerer/triggerer-deployment.yaml"], + ) + assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) diff --git a/tests/charts/test_git_sync_webserver.py b/tests/charts/test_git_sync_webserver.py index 545abed9a2845..cc67c9d9f5a15 100644 --- a/tests/charts/test_git_sync_webserver.py +++ b/tests/charts/test_git_sync_webserver.py @@ -170,3 +170,21 @@ def test_resources_are_configurable(self): "spec.template.spec.containers[1].resources.requests.memory", docs[0] ) assert "300m" == jmespath.search("spec.template.spec.containers[1].resources.requests.cpu", docs[0]) + + def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): + docs = render_chart( + values={ + "dags": { + "gitSync": { + "enabled": True, + "containerName": "git-sync-test", + "sshKeySecret": "ssh-secret", + "knownHosts": None, + "branch": "test-branch", + }, + "persistence": {"enabled": True}, + } + }, + show_only=["templates/webserver/webserver-deployment.yaml"], + ) + assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) diff --git a/tests/charts/test_git_sync_worker.py b/tests/charts/test_git_sync_worker.py index 720b724747bdd..28c63f817e5c1 100644 --- a/tests/charts/test_git_sync_worker.py +++ b/tests/charts/test_git_sync_worker.py @@ -112,3 +112,22 @@ def test_resources_are_configurable(self): "spec.template.spec.containers[1].resources.requests.memory", docs[0] ) assert "300m" == jmespath.search("spec.template.spec.containers[1].resources.requests.cpu", docs[0]) + + def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): + docs = render_chart( + values={ + "dags": { + "gitSync": { + "enabled": True, + "containerName": "git-sync-test", + "sshKeySecret": "ssh-secret", + "knownHosts": None, + "branch": "test-branch", + }, + "persistence": {"enabled": True}, + } + }, + show_only=["templates/workers/worker-deployment.yaml"], + ) + + assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) From 3a0f9a5375ec291baedd04238b88faafffac7b27 Mon Sep 17 00:00:00 2001 From: Ephraim Anierobi Date: Wed, 13 Apr 2022 12:36:01 +0100 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> --- chart/templates/scheduler/scheduler-deployment.yaml | 2 +- chart/templates/triggerer/triggerer-deployment.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/chart/templates/scheduler/scheduler-deployment.yaml b/chart/templates/scheduler/scheduler-deployment.yaml index 00c1b042d548a..1752e84843e6a 100644 --- a/chart/templates/scheduler/scheduler-deployment.yaml +++ b/chart/templates/scheduler/scheduler-deployment.yaml @@ -253,7 +253,7 @@ spec: {{- else if .Values.dags.gitSync.enabled }} - name: dags emptyDir: {} - {{- 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 }} diff --git a/chart/templates/triggerer/triggerer-deployment.yaml b/chart/templates/triggerer/triggerer-deployment.yaml index 4739d7620477a..b0464dbc1341c 100644 --- a/chart/templates/triggerer/triggerer-deployment.yaml +++ b/chart/templates/triggerer/triggerer-deployment.yaml @@ -199,7 +199,7 @@ spec: {{- else if .Values.dags.gitSync.enabled }} - name: dags emptyDir: {} - {{- 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 }} From d8b269e59d1a7cdebc15119d2535ed5c78cf0491 Mon Sep 17 00:00:00 2001 From: Ephraim Anierobi Date: Thu, 28 Jul 2022 12:29:51 +0100 Subject: [PATCH 3/5] Update tests/charts/test_git_sync_scheduler.py Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> --- tests/charts/test_git_sync_scheduler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/charts/test_git_sync_scheduler.py b/tests/charts/test_git_sync_scheduler.py index caa37c1426269..a7dd5ad5c9138 100644 --- a/tests/charts/test_git_sync_scheduler.py +++ b/tests/charts/test_git_sync_scheduler.py @@ -147,7 +147,7 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): }, show_only=["templates/scheduler/scheduler-deployment.yaml"], ) - assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) def test_should_set_username_and_pass_env_variables(self): docs = render_chart( From fd8ad28890ccca87cbdea316d2e7ee7cfe3d4ccc Mon Sep 17 00:00:00 2001 From: Ephraim Anierobi Date: Mon, 11 Apr 2022 17:03:59 +0100 Subject: [PATCH 4/5] Do not declare a volume for sshKeySecret if dag persistence is enabled In scheduler and triggerer components, git-sync-ssh-key volume was created even when persistence is enabled. This PR fixes that and added tests in other components to avoid regression --- tests/charts/test_git_sync_triggerer.py | 2 +- tests/charts/test_git_sync_webserver.py | 2 +- tests/charts/test_git_sync_worker.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/charts/test_git_sync_triggerer.py b/tests/charts/test_git_sync_triggerer.py index 5942d0c17e62b..23f89b350fe55 100644 --- a/tests/charts/test_git_sync_triggerer.py +++ b/tests/charts/test_git_sync_triggerer.py @@ -39,4 +39,4 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): }, show_only=["templates/triggerer/triggerer-deployment.yaml"], ) - assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) diff --git a/tests/charts/test_git_sync_webserver.py b/tests/charts/test_git_sync_webserver.py index cc67c9d9f5a15..3d70400b98c7d 100644 --- a/tests/charts/test_git_sync_webserver.py +++ b/tests/charts/test_git_sync_webserver.py @@ -187,4 +187,4 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): }, show_only=["templates/webserver/webserver-deployment.yaml"], ) - assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) diff --git a/tests/charts/test_git_sync_worker.py b/tests/charts/test_git_sync_worker.py index 28c63f817e5c1..8420797512a1e 100644 --- a/tests/charts/test_git_sync_worker.py +++ b/tests/charts/test_git_sync_worker.py @@ -130,4 +130,4 @@ def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self): show_only=["templates/workers/worker-deployment.yaml"], ) - assert {"name": "git-sync-ssh-key"} not in jmespath.search("spec.template.spec.volumes", docs[0]) + assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0]) From 04c59040915f1e2ea6b7bdce39b9833b3011f898 Mon Sep 17 00:00:00 2001 From: Ephraim Anierobi Date: Sat, 6 Aug 2022 09:20:48 +0100 Subject: [PATCH 5/5] fixup! Do not declare a volume for sshKeySecret if dag persistence is enabled --- chart/templates/scheduler/scheduler-deployment.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/chart/templates/scheduler/scheduler-deployment.yaml b/chart/templates/scheduler/scheduler-deployment.yaml index 1752e84843e6a..3e9b94e7d83a6 100644 --- a/chart/templates/scheduler/scheduler-deployment.yaml +++ b/chart/templates/scheduler/scheduler-deployment.yaml @@ -256,6 +256,7 @@ spec: {{- if .Values.dags.gitSync.sshKeySecret }} {{- include "git_sync_ssh_key_volume" . | indent 8 }} {{- end }} + {{- end}} {{- end }} {{- if .Values.scheduler.extraVolumes }} {{ toYaml .Values.scheduler.extraVolumes | indent 8 }}