potiuk commented on code in PR #72209:
URL: https://github.com/apache/airflow/pull/72209#discussion_r4044124108
##########
chart/docs/production-guide.rst:
##########
@@ -227,6 +227,41 @@ In the ``values.yaml`` below secret-related parameters
should be adjusted like:
# The maximum number of server connections to the result backend
database from PgBouncer
resultBackendPoolSize: 5
+Creating the first user
+-----------------------
+
+The chart does not create an Airflow user for you.
+
+Earlier versions ran a create-user job by default that provisioned an
``admin`` account
+with the password ``admin``. Those credentials were the same on every
installation, so
+anything able to reach the API server -- by default any workload in the
cluster, since
+``networkPolicies.enabled`` is ``false`` -- could sign in with the Admin role.
+
+``createUserJob`` is therefore disabled by default and the chart ships no
default
+username or password. Enabling the job without supplying both fails the render
with a
+message saying so, rather than creating an account with well-known credentials.
+
+Create the user yourself after installing:
Review Comment:
Agreed on the second half: the production guide should not be where we show
people how to stand up a FAB user, because it reads as blessing FAB as the
production path when it is not. Taken your wording.
On the first half — I've kept the behavioural statement but trimmed it. This
is a breaking change with a `significant` newsfragment, and an operator whose
`createUserJob` stops working needs a sentence saying the default flipped and
why. What's gone is the narration of what the old behaviour was; what stays is
that the job is off by default and that enabling it without credentials fails
the render rather than creating a well-known account.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
chart/docs/production-guide.rst:
##########
@@ -227,6 +227,41 @@ In the ``values.yaml`` below secret-related parameters
should be adjusted like:
# The maximum number of server connections to the result backend
database from PgBouncer
resultBackendPoolSize: 5
+Creating the first user
+-----------------------
+
+The chart does not create an Airflow user for you.
+
+Earlier versions ran a create-user job by default that provisioned an
``admin`` account
+with the password ``admin``. Those credentials were the same on every
installation, so
+anything able to reach the API server -- by default any workload in the
cluster, since
+``networkPolicies.enabled`` is ``false`` -- could sign in with the Admin role.
+
+``createUserJob`` is therefore disabled by default and the chart ships no
default
+username or password. Enabling the job without supplying both fails the render
with a
+message saying so, rather than creating an account with well-known credentials.
+
+Create the user yourself after installing:
+
+.. code-block:: bash
+
+ kubectl exec -it deploy/<RELEASE_NAME>-api-server -- \
+ airflow users create -r Admin -u <username> -e <email> -f <first> -l
<last> -p <password>
+
+Or enable the job with credentials of your own:
+
+.. code-block:: yaml
+
+ createUserJob:
+ enabled: true
+ defaultUser:
+ username: <username>
+ password: <password>
+
+Values files are frequently committed to source control, so where your
deployment
+tooling supports it, supply the password from a Kubernetes Secret rather than
writing it
+into ``values.yaml``.
Review Comment:
Taken, for consistency with the other pages.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
chart/tests/helm_tests/airflow_aux/test_create_user_job.py:
##########
@@ -18,7 +18,34 @@
import jmespath
import pytest
-from chart_utils.helm_template_generator import render_chart
+from chart_utils.helm_template_generator import render_chart as _render_chart
+
+
+def _deep_merge(base: dict, override: dict) -> dict:
Review Comment:
I'll meet you most of the way, though for a reason slightly different from
the one you gave.
The concrete problem with the wrapper wasn't maintenance, it was that it
made a test name false: `test_should_run_by_default` asserted the job renders,
while the whole point of the change is that it does not render by default. It
passed only because the helper silently switched the job on. A reader who
trusts the name comes away with the wrong idea about the chart.
What I wanted to avoid is pasting the same four lines of opt-in into thirty
tests, which has its own maintenance cost — and a fixture would need touching
every signature while staying just as implicit. So instead of hiding the
opt-in, I stopped shadowing it: the imported `render_chart` keeps its own name,
the helper is now `render_chart_with_user_job`, and every call site says which
one it means. The two tests that are *about* the job being off call the plain
one. `test_should_run_by_default` is now
`test_should_run_when_enabled_with_credentials`, and a new
`test_should_not_create_job_by_default` renders the chart untouched.
Pushed as `bd51cf35a4`. Reasonable?
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
chart/values.yaml:
##########
@@ -1427,17 +1427,22 @@ scheduler:
# Airflow create user job settings
createUserJob:
- # Whether the create user job should be created
- enabled: true
+ # Whether the create user job should be created.
+ # Disabled by default: an account created here exists on every install with
the
+ # same credentials, so enable it only together with credentials of your own.
Review Comment:
Half agreed. "Disabled by default" does restate `enabled: false` on the very
next line, and `AGENTS.md` is explicit that comments should not narrate the
line below — dropped.
The rest isn't an assumption, though: before this change the chart shipped
`username: admin` / `password: admin`, so every installation that left the
defaults alone did have an Admin account with identical credentials. That is
the reason the default is flipping, and it's the one thing a reader of
`values.yaml` cannot reconstruct from the values themselves. Kept as a single
line:
```yaml
# Creating a user here previously produced the same credentials on every
install.
enabled: false
```
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
chart/values.yaml:
##########
@@ -1427,17 +1427,22 @@ scheduler:
# Airflow create user job settings
createUserJob:
- # Whether the create user job should be created
- enabled: true
+ # Whether the create user job should be created.
+ # Disabled by default: an account created here exists on every install with
the
+ # same credentials, so enable it only together with credentials of your own.
+ enabled: false
- # Create initial user.
+ # Initial user to create. Only used when `createUserJob.enabled` is true,
and both
+ # `username` and `password` must be supplied - the chart ships no defaults
for them,
+ # so that enabling the job cannot create an account whose credentials are
the same on
+ # every installation.
Review Comment:
Taken.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
chart/templates/NOTES.txt:
##########
@@ -51,9 +51,12 @@ Flower Dashboard: kubectl port-forward svc/{{ include
"airflow.fullname" .
{{- end }}
{{- if .Values.createUserJob.enabled }}
-Default user (Airflow UI) Login credentials:
+User created by the create-user job (Airflow UI):
username: {{ .Values.createUserJob.defaultUser.username }}
- password: {{ .Values.createUserJob.defaultUser.password }}
+{{- else }}
+No Airflow UI user has been created. Create one with:
+ kubectl exec -it deploy/{{ include "airflow.fullname" . }}-api-server
--namespace {{ .Release.Namespace }} -- \
+ airflow users create -r Admin -u <username> -e <email> -f <first> -l
<last> -p <password>
Review Comment:
This branch renders only when `createUserJob.enabled` is false, which after
this change is every default install. That's exactly the case where the
operator has no UI login and no indication they need to make one — deleting it
means a fresh install finishes with a working Airflow that nobody can sign in
to and no hint as to why.
NOTES also renders for every install, not only production ones, so the noise
argument cuts differently here than it does in the production guide.
I've shortened it rather than dropping it — one line and a pointer, with the
inline command gone:
```
No Airflow UI user has been created. See the chart's production guide for
how to create one.
```
If the `airflow users create` invocation appearing in NOTES was the concern,
that covers it.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
