[Helm-Chart] BugFix: Wrong broker URL secret ref

[Desdroid]

If the helm chart creates the secret for the broker URL it uses the template {{ include "airflow.fullname" . }}-broker-url - see Source

However the default airflow env variables use a different template, just using the Release. {{ default (printf "%s-broker-url" .Release.Name) .Values.data.brokerUrlSecretName }} see Source

This is fatal as soon as someone uses the original chart as a dependency, so that airflow-fullname != Release.Name resulting in wrong secret key references and therefore a failed deployment due to Init:CreateContainerConfigError.
It is broken since eabe6b8 -> since v1.19. We hit it today while upgrading from 1.18 -> 1.20

This PR fixes this issue.

Please note, that I saw that the secrets are all not using the secrets names as set in the helpers, so that the name is duplicated.
https://github.com/apache/airflow/blob/main/chart/templates/_helpers.yaml#L406-L480 and following lines.
If it is desired I could use the helper templates also to set the name of the secrets in the secrets dir. This could be done in another PR ofc.

---

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[Desdroid]

A Workaround for this bug is this config:

# Workaround until https://github.com/apache/airflow/pull/65006 gets merged
enableBuiltInSecretEnvVars:
  AIRFLOW__CELERY__BROKER_URL: false
extraEnv: |
  - name: AIRFLOW__CELERY__BROKER_URL
    valueFrom:
      secretKeyRef:
        name: '{{ include "airflow.fullname" . }}-broker-url'
        key: connection

[Desdroid]

On a side note #52953
(PR for eabe6b8 ) also made the upgrade from 1.18 not straight forward.
This PR was only in the Bug Fixes section and no migration hint provided but effectively leads to renaming a bunch of k8s resources which helm does not automatically migrate or doesn't support changing.
E.g. it seems like the serviceName of the Triggerer StatefulSet was also changed, which I think is not supported by helm. We then get Error: UPGRADE FAILED: cannot patch "pipelines-airflow-triggerer" with kind StatefulSet: StatefulSet.apps "pipelines-airflow-triggerer" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden trying to do a simple helm upgrade.

So we ended up with uninstalling the entire helm chart release and doing a fresh install, while retaining the persistent volumes of db etc.
While I think this is a bit of an edge case (Having the helm chart of airflow as a dependency) - should I add some Info in the docs ? Or the GH Releases notes be adapted? What do you think?

[Miretpl]

Good catch! Could you add a test case for?

[Miretpl]

It would be good to do a template and use it in a proper secret file (also for consistency matter with other secrets).

[Desdroid]

There are no tests for using the chart as a dependency. However this bug has 2 prerequisites:
useStandardNaming: true and having fullnameOverride != Release.Name, nameOverride != Release.Name or use the airflow helm chart as a dependency.
So i added a test for the fullnameOverride case. If you want a different test case, let me know.

[Miretpl]

I was thinking more about the name matches test between the given secret and the environment variable. I think that this issue is not chart dependency specific, but is affecting any setup where .Relase.Name != airflow.fullname which is possible to do by using different configuration options.

[Miretpl]

This PR was only in the Bug Fixes section and no migration hint provided but effectively leads to renaming a bunch of k8s resources which helm does not automatically migrate or doesn't support changing.
E.g. it seems like the serviceName of the Triggerer StatefulSet was also changed, which I think is not supported by helm. We then get Error: UPGRADE FAILED: cannot patch "pipelines-airflow-triggerer" with kind StatefulSet: StatefulSet.apps "pipelines-airflow-triggerer" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden trying to do a simple helm upgrade.

I think it was just missed before the release that it can, potentially, break some stuff. Sorry for the inconvenience and thanks for the PR!

While I think this is a bit of an edge case (Having the helm chart of airflow as a dependency) - should I add some Info in the docs? Or the GH Releases notes be adapted? What do you think?

I think some extension to the extending-the-chart.rst would be good to add, and maybe a newsfragment to make this bugfix more visible during the next release, as it changes the behaviour, and I would not be surprised if some people are depending on the current behaviour even if they do not know about it.

[Desdroid]

I think it was just missed before the release that it can, potentially, break some stuff. Sorry for the inconvenience and thanks for the PR!

Sure 😃 I think that also useStandardNaming: true is a prerequisite for this bug. I found in the values.yaml that there was a Note that fernet-key,redis-password and broker-url secrets deliberately did not use the airflow-fullname helper as it might break existing installations (see https://github.com/apache/airflow/blob/main/chart/values.yaml#L38-L40)
I removed that note, as it is outdated since #52953

I think some extension to the extending-the-chart.rst would be good to add, and maybe a newsfragment to make this bugfix more visible during the next release, as it changes the behaviour, and I would not be surprised if some people are depending on the current behaviour even if they do not know about it.

I added a newsfragment and also included a possible workaround, though I'm not 100% sure that the workaround is correct. I'll try to test with our installation.
I don't know what should be worded differently / added in extending-the-chart.rst - If you want a change there please tell me what you think of. (This bug is not a general rule on how to do something differently when extending the chart, is it?)

[Desdroid]

I added a newsfragment and also included a possible workaround, though I'm not 100% sure that the workaround is correct. I'll try to test with our installation.

Dang, it's not everything. Since there is also the change in the service name of the statefulset as I said previously and also the ingress can't be replaced if it is used. Tried to use fullnameOverride=Release.Name and manually removing the ingress before helm upgrade but that then creates new log pvcs and so on.
So i don't think there is a clear path for a clean helm update if you are affected by the changes of #52953 or at least it is very specific to each installation.
I therefore removed the workaround in the newsfragment.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[Copilot]

This newsfragment appears to overstate the scope of the change: it says fernet-key and redis-password secrets (and "some other K8s resources") were migrated to standard naming, but the PR changes shown are specifically about the broker URL secret reference (secretKeyRef name) matching airflow.fullname. Please rewrite this fragment to describe the actual behavior change (fixing broker URL secret name resolution when useStandardNaming/fullnameOverride cause airflow.fullname != .Release.Name) and only call out breaking behavior if there is a confirmed upgrade impact.

Suggested change

	``fernet-key``, ``redis-password`` and ``broker-url`` secrets migrated to use standard naming as well as some other K8s resources.
	This likely breaks existing installations with ``useStandardNaming=True`` that have a ``fullnameOverride != Release.Name``, ``nameOverride != Release.Name`` or use the airflow helm chart as a dependency.
	Fixed broker URL secret name resolution to use ``airflow.fullname`` consistently.
	This fixes deployments where ``useStandardNaming=True`` and ``airflow.fullname != .Release.Name``, for example when using ``fullnameOverride``, ``nameOverride``, or the Airflow Helm chart as a dependency.

[Desdroid]

I somewhat agree with this. My change is only a bugfix not a breaking change.
If we include this newsfragment now, it looks like the change is only in there since the next release, which simply is wrong and misleading for users.
But in fact it's been changed since v.1.19 - so imo the release notes there should be adapted and the newsfragment here be removed.
@Miretpl @jscheffl What do you think?

[Miretpl]

Not sure if we are doing something like that - updating documentation back. I recall that it happened in the past, but it was like 1 or 2 times (at least since I'm following Airflow, which is a couple of years already). Updating release notes for 1.19 would be the best, and here, we have only the fix-related info, but if an update of the past will not be possible, I think it would be good to inform users 2 releases later than not at all.

[Desdroid]

I'm fine with whatever you decide.

[Miretpl]

I found in the values.yaml that there was a Note that fernet-key,redis-password and broker-url secrets deliberately did not use the airflow-fullname helper as it might break existing installations (see https://github.com/apache/airflow/blob/main/chart/values.yaml#L38-L40)
I removed that note, as it is outdated since #52953

Good catch!

Dang, it's not everything. Since there is also the change in the service name of the statefulset as I said previously and also the ingress can't be replaced if it is used. Tried to use fullnameOverride=Release.Name and manually removing the ingress before helm upgrade but that then creates new log pvcs and so on.

I went through all of the doc, and I haven't found any information regarding the old setup. So I would say that we are good with the doc, and newsfragment should be sufficient.

[Desdroid]

Anything still needed to get this merged ?