Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make API auth_backend compatible to 1.10.x and 2.0 #394

Merged
merged 2 commits into from Dec 8, 2020
Merged

Conversation

kaxil
Copy link
Contributor

@kaxil kaxil commented Dec 6, 2020

This commit does the following:

  • uses airflow.api.auth.backend.basic_auth for Airflow >= 2.0
  • uses airflow.api.auth.backend.default for Airflow <2
  • allow overriding API auth backend by setting AIRFLOW__API__AUTH_BACKEND in Dockerfile or .env

I have tested this with the following Docker files:

FROM quay.io/astronomer/ap-airflow-dev:2.0.0-buster-onbuild-26413
ENV AIRFLOW__WEBSERVER__EXPOSE_CONFIG=True

image

FROM quay.io/astronomer/ap-airflow:1.10.12-2-buster-onbuild
ENV AIRFLOW__WEBSERVER__EXPOSE_CONFIG=True

image

FROM astronomerio/ap-airflow:1.10.12-buster-onbuild-23518
ENV AIRFLOW__WEBSERVER__EXPOSE_CONFIG=True

and by overriding the value

FROM quay.io/astronomer/ap-airflow-dev:2.0.0-buster-onbuild-26413

ENV AIRFLOW__WEBSERVER__EXPOSE_CONFIG=True
ENV AIRFLOW__API__AUTH_BACKEND="airflow.api.auth.backend.default"

This commit does the following:

- uses `airflow.api.auth.backend.basic_auth` for Airflow >= 2.0
- uses `airflow.api.auth.backend.default` for Airflow <2
- allow overriding API auth backend by setting `AIRFLOW__API__AUTH_BACKEND` in Dockerfile or `.env`
@codecov
Copy link

codecov bot commented Dec 6, 2020

Codecov Report

Merging #394 (d7f0227) into main (d42cfb7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #394   +/-   ##
=======================================
  Coverage   54.51%   54.51%           
=======================================
  Files          32       32           
  Lines        2313     2313           
=======================================
  Hits         1261     1261           
  Misses        963      963           
  Partials       89       89           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d42cfb7...d7f0227. Read the comment docs.

@@ -62,7 +62,12 @@ services:
webserver:
image: {{ .AirflowImage }}
command: >
bash -c '{ airflow create_user "$$@" || airflow users create "$$@"; } && { airflow sync_perm || airflow sync-perm; } && airflow webserver' -- -r Admin -u admin -e admin@example.com -f admin -l user -p admin
bash -c 'if [[ -z "$$AIRFLOW__API__AUTH_BACKEND" ]] && [[ $$(airflow version | tail -1) =~ "2."[0-9]+\.[0.9]+.* ]];
Copy link
Contributor Author

@kaxil kaxil Dec 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tail -1 is needed to cope up with the case like the following:

❯ docker exec --user root -it d9744db5a0fc bash
root@d9744db5a0fc:/usr/local/airflow# airflow version
[2020-12-06 00:44:44,070] {plugins_manager.py:111} ERROR - Failed to import plugin astronomer_version_check
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/airflow/plugins_manager.py", line 105, in load_entrypoint_plugins
    plugin_obj = entry_point.load()
  File "/usr/local/lib/python3.7/site-packages/pkg_resources/__init__.py", line 2446, in load
    self.require(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/pkg_resources/__init__.py", line 2469, in require
    items = working_set.resolve(reqs, env, installer, extras=self.extras)
  File "/usr/local/lib/python3.7/site-packages/pkg_resources/__init__.py", line 775, in resolve
    raise VersionConflict(dist, req).with_context(dependent_req)
pkg_resources.ContextualVersionConflict: (marshmallow 2.21.0 (/usr/local/lib/python3.7/site-packages), Requirement.parse('marshmallow>=3.0.0'), {'marshmallow-sqlalchemy'})
1.10.12.dev9+astro.2

Copy link

@jhtimmins jhtimmins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks Kaxil.

@kaxil
Copy link
Contributor Author

kaxil commented Dec 6, 2020

Ping @andriisoldatenko @ashb Will be need this for next 0.23 release

Instead of getting Airflow version and matching regex, we just check if `basic_auth.py` file exists or not.
@kaxil
Copy link
Contributor Author

kaxil commented Dec 8, 2020

@ashb Updated the PR to check for file existence. WDYT now?

Copy link
Contributor

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too happy with that blob-o-bash, but I don't see a better option, not without massively reworking something.

@kaxil kaxil merged commit f67cd49 into main Dec 8, 2020
@kaxil kaxil deleted the fix-auth-backend branch December 8, 2020 23:55
kaxil added a commit that referenced this pull request Dec 8, 2020
This commit does the following:

- uses `airflow.api.auth.backend.basic_auth` for Airflow >= 2.0
- uses `airflow.api.auth.backend.default` for Airflow <2
- allow overriding API auth backend by setting `AIRFLOW__API__AUTH_BACKEND` in Dockerfile or `.env`

(cherry picked from commit f67cd49)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants