Skip to content

Enable setting the base url of the website#9778

Closed
vsimon wants to merge 1 commit into
apache:masterfrom
vsimon:baseurl
Closed

Enable setting the base url of the website#9778
vsimon wants to merge 1 commit into
apache:masterfrom
vsimon:baseurl

Conversation

@vsimon

@vsimon vsimon commented Jul 12, 2020

Copy link
Copy Markdown
Contributor

Take into account the path of the base url in liveness and readiness
check probes.

This introduces baseUrl as a value to use for the chart's health
checks and also to use to set the AIRFLOW__WEBSERVER__BASE_URL
environment variable.

Fixes #9777

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

Take into account the path of the base url in liveness and readiness
check probes.

This introduces `baseUrl` as a value to use for the chart's health
checks and also to use to set the AIRFLOW__WEBSERVER__BASE_URL
environment variable.
@boring-cyborg boring-cyborg Bot added the area:helm-chart Airflow Helm Chart label Jul 12, 2020
@potiuk potiuk requested a review from dimberman July 12, 2020 09:21
@potiuk

potiuk commented Jul 12, 2020

Copy link
Copy Markdown
Member

LGTM But would love @dimberman to take a look

@dimberman

Copy link
Copy Markdown
Contributor

Why would the liveness and readiness probes need a base URL? Aren't those directly performed within the pod already?

@vsimon

vsimon commented Jul 13, 2020

Copy link
Copy Markdown
Contributor Author

I think this is in the case where Airflow is run behind a reverse proxy.

https://airflow.readthedocs.io/en/stable/howto/run-behind-proxy.html

As in an example, if the proxy is configured to serve airflow from /airflow in it's configuration, then the base_url gives the ability to set the correct endpoint.

What I notice without it is, when /airflow/home or /airflow/graph or /airflow/health is accessed, internally airflow tries to redirect and fetch /home or /graph or /health which are 404s.

@vsimon

vsimon commented Jul 13, 2020

Copy link
Copy Markdown
Contributor Author

I notice without a base_url set, the healthchecks /health endpoint returns 200 OK but accessing airflow through the reverse proxy fails.
Then with a base_url set (to say https://domain.com/airflow/), accessing airflow through the reverse proxy works, but the healthchecks fail (/health returns 404, /airflow/health returns 200 OK)

@dimberman

Copy link
Copy Markdown
Contributor

@vsimon I guess I'm a bit confused here, wouldn't that be testing the reverse proxy instead of airflow? Why would you need to access airflow via the reverse proxy for a health check?

@mik-laj

mik-laj commented Jul 14, 2020

Copy link
Copy Markdown
Member

@dimberman The base_url option adds a prefix to each address (including health check), so this change looks good. This option is implemented as WSGI middleware, so it affects all addresses. This option is not related to reverse proxy, but is often used together. We have a similar option - proxy_fix and it is closely related to the option and has a similar purpose, but a different way of working.
Please look here:
https://github.com/apache/airflow/blob/master/tests/www/test_app.py

@aneesh-joseph

aneesh-joseph commented Jul 18, 2020

Copy link
Copy Markdown
Contributor

@dimberman The base_url option adds a prefix to each address (including health check), so this change looks good. This option is implemented as WSGI middleware, so it affects all addresses. This option is not related to reverse proxy, but is often used together. We have a similar option - proxy_fix and it is closely related to the option and has a similar purpose, but a different way of working.
Please look here:
https://github.com/apache/airflow/blob/master/tests/www/test_app.py

So localhost:airflowport/health wont work if base_url is set? If that's the case may be the webserver should be tweaked to make sure that the healthcheck url is excluded from this prefixing? I think it doesn't make sense to set the probes on the reverse proxy url. Probes are a way for kubernetes to check if that specific pod-container is healthy, the reverse proxied health url doesn't give that information

@stale

stale Bot commented Sep 2, 2020

Copy link
Copy Markdown

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.

@stale stale Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 2, 2020
@stale stale Bot closed this Sep 11, 2020
@gowtamchandrahasa

Copy link
Copy Markdown

@vsimon , I'm facing this exact issue currently. Since this PR is not merged, is there some way to handle this issue for now? TIA!

@vsimon

vsimon commented Sep 26, 2021

Copy link
Copy Markdown
Contributor Author

hi @gowtamchandrahasa The way I handled it was within the helm chart. I happen to be using chart version: 7.16.0.

Assuming I have a dns name (the hostname of the reverse proxy) and a path (the path configured in the reverse proxy to map to the airflow web serevice)

$DNS_NAME
$PATH_NAME

I set these two fields:

      web:
        baseUrl: https://$DNS_NAME/$PATH_NAME

      ingress:
        web:
          livenessPath: /$PATH_NAME/health

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helm chart webserver pod does not start when setting the webserver base url

6 participants