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 2022/02/17 11:30:31 UTC
[GitHub] [airflow] norm opened a new pull request #21640: Change the default auth backend to session
norm opened a new pull request #21640:
URL: https://github.com/apache/airflow/pull/21640
As part of AIP-42, change the default auth backend to validate using the session, so that the UI can use the API. If `auth_backends` has been set to a non-default value, include the session in the list of backends.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r812241114
##########
File path: chart/values.yaml
##########
@@ -1382,9 +1382,6 @@ config:
# For Airflow 1.10, backward compatibility; moved to [logging] in 2.0
colored_console_log: 'False'
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
- # Authentication backend used for the experimental API
- api:
- auth_backends: airflow.api.auth.backend.deny_all
Review comment:
🤦♂️ oops, already done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r810538825
##########
File path: chart/values.yaml
##########
@@ -1384,7 +1384,7 @@ config:
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
# Authentication backend used for the experimental API
api:
- auth_backends: airflow.api.auth.backend.deny_all
+ auth_backends: airflow.api.auth.backend.session
Review comment:
Yeah, lets just remove this in the chart values entirely
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] norm commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
norm commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r811778695
##########
File path: chart/values.yaml
##########
@@ -1384,7 +1384,7 @@ config:
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
# Authentication backend used for the experimental API
api:
- auth_backends: airflow.api.auth.backend.deny_all
+ auth_backends: airflow.api.auth.backend.session
Review comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #21640:
URL: https://github.com/apache/airflow/pull/21640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r812248440
##########
File path: docs/apache-airflow/security/api.rst
##########
@@ -22,12 +22,12 @@ API Authentication
------------------
Authentication for the API is handled separately to the Web Authentication. The default is to
-deny all requests:
+check the user session:
.. code-block:: ini
[api]
- auth_backends = airflow.api.auth.backend.deny_all
+ auth_backends = airflow.api.auth.backend.session
Review comment:
Should we add a note to the versionchanged 2.3 section below that it now uses `session` vs `deny_all`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r810577276
##########
File path: airflow/configuration.py
##########
@@ -350,10 +350,11 @@ def _using_old_value(self, old, current_value):
return old.search(current_value) is not None
def _update_env_var(self, section, name, new_value):
- # Make sure the env var option is removed, otherwise it
- # would be read and used instead of the value we set
env_var = self._env_var_name(section, name)
- os.environ.pop(env_var, None)
+ # If the config comes from environment, set it there so that any subprocesses keep the same override!
+ if os.environ.get(env_var):
Review comment:
```suggestion
if env_var in os.environ:
```
This matters because it is possible to set an environment variable to an empty string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r812239205
##########
File path: chart/values.yaml
##########
@@ -1382,9 +1382,6 @@ config:
# For Airflow 1.10, backward compatibility; moved to [logging] in 2.0
colored_console_log: 'False'
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
- # Authentication backend used for the experimental API
- api:
- auth_backends: airflow.api.auth.backend.deny_all
Review comment:
This looks good, though we should probably add something in `chart/UPDATING.md` about it no longer being set.
##########
File path: docs/helm-chart/airflow-configuration.rst
##########
@@ -46,7 +46,7 @@ application. See the bottom line of the example:
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
# Authentication backend used for the experimental API
api:
Review comment:
```suggestion
```
##########
File path: docs/helm-chart/airflow-configuration.rst
##########
@@ -46,7 +46,7 @@ application. See the bottom line of the example:
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
# Authentication backend used for the experimental API
api:
- auth_backends: airflow.api.auth.backend.deny_all
+ auth_backends: airflow.api.auth.backend.session
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r813031421
##########
File path: docs/helm-chart/airflow-configuration.rst
##########
@@ -46,7 +46,7 @@ application. See the bottom line of the example:
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
# Authentication backend used for the experimental API
api:
- auth_backends: airflow.api.auth.backend.deny_all
+ auth_backends: airflow.api.auth.backend.session
Review comment:
I removed the noise from this example already in another 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#issuecomment-1048098548
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #21640: Change the default auth backend to session
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #21640:
URL: https://github.com/apache/airflow/pull/21640#discussion_r809586118
##########
File path: chart/values.yaml
##########
@@ -1384,7 +1384,7 @@ config:
remote_logging: '{{- ternary "True" "False" .Values.elasticsearch.enabled }}'
# Authentication backend used for the experimental API
api:
- auth_backends: airflow.api.auth.backend.deny_all
+ auth_backends: airflow.api.auth.backend.session
Review comment:
This will need to be conditional based on the airflow version.
We might also consider just removing this completely and let the default handle it - the default changed back in 1.10.11, I think it'd be okay as long as we add it to the charts UPDATING.md too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org