Support external redis in helm chart

[FloChehab]

Hello @mik-laj ,

Here we go for the external redis support ! (Closes #11705)

NB: the first commit is the same as in #12009

I'll test this final version in the real world on Monday ; I am opening the PR to check that we are on the good path.

Description from the main commit:

The main objective here is to support the use
of an external redis instance in the helm chart.
The values 'data.brokerUrl' and
'data.brokerUrlSecretName' are added and
templates are updated.

This support is added with no breaking changes
(hopefully); only the redis.brokerURLSecretName
value is removed, but it wasn't actually used in
the chart.

Extensive tests for the redis related part of this
chart are also added (including runtime checks on
the values).

Docs also updated.

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

[mik-laj]

This complicates the tests a bit. What do you think to break this function into several functions so that each function is responsible for one assertion?

[FloChehab]

I wasn't sure about that part when coding it. Could you be a bit more specific regarding what I should aim for ?

• Simply split this helper into multiple more specific helpers,
• Or redisign a bit the tests so that each "test" perform by pytest corresponds to one of the main parts of this big helper
• Or something else ?

[mik-laj]

Basically, I would like to simplify this text so that it does not contain as many loops and that the main logic of the test is contained in the test function, not in the helper functions.

Here is an example that creates a simple assertion based on comparing two sets instead of using a list and negation.

From 12fff535b966e1b1fc881cadecd30f09a70a1df0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Bregu=C5=82a?= <[email protected]>
Date: Sun, 1 Nov 2020 12:53:57 +0100
Subject: Simplify assertion

---
 chart/tests/test_redis.py | 68 +++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/chart/tests/test_redis.py b/chart/tests/test_redis.py
index 117487127..6e365437f 100644
--- a/chart/tests/test_redis.py
+++ b/chart/tests/test_redis.py
@@ -18,7 +18,7 @@ import re
 import unittest
 from base64 import b64decode
 from subprocess import CalledProcessError
-from typing import Optional, Pattern, Set, Tuple
+from typing import Optional, Pattern
 
 from parameterized import parameterized
 
@@ -42,7 +42,6 @@ class RedisTest(unittest.TestCase):
     def check_redis_objects_created_by_chart(
         self,
         k8s_objects,
-        expected_obj_keys_not_present: Set[Tuple[str, str]],
         expected_redis_password_regex: Optional[Pattern] = None,
         expected_redis_broker_url_regex: Optional[Pattern] = None,
         expected_broker_url_secret_name: str = REDIS_OBJECTS["SECRET_BROKER_URL"][1],
@@ -50,17 +49,7 @@ class RedisTest(unittest.TestCase):
         """
         Helper to perform checks for the redis integration on the rendered objects
         """
-        k8s_obj_by_key = {
-            (k8s_object["kind"], k8s_object["metadata"]["name"]): k8s_object for k8s_object in k8s_objects
-        }
-
-        # check that the expected objects are created and that no extra objects are created
-        expected_obj_key_present = set(REDIS_OBJECTS.values()) - expected_obj_keys_not_present
-        for obj_key in expected_obj_key_present:
-            self.assertIn(obj_key, k8s_obj_by_key.keys())
-
-        for obj_key in expected_obj_keys_not_present:
-            self.assertNotIn(obj_key, k8s_obj_by_key)
+        k8s_obj_by_key = self.prepare_k8s_lookup_dict(k8s_objects)
 
         # check the password value in the secret
         if expected_redis_password_regex is not None:
@@ -95,6 +84,12 @@ class RedisTest(unittest.TestCase):
             "Deployment", "scheduler"
         ), "Couldn't verify that AIRFLOW__CELERY__BROKER_URL is properly set in scheduler."
 
+    def prepare_k8s_lookup_dict(self, k8s_objects):
+        k8s_obj_by_key = {
+            (k8s_object["kind"], k8s_object["metadata"]["name"]): k8s_object for k8s_object in k8s_objects
+        }
+        return k8s_obj_by_key
+
     @parameterized.expand(CELERY_EXECUTORS_PARAMS)
     def test_redis_by_chart_default(self, executor):
         k8s_objects = render_chart(
@@ -107,7 +102,6 @@ class RedisTest(unittest.TestCase):
         )
         self.check_redis_objects_created_by_chart(
             k8s_objects,
-            expected_obj_keys_not_present=set(),
             expected_redis_password_regex=re.compile(r"\w+"),
             expected_redis_broker_url_regex=re.compile(fr"redis://:\w+@{RELEASE_NAME_REDIS}-redis:6379/0"),
         )
@@ -122,9 +116,22 @@ class RedisTest(unittest.TestCase):
                 "redis": {"enabled": True, "password": "test-redis-password"},
             },
         )
+        k8s_obj_by_key = self.prepare_k8s_lookup_dict(k8s_objects)
+
+        # assert resources created
+        self.assertEqual(
+            set(REDIS_OBJECTS.values()).intersection(set(k8s_obj_by_key.keys())),
+            {
+                ('Secret', 'TEST-REDIS-redis-password'),
+                ('NetworkPolicy', 'TEST-REDIS-redis-policy'),
+                ('StatefulSet', 'TEST-REDIS-redis'),
+                ('Service', 'TEST-REDIS-redis'),
+                ('Secret', 'TEST-REDIS-broker-url')
+            },
+        )
+
         self.check_redis_objects_created_by_chart(
             k8s_objects,
-            expected_obj_keys_not_present=set(),
             expected_redis_password_regex=re.compile(r"test-redis-password"),
             expected_redis_broker_url_regex=re.compile(
                 fr"redis://:test-redis-password@{RELEASE_NAME_REDIS}-redis:6379/0"
@@ -159,12 +166,20 @@ class RedisTest(unittest.TestCase):
                 },
             },
         )
+        k8s_obj_by_key = self.prepare_k8s_lookup_dict(k8s_objects)
+
+        # assert resources created
+        self.assertEqual(
+            set(REDIS_OBJECTS.values()).intersection(set(k8s_obj_by_key.keys())),
+            {
+                ('NetworkPolicy', 'TEST-REDIS-redis-policy'),
+                ('StatefulSet', 'TEST-REDIS-redis'),
+                ('Service', 'TEST-REDIS-redis'),
+            },
+        )
+
         self.check_redis_objects_created_by_chart(
             k8s_objects,
-            expected_obj_keys_not_present={
-                REDIS_OBJECTS["SECRET_PASSWORD"],
-                REDIS_OBJECTS["SECRET_BROKER_URL"],
-            },
             expected_broker_url_secret_name="test-redis-broker-url-secret-name",
         )
 
@@ -181,9 +196,16 @@ class RedisTest(unittest.TestCase):
                 "redis": {"enabled": False},
             },
         )
+        k8s_obj_by_key = self.prepare_k8s_lookup_dict(k8s_objects)
+
+        # assert resources created
+        self.assertEqual(
+            set(REDIS_OBJECTS.values()).intersection(set(k8s_obj_by_key.keys())),
+            {('Secret', 'TEST-REDIS-broker-url')},
+        )
+
         self.check_redis_objects_created_by_chart(
             k8s_objects,
-            expected_obj_keys_not_present=set(REDIS_OBJECTS.values()) - {REDIS_OBJECTS["SECRET_BROKER_URL"]},
             expected_redis_broker_url_regex=re.compile("redis://redis-user:password@redis-host:6379/0"),
         )
 
@@ -198,8 +220,12 @@ class RedisTest(unittest.TestCase):
                 "redis": {"enabled": False},
             },
         )
+        k8s_obj_by_key = self.prepare_k8s_lookup_dict(k8s_objects)
+
+        # assert resources created
+        self.assertEqual(set(REDIS_OBJECTS.values()).intersection(set(k8s_obj_by_key.keys())), set())
+
         self.check_redis_objects_created_by_chart(
             k8s_objects,
-            expected_obj_keys_not_present=set(REDIS_OBJECTS.values()),
             expected_broker_url_secret_name="redis-broker-url-secret-name",
         )
-- 
2.28.0

[mik-laj]

I would be happy if you would make separate functions that perform assertions for environment variables and secrets.

        self.assert_secrets(
            k8s_objects,
            expected_redis_password_regex=r"\w+",
            expected_redis_broker_url_regex=fr"redis://:\w+@{RELEASE_NAME_REDIS}-redis:6379/0",
        )
        self._assert_broker_url_envs(k8s_obj_by_key, 'redis-broker-url-secret-name')

[FloChehab]

I've done two fixups on the tests to address your comments ; I hope it's be better now.

[FloChehab]

I am not familiar with helm hooks, but we might need to add the pre-upgrade hook to this when using an external redis.

[FloChehab]

Just tested the change in practice. Looks good to me.

[mik-laj]

@OmairK Can you look at it?

[kaxil]

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

[FloChehab]

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

Done.

[FloChehab]

Hello,

Just wanted to get a small status update :)
On my side, I've rebased the branch to resolve the conflicts.

[kaxil]

Thanks @FloChehab .

@dimberman Can you take a look please

[FloChehab]

Thanks @FloChehab .

@dimberman Can you take a look please

Hello and happy new year!
I have rebased again the PR.

Maybe @potiuk you can have a look at this too as I have seen you review other chart related PRs in the past ?

Thanks.

[FloChehab]

NB: as this is a "big feature" and not that easy to test, I'll be able to provide bug fix support related to this in the coming weeks if needed.

[mik-laj]

@FloChehab Please let me know, when CI is green I will merge this change.

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

[FloChehab]

@FloChehab Please let me know, when CI is green I will merge this change.

CI is green :)

[FloChehab]

Thanks @mik-laj !

[mik-laj]

@FloChehab Are you using Google Cloud Platform? I am thinking to write system tests for Helm Chart. If you use it, you could work on system tests for this integration as well. It would be great to have a tool to quickly test a Helm Cart on GKE using all the possibilities that the cloud gives e.g. Cloud SQL, Cloud Memorystore. I would like to contribute a simple example so that the community can later develop it with other components and test cases.

[FloChehab]

Yes, we are using GCP & GKE.
By the way, for a bit of background we (https://github.com/MeilleursAgents/) are migrating our legacy production airflow deployment to a kubernetes setup with this chart. Hence the several contributions. This is also enabling us to maximize reproducibility when developing dags (each developer starts its own "standardized" airflow with this chart and copies of the same secrets & configmaps for configuration -- and LocalExecutor -- all with skaffold so that the local code syncs to the containers). Just saying that if it were interesting.

If you use it, you could work on system tests for this integration as well. It would be great to have a tool to quickly test a Helm Cart on GKE using all the possibilities that the cloud gives e.g. Cloud SQL, Cloud Memorystore. I would like to contribute a simple example so that the community can later develop it with other components and test cases.

I am not sure I have a clear idea about would you have in mind. Could you be a bit more precise?

While you are here, I have another small question:
When deploying on GKE, our ops/sre team played with the ingress configuration and needed to modify the chart to increase security with the use of a "backend config" (I am not familiar with the details, but the commit is here: MeilleursAgents@b92f630). Is it something that would be accepted as a contribution here or is too much vendor specific?

[FloChehab]

What I would suggest follow:

* Putting this (external) broker under `data` is not very proper to me. Putting it under `redis` is not making sense as well (this `redis` section is for the Redis instance provisioned by the Chart). So shall we have a dedicated section, say `broker`? So that it's less confusing.

* Meanwhile, the scope of this change can be expanded from "_support external Redis_" to "_support external Broker_" (it can be Redis, RabbitMQ, or experimentally a sqlalchemy database, according to our doc). It should be straightforward because in whichever case among these three, we always deal with a URI.

Please share your thoughts or correct me if I missed anything. Cheers

Regarding the confusion, I certainly don't have enough airflow experience to see the issue here; so I'd agree on all of that and I can quickly work on a PR for this if everyone agrees.

[mik-laj]

in values.yaml, shouldn't this new item brokerUrl be added under redis rather than data? data is for database based on what's under it earlier (metadatabase and broker backend db, both are actually database).

In my opinion it would be confusing to add this key in the "Redis" section, because the first subkey is "enabled". The developer may get the impression that setting redis.enabled to "false" all of the following keys are being ignored.

data is for database based on what's under it earlier (metadatabase and broker backend db, both are actually database).

Redis is also a database. On the other hand, A is also used as a queue by Airflow because we have queued items (TI with queued state).

the name brokerUrl itself is misleading, because broker can be something else, like RabbitMQ.

It's a good idea to add support for other queuing brokers.

So shall we have a dedicated section, say broker? So that it's less confusing.

A good idea. We can change it. Have you looked at how external Redis setup is done in other Helm Chart? I think this can help us choose the best approach.

Please share your thoughts or correct me if I missed anything. Cheers

Your suggestions sound very good to me. I have recently had limited activity in this area. Do you have time to share your thoughts for other changes as well? area:helm-chart Airflow Helm Chart

It would be great if we could get it to a state that allows it to be released.

[potiuk]

I think this is something we should treat seriously - to get the helm chart released "officially" and to know what we need to get there.

I think it would be great (maybe at the next dev call in a week) to draw a plan on what we need to do to release the helm chart. Since it was donated from Astronomer, and I know there is a continuous sync of features Community vs. Astronomer chart (am I right ?) maybe Astronomer people could play a bigger role there and play a coordination role on it ?

I am happy to help as well and work together on the planing/execution - especially in the toold/dev/testing improvements area and I can help with preparing to have it 'releasable' via a Github Repo (I plan to have the prod image 'extracted' automatically to a "read-only" repo #11740 - we could do the same with the chart and have it installable from there.

@dimberman @vikramkoka @ashb WDYT ?