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

Allow to configure Flower's url_prefix #12630

Merged
merged 1 commit into from
Jan 5, 2021
Merged

Conversation

ecerulm
Copy link
Contributor

@ecerulm ecerulm commented Nov 25, 2020

When deploying with the Helm chart using an ingress for flower that has a path (ingress.flower.path) flower is currently unusable.
Flower by default assumes that it will be serving url starting on / and it must be told via url_prefix if it's actual url is not based directly on /.
Currently there is no way (that I know of) to pass url_prefix to the Flower deployment/pod.

This PR introduces a new helm chart value flower.url_prefix to allow for that.


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

@junnplus
Copy link
Contributor

junnplus commented Jan 4, 2021

What's the progress for this pr? I also need flower.url_prefix for flower ui

@ashb
Copy link
Member

ashb commented Jan 4, 2021

There is already a config setting for this -- flower_url_prefix under the celery section -- we don't need a second method of configuring this do we?

Comment on lines 114 to 117
{{- if .Values.flower.url_prefix }}
- name: FLOWER_URL_PREFIX
value: {{.Values.flower.url_prefix }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Ah.

Instead of adding a new setting for this, we should

Suggested change
{{- if .Values.flower.url_prefix }}
- name: FLOWER_URL_PREFIX
value: {{.Values.flower.url_prefix }}
{{- end }}
{{- include "custom_airflow_environment" . | indent 10 }}

Then the existing environment variable would work. Right now without any changes this can be set in the airflow.cfg config map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've dropped all other changes and just added this include.

@ecerulm
Copy link
Contributor Author

ecerulm commented Jan 5, 2021

There is already a config setting for this -- flower_url_prefix under the celery section -- we don't need a second method of configuring this do we?

@ashb, I don't understand. I can't find any flower_url_prefix in the project. I assume that you mean the existing ingress.flower.path.
The problem is that if you expose flower in a path that is not just / then flower will refuse to work. You need to tell flower that it's being server under another /path/* in order to do that I use the FLOWER_URL_PREFIX environment variable.

The ideal would be to generate the FLOWER_URL_PREFIX from ingress.flower.path but it's not trivial in the general case (ingress.flower.path as it's has the default pathType: ImplementationSpecific ) so I added as a new chart value.

@ecerulm
Copy link
Contributor Author

ecerulm commented Jan 5, 2021

Ok, nevermind. Yes there is flower_url_prefix that can be set with AIRFLOW__CELERY__FLOWER_URL_PREFIX

@ashb ashb merged commit c2ead47 into apache:master Jan 5, 2021
kaxil pushed a commit that referenced this pull request Jan 21, 2021
@ecerulm ecerulm deleted the flower-url-prefix branch June 16, 2021 14:53
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants