Chart: warn when webserver secret key isn't set (apache#18306)

We will start warning chart admins when they deploy with the default
random webserver secret key, as it can lead to unnecessary restarts of
the Airflow components.

chart/templates/NOTES.txt

@@ -129,3 +129,17 @@ DEPRECATION WARNING:
     Please change your values as support for the old name will be dropped in a future release.
 
 {{- end }}
+
+{{- if not (or .Values.webserverSecretKey .Values.webserverSecretKeySecretName) }}
+
+###########################################################
+#  WARNING: You should set a static webserver secret key  #
+###########################################################
+
+You are using a dynamically generated webserver secret key, which can lead to
+unnecessary restarts of your Airflow components.
+
+Information on how to set a static webserver secret key can be found here:
+https://airflow.apache.org/docs/helm-chart/stable/production-guide.html#webserver-secret-key
+
+{{- end }}

chart/tests/test_configmap.py

@@ -18,6 +18,7 @@
 import unittest
 
 import jmespath
+from parameterized import parameterized
 
 from tests.helm_template_generator import render_chart
 
@@ -46,12 +47,31 @@ def test_multiple_annotations(self):
         assert "value" == annotations.get("key")
         assert "value-two" == annotations.get("key-two")
 
-    def test_no_airflow_local_settings_by_default(self):
+    @parameterized.expand(
+        [
+            ('2.2.0', None, None, True),
+            ('2.2.0', "foo", None, False),
+            ('2.2.0', None, "foo", False),
+            ('2.1.3', None, None, False),
+            ('2.1.3', "foo", None, False),
+        ]
+    )
+    def test_default_airflow_local_settings(self, af_version, secret_key, secret_key_name, expected):
         docs = render_chart(
+            values={
+                "airflowVersion": af_version,
+                "webserverSecretKey": secret_key,
+                "webserverSecretKeySecretName": secret_key_name,
+            },
             show_only=["templates/configmaps/configmap.yaml"],
         )
-
-        assert "airflow_local_settings.py" not in jmespath.search("data", docs[0])
+        if expected:
+            assert (
+                "Usage of a dynamic webserver secret key detected"
+                in jmespath.search('data."airflow_local_settings.py"', docs[0]).strip()
+            )
+        else:
+            assert "" == jmespath.search('data."airflow_local_settings.py"', docs[0]).strip()
 
     def test_airflow_local_settings(self):
         docs = render_chart(

chart/tests/test_pod_template_file.py

@@ -294,15 +294,15 @@ def test_mount_airflow_cfg(self):
         )
 
         assert re.search("Pod", docs[0]["kind"])
-        assert {'configMap': {'name': 'RELEASE-NAME-airflow-config'}, 'name': 'config'} == jmespath.search(
-            "spec.volumes[1]", docs[0]
+        assert {'configMap': {'name': 'RELEASE-NAME-airflow-config'}, 'name': 'config'} in jmespath.search(
+            "spec.volumes", docs[0]
         )
         assert {
             'name': 'config',
             'mountPath': '/opt/airflow/airflow.cfg',
             'subPath': 'airflow.cfg',
             'readOnly': True,
-        } == jmespath.search("spec.containers[0].volumeMounts[1]", docs[0])
+        } in jmespath.search("spec.containers[0].volumeMounts", docs[0])
 
     def test_should_create_valid_affinity_and_node_selector(self):
         docs = render_chart(
@@ -368,12 +368,12 @@ def test_should_create_valid_volume_mount_and_volume(self):
             chart_dir=self.temp_chart_dir,
         )
 
-        assert "test-volume" == jmespath.search(
-            "spec.volumes[2].name",
+        assert "test-volume" in jmespath.search(
+            "spec.volumes[*].name",
             docs[0],
         )
-        assert "test-volume" == jmespath.search(
-            "spec.containers[0].volumeMounts[2].name",
+        assert "test-volume" in jmespath.search(
+            "spec.containers[0].volumeMounts[*].name",
             docs[0],
         )
 
@@ -393,8 +393,12 @@ def test_should_add_env_for_gitsync(self):
 
         assert {"name": "FOO", "value": "bar"} in jmespath.search("spec.initContainers[0].env", docs[0])
 
-    def test_no_airflow_local_settings_by_default(self):
-        docs = render_chart(show_only=["templates/pod-template-file.yaml"], chart_dir=self.temp_chart_dir)
+    def test_no_airflow_local_settings(self):
+        docs = render_chart(
+            values={"airflowLocalSettings": None},
+            show_only=["templates/pod-template-file.yaml"],
+            chart_dir=self.temp_chart_dir,
+        )
         volume_mounts = jmespath.search("spec.containers[0].volumeMounts", docs[0])
         assert "airflow_local_settings.py" not in str(volume_mounts)
 

chart/tests/test_scheduler.py

@@ -96,9 +96,9 @@ def test_should_add_extra_volume_and_extra_volume_mount(self):
             show_only=["templates/scheduler/scheduler-deployment.yaml"],
         )
 
-        assert "test-volume" == jmespath.search("spec.template.spec.volumes[1].name", docs[0])
-        assert "test-volume" == jmespath.search(
-            "spec.template.spec.containers[0].volumeMounts[3].name", docs[0]
+        assert "test-volume" in jmespath.search("spec.template.spec.volumes[*].name", docs[0])
+        assert "test-volume" in jmespath.search(
+            "spec.template.spec.containers[0].volumeMounts[*].name", docs[0]
         )
 
     def test_should_create_valid_affinity_tolerations_and_node_selector(self):
@@ -197,9 +197,7 @@ def test_logs_persistence_changes_volume(self, log_persistence_values, expected_
             show_only=["templates/scheduler/scheduler-deployment.yaml"],
         )
 
-        assert {"name": "logs", **expected_volume} == jmespath.search(
-            "spec.template.spec.volumes[1]", docs[0]
-        )
+        assert {"name": "logs", **expected_volume} in jmespath.search("spec.template.spec.volumes", docs[0])
 
     def test_scheduler_resources_are_configurable(self):
         docs = render_chart(
@@ -237,8 +235,10 @@ def test_scheduler_resources_are_not_added_by_default(self):
         )
         assert jmespath.search("spec.template.spec.containers[0].resources", docs[0]) == {}
 
-    def test_no_airflow_local_settings_by_default(self):
-        docs = render_chart(show_only=["templates/scheduler/scheduler-deployment.yaml"])
+    def test_no_airflow_local_settings(self):
+        docs = render_chart(
+            values={"airflowLocalSettings": None}, show_only=["templates/scheduler/scheduler-deployment.yaml"]
+        )
         volume_mounts = jmespath.search("spec.template.spec.containers[0].volumeMounts", docs[0])
         assert "airflow_local_settings.py" not in str(volume_mounts)
 

chart/tests/test_webserver.py

@@ -301,8 +301,10 @@ def test_update_strategy(self):
 
         assert jmespath.search("spec.strategy", docs[0]) == expected_strategy
 
-    def test_no_airflow_local_settings_by_default(self):
-        docs = render_chart(show_only=["templates/webserver/webserver-deployment.yaml"])
+    def test_no_airflow_local_settings(self):
+        docs = render_chart(
+            values={"airflowLocalSettings": None}, show_only=["templates/webserver/webserver-deployment.yaml"]
+        )
         volume_mounts = jmespath.search("spec.template.spec.containers[0].volumeMounts", docs[0])
         assert "airflow_local_settings.py" not in str(volume_mounts)
 

chart/tests/test_worker.py

@@ -292,9 +292,7 @@ def test_logs_persistence_changes_volume(self, log_persistence_values, expected_
             show_only=["templates/workers/worker-deployment.yaml"],
         )
 
-        assert {"name": "logs", **expected_volume} == jmespath.search(
-            "spec.template.spec.volumes[1]", docs[0]
-        )
+        assert {"name": "logs", **expected_volume} in jmespath.search("spec.template.spec.volumes", docs[0])
 
     def test_worker_resources_are_configurable(self):
         docs = render_chart(
@@ -336,8 +334,10 @@ def test_worker_resources_are_not_added_by_default(self):
         )
         assert jmespath.search("spec.template.spec.containers[0].resources", docs[0]) == {}
 
-    def test_no_airflow_local_settings_by_default(self):
-        docs = render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+    def test_no_airflow_local_settings(self):
+        docs = render_chart(
+            values={"airflowLocalSettings": None}, show_only=["templates/workers/worker-deployment.yaml"]
+        )
         volume_mounts = jmespath.search("spec.template.spec.containers[0].volumeMounts", docs[0])
         assert "airflow_local_settings.py" not in str(volume_mounts)
 

chart/values.schema.json

@@ -274,7 +274,7 @@
                 "null"
             ],
             "x-docsSection": "Common",
-            "default": null
+            "default": "See values.yaml"
         },
         "rbac": {
             "description": "Enable RBAC (default on most clusters these days).",

chart/values.yaml

@@ -172,7 +172,24 @@ airflowPodAnnotations: {}
 airflowConfigAnnotations: {}
 
 # `airflow_local_settings` file as a string (can be templated).
-airflowLocalSettings: ~
+airflowLocalSettings: |-
+  {{- if semverCompare ">=2.2.0" .Values.airflowVersion }}
+  {{- if not (or .Values.webserverSecretKey .Values.webserverSecretKeySecretName) }}
+  from airflow.www.utils import UIAlert
+
+  DASHBOARD_UIALERTS = [
+    UIAlert(
+      'Usage of a dynamic webserver secret key detected. We recommend a static webserver secret key instead.'
+      ' See the <a href='
+      '"https://airflow.apache.org/docs/helm-chart/stable/production-guide.html#webserver-secret-key">'
+      'Helm Chart Production Guide</a> for more details.',
+      category="warning",
+      roles=["Admin"],
+      html=True,
+    )
+  ]
+  {{- end }}
+  {{- end }}
 
 # Enable RBAC (default on most clusters these days)
 rbac:

docs/helm-chart/production-guide.rst

@@ -69,6 +69,35 @@ Depending on the size of you Airflow instance, you may want to adjust the follow
     # The maximum number of server connections to the result backend database from PgBouncer
     resultBackendPoolSize: 5
 
+Webserver Secret Key
+--------------------
+
+You should set a static webserver secret key when deploying with this chart as it will help ensure
+your Airflow components only restart when necessary.
+
+.. warning::
+  You should use a different secret key for every instance you run, as this key is used to sign
+  session cookies and perform other security related functions!
+
+First, generate a strong secret key:
+
+.. code-block:: bash
+
+    python3 -c 'import secrets; print(secrets.token_hex(16))'
+
+Now add the secret to your values file:
+
+.. code-block:: yaml
+
+    webserverSecretKey: <secret_key>
+
+Alternatively, create a kubernetes Secret and use ``webserverSecretKeySecretName``:
+
+.. code-block:: yaml
+
+    webserverSecretKey: my-webserver-secret
+    # where the random key is under `webserver-secret-key` in the k8s Secret
+
 Extending and customizing Airflow Image
 ---------------------------------------