ArgoCD deployment: build in redis is not restarted on password change

[Udbv]

Official Helm Chart version

1.4.0 (latest released)

Apache Airflow version

2.2.3 (latest released)

Kubernetes Version

1.21.x,1.18.x

Helm Chart configuration

KubernetesCeleryExecutor used

Docker Image customisations

No response

What happened

On each argocd airflow app update {{ .Release.Name }}-redis-password and {{ .Release.Name }}-broker-url is regenerated(since argocd do not honor "helm.sh/hook": "pre-install"). airflow pods restarted(as expected),get new redis connection and connection start failing with WRONG_PASSWORD, since redis is not restarted(old password used). Redis in chart 1.4 have no health checks.

What you expected to happen

Generally, I have two options:

1. (Prefered) Add health check to Redis with login sequence. The password should be updated "on the fly"(read from the mounted secret and try to connect)
2. (Workeraund) I implemented a workaround with the parent chart. {{ .Release.Name }}-Redis-password and {{ .Release.Name }}-broker-url secrets generated from template where immutable: true added to the secret definition.

How to reproduce

No response

Anything else

No response

Are you willing to submit PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

[boring-cyborg]

Thanks for opening your first issue here! Be sure to follow the issue template!

[sudeepgupta90]

This isn't a bug per say with Airflow, but rather how you deploy your Airflow chart. Airflow isn't bound to support ArgoCD specifically.

There are a few ways to solve this:

1. Static redis secret - which you should aim for in a production environment. There may be a number of ways to achieve this, and you have already listed in your issue
2. As an alternative to helm-hooks use Argo Resource

[potiuk]

Possibly we could going it with #28637 and add a bit more comprehensive support for ArgoCD (Since we seem to have more and more users using it) @sudeepgupta90 - WDYT?

This could be be both - documentation on how to do, or maybe even modifications in our Helm Chart to make it ArgoCD compliant. I think it is popular enough and we have a lot of users who use it.

And @jedcunningham @dstandish WDYY?

[sudeepgupta90]

@potiuk I am all for adding ArgoCD support docs (my personal bias as I am using Argo, and it is one of the more popular offerings these days). But then it's also a slippery slope- I fear our users will raise even more issues which are essentially Argo and not Airflow. And use the production guide to compete with even managed services perhaps?

tldr: For Chart and docs - Argo compliance is a bonus, but not a goal

[sudeepgupta90]

@potiuk
I was thinking more about this issue, and I realised another thing. The Airflow chart can be made simpler if we removed redis, altogether and make it optional (similar to how we do with Postgres). This has 2 effects:

1. This removes the complexity and allows users to deploy redis as they may through another upstream chart
2. Also, allows users to rely on an externally managed redis service for increased performance + simplicity of deployment.

[potiuk]

@potiuk I was thinking more about this issue, and I realised another thing. The Airflow chart can be made simpler if we removed redis, altogether and make it optional (similar to how we do with Postgres). This has 2 effects:

1. This removes the complexity and allows users to deploy redis as they may through another upstream chart
2. Also, allows users to rely on an externally managed redis service for increased performance + simplicity of deployment.

Yep. Great idea.

[jedcunningham]

Redis can already be disabled and deployed separately (redis.enabled and data.resultBackendSecretName). Is there a gap I'm missing?

[potiuk]

Redis can already be disabled and deployed separately (redis.enabled and data.resultBackendSecretName). Is there a gap I'm missing?

Oops :)

[sudeepgupta90]

@jedcunningham I agree that redis can be disabled. I actually got confused after seeing only if and .Values.redis.enabled... (and thought redis has to be always enabled) in redis manifests inside the Airflow chart and missed that we can use external redis. But my point was that this is extra appendage being added to the Airflow chart which is not really needed.

From what I understand, the earlier versions of Airflow only had CeleryExecutor so having a simplified redis distribution bundled inside the chart - made it easy to deploy. From my earlier comments, and users raising issues in redis components - it would fall on the Airflow maintenance to fix them, when it should be infact managed at redis chart level.

Doing this refactoring, does not add immediate value to the Airflow chart, but this is something which may be looked into for long term maintenance goal to reduce clutter and issues. Wdyt?

[potiuk]

I think - it is REALLY useful to have redis managed internally same as postgres. We already have a huge problem with having to manage many things outside of the chart to be able to run and test airflow in a simple way with celery. Having an easy way where both Redis and Postgres are run inside Airflow Chart is the best way (and should be the default) so that you can do

helm install apache/airflow

and get it working. Without any extra setup.

If anything is needed here, is possibly - similarly as in case of Postgres, clearly document and possibly even raise a warnign that using internal redis is meant to be used for quick start only and if you want to handle more "production" ready setup and include things like rotating passwords etc., you should configure your own redis externally.

This is mostly a documentation and possibly warning change - nothing to change in the basic flag behaviour.

@sudeepgupta90 - maybe you can create such PR with doc change, and possibly adding a warning. As someone who was mislead by thinking you should not use external redis, you are probably the best person to asses where as a user you would look for such an information and what warning wording would be best for people like you to make them aware that the basic setup is not handling all "production" cases.

It's super easy - go to our docs and choose "Suggest a change on this page" - and you wil get a PR where you will be able to update the docs. (and adding warning could be a follow - up maybe).

WDYT?

[sudeepgupta90]

Sure, I wasnt aware of the complexities associated with the redis not being internal as you explained above.

Can you please assign this issue, and I will work on both this and #28637 in the same PR

[potiuk]

Assigned :)