From 618c77b0ffd24897df37d8605230e80e156be890 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 11:12:55 -0600 Subject: [PATCH 01/21] ES helm tests --- helm_tests/airflow_core/test_env.py | 43 +++++++++++++++++++++++ helm_tests/airflow_core/test_scheduler.py | 26 ++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/helm_tests/airflow_core/test_env.py b/helm_tests/airflow_core/test_env.py index 5a5ee6580a66a..0e5899794a9f9 100644 --- a/helm_tests/airflow_core/test_env.py +++ b/helm_tests/airflow_core/test_env.py @@ -40,3 +40,46 @@ def test_should_add_airflow_home_notset(): assert {"name": "AIRFLOW_HOME", "value": "/opt/airflow"} in jmespath.search( "spec.template.spec.containers[0].env", docs[0] ) + + +def test_should_omit_elasticsearch_host_vars_if_es_disabled(): + docs = render_chart( + values={ + "elasticsearch": { + "enabled": False, + "secretName": "test-elastic-secret", + }, + }, + show_only=["templates/webserver/webserver-deployment.yaml"], + ) + + container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) + container_env_keys = [entry["name"] for entry in container_env] + + assert "AIRFLOW__ELASTICSEARCH__HOST" not in container_env_keys + assert "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST" not in container_env_keys + + +def test_should_add_elasticsearch_host_vars(): + docs = render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=["templates/webserver/webserver-deployment.yaml"], + ) + + expected_value_from = { + "secretKeyRef": { + "name": "test-elastic-secret", + "key": "connection" + } + } + + container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) + + assert {"name": "AIRFLOW__ELASTICSEARCH__HOST", "valueFrom": expected_value_from} in container_env + assert {"name": "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST", + "valueFrom": expected_value_from} in container_env diff --git a/helm_tests/airflow_core/test_scheduler.py b/helm_tests/airflow_core/test_scheduler.py index 0bef3e7e1322b..68907862da2d2 100644 --- a/helm_tests/airflow_core/test_scheduler.py +++ b/helm_tests/airflow_core/test_scheduler.py @@ -900,6 +900,32 @@ def test_scheduler_termination_grace_period_seconds(self, scheduler_values, expe ) assert expected == jmespath.search("spec.template.spec.terminationGracePeriodSeconds", docs[0]) + def test_should_add_log_port_when_local_executor_and_elasticsearch_disabled(self): + docs = render_chart( + values={ + "executor": "LocalExecutor", + "elasticsearch": {"enabled": False}}, + show_only=["templates/scheduler/scheduler-deployment.yaml"], + ) + + assert jmespath.search("spec.template.spec.containers[0].ports", docs[0]) == [{ + "name": "worker-logs", + "containerPort": 8793 + }] + + def test_should_omit_log_port_when_elasticsearch_enabled(self): + docs = render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=["templates/scheduler/scheduler-deployment.yaml"], + ) + + assert "ports" not in jmespath.search("spec.template.spec.containers[0]", docs[0]) + class TestSchedulerNetworkPolicy: """Tests scheduler network policy.""" From 0acb5f7ca62a4f50e7f03d99fccdb909c16cfc78 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 11:17:57 -0600 Subject: [PATCH 02/21] ES helm tests 2 --- helm_tests/airflow_aux/test_remote_logging.py | 164 ++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 helm_tests/airflow_aux/test_remote_logging.py diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py new file mode 100644 index 0000000000000..57f2f93f0d079 --- /dev/null +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -0,0 +1,164 @@ +# 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 base64 +from subprocess import CalledProcessError + +import jmespath +import pytest + +from tests.charts.helm_template_generator import render_chart + + +class TestElasticsearchSecret: + """Tests elasticsearch secret.""" + + def test_should_not_generate_a_document_if_elasticsearch_disabled(self): + docs = render_chart( + values={"elasticsearch": {"enabled": False}}, + show_only=["templates/secrets/elasticsearch-secret.yaml"], + ) + + assert len(docs) == 0 + + def test_should_raise_error_when_connection_not_provided(self): + with pytest.raises(CalledProcessError) as ex_ctx: + render_chart( + values={ + "elasticsearch": { + "enabled": True, + } + }, + show_only=["templates/secrets/elasticsearch-secret.yaml"], + ) + assert ( + "You must set one of the values elasticsearch.secretName or elasticsearch.connection " + "when using a Elasticsearch" in ex_ctx.value.stderr.decode() + ) + + def test_should_raise_error_when_conflicting_options(self): + with pytest.raises(CalledProcessError) as ex_ctx: + render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "my-test", + "connection": { + "user": "username!@#$%%^&*()", + "pass": "password!@#$%%^&*()", + "host": "elastichostname", + }, + }, + }, + show_only=["templates/secrets/elasticsearch-secret.yaml"], + ) + assert ( + "You must not set both values elasticsearch.secretName and elasticsearch.connection" + in ex_ctx.value.stderr.decode() + ) + + def _get_connection(self, values: dict) -> str: + docs = render_chart( + values=values, + show_only=["templates/secrets/elasticsearch-secret.yaml"], + ) + encoded_connection = jmespath.search("data.connection", docs[0]) + return base64.b64decode(encoded_connection).decode() + + def test_should_correctly_handle_password_with_special_characters(self): + connection = self._get_connection( + { + "elasticsearch": { + "enabled": True, + "connection": { + "user": "username!@#$%%^&*()", + "pass": "password!@#$%%^&*()", + "host": "elastichostname", + }, + } + } + ) + + assert ( + connection + == "http://username%21%40%23$%25%25%5E&%2A%28%29:password%21%40%23$%25%25%5E&%2A%28%29@" + "elastichostname:9200" + ) + + def test_should_generate_secret_with_specified_port(self): + connection = self._get_connection( + { + "elasticsearch": { + "enabled": True, + "connection": { + "user": "username", + "pass": "password", + "host": "elastichostname", + "port": 2222, + }, + } + } + ) + + assert connection == "http://username:password@elastichostname:2222" + + @pytest.mark.parametrize("scheme", ["http", "https"]) + def test_should_generate_secret_with_specified_schemes(self, scheme): + connection = self._get_connection( + { + "elasticsearch": { + "enabled": True, + "connection": { + "scheme": scheme, + "user": "username", + "pass": "password", + "host": "elastichostname", + }, + } + } + ) + + assert f"{scheme}://username:password@elastichostname:9200" == connection + + @pytest.mark.parametrize( + "extra_conn_kwargs, expected_user_info", + [ + # When both user and password are empty. + ({}, ""), + # When password is empty + ({"user": "admin"}, ""), + # When user is empty + ({"pass": "password"}, ""), + # Valid username/password + ({"user": "admin", "pass": "password"}, "admin:password"), + ], + ) + def test_url_generated_when_user_pass_empty_combinations(self, extra_conn_kwargs, expected_user_info): + connection = self._get_connection( + { + "elasticsearch": { + "enabled": True, + "connection": {"host": "elastichostname", "port": 8080, **extra_conn_kwargs}, + } + } + ) + + if not expected_user_info: + assert connection == "http://elastichostname:8080" + else: + assert f"http://{expected_user_info}@elastichostname:8080" == connection From 17ce4ea23979636dfebdd559cbe1806ee2e85d70 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 11:27:53 -0600 Subject: [PATCH 03/21] more ES tests --- helm_tests/airflow_aux/test_remote_logging.py | 94 +------------------ .../security/test_elasticsearch_secret.py | 44 --------- 2 files changed, 2 insertions(+), 136 deletions(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 57f2f93f0d079..2ebb151d33405 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -25,8 +25,8 @@ from tests.charts.helm_template_generator import render_chart -class TestElasticsearchSecret: - """Tests elasticsearch secret.""" +class TestElasticsearchConfig: + """Tests elasticsearch configuration behaviors.""" def test_should_not_generate_a_document_if_elasticsearch_disabled(self): docs = render_chart( @@ -72,93 +72,3 @@ def test_should_raise_error_when_conflicting_options(self): in ex_ctx.value.stderr.decode() ) - def _get_connection(self, values: dict) -> str: - docs = render_chart( - values=values, - show_only=["templates/secrets/elasticsearch-secret.yaml"], - ) - encoded_connection = jmespath.search("data.connection", docs[0]) - return base64.b64decode(encoded_connection).decode() - - def test_should_correctly_handle_password_with_special_characters(self): - connection = self._get_connection( - { - "elasticsearch": { - "enabled": True, - "connection": { - "user": "username!@#$%%^&*()", - "pass": "password!@#$%%^&*()", - "host": "elastichostname", - }, - } - } - ) - - assert ( - connection - == "http://username%21%40%23$%25%25%5E&%2A%28%29:password%21%40%23$%25%25%5E&%2A%28%29@" - "elastichostname:9200" - ) - - def test_should_generate_secret_with_specified_port(self): - connection = self._get_connection( - { - "elasticsearch": { - "enabled": True, - "connection": { - "user": "username", - "pass": "password", - "host": "elastichostname", - "port": 2222, - }, - } - } - ) - - assert connection == "http://username:password@elastichostname:2222" - - @pytest.mark.parametrize("scheme", ["http", "https"]) - def test_should_generate_secret_with_specified_schemes(self, scheme): - connection = self._get_connection( - { - "elasticsearch": { - "enabled": True, - "connection": { - "scheme": scheme, - "user": "username", - "pass": "password", - "host": "elastichostname", - }, - } - } - ) - - assert f"{scheme}://username:password@elastichostname:9200" == connection - - @pytest.mark.parametrize( - "extra_conn_kwargs, expected_user_info", - [ - # When both user and password are empty. - ({}, ""), - # When password is empty - ({"user": "admin"}, ""), - # When user is empty - ({"pass": "password"}, ""), - # Valid username/password - ({"user": "admin", "pass": "password"}, "admin:password"), - ], - ) - def test_url_generated_when_user_pass_empty_combinations(self, extra_conn_kwargs, expected_user_info): - connection = self._get_connection( - { - "elasticsearch": { - "enabled": True, - "connection": {"host": "elastichostname", "port": 8080, **extra_conn_kwargs}, - } - } - ) - - if not expected_user_info: - assert connection == "http://elastichostname:8080" - else: - assert f"http://{expected_user_info}@elastichostname:8080" == connection diff --git a/helm_tests/security/test_elasticsearch_secret.py b/helm_tests/security/test_elasticsearch_secret.py index 57f2f93f0d079..bf74e2b358a5a 100644 --- a/helm_tests/security/test_elasticsearch_secret.py +++ b/helm_tests/security/test_elasticsearch_secret.py @@ -28,50 +28,6 @@ class TestElasticsearchSecret: """Tests elasticsearch secret.""" - def test_should_not_generate_a_document_if_elasticsearch_disabled(self): - docs = render_chart( - values={"elasticsearch": {"enabled": False}}, - show_only=["templates/secrets/elasticsearch-secret.yaml"], - ) - - assert len(docs) == 0 - - def test_should_raise_error_when_connection_not_provided(self): - with pytest.raises(CalledProcessError) as ex_ctx: - render_chart( - values={ - "elasticsearch": { - "enabled": True, - } - }, - show_only=["templates/secrets/elasticsearch-secret.yaml"], - ) - assert ( - "You must set one of the values elasticsearch.secretName or elasticsearch.connection " - "when using a Elasticsearch" in ex_ctx.value.stderr.decode() - ) - - def test_should_raise_error_when_conflicting_options(self): - with pytest.raises(CalledProcessError) as ex_ctx: - render_chart( - values={ - "elasticsearch": { - "enabled": True, - "secretName": "my-test", - "connection": { - "user": "username!@#$%%^&*()", - "pass": "password!@#$%%^&*()", - "host": "elastichostname", - }, - }, - }, - show_only=["templates/secrets/elasticsearch-secret.yaml"], - ) - assert ( - "You must not set both values elasticsearch.secretName and elasticsearch.connection" - in ex_ctx.value.stderr.decode() - ) - def _get_connection(self, values: dict) -> str: docs = render_chart( values=values, From 7e69814277fb7939bb3bc08c4b18b68adb62eafc Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 11:35:25 -0600 Subject: [PATCH 04/21] more ES tests 2 --- helm_tests/airflow_aux/test_remote_logging.py | 63 ++++++++++++++++++- helm_tests/airflow_core/test_scheduler.py | 26 -------- 2 files changed, 62 insertions(+), 27 deletions(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 2ebb151d33405..e74fc8e034ed8 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -28,7 +28,7 @@ class TestElasticsearchConfig: """Tests elasticsearch configuration behaviors.""" - def test_should_not_generate_a_document_if_elasticsearch_disabled(self): + def test_should_not_generate_a_secret_document_if_elasticsearch_disabled(self): docs = render_chart( values={"elasticsearch": {"enabled": False}}, show_only=["templates/secrets/elasticsearch-secret.yaml"], @@ -72,3 +72,64 @@ def test_should_raise_error_when_conflicting_options(self): in ex_ctx.value.stderr.decode() ) + def test_scheduler_should_add_log_port_when_local_executor_and_elasticsearch_disabled(self): + docs = render_chart( + values={ + "executor": "LocalExecutor", + "elasticsearch": {"enabled": False}}, + show_only=["templates/scheduler/scheduler-deployment.yaml"], + ) + + assert jmespath.search("spec.template.spec.containers[0].ports", docs[0]) == [{ + "name": "worker-logs", + "containerPort": 8793 + }] + + def test_scheduler_should_omit_log_port_when_elasticsearch_enabled(self): + docs = render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=["templates/scheduler/scheduler-deployment.yaml"], + ) + + assert "ports" not in jmespath.search("spec.template.spec.containers[0]", docs[0]) + + def test_env_should_omit_elasticsearch_host_vars_if_es_disabled(self): + docs = render_chart( + values={}, + show_only=["templates/scheduler/scheduler-deployment.yaml"], + ) + + container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) + container_env_keys = [entry["name"] for entry in container_env] + + assert "AIRFLOW__ELASTICSEARCH__HOST" not in container_env_keys + assert "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST" not in container_env_keys + + def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): + docs = render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=["templates/scheduler/scheduler-deployment.yaml"], + ) + + expected_value_from = { + "secretKeyRef": { + "name": "test-elastic-secret", + "key": "connection" + } + } + + container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) + + assert {"name": "AIRFLOW__ELASTICSEARCH__HOST", "valueFrom": expected_value_from} in container_env + assert {"name": "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST", + "valueFrom": expected_value_from} in container_env diff --git a/helm_tests/airflow_core/test_scheduler.py b/helm_tests/airflow_core/test_scheduler.py index 68907862da2d2..0bef3e7e1322b 100644 --- a/helm_tests/airflow_core/test_scheduler.py +++ b/helm_tests/airflow_core/test_scheduler.py @@ -900,32 +900,6 @@ def test_scheduler_termination_grace_period_seconds(self, scheduler_values, expe ) assert expected == jmespath.search("spec.template.spec.terminationGracePeriodSeconds", docs[0]) - def test_should_add_log_port_when_local_executor_and_elasticsearch_disabled(self): - docs = render_chart( - values={ - "executor": "LocalExecutor", - "elasticsearch": {"enabled": False}}, - show_only=["templates/scheduler/scheduler-deployment.yaml"], - ) - - assert jmespath.search("spec.template.spec.containers[0].ports", docs[0]) == [{ - "name": "worker-logs", - "containerPort": 8793 - }] - - def test_should_omit_log_port_when_elasticsearch_enabled(self): - docs = render_chart( - values={ - "elasticsearch": { - "enabled": True, - "secretName": "test-elastic-secret", - }, - }, - show_only=["templates/scheduler/scheduler-deployment.yaml"], - ) - - assert "ports" not in jmespath.search("spec.template.spec.containers[0]", docs[0]) - class TestSchedulerNetworkPolicy: """Tests scheduler network policy.""" From dd9663b30cc1ba61272c990e77f40c959a3d7ca9 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:39:46 -0600 Subject: [PATCH 05/21] don't modify test_env --- helm_tests/airflow_core/test_env.py | 43 ----------------------------- 1 file changed, 43 deletions(-) diff --git a/helm_tests/airflow_core/test_env.py b/helm_tests/airflow_core/test_env.py index 0e5899794a9f9..5a5ee6580a66a 100644 --- a/helm_tests/airflow_core/test_env.py +++ b/helm_tests/airflow_core/test_env.py @@ -40,46 +40,3 @@ def test_should_add_airflow_home_notset(): assert {"name": "AIRFLOW_HOME", "value": "/opt/airflow"} in jmespath.search( "spec.template.spec.containers[0].env", docs[0] ) - - -def test_should_omit_elasticsearch_host_vars_if_es_disabled(): - docs = render_chart( - values={ - "elasticsearch": { - "enabled": False, - "secretName": "test-elastic-secret", - }, - }, - show_only=["templates/webserver/webserver-deployment.yaml"], - ) - - container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) - container_env_keys = [entry["name"] for entry in container_env] - - assert "AIRFLOW__ELASTICSEARCH__HOST" not in container_env_keys - assert "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST" not in container_env_keys - - -def test_should_add_elasticsearch_host_vars(): - docs = render_chart( - values={ - "elasticsearch": { - "enabled": True, - "secretName": "test-elastic-secret", - }, - }, - show_only=["templates/webserver/webserver-deployment.yaml"], - ) - - expected_value_from = { - "secretKeyRef": { - "name": "test-elastic-secret", - "key": "connection" - } - } - - container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) - - assert {"name": "AIRFLOW__ELASTICSEARCH__HOST", "valueFrom": expected_value_from} in container_env - assert {"name": "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST", - "valueFrom": expected_value_from} in container_env From ac97bc5244c1930a60908f1e3b1befb8ba4a53e5 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:01:44 -0600 Subject: [PATCH 06/21] nits --- helm_tests/airflow_aux/test_remote_logging.py | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index e74fc8e034ed8..bb00baca544af 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -24,14 +24,18 @@ from tests.charts.helm_template_generator import render_chart +SCHEDULER_DEPLOYMENT_TEMPLATE = "templates/scheduler/scheduler-deployment.yaml" + +ES_SECRET_TEMPLATE = "templates/secrets/elasticsearch-secret.yaml" + class TestElasticsearchConfig: """Tests elasticsearch configuration behaviors.""" - def test_should_not_generate_a_secret_document_if_elasticsearch_disabled(self): + def test_should_not_generate_secret_document_if_elasticsearch_disabled(self): docs = render_chart( values={"elasticsearch": {"enabled": False}}, - show_only=["templates/secrets/elasticsearch-secret.yaml"], + show_only=[ES_SECRET_TEMPLATE], ) assert len(docs) == 0 @@ -44,7 +48,7 @@ def test_should_raise_error_when_connection_not_provided(self): "enabled": True, } }, - show_only=["templates/secrets/elasticsearch-secret.yaml"], + show_only=[ES_SECRET_TEMPLATE], ) assert ( "You must set one of the values elasticsearch.secretName or elasticsearch.connection " @@ -65,7 +69,7 @@ def test_should_raise_error_when_conflicting_options(self): }, }, }, - show_only=["templates/secrets/elasticsearch-secret.yaml"], + show_only=[ES_SECRET_TEMPLATE], ) assert ( "You must not set both values elasticsearch.secretName and elasticsearch.connection" @@ -77,7 +81,7 @@ def test_scheduler_should_add_log_port_when_local_executor_and_elasticsearch_dis values={ "executor": "LocalExecutor", "elasticsearch": {"enabled": False}}, - show_only=["templates/scheduler/scheduler-deployment.yaml"], + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) assert jmespath.search("spec.template.spec.containers[0].ports", docs[0]) == [{ @@ -93,7 +97,7 @@ def test_scheduler_should_omit_log_port_when_elasticsearch_enabled(self): "secretName": "test-elastic-secret", }, }, - show_only=["templates/scheduler/scheduler-deployment.yaml"], + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) assert "ports" not in jmespath.search("spec.template.spec.containers[0]", docs[0]) @@ -101,7 +105,7 @@ def test_scheduler_should_omit_log_port_when_elasticsearch_enabled(self): def test_env_should_omit_elasticsearch_host_vars_if_es_disabled(self): docs = render_chart( values={}, - show_only=["templates/scheduler/scheduler-deployment.yaml"], + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) @@ -118,7 +122,7 @@ def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): "secretName": "test-elastic-secret", }, }, - show_only=["templates/scheduler/scheduler-deployment.yaml"], + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) expected_value_from = { From 3c4ab683206d7669885eed22afa5249a0f66d0a5 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:21:55 -0600 Subject: [PATCH 07/21] test airflow.cfg ES settings --- helm_tests/airflow_aux/test_remote_logging.py | 42 +++++++++++++++++-- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index bb00baca544af..2c3fabc2505a6 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -16,7 +16,7 @@ # under the License. from __future__ import annotations -import base64 +import re from subprocess import CalledProcessError import jmespath @@ -28,6 +28,9 @@ ES_SECRET_TEMPLATE = "templates/secrets/elasticsearch-secret.yaml" +CORE_CFG_REGEX = re.compile(r"\[core]\n.*?\n\n", flags=re.RegexFlag.DOTALL) +LOGGING_CFG_REGEX = re.compile(r"\[logging]\n.*?\n\n", flags=re.RegexFlag.DOTALL) + class TestElasticsearchConfig: """Tests elasticsearch configuration behaviors.""" @@ -78,9 +81,7 @@ def test_should_raise_error_when_conflicting_options(self): def test_scheduler_should_add_log_port_when_local_executor_and_elasticsearch_disabled(self): docs = render_chart( - values={ - "executor": "LocalExecutor", - "elasticsearch": {"enabled": False}}, + values={"executor": "LocalExecutor"}, show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) @@ -137,3 +138,36 @@ def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): assert {"name": "AIRFLOW__ELASTICSEARCH__HOST", "valueFrom": expected_value_from} in container_env assert {"name": "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST", "valueFrom": expected_value_from} in container_env + + def test_config_should_set_remote_logging_false_if_es_disabled(self): + docs = render_chart( + values={}, + show_only=["templates/configmaps/configmap.yaml"], + ) + + airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) + + core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = False" in core_lines + + logging_lines = LOGGING_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = False" in logging_lines + + def test_config_should_set_remote_logging_true_if_es_enabled(self): + docs = render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=["templates/configmaps/configmap.yaml"], + ) + + airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) + + core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = True" in core_lines + + logging_lines = LOGGING_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = True" in logging_lines From 1e50dbb4b630dbb09255496185708052b27ccab7 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:23:13 -0600 Subject: [PATCH 08/21] ruff --- helm_tests/airflow_aux/test_remote_logging.py | 20 ++++++++----------- .../security/test_elasticsearch_secret.py | 1 - 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 2c3fabc2505a6..5e366aec651ef 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -85,10 +85,9 @@ def test_scheduler_should_add_log_port_when_local_executor_and_elasticsearch_dis show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) - assert jmespath.search("spec.template.spec.containers[0].ports", docs[0]) == [{ - "name": "worker-logs", - "containerPort": 8793 - }] + assert jmespath.search("spec.template.spec.containers[0].ports", docs[0]) == [ + {"name": "worker-logs", "containerPort": 8793} + ] def test_scheduler_should_omit_log_port_when_elasticsearch_enabled(self): docs = render_chart( @@ -126,18 +125,15 @@ def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) - expected_value_from = { - "secretKeyRef": { - "name": "test-elastic-secret", - "key": "connection" - } - } + expected_value_from = {"secretKeyRef": {"name": "test-elastic-secret", "key": "connection"}} container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) assert {"name": "AIRFLOW__ELASTICSEARCH__HOST", "valueFrom": expected_value_from} in container_env - assert {"name": "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST", - "valueFrom": expected_value_from} in container_env + assert { + "name": "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST", + "valueFrom": expected_value_from, + } in container_env def test_config_should_set_remote_logging_false_if_es_disabled(self): docs = render_chart( diff --git a/helm_tests/security/test_elasticsearch_secret.py b/helm_tests/security/test_elasticsearch_secret.py index bf74e2b358a5a..78bc3da9ae99c 100644 --- a/helm_tests/security/test_elasticsearch_secret.py +++ b/helm_tests/security/test_elasticsearch_secret.py @@ -17,7 +17,6 @@ from __future__ import annotations import base64 -from subprocess import CalledProcessError import jmespath import pytest From 1f3651aa561fd13eae1cb4595692dd16cc5daa78 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:30:37 -0600 Subject: [PATCH 09/21] tweaks --- helm_tests/airflow_aux/test_remote_logging.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 5e366aec651ef..5891e37b633bd 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -24,9 +24,9 @@ from tests.charts.helm_template_generator import render_chart -SCHEDULER_DEPLOYMENT_TEMPLATE = "templates/scheduler/scheduler-deployment.yaml" - ES_SECRET_TEMPLATE = "templates/secrets/elasticsearch-secret.yaml" +SCHEDULER_DEPLOYMENT_TEMPLATE = "templates/scheduler/scheduler-deployment.yaml" +CONFIGMAP_TEMPLATE = "templates/configmaps/configmap.yaml" CORE_CFG_REGEX = re.compile(r"\[core]\n.*?\n\n", flags=re.RegexFlag.DOTALL) LOGGING_CFG_REGEX = re.compile(r"\[logging]\n.*?\n\n", flags=re.RegexFlag.DOTALL) @@ -135,10 +135,10 @@ def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): "valueFrom": expected_value_from, } in container_env - def test_config_should_set_remote_logging_false_if_es_disabled(self): + def test_airflow_cfg_should_set_remote_logging_false_if_es_disabled(self): docs = render_chart( values={}, - show_only=["templates/configmaps/configmap.yaml"], + show_only=[CONFIGMAP_TEMPLATE], ) airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) @@ -149,7 +149,7 @@ def test_config_should_set_remote_logging_false_if_es_disabled(self): logging_lines = LOGGING_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() assert "remote_logging = False" in logging_lines - def test_config_should_set_remote_logging_true_if_es_enabled(self): + def test_airflow_cfg_should_set_remote_logging_true_if_es_enabled(self): docs = render_chart( values={ "elasticsearch": { @@ -157,7 +157,7 @@ def test_config_should_set_remote_logging_true_if_es_enabled(self): "secretName": "test-elastic-secret", }, }, - show_only=["templates/configmaps/configmap.yaml"], + show_only=[CONFIGMAP_TEMPLATE], ) airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) From 4193784b32c6a7e56319dc3f5a29abfb630d76b4 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:12:33 -0600 Subject: [PATCH 10/21] test nits --- helm_tests/airflow_aux/test_airflow_common.py | 2 + helm_tests/airflow_aux/test_remote_logging.py | 46 ++++++++++++++----- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/helm_tests/airflow_aux/test_airflow_common.py b/helm_tests/airflow_aux/test_airflow_common.py index d56d648f19af4..da1b2680641ef 100644 --- a/helm_tests/airflow_aux/test_airflow_common.py +++ b/helm_tests/airflow_aux/test_airflow_common.py @@ -321,7 +321,9 @@ def test_should_disable_some_variables(self): "AIRFLOW__CORE__SQL_ALCHEMY_CONN": False, "AIRFLOW__DATABASE__SQL_ALCHEMY_CONN": False, "AIRFLOW__WEBSERVER__SECRET_KEY": False, + # the following vars only appear if remote logging is set, so disabling them in this test is kind of a no-op "AIRFLOW__ELASTICSEARCH__HOST": False, + "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST": False, }, }, show_only=[ diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 5891e37b633bd..37772092284b6 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -92,6 +92,7 @@ def test_scheduler_should_add_log_port_when_local_executor_and_elasticsearch_dis def test_scheduler_should_omit_log_port_when_elasticsearch_enabled(self): docs = render_chart( values={ + "executor": "LocalExecutor", "elasticsearch": { "enabled": True, "secretName": "test-elastic-secret", @@ -108,11 +109,9 @@ def test_env_should_omit_elasticsearch_host_vars_if_es_disabled(self): show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) - container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) - container_env_keys = [entry["name"] for entry in container_env] - - assert "AIRFLOW__ELASTICSEARCH__HOST" not in container_env_keys - assert "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST" not in container_env_keys + scheduler_env_keys = jmespath.search("spec.template.spec.containers[0].env[*].name", docs[0]) + assert "AIRFLOW__ELASTICSEARCH__HOST" not in scheduler_env_keys + assert "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST" not in scheduler_env_keys def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): docs = render_chart( @@ -143,9 +142,6 @@ def test_airflow_cfg_should_set_remote_logging_false_if_es_disabled(self): airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) - core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() - assert "remote_logging = False" in core_lines - logging_lines = LOGGING_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() assert "remote_logging = False" in logging_lines @@ -162,8 +158,36 @@ def test_airflow_cfg_should_set_remote_logging_true_if_es_enabled(self): airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) - core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() - assert "remote_logging = True" in core_lines - logging_lines = LOGGING_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() assert "remote_logging = True" in logging_lines + + def test_airflow_cfg_should_set_remote_logging_false_if_es_disabled_legacy(self): + """core.remote_logging was the config location prior to Airflow 2.0.0, this test can be removed + when the Helm chart no longer supports Airflow 1.x""" + docs = render_chart( + values={}, + show_only=[CONFIGMAP_TEMPLATE], + ) + + airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) + + core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = False" in core_lines + + def test_airflow_cfg_should_set_remote_logging_true_if_es_enabled_legacy(self): + """core.remote_logging was the config location prior to Airflow 2.0.0, this test can be removed + when the Helm chart no longer supports Airflow 1.x""" + docs = render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=[CONFIGMAP_TEMPLATE], + ) + + airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) + + core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = True" in core_lines From f61fc5009953f1d78f039c0cb3bcc2b48cc0852b Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 08:12:09 -0600 Subject: [PATCH 11/21] add tests for opensearch secret and config --- helm_tests/airflow_aux/test_remote_logging.py | 49 +++++++ helm_tests/security/test_opensearch_secret.py | 120 ++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100644 helm_tests/security/test_opensearch_secret.py diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 37772092284b6..31f2d326ebc15 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -25,6 +25,7 @@ from tests.charts.helm_template_generator import render_chart ES_SECRET_TEMPLATE = "templates/secrets/elasticsearch-secret.yaml" +OS_SECRET_TEMPLATE = "templates/secrets/opensearch-secret.yaml" SCHEDULER_DEPLOYMENT_TEMPLATE = "templates/scheduler/scheduler-deployment.yaml" CONFIGMAP_TEMPLATE = "templates/configmaps/configmap.yaml" @@ -191,3 +192,51 @@ def test_airflow_cfg_should_set_remote_logging_true_if_es_enabled_legacy(self): core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() assert "remote_logging = True" in core_lines + + +class TestOpenSearchConfig: + """Tests opensearch configuration behaviors.""" + + def test_should_not_generate_a_document_if_opensearch_disabled(self): + docs = render_chart( + values={"opensearch": {"enabled": False}}, + show_only=[OS_SECRET_TEMPLATE], + ) + + assert len(docs) == 0 + + def test_should_raise_error_when_connection_not_provided(self): + with pytest.raises(CalledProcessError) as ex_ctx: + render_chart( + values={ + "opensearch": { + "enabled": True, + } + }, + show_only=[OS_SECRET_TEMPLATE], + ) + assert ( + "You must set one of the values opensearch.secretName or opensearch.connection " + "when using OpenSearch" in ex_ctx.value.stderr.decode() + ) + + def test_should_raise_error_when_conflicting_options(self): + with pytest.raises(CalledProcessError) as ex_ctx: + render_chart( + values={ + "opensearch": { + "enabled": True, + "secretName": "my-test", + "connection": { + "user": "username!@#$%%^&*()", + "pass": "password!@#$%%^&*()", + "host": "opensearchhostname", + }, + }, + }, + show_only=[OS_SECRET_TEMPLATE], + ) + assert ( + "You must not set both values opensearch.secretName and opensearch.connection" + in ex_ctx.value.stderr.decode() + ) diff --git a/helm_tests/security/test_opensearch_secret.py b/helm_tests/security/test_opensearch_secret.py new file mode 100644 index 0000000000000..2555232e0fddc --- /dev/null +++ b/helm_tests/security/test_opensearch_secret.py @@ -0,0 +1,120 @@ +# 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 base64 +from subprocess import CalledProcessError + +import jmespath +import pytest + +from tests.charts.helm_template_generator import render_chart + + +class TestOpenSearchSecret: + """Tests opensearch secret.""" + + def _get_connection(self, values: dict) -> str: + docs = render_chart( + values=values, + show_only=["templates/secrets/opensearch-secret.yaml"], + ) + encoded_connection = jmespath.search("data.connection", docs[0]) + return base64.b64decode(encoded_connection).decode() + + def test_should_correctly_handle_password_with_special_characters(self): + connection = self._get_connection( + { + "opensearch": { + "enabled": True, + "connection": { + "user": "username!@#$%%^&*()", + "pass": "password!@#$%%^&*()", + "host": "opensearchhostname", + }, + } + } + ) + + assert ( + connection + == "http://username%21%40%23$%25%25%5E&%2A%28%29:password%21%40%23$%25%25%5E&%2A%28%29@" + "opensearchhostname:9200" + ) + + def test_should_generate_secret_with_specified_port(self): + connection = self._get_connection( + { + "opensearch": { + "enabled": True, + "connection": { + "user": "username", + "pass": "password", + "host": "opensearchhostname", + "port": 2222, + }, + } + } + ) + + assert connection == "http://username:password@opensearchhostname:2222" + + @pytest.mark.parametrize("scheme", ["http", "https"]) + def test_should_generate_secret_with_specified_schemes(self, scheme): + connection = self._get_connection( + { + "opensearch": { + "enabled": True, + "connection": { + "scheme": scheme, + "user": "username", + "pass": "password", + "host": "opensearchhostname", + }, + } + } + ) + + assert f"{scheme}://username:password@opensearchhostname:9200" == connection + + @pytest.mark.parametrize( + "extra_conn_kwargs, expected_user_info", + [ + # When both user and password are empty. + ({}, ""), + # When password is empty + ({"user": "admin"}, ""), + # When user is empty + ({"pass": "password"}, ""), + # Valid username/password + ({"user": "admin", "pass": "password"}, "admin:password"), + ], + ) + def test_url_generated_when_user_pass_empty_combinations(self, extra_conn_kwargs, expected_user_info): + connection = self._get_connection( + { + "opensearch": { + "enabled": True, + "connection": {"host": "opensearchhostname", "port": 8080, **extra_conn_kwargs}, + } + } + ) + + if not expected_user_info: + assert connection == "http://opensearchhostname:8080" + else: + assert f"http://{expected_user_info}@opensearchhostname:8080" == connection From 984c28d13dad56a3b880d0fe1b22562077a31881 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 08:18:38 -0600 Subject: [PATCH 12/21] add template opensearch-secret.yaml and opensearch section to values.yaml --- .../templates/secrets/opensearch-secret.yaml | 44 +++++++++++++++++++ chart/values.yaml | 16 +++++++ 2 files changed, 60 insertions(+) create mode 100644 chart/templates/secrets/opensearch-secret.yaml diff --git a/chart/templates/secrets/opensearch-secret.yaml b/chart/templates/secrets/opensearch-secret.yaml new file mode 100644 index 0000000000000..e0e281d6f4113 --- /dev/null +++ b/chart/templates/secrets/opensearch-secret.yaml @@ -0,0 +1,44 @@ +{{/* + 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. +*/}} + +################################ +## OpenSearch Secret +################################# +{{- if (and .Values.opensearch.enabled (not .Values.opensearch.secretName)) }} +apiVersion: v1 +kind: Secret +metadata: + name: {{ include "airflow.fullname" . }}-opensearch + labels: + release: {{ .Release.Name }} + chart: {{ .Chart.Name }} + heritage: {{ .Release.Service }} + {{- with .Values.labels }} + {{- toYaml . | nindent 4 }} + {{- end }} +type: Opaque +data: + {{- with .Values.opensearch.connection }} + {{- if and .user .pass }} + connection: {{ urlJoin (dict "scheme" (default "http" .scheme) "userinfo" (printf "%s:%s" (.user | urlquery) (.pass | urlquery)) "host" (printf "%s:%s" .host ((default 9200 .port) | toString) ) ) | b64enc | quote }} + {{- else }} + connection: {{ urlJoin (dict "scheme" (default "http" .scheme) "host" (printf "%s:%s" .host ((default 9200 .port) | toString))) | b64enc | quote }} + {{- end }} + {{- end }} +{{- end }} diff --git a/chart/values.yaml b/chart/values.yaml index adf68c3a194d3..4971a4e72d622 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -2473,6 +2473,22 @@ elasticsearch: # port: ~ connection: {} +# OpenSearch logging configuration +opensearch: + # Enable opensearch task logging + enabled: false + # A secret containing the connection + secretName: ~ + # Or an object representing the connection + # Example: + # connection: + # scheme: ~ + # user: ~ + # pass: ~ + # host: ~ + # port: ~ + connection: {} + # All ports used by chart ports: flowerUI: 5555 From 8607ccbb1f46722f4aa962c273671542e9824fde Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 08:38:01 -0600 Subject: [PATCH 13/21] check-values.yaml --- chart/templates/check-values.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/chart/templates/check-values.yaml b/chart/templates/check-values.yaml index 6dfbbd9554108..62021eb9e8b0c 100644 --- a/chart/templates/check-values.yaml +++ b/chart/templates/check-values.yaml @@ -72,3 +72,14 @@ The sole purpose of this yaml file is it to check the values file is consistent {{- end }} {{- end }} + + {{- if .Values.opensearch.enabled }} + {{- if and .Values.opensearch.secretName .Values.opensearch.connection }} + {{ required "You must not set both values opensearch.secretName and opensearch.connection" nil }} + {{- end }} + + {{- if not (or .Values.opensearch.secretName .Values.opensearch.connection) }} + {{ required "You must set one of the values opensearch.secretName or opensearch.connection when using OpenSearch" nil }} + {{- end }} + + {{- end }} From b00e2769d2a954548f976b7926f8770692cba5f0 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:15:53 -0600 Subject: [PATCH 14/21] add opensearch node to schema definition --- chart/values.schema.json | 63 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/chart/values.schema.json b/chart/values.schema.json index 52939b48c5c98..2f36225b70d25 100644 --- a/chart/values.schema.json +++ b/chart/values.schema.json @@ -8044,6 +8044,69 @@ } } }, + "opensearch": { + "description": "OpenSearch logging configuration.", + "type": "object", + "x-docsSection": "Airflow", + "additionalProperties": false, + "properties": { + "enabled": { + "description": "Enable OpenSearch task logging.", + "type": "boolean", + "default": false + }, + "secretName": { + "description": "A secret containing the connection string.", + "type": [ + "string", + "null" + ], + "default": null + }, + "connection": { + "description": "OpenSearch connection configuration.", + "type": "object", + "default": {}, + "additionalProperties": false, + "properties": { + "scheme": { + "description": "Scheme", + "type": "string", + "default": "http" + }, + "user": { + "description": "Username", + "type": "string", + "default": "" + }, + "pass": { + "description": "Password", + "type": "string", + "default": "" + }, + "host": { + "description": "Host", + "type": "string", + "default": "" + }, + "port": { + "description": "Port", + "type": "number", + "default": 80 + } + }, + "examples": [ + { + "scheme": "https", + "user": "...", + "pass": "...", + "host": "...", + "port": "..." + } + ] + } + } + }, "ports": { "description": "All ports used by chart.", "type": "object", From 23deeb7fb465dfb79deee7f18b0aa6613727513c Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:35:28 -0600 Subject: [PATCH 15/21] ruff --- helm_tests/security/test_opensearch_secret.py | 1 - 1 file changed, 1 deletion(-) diff --git a/helm_tests/security/test_opensearch_secret.py b/helm_tests/security/test_opensearch_secret.py index 2555232e0fddc..fa6803782f6df 100644 --- a/helm_tests/security/test_opensearch_secret.py +++ b/helm_tests/security/test_opensearch_secret.py @@ -17,7 +17,6 @@ from __future__ import annotations import base64 -from subprocess import CalledProcessError import jmespath import pytest From d3f6268d264ae0577bb05a60c51fa856dd3cf9a8 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 20:13:59 -0600 Subject: [PATCH 16/21] add more OS tests --- helm_tests/airflow_aux/test_remote_logging.py | 106 +++++++++++++++++- 1 file changed, 105 insertions(+), 1 deletion(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 31f2d326ebc15..6e279968b9fa1 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -197,7 +197,7 @@ def test_airflow_cfg_should_set_remote_logging_true_if_es_enabled_legacy(self): class TestOpenSearchConfig: """Tests opensearch configuration behaviors.""" - def test_should_not_generate_a_document_if_opensearch_disabled(self): + def test_should_not_generate_secret_document_if_opensearch_disabled(self): docs = render_chart( values={"opensearch": {"enabled": False}}, show_only=[OS_SECRET_TEMPLATE], @@ -240,3 +240,107 @@ def test_should_raise_error_when_conflicting_options(self): "You must not set both values opensearch.secretName and opensearch.connection" in ex_ctx.value.stderr.decode() ) + + def test_scheduler_should_add_log_port_when_local_executor_and_opensearch_disabled(self): + docs = render_chart( + values={"executor": "LocalExecutor"}, + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], + ) + + assert jmespath.search("spec.template.spec.containers[0].ports", docs[0]) == [ + {"name": "worker-logs", "containerPort": 8793} + ] + + def test_scheduler_should_omit_log_port_when_opensearch_enabled(self): + docs = render_chart( + values={ + "executor": "LocalExecutor", + "opensearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], + ) + + assert "ports" not in jmespath.search("spec.template.spec.containers[0]", docs[0]) + + def test_env_should_omit_opensearch_host_var_if_os_disabled(self): + docs = render_chart( + values={}, + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], + ) + + scheduler_env_keys = jmespath.search("spec.template.spec.containers[0].env[*].name", docs[0]) + assert "AIRFLOW__OPENSEARCH__HOST" not in scheduler_env_keys + + def test_env_should_add_opensearch_host_var_if_os_enabled(self): + docs = render_chart( + values={ + "opensearch": { + "enabled": True, + "secretName": "test-opensearch-secret", + }, + }, + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], + ) + + scheduler_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) + assert { + "name": "AIRFLOW__OPENSEARCH__HOST", + "valueFrom": {"secretKeyRef": {"name": "test-opensearch-secret", "key": "connection"}}, + } in scheduler_env + + def test_airflow_cfg_should_set_remote_logging_false_if_os_disabled(self): + docs = render_chart( + values={}, + show_only=[CONFIGMAP_TEMPLATE], + ) + + airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) + + core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = False" in core_lines + + logging_lines = LOGGING_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = False" in logging_lines + + def test_airflow_cfg_should_set_remote_logging_true_if_os_enabled(self): + docs = render_chart( + values={ + "opensearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=[CONFIGMAP_TEMPLATE], + ) + + airflow_cfg_text = jmespath.search('data."airflow.cfg"', docs[0]) + + core_lines = CORE_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = True" in core_lines + + logging_lines = LOGGING_CFG_REGEX.findall(airflow_cfg_text)[0].strip().splitlines() + assert "remote_logging = True" in logging_lines + + +def test_should_raise_error_when_both_elasticsearch_and_opensearch_enabled(): + with pytest.raises(CalledProcessError) as ex_ctx: + render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + "opensearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], + ) + assert ( + "You must not set both values elasticsearch.enabled and opensearch.enabled" + in ex_ctx.value.stderr.decode() + ) From 7bb924b67fea42ab970697e22f07872bb7b72e6f Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 20:30:26 -0600 Subject: [PATCH 17/21] raise error if ES and OS both set --- chart/templates/check-values.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/chart/templates/check-values.yaml b/chart/templates/check-values.yaml index 62021eb9e8b0c..a4ede57f3bc9c 100644 --- a/chart/templates/check-values.yaml +++ b/chart/templates/check-values.yaml @@ -62,6 +62,10 @@ The sole purpose of this yaml file is it to check the values file is consistent {{- end }} + {{- if and .Values.elasticsearch.enabled .Values.opensearch.enabled }} + {{ required "You must not set both values elasticsearch.enabled and opensearch.enabled" nil }} + {{- end }} + {{- if .Values.elasticsearch.enabled }} {{- if and .Values.elasticsearch.secretName .Values.elasticsearch.connection }} {{ required "You must not set both values elasticsearch.secretName and elasticsearch.connection" nil }} From 2b7354d3e551779ec508301355c11829cebed26c Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Wed, 18 Dec 2024 20:39:13 -0600 Subject: [PATCH 18/21] cfg sets remote_logging for ES or OS --- chart/templates/scheduler/scheduler-deployment.yaml | 8 ++++---- chart/values.yaml | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/chart/templates/scheduler/scheduler-deployment.yaml b/chart/templates/scheduler/scheduler-deployment.yaml index 551ab94c8c48d..ed63f1ef46903 100644 --- a/chart/templates/scheduler/scheduler-deployment.yaml +++ b/chart/templates/scheduler/scheduler-deployment.yaml @@ -30,8 +30,8 @@ {{- $stateful := and $local $persistence }} # We can skip DAGs mounts on scheduler if dagProcessor is enabled, except with $local mode {{- $localOrDagProcessorDisabled := or (not .Values.dagProcessor.enabled) $local }} -# If we're using elasticsearch logging -{{- $elasticsearch := .Values.elasticsearch.enabled }} +# If we're using elasticsearch or opensearch logging +{{- $remoteLogging := or .Values.elasticsearch.enabled .Values.opensearch.enabled }} {{- $nodeSelector := or .Values.scheduler.nodeSelector .Values.nodeSelector }} {{- $affinity := or .Values.scheduler.affinity .Values.affinity }} {{- $tolerations := or .Values.scheduler.tolerations .Values.tolerations }} @@ -217,8 +217,8 @@ spec: {{- else }} {{- include "scheduler_startup_check_command" . | indent 14 }} {{- end }} - {{- if and $local (not $elasticsearch) }} - # Serve logs if we're in local mode and we don't have elasticsearch enabled. + {{- if and $local (not $remoteLogging) }} + # Serve logs if we're in local mode and we have neither elasticsearch nor opensearch enabled. ports: - name: worker-logs containerPort: {{ .Values.ports.workerLogs }} diff --git a/chart/values.yaml b/chart/values.yaml index 4971a4e72d622..3e0ce8e4b5021 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -2607,9 +2607,9 @@ config: executor: '{{ .Values.executor }}' # For Airflow 1.10, backward compatibility; moved to [logging] in 2.0 colored_console_log: 'False' - remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}' + remote_logging: '{{- ternary "True" "False" (or .Values.elasticsearch.enabled .Values.opensearch.enabled) }}' logging: - remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}' + remote_logging: '{{- ternary "True" "False" (or .Values.elasticsearch.enabled .Values.opensearch.enabled) }}' colored_console_log: 'False' metrics: statsd_on: '{{ ternary "True" "False" .Values.statsd.enabled }}' From abe938451b9df4a84a8776077b29289e9bff88de Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Thu, 19 Dec 2024 09:45:20 -0600 Subject: [PATCH 19/21] add env var for opensearch --- chart/templates/_helpers.yaml | 13 +++++++++++++ chart/values.schema.json | 5 +++++ chart/values.yaml | 1 + 3 files changed, 19 insertions(+) diff --git a/chart/templates/_helpers.yaml b/chart/templates/_helpers.yaml index 887e9bd33cbb1..c76cefa843e34 100644 --- a/chart/templates/_helpers.yaml +++ b/chart/templates/_helpers.yaml @@ -142,6 +142,15 @@ If release name contains chart name it will be used as a full name. key: connection {{- end }} {{- end }} + {{- if .Values.opensearch.enabled }} + {{- if .Values.enableBuiltInSecretEnvVars.AIRFLOW__OPENSEARCH__HOST }} + - name: AIRFLOW__OPENSEARCH__HOST + valueFrom: + secretKeyRef: + name: {{ template "opensearch_secret" . }} + key: connection + {{- end }} + {{- end }} {{- end }} {{/* User defined Airflow environment variables */}} @@ -427,6 +436,10 @@ If release name contains chart name it will be used as a full name. {{- default (printf "%s-elasticsearch" (include "airflow.fullname" .)) .Values.elasticsearch.secretName }} {{- end }} +{{- define "opensearch_secret" -}} + {{- default (printf "%s-opensearch" (include "airflow.fullname" .)) .Values.opensearch.secretName }} +{{- end }} + {{- define "flower_secret" -}} {{- default (printf "%s-flower" (include "airflow.fullname" .)) .Values.flower.secretName }} {{- end }} diff --git a/chart/values.schema.json b/chart/values.schema.json index 2f36225b70d25..4296bf2665f7a 100644 --- a/chart/values.schema.json +++ b/chart/values.schema.json @@ -1093,6 +1093,11 @@ "description": "Enable ``AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST`` variable to be read from the Elasticsearch Host Secret - Airflow <1.10.4 variant", "type": "boolean", "default": true + }, + "AIRFLOW__OPENSEARCH__HOST": { + "description": "Enable ``AIRFLOW__OPENSEARCH__HOST`` variable to be read from the OpenSearch Host Secret", + "type": "boolean", + "default": true } } }, diff --git a/chart/values.yaml b/chart/values.yaml index 3e0ce8e4b5021..2e27b7a4686ee 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -366,6 +366,7 @@ enableBuiltInSecretEnvVars: AIRFLOW__CELERY__BROKER_URL: true AIRFLOW__ELASTICSEARCH__HOST: true AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST: true + AIRFLOW__OPENSEARCH__HOST: true # Priority Classes that will be installed by charts. # Ideally, there should be an entry for dagProcessor, flower, From b54829edddd5fe45e65f2f1089f091a7bc77e907 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Thu, 19 Dec 2024 09:57:01 -0600 Subject: [PATCH 20/21] nit and comment --- helm_tests/airflow_aux/test_airflow_common.py | 1 + 1 file changed, 1 insertion(+) diff --git a/helm_tests/airflow_aux/test_airflow_common.py b/helm_tests/airflow_aux/test_airflow_common.py index da1b2680641ef..0cd4218f7627b 100644 --- a/helm_tests/airflow_aux/test_airflow_common.py +++ b/helm_tests/airflow_aux/test_airflow_common.py @@ -324,6 +324,7 @@ def test_should_disable_some_variables(self): # the following vars only appear if remote logging is set, so disabling them in this test is kind of a no-op "AIRFLOW__ELASTICSEARCH__HOST": False, "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST": False, + "AIRFLOW__OPENSEARCH__HOST": False, }, }, show_only=[ From 5435776f558c225af8d127c89de420c136322390 Mon Sep 17 00:00:00 2001 From: "Christopher P. Anderson" <48180628+topherinternational@users.noreply.github.com> Date: Thu, 19 Dec 2024 21:26:19 -0600 Subject: [PATCH 21/21] split out more legacy config tests --- helm_tests/airflow_aux/test_remote_logging.py | 44 +++++++++++++++---- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/helm_tests/airflow_aux/test_remote_logging.py b/helm_tests/airflow_aux/test_remote_logging.py index 6e279968b9fa1..b6de5ef6288db 100644 --- a/helm_tests/airflow_aux/test_remote_logging.py +++ b/helm_tests/airflow_aux/test_remote_logging.py @@ -104,7 +104,7 @@ def test_scheduler_should_omit_log_port_when_elasticsearch_enabled(self): assert "ports" not in jmespath.search("spec.template.spec.containers[0]", docs[0]) - def test_env_should_omit_elasticsearch_host_vars_if_es_disabled(self): + def test_env_should_omit_elasticsearch_host_var_if_es_disabled(self): docs = render_chart( values={}, show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], @@ -112,9 +112,8 @@ def test_env_should_omit_elasticsearch_host_vars_if_es_disabled(self): scheduler_env_keys = jmespath.search("spec.template.spec.containers[0].env[*].name", docs[0]) assert "AIRFLOW__ELASTICSEARCH__HOST" not in scheduler_env_keys - assert "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST" not in scheduler_env_keys - def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): + def test_env_should_add_elasticsearch_host_var_if_es_enabled(self): docs = render_chart( values={ "elasticsearch": { @@ -125,15 +124,44 @@ def test_env_should_add_elasticsearch_host_vars_if_es_enabled(self): show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], ) - expected_value_from = {"secretKeyRef": {"name": "test-elastic-secret", "key": "connection"}} + scheduler_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) + + assert { + "name": "AIRFLOW__ELASTICSEARCH__HOST", + "valueFrom": {"secretKeyRef": {"name": "test-elastic-secret", "key": "connection"}}, + } in scheduler_env + + def test_env_should_omit_elasticsearch_host_var_if_es_disabled_legacy(self): + """AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST was the environment key prior to Airflow 1.10.4 + (see https://github.com/apache/airflow/pull/5048), this test can be removed when the Helm chart + no longer supports Airflow 1.10.3""" + docs = render_chart( + values={}, + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], + ) + + scheduler_env_keys = jmespath.search("spec.template.spec.containers[0].env[*].name", docs[0]) + assert "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST" not in scheduler_env_keys - container_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) + def test_env_should_add_elasticsearch_host_var_if_es_enabled_legacy(self): + """AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST was the environment key prior to Airflow 1.10.4 + (see https://github.com/apache/airflow/pull/5048), this test can be removed when the Helm chart + no longer supports Airflow 1.10.3""" + docs = render_chart( + values={ + "elasticsearch": { + "enabled": True, + "secretName": "test-elastic-secret", + }, + }, + show_only=[SCHEDULER_DEPLOYMENT_TEMPLATE], + ) - assert {"name": "AIRFLOW__ELASTICSEARCH__HOST", "valueFrom": expected_value_from} in container_env + scheduler_env = jmespath.search("spec.template.spec.containers[0].env", docs[0]) assert { "name": "AIRFLOW__ELASTICSEARCH__ELASTICSEARCH_HOST", - "valueFrom": expected_value_from, - } in container_env + "valueFrom": {"secretKeyRef": {"name": "test-elastic-secret", "key": "connection"}}, + } in scheduler_env def test_airflow_cfg_should_set_remote_logging_false_if_es_disabled(self): docs = render_chart(