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 2020/08/26 15:15:55 UTC
[GitHub] [airflow] FloChehab opened a new pull request #10584: feat(chart): support envFrom in the chart
FloChehab opened a new pull request #10584:
URL: https://github.com/apache/airflow/pull/10584
* add envFromConfigMap so that we can use a configMap for all env configuration
(moving it outside of helm for instance)
* add envFromSecret so that we can use a secret for further env configuration
(moving it outside of helm for instance et preventing secrets to be visible in the pod configuration)
* Airflow configuration for kubernetes executor is also updated accordingly
---
**^ 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] FloChehab edited a comment on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab edited a comment on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-712721095
> I don't know if we should adopt a more generic approach to `EnvFrom`.
> https://github.com/apache/airflow/pull/11426/files
> https://github.com/codecentric/helm-charts/tree/master/charts/keycloak#using-a-secret-managed-by-the-chart
> What do you think?
Hello,
The second link looks pretty nice to me and I was initially going for something like that if I remember well.
The issue was about the integration with the KubernetesExecutor.
In my emails, I can see another comment of yours linked to that (that I guess you have deleted since):
> Hi. We currently want to significantly simplify KubernetesExecutor/KubernetesPodOperators and delete many configuration options. https://lists.apache.org/thread.html/rb1bcafea1b8f86e6d155aea7169c05805f7021e90f1636afc8019074%40%3Cdev.airflow.apache.org%3E
For this reason, it seems to me that your change is not very useful. Can you make sure that the appropriate features options can be passed using pod_template_file?
https://github.com/apache/airflow/blob/master/chart/values.yaml#L636
So what would you recommend ?
* Refactoring to use something like https://github.com/codecentric/helm-charts/tree/master/charts/keycloak#using-a-secret-managed-by-the-chart and ignoring the kubernetes executor ?
* Doing the first point ; but doing the integration with the KubernetesExecutor another way ?
* Something else ?
----------------------------------------------------------------
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 #10584: feat(chart): support envFrom in the chart
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-680944501
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/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://apache-airflow-slack.herokuapp.com/
----------------------------------------------------------------
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] potiuk closed pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #10584:
URL: https://github.com/apache/airflow/pull/10584
----------------------------------------------------------------
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] FloChehab commented on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-682585084
Hi @dimberman, just tested with the kubernetes executor. Everything looks good to me ; the "automated" injection for Secrets / ConfigMaps works as expected. There is just one weird thing going on (not related to this PR I think): Secrets get included twice (only once, as execpected, for the CMs).
For instance if in the scheduler pod we have:
```yaml
env:
- name: AIRFLOW__KUBERNETES__ENV_FROM_SECRET_REF
value: mysecret,mysecret2
- name: AIRFLOW__KUBERNETES__ENV_FROM_CONFIGMAP_REF
value: test-airflow,test-airflow2
```
In the pod created by the executor, I have:
```yaml
envFrom:
- configMapRef:
name: test-airflow
- configMapRef:
name: test-airflow2
- secretRef:
name: mysecret
- secretRef:
name: mysecret2
- secretRef:
name: mysecret
- secretRef:
name: mysecret2
```
----------------------------------------------------------------
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] FloChehab commented on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-707161564
Not really stale, waiting for review.
----------------------------------------------------------------
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] FloChehab edited a comment on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab edited a comment on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-715530418
Hello @mik-laj / @dimberman , I have the following proposition to make (inspired by https://github.com/codecentric/helm-charts/tree/master/charts/keycloak#using-a-secret-managed-by-the-chart) (i'll start from scratch again if this proposition sounds good to you):
1. Creating ConfigMaps & secrets from the chart:
* adding the value `extraSecrets`:
```yaml
extraSecrets:
airflow-connections:
stringData:
AIRFLOW_CONN_GCP: '...'
AIRFLOW_CONN_INTERNAL_DB: '...'
other-secret-variables:
stringData:
SECRET_VAR_1: '...'
SECRET_VAR_2: '...'
```
Which would yield 2 secrets:
```yaml
apiVersion: v1
kind: Secret
type: Opaque
metadata:
name: airflow-connections
...
stringData:
AIRFLOW_CONN_GCP: '...'
AIRFLOW_CONN_INTERNAL_DB: '...'
---
apiVersion: v1
kind: Secret
type: Opaque
metadata:
name: other-secret-variables
...
stringData:
SECRET_VAR_1: '...'
SECRET_VAR_2: '...'
```
Those secrets would be mounted with envFrom.
* adding the value `extraConfigMaps`, with exactly the same idea.
* For the support of the kubernetes executor, I'll add the envFrom in pod template and nothing more (so no support for previous airflow versions and the old configuration)
This should be the best in terms of flexibility without getting too complex.
2. Adding the value `extraEnv` & `extraEnvFrom`, which would use the standard `env` & `envFrom` syntax (again not supporting the "old" KubernetesExecutor configuration).
Given all of this and if we decide to drop the support for the old KubernetesExecutor configuration, we could remove the `env` and `secret` values from this chart (and do a lot cleaning in the helpers).
----------------------------------------------------------------
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] FloChehab edited a comment on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab edited a comment on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-682585084
Hi @dimberman, I just tested with the kubernetes executor. Everything looks good to me ; the "automated" injection for Secrets / ConfigMaps works as expected. There is just one weird thing going on (not related to this PR I think): Secrets get included twice (only once, as execpected, for the CMs).
For instance if in the scheduler pod we have:
```yaml
env:
- name: AIRFLOW__KUBERNETES__ENV_FROM_SECRET_REF
value: mysecret,mysecret2
- name: AIRFLOW__KUBERNETES__ENV_FROM_CONFIGMAP_REF
value: test-airflow,test-airflow2
```
In the pod created by the executor, I have:
```yaml
envFrom:
- configMapRef:
name: test-airflow
- configMapRef:
name: test-airflow2
- secretRef:
name: mysecret
- secretRef:
name: mysecret2
- secretRef:
name: mysecret
- secretRef:
name: mysecret2
```
----------------------------------------------------------------
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] FloChehab edited a comment on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab edited a comment on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-723475213
Hello, I've created a new PR for the new solution I was describing: #12164
----------------------------------------------------------------
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] FloChehab commented on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-712721095
> I don't know if we should adopt a more generic approach to `EnvFrom`.
> https://github.com/apache/airflow/pull/11426/files
> https://github.com/codecentric/helm-charts/tree/master/charts/keycloak#using-a-secret-managed-by-the-chart
> What do you think?
Hello,
The second link look pretty nice to me and I was initially going for something like that if I remember well.
The issue was about the integration with the KubernetesExecutor.
In my emails, I can see another comment of yours linked to that (that I guess you have deleted since):
> Hi. We currently want to significantly simplify KubernetesExecutor/KubernetesPodOperators and delete many configuration options. https://lists.apache.org/thread.html/rb1bcafea1b8f86e6d155aea7169c05805f7021e90f1636afc8019074%40%3Cdev.airflow.apache.org%3E
For this reason, it seems to me that your change is not very useful. Can you make sure that the appropriate features options can be passed using pod_template_file?
https://github.com/apache/airflow/blob/master/chart/values.yaml#L636
So what would you recommend ?
* Refactoring to use something like https://github.com/codecentric/helm-charts/tree/master/charts/keycloak#using-a-secret-managed-by-the-chart and ignoring the kubernetes executor ?
* Doing the first point ; but doing the integration with the KubernetesExecutor another way ?
* Something else ?
----------------------------------------------------------------
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] FloChehab commented on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-715530418
Hello @mik-laj / @dimberman , I have the following proposition to make (inspired by https://github.com/codecentric/helm-charts/tree/master/charts/keycloak#using-a-secret-managed-by-the-chart) (i'll start from scratch again if this proposition sounds good to you):
1. Creating ConfigMaps & secrets from the chart:
* adding the value `extraSecrets`:
```yaml
secrets:
airflow-connections:
stringData:
AIRFLOW_CONN_GCP: '...'
AIRFLOW_CONN_INTERNAL_DB: '...'
other-secret-variables:
stringData:
SECRET_VAR_1: '...'
SECRET_VAR_2: '...'
```
Which would yield 2 secrets:
```yaml
apiVersion: v1
kind: Secret
type: Opaque
metadata:
name: airflow-connections
...
stringData:
AIRFLOW_CONN_GCP: '...'
AIRFLOW_CONN_INTERNAL_DB: '...'
---
apiVersion: v1
kind: Secret
type: Opaque
metadata:
name: other-secret-variables
...
stringData:
SECRET_VAR_1: '...'
SECRET_VAR_2: '...'
```
Those secrets would be mounted with envFrom.
* adding the value `extraConfigMaps`, with exactly the same idea.
* For the support of the kubernetes executor, I'll add the envFrom in pod template and nothing more (so no support for previous airflow versions and the old configuration)
This should be the best in terms of flexibility without getting too complex.
2. Adding the value `extraEnv` & `extraEnvFrom`, which would use the standard `env` & `envFrom` syntax (again not supporting the "old" KubernetesExecutor configuration).
Given all of this and if we decide to drop the support for the old KubernetesExecutor configuration, we could remove the `env` and `secret` values from this chart (and do a lot cleaning in the helpers).
----------------------------------------------------------------
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 removed a comment on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
mik-laj removed a comment on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-712541060
Hi. We currently want to significantly simplify KubernetesExecutor/KubernetesPodOperators and delete many configuration options. https://lists.apache.org/thread.html/rb1bcafea1b8f86e6d155aea7169c05805f7021e90f1636afc8019074%40%3Cdev.airflow.apache.org%3E
For this reason, it seems to me that your change is not very useful. Can you make sure that the appropriate features options can be passed using pod_template_file?
https://github.com/apache/airflow/blob/master/chart/values.yaml#L636
----------------------------------------------------------------
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] FloChehab commented on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-680952843
Also,
regarding:
> "Automated" airflow configuration for kubernetes executor in the chart also updated accordingly.
I am not using the KubernetesExecutor, nor am I familiar with its configuration. I've done my best based on the existing logic in the helm chart and airflow default config file; I hope it's ok.
----------------------------------------------------------------
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] FloChehab commented on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-723475213
Hello, I've create a new PR for the new solution I was describing: #12164
----------------------------------------------------------------
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] stale[bot] commented on pull request #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-707139602
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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 #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-712543186
I don't know if we should adopt a more generic approach to `EnvFrom`.
https://github.com/apache/airflow/pull/11426/files
https://github.com/codecentric/helm-charts/tree/master/charts/keycloak#using-a-secret-managed-by-the-chart
What do you think?
----------------------------------------------------------------
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 #10584: Add support for envFrom in helm chart
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10584:
URL: https://github.com/apache/airflow/pull/10584#issuecomment-712541060
Hi. We currently want to significantly simplify KubernetesExecutor/KubernetesPodOperators and delete many configuration options. https://lists.apache.org/thread.html/rb1bcafea1b8f86e6d155aea7169c05805f7021e90f1636afc8019074%40%3Cdev.airflow.apache.org%3E
For this reason, it seems to me that your change is not very useful. Can you make sure that the appropriate features options can be passed using pod_template_file?
https://github.com/apache/airflow/blob/master/chart/values.yaml#L636
----------------------------------------------------------------
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