You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/02/09 17:17:45 UTC
[GitHub] [airflow] DerekHeldtWerle opened a new pull request #14152: Fix/rbac
DerekHeldtWerle opened a new pull request #14152:
URL: https://github.com/apache/airflow/pull/14152
This PR builds off of and supersedes @jaydesl's work on his [PR](https://github.com/apache/airflow/pull/11769) to move forward with properly following [helm's rbac best practices](https://helm.sh/docs/chart_best_practices/rbac/). This PR updates every potential pod that can be deployed to include the option to either create or use an existing service account. This is the first step towards supporting environments where users have the [PodSecurityPolicy](https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#podsecuritypolicy) admission controller enabled without forcing such users to provide any additional permissions to the default service account in the namespace this is deployed to.
closes: https://github.com/apache/airflow/issues/11755
related: https://github.com/apache/airflow/issues/13643
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573663354
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
This is not in the chart README.
##########
File path: chart/templates/rbac/pod-cleanup-role.yaml
##########
@@ -18,7 +18,7 @@
################################
## Airflow Cleanup Role
#################################
-{{- if and .Values.rbacEnabled .Values.cleanup.enabled }}
+{{- if and .Values.rbac.create .Values.cleanup.enabled }}
Review comment:
Why `rbac.create` here, but `cleanup.serviceAccount.create` elsewhere?
More specifically: why do we need _two_ different config options for each role we create?
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
Why do we need two separate override values?
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
Review comment:
`nameOverride` is not in the chart readme.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623386662
##########
File path: chart/values.yaml
##########
@@ -427,6 +453,20 @@ scheduler:
affinity: {}
tolerations: []
+# Airflow migration and create user job settings
+jobs:
Review comment:
Hi @kaxil, since the service account is used by both the create user job and the database migration job at this time, would you prefer to have that split into two separate accounts? I went with the generic `jobs` because of it mapping to both jobs that run during upgrade/install, but I'm good with splitting it out if desired.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ianstanton commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
ianstanton commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r620257779
##########
File path: chart/values.yaml
##########
@@ -445,6 +485,18 @@ webserver:
# Number of webservers
replicas: 1
+ # Create ServiceAccount
+ serviceAccount:
+ # Specifies whether a ServiceAccount should be created
+ create: true
+ # The name of the ServiceAccount to use.
+ # If not set and create is true, a name is generated using the release name
+ # name:
+
+ # Annotations to add to webserver kubernetes service account.
+ annotations: {}
+
+
Review comment:
```suggestion
```
We can remove this extra newline
##########
File path: chart/values.schema.json
##########
@@ -1494,5 +1673,5 @@
}
}
}
- }
+ }
Review comment:
```suggestion
}
```
@DerekHeldtWerle Nitpick, don't think this extra space should be here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830429391
Unit test is failing: https://github.com/apache/airflow/pull/14152/checks?check_run_id=2479497469#step:6:3089
could not find template templates/create-user-job.yaml in chart\n'
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r619682130
##########
File path: chart/templates/cleanup/cleanup-serviceaccount.yaml
##########
@@ -18,11 +18,11 @@
################################
## Airflow Cleanup ServiceAccount
#################################
-{{- if and .Values.rbacEnabled .Values.cleanup.enabled }}
+{{- if and .Values.cleanup.serviceAccount.create .Values.cleanup.enabled }}
kind: ServiceAccount
apiVersion: v1
metadata:
- name: {{ .Release.Name }}-cleanup
+ name: {{ include "cleanup.serviceAccountName" . }}
labels:
Review comment:
We should support annotation for the service account. It is needed for Workload Identity https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624200016
##########
File path: chart/tests/test_basic_helm_chart.py
##########
@@ -36,13 +36,19 @@ def test_basic_deployments(self):
'metadata': 'AA',
},
'labels': {"TEST-LABEL": "TEST-VALUE"},
+ "fullnameOverride": "TEST-BASIC",
Review comment:
Sounds good, we can keep it as it is
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14152: Fix/rbac
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-776099452
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830447651
Thanks @DerekHeldtWerle π
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r603658963
##########
File path: chart/templates/_helpers.yaml
##########
@@ -361,6 +387,105 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s-airflow-config" .Release.Name) }}
{{- end }}
+{{/*
+Create the name of the webserver service account to use
+*/}}
+{{- define "webserver.serviceAccountName" -}}
+{{- if .Values.webserver.serviceAccount.create -}}
+ {{ default (printf "%s-webserver" (include "airflow.fullname" .)) .Values.webserver.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.webserver.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
Review comment:
@DerekHeldtWerle any luck seeing if this was possible via loop?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jaydesl commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
jaydesl commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-777509597
Thanks for carrying this @DerekHeldtWerle !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ianstanton commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
ianstanton commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623014557
##########
File path: chart/tests/test_annotations.py
##########
@@ -0,0 +1,129 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+from tests.helm_template_generator import render_chart
+
+# Values for each service mapped to the 'example'
+# key annotation
+CUSTOM_ANNOTATION_VALUES = (
+ CUSTOM_SCHEDULER_ANNOTATION,
+ CUSTOM_WEBSERVER_ANNOTATION,
+ CUSTOM_WORKER_ANNOTATION,
+ CUSTOM_CLEANUP_ANNOTATION,
+ CUSTOM_FLOWER_ANNOTATION,
+ CUSTOM_PGBOUNCER_ANNOTATION,
+ CUSTOM_STATSD_ANNOTATION,
+ CUSTOM_JOBS_ANNOTATION,
+ CUSTOM_REDIS_ANNOTATION,
+) = (
+ "scheduler",
+ "webserver",
+ "worker",
+ "cleanup",
+ "flower",
+ "PGBouncer",
+ "statsd",
+ "jobs",
+ "redis",
+)
+
+
+class AnnotationsTest(unittest.TestCase):
+ def test_service_account_annotations(self):
+ k8s_objects = render_chart(
+ values={
+ "cleanup": {
+ "enabled": True,
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_CLEANUP_ANNOTATION,
+ },
+ },
+ },
+ "scheduler": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_SCHEDULER_ANNOTATION,
+ },
+ },
+ },
+ "webserver": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_WEBSERVER_ANNOTATION,
+ },
+ },
+ },
+ "workers": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_WORKER_ANNOTATION,
+ },
+ },
+ },
+ "flower": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_FLOWER_ANNOTATION,
+ },
+ },
+ },
+ "statsd": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_STATSD_ANNOTATION,
+ },
+ },
+ },
+ "redis": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_REDIS_ANNOTATION,
+ },
+ },
+ },
+ "pgbouncer": {
+ "enabled": True,
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_PGBOUNCER_ANNOTATION,
+ },
+ },
+ },
+ "jobs": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_JOBS_ANNOTATION,
+ },
+ },
+ },
+ "executor": "CeleryExecutor", # create worker deployment
+ },
+ )
+
+ list_of_annotation_values_in_objects = [
+ k8s_object['metadata']['annotations']['example']
+ for k8s_object in k8s_objects
+ if k8s_object['kind'] == "ServiceAccount"
+ ]
+
+ self.assertCountEqual(
+ list_of_annotation_values_in_objects,
+ CUSTOM_ANNOTATION_VALUES,
+ )
Review comment:
@DerekHeldtWerle In addition to count, can we also assert that the values match?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623416129
##########
File path: chart/templates/_helpers.yaml
##########
@@ -364,6 +383,105 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s-airflow-config" .Release.Name) }}
{{- end }}
+{{/*
+Create the name of the webserver service account to use
+*/}}
+{{- define "webserver.serviceAccountName" -}}
+{{- if .Values.webserver.serviceAccount.create -}}
+ {{ default (printf "%s-webserver" (include "airflow.fullname" .)) .Values.webserver.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.webserver.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the redis service account to use
+*/}}
+{{- define "redis.serviceAccountName" -}}
+{{- if .Values.redis.serviceAccount.create -}}
+ {{ default (printf "%s-redis" (include "airflow.fullname" .)) .Values.redis.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.redis.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the flower service account to use
+*/}}
+{{- define "flower.serviceAccountName" -}}
+{{- if .Values.flower.serviceAccount.create -}}
+ {{ default (printf "%s-flower" (include "airflow.fullname" .)) .Values.flower.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.flower.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the scheduler service account to use
+*/}}
+{{- define "scheduler.serviceAccountName" -}}
+{{- if .Values.scheduler.serviceAccount.create -}}
+ {{ default (printf "%s-scheduler" (include "airflow.fullname" .)) .Values.scheduler.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.scheduler.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the statsd service account to use
+*/}}
+{{- define "statsd.serviceAccountName" -}}
+{{- if .Values.statsd.serviceAccount.create -}}
+ {{ default (printf "%s-statsd" (include "airflow.fullname" .)) .Values.statsd.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.statsd.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the jobs service account to use
+*/}}
+{{- define "jobs.serviceAccountName" -}}
+{{- if .Values.jobs.serviceAccount.create -}}
+ {{ default (printf "%s-jobs" (include "airflow.fullname" .)) .Values.jobs.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.jobs.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
Review comment:
```suggestion
{{- define "userCreateJob.serviceAccountName" -}}
{{- if .Values.userCreateJob.serviceAccount.create -}}
{{ default (printf "%s-jobs" (include "airflow.fullname" .)) .Values.userCreateJob.serviceAccount.name }}
{{- else -}}
{{ default "default" .Values.userCreateJob.serviceAccount.name }}
{{- end -}}
{{- end -}}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624184513
##########
File path: chart/tests/test_rbac.py
##########
@@ -0,0 +1,294 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+CLEANUP_DEPLOYMENT_KIND_NAME_TUPLES = [
Review comment:
well it has lot more than that, it has statsd, webserver. etc.
maybe `DEPLOYMENT_NO_RBAC_NO_SA_KIND_NAME_TUPLES` to match the name of the test using it ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-818310728
@ashb, @mik-laj, @kaxil This has been rebased with master again. Let me know if there's anything that needs to be addressed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-825843702
[The Workflow run](https://github.com/apache/airflow/actions/runs/778647661) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623262956
##########
File path: chart/tests/test_annotations.py
##########
@@ -0,0 +1,129 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+from tests.helm_template_generator import render_chart
+
+# Values for each service mapped to the 'example'
+# key annotation
+CUSTOM_ANNOTATION_VALUES = (
+ CUSTOM_SCHEDULER_ANNOTATION,
+ CUSTOM_WEBSERVER_ANNOTATION,
+ CUSTOM_WORKER_ANNOTATION,
+ CUSTOM_CLEANUP_ANNOTATION,
+ CUSTOM_FLOWER_ANNOTATION,
+ CUSTOM_PGBOUNCER_ANNOTATION,
+ CUSTOM_STATSD_ANNOTATION,
+ CUSTOM_JOBS_ANNOTATION,
+ CUSTOM_REDIS_ANNOTATION,
+) = (
+ "scheduler",
+ "webserver",
+ "worker",
+ "cleanup",
+ "flower",
+ "PGBouncer",
+ "statsd",
+ "jobs",
+ "redis",
+)
+
+
+class AnnotationsTest(unittest.TestCase):
+ def test_service_account_annotations(self):
+ k8s_objects = render_chart(
+ values={
+ "cleanup": {
+ "enabled": True,
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_CLEANUP_ANNOTATION,
+ },
+ },
+ },
+ "scheduler": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_SCHEDULER_ANNOTATION,
+ },
+ },
+ },
+ "webserver": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_WEBSERVER_ANNOTATION,
+ },
+ },
+ },
+ "workers": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_WORKER_ANNOTATION,
+ },
+ },
+ },
+ "flower": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_FLOWER_ANNOTATION,
+ },
+ },
+ },
+ "statsd": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_STATSD_ANNOTATION,
+ },
+ },
+ },
+ "redis": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_REDIS_ANNOTATION,
+ },
+ },
+ },
+ "pgbouncer": {
+ "enabled": True,
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_PGBOUNCER_ANNOTATION,
+ },
+ },
+ },
+ "jobs": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_JOBS_ANNOTATION,
+ },
+ },
+ },
+ "executor": "CeleryExecutor", # create worker deployment
+ },
+ )
+
+ list_of_annotation_values_in_objects = [
+ k8s_object['metadata']['annotations']['example']
+ for k8s_object in k8s_objects
+ if k8s_object['kind'] == "ServiceAccount"
+ ]
+
+ self.assertCountEqual(
+ list_of_annotation_values_in_objects,
+ CUSTOM_ANNOTATION_VALUES,
+ )
Review comment:
@ianstanton [assertCountEqual](https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertCountEqual) actually does exactly what you've described π .
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-825812869
Hey @ianstanton, nice finds. The latest commit should address both items you mention above.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623445885
##########
File path: chart/values.yaml
##########
@@ -427,6 +453,20 @@ scheduler:
affinity: {}
tolerations: []
+# Airflow migration and create user job settings
+jobs:
Review comment:
Yea let's separate them, we also have a `cleanupJob` -- while we just use `cleanup` name for it --- it is a bit confusing if we just have `jobs`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623447730
##########
File path: chart/templates/statsd/statsd-serviceaccount.yaml
##########
@@ -0,0 +1,38 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+######################################
+## Airflow StatsD ServiceAccount
+######################################
+{{- if .Values.statsd.serviceAccount.create }}
Review comment:
```suggestion
{{- if and .Values.statsd.enabled .Values.statsd.serviceAccount.create }}
```
To make it consistent with `redis` and others
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623417179
##########
File path: chart/values.yaml
##########
@@ -427,6 +453,20 @@ scheduler:
affinity: {}
tolerations: []
+# Airflow migration and create user job settings
+jobs:
Review comment:
hmm.. π€ I am trying to think if in future we have more jobs that if name itself is self-descriptive it would be better
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830438908
@kaxil beat me to it before I had seen you had pushed up your fix π
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623416129
##########
File path: chart/templates/_helpers.yaml
##########
@@ -364,6 +383,105 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s-airflow-config" .Release.Name) }}
{{- end }}
+{{/*
+Create the name of the webserver service account to use
+*/}}
+{{- define "webserver.serviceAccountName" -}}
+{{- if .Values.webserver.serviceAccount.create -}}
+ {{ default (printf "%s-webserver" (include "airflow.fullname" .)) .Values.webserver.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.webserver.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the redis service account to use
+*/}}
+{{- define "redis.serviceAccountName" -}}
+{{- if .Values.redis.serviceAccount.create -}}
+ {{ default (printf "%s-redis" (include "airflow.fullname" .)) .Values.redis.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.redis.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the flower service account to use
+*/}}
+{{- define "flower.serviceAccountName" -}}
+{{- if .Values.flower.serviceAccount.create -}}
+ {{ default (printf "%s-flower" (include "airflow.fullname" .)) .Values.flower.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.flower.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the scheduler service account to use
+*/}}
+{{- define "scheduler.serviceAccountName" -}}
+{{- if .Values.scheduler.serviceAccount.create -}}
+ {{ default (printf "%s-scheduler" (include "airflow.fullname" .)) .Values.scheduler.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.scheduler.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the statsd service account to use
+*/}}
+{{- define "statsd.serviceAccountName" -}}
+{{- if .Values.statsd.serviceAccount.create -}}
+ {{ default (printf "%s-statsd" (include "airflow.fullname" .)) .Values.statsd.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.statsd.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the jobs service account to use
+*/}}
+{{- define "jobs.serviceAccountName" -}}
+{{- if .Values.jobs.serviceAccount.create -}}
+ {{ default (printf "%s-jobs" (include "airflow.fullname" .)) .Values.jobs.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.jobs.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
Review comment:
```suggestion
{{- define "userCreateJob.serviceAccountName" -}}
{{- if .Values.userCreateJob.serviceAccount.create -}}
{{ default (printf "%s-jobs" (include "airflow.fullname" .)) .Values.userCreateJob.serviceAccount.name }}
{{- else -}}
{{ default "default" .Values.userCreateJob.serviceAccount.name }}
{{- end -}}
{{- end -}}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573457512
##########
File path: chart/templates/_helpers.yaml
##########
@@ -361,6 +387,105 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s-airflow-config" .Release.Name) }}
{{- end }}
+{{/*
+Create the name of the webserver service account to use
+*/}}
+{{- define "webserver.serviceAccountName" -}}
+{{- if .Values.webserver.serviceAccount.create -}}
+ {{ default (printf "%s-webserver" (include "airflow.fullname" .)) .Values.webserver.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.webserver.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
Review comment:
is it possible to create all of these svc accts with a loop, to avoid some repetition?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624194368
##########
File path: chart/tests/test_basic_helm_chart.py
##########
@@ -36,13 +36,19 @@ def test_basic_deployments(self):
'metadata': 'AA',
},
'labels': {"TEST-LABEL": "TEST-VALUE"},
+ "fullnameOverride": "TEST-BASIC",
Review comment:
It is not a hard requirement, just allows for every service account to "match" the exact form of the other items in the `list_of_kind_names_tuples`. Without it set, each service account would change to `TEST-BASIC-airflow-<service>` based on how the [templating of the accounts](https://github.com/apache/airflow/pull/14152/files#diff-dc12e5cc8016c85ad9e662af7c8b8cfb49d91965225e94aa12146dc1f371fc86R421) is setup. I'm totally fine with removing it and changing the tuple entries.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623453566
##########
File path: chart/tests/test_rbac.py
##########
@@ -0,0 +1,294 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+CLEANUP_DEPLOYMENT_KIND_NAME_TUPLES = [
Review comment:
Curious about the name of this variable, why `CLEANUP_`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624182625
##########
File path: chart/tests/test_basic_helm_chart.py
##########
@@ -36,13 +36,19 @@ def test_basic_deployments(self):
'metadata': 'AA',
},
'labels': {"TEST-LABEL": "TEST-VALUE"},
+ "fullnameOverride": "TEST-BASIC",
Review comment:
Why do we need to pass `fullnameOverride` ? What will be the value if we don't pass it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624034074
##########
File path: chart/tests/test_rbac.py
##########
@@ -0,0 +1,294 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+CLEANUP_DEPLOYMENT_KIND_NAME_TUPLES = [
Review comment:
This was actually from @jaydesl's original PR where the test included the enabling of the cleanup job. Since it now also includes the `pgBouncer` deployment it admittedly doesn't carry the exact same weight as before.
I could change it to `CLEANUP_PGBOUNCER_DEPLOYMENT_KIND_NAME_TUPLES`? Or maybe even `CLEANUP_PGBOUNCER_ENABLED_DEPLOYMENT_KIND_NAME_TUPLES`? Bit wordy but likely a bit more clear. Curious on your thoughts
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r619682130
##########
File path: chart/templates/cleanup/cleanup-serviceaccount.yaml
##########
@@ -18,11 +18,11 @@
################################
## Airflow Cleanup ServiceAccount
#################################
-{{- if and .Values.rbacEnabled .Values.cleanup.enabled }}
+{{- if and .Values.cleanup.serviceAccount.create .Values.cleanup.enabled }}
kind: ServiceAccount
apiVersion: v1
metadata:
- name: {{ .Release.Name }}-cleanup
+ name: {{ include "cleanup.serviceAccountName" . }}
labels:
Review comment:
We should support annotation for the service account. It is needed for Workload Identity https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-818314692
> @ashb, @mik-laj, @kaxil This has been rebased with master again. Let me know if there's anything that needs to be addressed.
Thanks, I will take a look in coming days, thanks
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-808557088
@ashb, did you have anything else that needs to be addressed for this PR?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623262956
##########
File path: chart/tests/test_annotations.py
##########
@@ -0,0 +1,129 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+from tests.helm_template_generator import render_chart
+
+# Values for each service mapped to the 'example'
+# key annotation
+CUSTOM_ANNOTATION_VALUES = (
+ CUSTOM_SCHEDULER_ANNOTATION,
+ CUSTOM_WEBSERVER_ANNOTATION,
+ CUSTOM_WORKER_ANNOTATION,
+ CUSTOM_CLEANUP_ANNOTATION,
+ CUSTOM_FLOWER_ANNOTATION,
+ CUSTOM_PGBOUNCER_ANNOTATION,
+ CUSTOM_STATSD_ANNOTATION,
+ CUSTOM_JOBS_ANNOTATION,
+ CUSTOM_REDIS_ANNOTATION,
+) = (
+ "scheduler",
+ "webserver",
+ "worker",
+ "cleanup",
+ "flower",
+ "PGBouncer",
+ "statsd",
+ "jobs",
+ "redis",
+)
+
+
+class AnnotationsTest(unittest.TestCase):
+ def test_service_account_annotations(self):
+ k8s_objects = render_chart(
+ values={
+ "cleanup": {
+ "enabled": True,
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_CLEANUP_ANNOTATION,
+ },
+ },
+ },
+ "scheduler": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_SCHEDULER_ANNOTATION,
+ },
+ },
+ },
+ "webserver": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_WEBSERVER_ANNOTATION,
+ },
+ },
+ },
+ "workers": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_WORKER_ANNOTATION,
+ },
+ },
+ },
+ "flower": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_FLOWER_ANNOTATION,
+ },
+ },
+ },
+ "statsd": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_STATSD_ANNOTATION,
+ },
+ },
+ },
+ "redis": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_REDIS_ANNOTATION,
+ },
+ },
+ },
+ "pgbouncer": {
+ "enabled": True,
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_PGBOUNCER_ANNOTATION,
+ },
+ },
+ },
+ "jobs": {
+ "serviceAccount": {
+ "annotations": {
+ "example": CUSTOM_JOBS_ANNOTATION,
+ },
+ },
+ },
+ "executor": "CeleryExecutor", # create worker deployment
+ },
+ )
+
+ list_of_annotation_values_in_objects = [
+ k8s_object['metadata']['annotations']['example']
+ for k8s_object in k8s_objects
+ if k8s_object['kind'] == "ServiceAccount"
+ ]
+
+ self.assertCountEqual(
+ list_of_annotation_values_in_objects,
+ CUSTOM_ANNOTATION_VALUES,
+ )
Review comment:
@ianstanton [assertCountEqual](https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertCountEqual) actually does exactly what you've described.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830443162
[The Workflow run](https://github.com/apache/airflow/actions/runs/800933038) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830429391
Unit test is failing: https://github.com/apache/airflow/pull/14152/checks?check_run_id=2479497469#step:6:3089
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624273293
##########
File path: docs/helm-chart/parameters-ref.rst
##########
@@ -474,6 +537,33 @@ The following tables lists the configurable parameters of the Airflow chart and
* - ``cleanup.tolerations``
- Toleration labels for pod assignment
- ``1``
+ * - ``cleanup.serviceAccount.create``
+ - Create ServiceAccount for cleanup pods
+ - ``true``
+ * - ``cleanup.serviceAccount.name``
+ - Name of ServiceAccount. If not set and create is true, a name is generated using the release name.
+ - ``~``
+ * - ``cleanup.serviceAccount.annotations``
+ - Annotations to add to cleanup cronjob kubernetes service account
+ - ``{}``
+ * - ``createUserJob.serviceAccount.create``
+ - Create ServiceAccount for create user job
+ - ``true``
+ * - ``createUserJob.serviceAccount.name``
+ - Name of ServiceAccount. If not set and create is true, a name is generated using the release name.
+ - ``~``
+ * - ``createUserJob.serviceAccount.annotations``
+ - Annotations to add to createUserJob kubernetes service account
+ - ``{}``
+ * - ``migrateDatabaseJob.serviceAccount.create``
+ - Create ServiceAccount for migrate database job
+ - ``true``
+ * - ``migrateDatabaseJob.serviceAccount.name``
+ - Name of ServiceAccount. If not set and create is true, a name is generated using the release name.
+ - ``~``
+ * - ``migrateDatabaseJob.serviceAccount.annotations``
+ - Annotations to add to migrateDatabaseJob kubernetes service account
Review comment:
```suggestion
- Annotations to add to ``migrateDatabaseJob`` kubernetes service account
```
##########
File path: docs/helm-chart/parameters-ref.rst
##########
@@ -474,6 +537,33 @@ The following tables lists the configurable parameters of the Airflow chart and
* - ``cleanup.tolerations``
- Toleration labels for pod assignment
- ``1``
+ * - ``cleanup.serviceAccount.create``
+ - Create ServiceAccount for cleanup pods
+ - ``true``
+ * - ``cleanup.serviceAccount.name``
+ - Name of ServiceAccount. If not set and create is true, a name is generated using the release name.
+ - ``~``
+ * - ``cleanup.serviceAccount.annotations``
+ - Annotations to add to cleanup cronjob kubernetes service account
+ - ``{}``
+ * - ``createUserJob.serviceAccount.create``
+ - Create ServiceAccount for create user job
+ - ``true``
+ * - ``createUserJob.serviceAccount.name``
+ - Name of ServiceAccount. If not set and create is true, a name is generated using the release name.
+ - ``~``
+ * - ``createUserJob.serviceAccount.annotations``
+ - Annotations to add to createUserJob kubernetes service account
Review comment:
```suggestion
- Annotations to add to ``createUserJob`` kubernetes service account
```
##########
File path: docs/helm-chart/parameters-ref.rst
##########
@@ -420,6 +465,15 @@ The following tables lists the configurable parameters of the Airflow chart and
* - ``pgbouncer.configSecretName``
- Name of existing PgBouncer config secret
- ``~``
+ * - ``pgbouncer.serviceAccount.create``
+ - Create ServiceAccount for pgbouncer
+ - ``true``
+ * - ``pgbouncer.serviceAccount.name``
+ - Name of ServiceAccount. If not set and create is true, a name is generated using the release name.
+ - ``~``
+ * - ``pgbouncer.serviceAccount.annotations``
+ - Annotations to add to pgbouncer kubernetes service account
Review comment:
```suggestion
- Annotations to add to PgBouncer kubernetes service account
```
##########
File path: docs/helm-chart/parameters-ref.rst
##########
@@ -420,6 +465,15 @@ The following tables lists the configurable parameters of the Airflow chart and
* - ``pgbouncer.configSecretName``
- Name of existing PgBouncer config secret
- ``~``
+ * - ``pgbouncer.serviceAccount.create``
+ - Create ServiceAccount for pgbouncer
Review comment:
```suggestion
- Create ServiceAccount for PgBouncer
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ianstanton commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
ianstanton commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-827513118
@DerekHeldtWerle Do we have tests for the service account annotations?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623333603
##########
File path: chart/values.yaml
##########
@@ -427,6 +453,20 @@ scheduler:
affinity: {}
tolerations: []
+# Airflow migration and create user job settings
+jobs:
Review comment:
We need to explicitly call out which job it is, so probably `createUserJob` would be more apt.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573883993
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
I'll get them added to the README π
When you issue a [helm create](https://helm.sh/docs/helm/helm_create/) command, the additions above (and [others](https://helm.sh/docs/helm/helm_create/)) are added to the `_helpers.yaml` file to help follow the best practices set forward by the helm team. Any explanation on the need for the two separate options can be found [here](https://stackoverflow.com/a/63839389)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624205887
##########
File path: chart/tests/test_basic_helm_chart.py
##########
@@ -36,13 +36,19 @@ def test_basic_deployments(self):
'metadata': 'AA',
},
'labels': {"TEST-LABEL": "TEST-VALUE"},
+ "fullnameOverride": "TEST-BASIC",
Review comment:
No, that's fine :) we can keep it as it is
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624180212
##########
File path: chart/templates/_helpers.yaml
##########
@@ -372,6 +391,127 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s-airflow-config" .Release.Name) }}
{{- end }}
+{{/*
+Create the name of the webserver service account to use
+*/}}
+{{- define "webserver.serviceAccountName" -}}
+{{- if .Values.webserver.serviceAccount.create -}}
+ {{ default (printf "%s-webserver" (include "airflow.fullname" .)) .Values.webserver.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.webserver.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the redis service account to use
+*/}}
+{{- define "redis.serviceAccountName" -}}
+{{- if .Values.redis.serviceAccount.create -}}
+ {{ default (printf "%s-redis" (include "airflow.fullname" .)) .Values.redis.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.redis.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the flower service account to use
+*/}}
+{{- define "flower.serviceAccountName" -}}
+{{- if .Values.flower.serviceAccount.create -}}
+ {{ default (printf "%s-flower" (include "airflow.fullname" .)) .Values.flower.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.flower.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the scheduler service account to use
+*/}}
+{{- define "scheduler.serviceAccountName" -}}
+{{- if .Values.scheduler.serviceAccount.create -}}
+ {{ default (printf "%s-scheduler" (include "airflow.fullname" .)) .Values.scheduler.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.scheduler.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the statsd service account to use
+*/}}
+{{- define "statsd.serviceAccountName" -}}
+{{- if .Values.statsd.serviceAccount.create -}}
+ {{ default (printf "%s-statsd" (include "airflow.fullname" .)) .Values.statsd.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.statsd.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the create user job service account to use
+*/}}
+{{- define "createUserJob.serviceAccountName" -}}
+{{- if .Values.createUserJob.serviceAccount.create -}}
+ {{ default (printf "%s-create-user-job" (include "airflow.fullname" .)) .Values.createUserJob.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.createUserJob.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the migrate database job service account to use
+*/}}
+{{- define "migrateDatabaseJob.serviceAccountName" -}}
+{{- if .Values.migrateDatabaseJob.serviceAccount.create -}}
+ {{ default (printf "%s-migrate-database-job" (include "airflow.fullname" .)) .Values.migrateDatabaseJob.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.migrateDatabaseJob.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create the name of the jobs service account to use
+*/}}
+{{- define "jobs.serviceAccountName" -}}
+{{- if .Values.jobs.serviceAccount.create -}}
+ {{ default (printf "%s-jobs" (include "airflow.fullname" .)) .Values.jobs.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.jobs.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
Review comment:
This is not needed anymore, right?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-818314692
> @ashb, @mik-laj, @kaxil This has been rebased with master again. Let me know if there's anything that needs to be addressed.
I will take a look in coming days, thanks
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624200327
##########
File path: chart/tests/test_rbac.py
##########
@@ -0,0 +1,294 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+CLEANUP_DEPLOYMENT_KIND_NAME_TUPLES = [
Review comment:
Only this comment is pending now :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573883993
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
I'll get them added to the README π
When you issue a [helm create](https://helm.sh/docs/helm/helm_create/) command, the additions above (and [others](https://helm.sh/docs/helm/helm_create/)) are added to the `_helpers.yaml` file to help follow the best practices set forward by the helm team. An explanation on the need for the two separate options can be found [here](https://stackoverflow.com/a/63839389)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573883993
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
I'll get them added to the README π
When you issue a [helm create](https://helm.sh/docs/helm/helm_create/) command, the additions above (and [others](https://helm.sh/docs/helm/helm_create/)) are added to the `_helpers.yaml` file to help follow the best practices set forward by the helm team. An explanation on the need for the two separate options can be found [here](https://stackoverflow.com/a/63839389).
With that in mind, I think future looking a different PR that adds the common labels and selector labels generated by the `helm create` command would also be worthwhile to push out to all of the manifests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-818354262
[The Workflow run](https://github.com/apache/airflow/actions/runs/743066973) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573929464
##########
File path: chart/templates/_helpers.yaml
##########
@@ -361,6 +387,105 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s-airflow-config" .Release.Name) }}
{{- end }}
+{{/*
+Create the name of the webserver service account to use
+*/}}
+{{- define "webserver.serviceAccountName" -}}
+{{- if .Values.webserver.serviceAccount.create -}}
+ {{ default (printf "%s-webserver" (include "airflow.fullname" .)) .Values.webserver.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.webserver.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
Review comment:
That's admittedly something I haven't seen before in other charts that create multiple service accounts, but that's not to say it isn't possible. I can look around, but I'm not too sure if its supported or not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-812850784
@kaxil @ashb Can I ask for a second look?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-827966507
Hey @ianstanton, I've added some tests specific to annotations that can be used down the line as the central hub to help address https://github.com/apache/airflow/issues/13643.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830433250
> Unit test is failing: https://github.com/apache/airflow/pull/14152/checks?check_run_id=2479497469#step:6:3089
>
> could not find template templates/create-user-job.yaml in chart\n'
Pushed a fix: https://github.com/apache/airflow/pull/14152/commits/9ca3d213cc13c1b20a17df2eab65e35ce0b42269
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-827973673
[The Workflow run](https://github.com/apache/airflow/actions/runs/790667259) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573883993
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
I'll get them added to the README π
When you issue a [helm create](https://helm.sh/docs/helm/helm_create/) command, the additions above (and [others](https://helm.sh/docs/helm/helm_create/)) are added to the `_helpers.yaml` file to help follow the best practices set forward by the helm team. An explanation on the need for the two separate options can be found [here](https://stackoverflow.com/a/63839389). Since we aren't leveraging the other items created by the `helm create` command (namely the common labels and selector labels) I can remove the `airflow.name` definition as its currently unused.
I think future looking a different PR that adds the common labels, selector labels, and the `airflow.name` to set the aforementioned labels generated by the `helm create` command would also be worthwhile to push out to all of the manifests
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
I'll get it added to the README π
When you issue a [helm create](https://helm.sh/docs/helm/helm_create/) command, the additions above (and [others](https://helm.sh/docs/helm/helm_create/)) are added to the `_helpers.yaml` file to help follow the best practices set forward by the helm team. An explanation on the need for the two separate options can be found [here](https://stackoverflow.com/a/63839389). Since we aren't leveraging the other items created by the `helm create` command (namely the common labels and selector labels) I can remove the `airflow.name` definition as its currently unused.
I think future looking a different PR that adds the common labels, selector labels, and the `airflow.name` to set the aforementioned labels generated by the `helm create` command would also be worthwhile to push out to all of the manifests
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r603693193
##########
File path: chart/templates/_helpers.yaml
##########
@@ -361,6 +387,105 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{ (printf "%s-airflow-config" .Release.Name) }}
{{- end }}
+{{/*
+Create the name of the webserver service account to use
+*/}}
+{{- define "webserver.serviceAccountName" -}}
+{{- if .Values.webserver.serviceAccount.create -}}
+ {{ default (printf "%s-webserver" (include "airflow.fullname" .)) .Values.webserver.serviceAccount.name }}
+{{- else -}}
+ {{ default "default" .Values.webserver.serviceAccount.name }}
+{{- end -}}
+{{- end -}}
Review comment:
Unfortunately, I was not able to find anything.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ianstanton commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
ianstanton commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r619368715
##########
File path: chart/templates/cleanup/cleanup-serviceaccount.yaml
##########
@@ -18,11 +18,11 @@
################################
## Airflow Cleanup ServiceAccount
#################################
-{{- if and .Values.rbacEnabled .Values.cleanup.enabled }}
+{{- if and .Values.cleanup.serviceAccount.create .Values.cleanup.enabled }}
kind: ServiceAccount
apiVersion: v1
metadata:
- name: {{ .Release.Name }}-cleanup
+ name: {{ include "cleanup.serviceAccountName" . }}
labels:
Review comment:
Is there a reason we don't have the option to add annotations in this service account?
##########
File path: docs/helm-chart/parameters-ref.rst
##########
@@ -480,9 +534,6 @@ The following tables lists the configurable parameters of the Airflow chart and
* - ``multiNamespaceMode``
- Whether the KubernetesExecutor can launch pods in multiple namespaces
- ``1``
- * - ``serviceAccountAnnottions.*``
- - Map of annotations for worker, webserver, scheduler kubernetes service accounts
- - ``{}``
Review comment:
@DerekHeldtWerle It looks like we need related documentation for `jobs`, `redis` and `cleanup` here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830414243
Static check is failing @DerekHeldtWerle
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-806048607
[The Workflow run](https://github.com/apache/airflow/actions/runs/684000958) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-776903873
[The Workflow run](https://github.com/apache/airflow/actions/runs/555468734) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573907920
##########
File path: chart/templates/rbac/pod-cleanup-role.yaml
##########
@@ -18,7 +18,7 @@
################################
## Airflow Cleanup Role
#################################
-{{- if and .Values.rbacEnabled .Values.cleanup.enabled }}
+{{- if and .Values.rbac.create .Values.cleanup.enabled }}
Review comment:
With the [new way](https://github.com/apache/airflow/pull/14152/files#diff-dc12e5cc8016c85ad9e662af7c8b8cfb49d91965225e94aa12146dc1f371fc86R472-R480) that this service account is generated, a user can technically set `rbac.create` to `true`, `.Values.cleanup.serviceAccont.create` to `false` and still use the namespaces [default](https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#use-multiple-service-accounts) service account. In that instance, the above role doesn't necessarily care if a service account for `cleanup` has been created as its going to use the `default` service account instead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] DerekHeldtWerle commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r624202957
##########
File path: chart/tests/test_basic_helm_chart.py
##########
@@ -36,13 +36,19 @@ def test_basic_deployments(self):
'metadata': 'AA',
},
'labels': {"TEST-LABEL": "TEST-VALUE"},
+ "fullnameOverride": "TEST-BASIC",
Review comment:
the change would look like the following if you want to remove the `fullnameOverride`:
```
--- a/chart/tests/test_basic_helm_chart.py
+++ b/chart/tests/test_basic_helm_chart.py
@@ -36,21 +36,20 @@ class TestBaseChartTest(unittest.TestCase):
'metadata': 'AA',
},
'labels': {"TEST-LABEL": "TEST-VALUE"},
- "fullnameOverride": "TEST-BASIC",
},
)
list_of_kind_names_tuples = [
(k8s_object['kind'], k8s_object['metadata']['name']) for k8s_object in k8s_objects
]
assert list_of_kind_names_tuples == [
- ('ServiceAccount', 'TEST-BASIC-flower'),
- ('ServiceAccount', 'TEST-BASIC-create-user-job'),
- ('ServiceAccount', 'TEST-BASIC-migrate-database-job'),
- ('ServiceAccount', 'TEST-BASIC-redis'),
- ('ServiceAccount', 'TEST-BASIC-scheduler'),
- ('ServiceAccount', 'TEST-BASIC-statsd'),
- ('ServiceAccount', 'TEST-BASIC-webserver'),
- ('ServiceAccount', 'TEST-BASIC-worker'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-flower'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-create-user-job'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-migrate-database-job'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-redis'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-scheduler'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-statsd'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-webserver'),
+ ('ServiceAccount', 'TEST-BASIC-airflow-worker'),
('Secret', 'TEST-BASIC-postgresql'),
('Secret', 'TEST-BASIC-airflow-metadata'),
('Secret', 'TEST-BASIC-airflow-result-backend'),
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #14152:
URL: https://github.com/apache/airflow/pull/14152
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r623449476
##########
File path: chart/tests/test_annotations.py
##########
@@ -0,0 +1,129 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+
+from tests.helm_template_generator import render_chart
+
+# Values for each service mapped to the 'example'
+# key annotation
+CUSTOM_ANNOTATION_VALUES = (
+ CUSTOM_SCHEDULER_ANNOTATION,
+ CUSTOM_WEBSERVER_ANNOTATION,
+ CUSTOM_WORKER_ANNOTATION,
+ CUSTOM_CLEANUP_ANNOTATION,
+ CUSTOM_FLOWER_ANNOTATION,
+ CUSTOM_PGBOUNCER_ANNOTATION,
+ CUSTOM_STATSD_ANNOTATION,
+ CUSTOM_JOBS_ANNOTATION,
+ CUSTOM_REDIS_ANNOTATION,
+) = (
+ "scheduler",
+ "webserver",
+ "worker",
+ "cleanup",
+ "flower",
+ "PGBouncer",
Review comment:
Is this a typo? shouldn't it be `pgbouncer`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14152: Helm RBAC Best Practices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#issuecomment-830447307
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org