Skip to content

Conversation

@ecerulm
Copy link
Contributor

@ecerulm ecerulm commented Nov 25, 2020

Closes #12632


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Nov 25, 2020
@ecerulm
Copy link
Contributor Author

ecerulm commented Nov 26, 2020

@mik-laj since you are reviewing #12619 , I guess it make sense if you review #12630 and #12634 as all of them are about getting ingresses to work properly.

@ecerulm
Copy link
Contributor Author

ecerulm commented Nov 27, 2020

I've been thinking about the whole ingress situation. Even after this and #12619 are merged, in order to get a working airflow ingress with a custom hostname and path in AWS EKS the following will be required

  • ingress.web.annotations Revert "Hide ToC from the Apache Airflow doc main page (#12589)" #12607
  • ingress.web.host: www.example.com
  • ingress.web.path: /airflow/*
  • config.web.server.base_url: https://www.example.com/airflow
  • webserver.livenessProbe.path: /airflow/health
  • webserver.livenessProbe.host: www.example.com
  • webserver.readinessProbe.path: /airflow/health
  • webserver.readinessProbe.host: www.example.com

So there is lot of repetition and matching required, the hostname appears in base_url, ingress.web.host and webserver.*.host.
The path prefix /airflow appear in ingress.web.path and webserver.*.path.

I'm thinking to create yet another PR after the others are merged that takes ingress.web.host and ingress.web.path and fills values for config.web.server.base_url and webserver.*.{path,host}if they are not explicitly provided. Is that something that you would be interested on having?

@ecerulm
Copy link
Contributor Author

ecerulm commented Jan 5, 2021

@ashb , since you are reviewing #12630 maybe you should also take a look to this one, since both are related ultimately to the helm chart ingresses

chart/README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever need different values here -- they are controlled by the same airflow.cfg so these two values should always be the same, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the values will be the same, and they need to be "in sync" also with .webserver.base_url in airflow.cfg. So now I changed so that it extract the host and path from the webserver.base_url if provided.

@ecerulm ecerulm force-pushed the probes-host-header branch from d1da283 to 6e282fc Compare January 7, 2021 13:25
@ecerulm ecerulm requested a review from ashb January 7, 2021 14:29
Copy link
Member

@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.

Looks good -- but could use the pytest "bare" assert style please:

assert .... is None or assert {"name": ...} in ... etc.

@ecerulm
Copy link
Contributor Author

ecerulm commented Jan 7, 2021

No problem, changed the self.assert* into assert xxx

@ecerulm ecerulm requested a review from ashb January 7, 2021 15:38
@github-actions
Copy link

github-actions bot commented Jan 8, 2021

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.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jan 8, 2021
@ecerulm ecerulm force-pushed the probes-host-header branch from 9e64bb9 to 6ca4fc3 Compare January 11, 2021 08:17
@potiuk potiuk merged commit 75fd5f8 into apache:master Jan 12, 2021
@ecerulm ecerulm deleted the probes-host-header branch January 12, 2021 12:14
kaxil pushed a commit that referenced this pull request Jan 21, 2021
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 okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

helm chart: webserver replies with 404 "Apache Airflow is not at this location" when using config.webserver.base_url

3 participants