Miretpl commented on code in PR #72209:
URL: https://github.com/apache/airflow/pull/72209#discussion_r3972657254
##########
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 would prefer modifying the tests instead of adding logic to handle the
change. Going in that direction, maintenance of the tests will get harder and
harder.
##########
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:
```suggestion
By default, the chart does not create an Airflow user. If you do not use any
external user provider with Airflow for user authentication, you may use the
FAB provider and create the user on your own by running the command:
```
In the production guide, we do not need the whole history context of why
something was changed or what the past was. Also, the production guide itself
is not a good place for this documentation. For production deployments, an Auth
Manager different from FAB or the Simple one should be used. Having that
documentation here could be read as this being the production-ready deployment.
##########
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:
```suggestion
# Initial user to create. Only used when `createUserJob.enabled` is true,
and both
# `username` and `password` must be supplied
```
Same case as with the comment above.
##########
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:
```suggestion
```
For production deployments, it will just be noise.
##########
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:
```suggestion
.. note::
For security reasons, avoid providing the ``password`` field directly in
the ``values.yaml`` chart file.
```
to match with different places of documentation.
##########
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:
```suggestion
```
In the line below, we see that it is disabled by default. The rest is based
on an assumption that may not be true and does not add any value here, IMHO.
--
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]
