[AIRFLOW-1755] Url prefix support for flower and webservice #2723
Conversation
gsemet
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2723 +/- ##
==========================================
+ Coverage 73.39% 73.39% +<.01%
==========================================
Files 160 160
Lines 12199 12208 +9
==========================================
+ Hits 8953 8960 +7
- Misses 3246 3248 +2
Continue to review full report at Codecov.
|
| @@ -31,14 +31,14 @@ <h3 style="float: left"> | |||
| {% block head %} | |||
| {{ super() }} | |||
| <link rel="stylesheet" type="text/css" href="{{ url_for("static", filename="dataTables.bootstrap.css") }}"> | |||
| <link href="/admin/static/vendor/select2/select2.css" rel="stylesheet"> | |||
| <link href="{{ url_for('admin.index')}}static/vendor/select2/select2.css" rel="stylesheet"> | |||
There was a problem hiding this comment.
Should this, (all many similar url_for calls in the templates) not be {{ url_for("static", filename="vendor/select2/select2.css") }}?
There was a problem hiding this comment.
probably, but can I open a ticket on JIRA for someone to take it later? I may not have time to do it on this pull request
There was a problem hiding this comment.
+1 to @ashb.. in the current form the code doesn't look good..
There was a problem hiding this comment.
It is actually harder than I though, I'll change the ones that are easy but some are tricky (ex: task_instances, what is the right url_for ? {{ url_for('admin.task_instances') }} does not work)
There was a problem hiding this comment.
When I try {{ url_for('admin.task_instances') }} Flask throws a helpful error:
werkzeug.routing.BuildError: Could not build url for endpoint 'task_instances'. Did you mean 'airflow.task_instances' instead?
And trying that we get the right URL: "/admin/airflow/object/task_instances"
| @@ -170,6 +170,9 @@ web_server_host = 0.0.0.0 | |||
| # The port on which to run the web server | |||
| web_server_port = 8080 | |||
|
|
|||
| # Root URL to use for the web server | |||
| web_server_url_prefix: / | |||
There was a problem hiding this comment.
keep the defaults empty.. also it should be = insted of :?
There was a problem hiding this comment.
It actually works with both : and =, but I put = to keep coherency
gsemet
force-pushed
the
url_prefix
branch
3 times, most recently
from
8f2c165
to
ea3d0f4
Compare
| @@ -29,7 +29,7 @@ | |||
| <a | |||
| class="btn" | |||
| style="border: none; background-color:{{ State.color(state)}}; color: {{ State.color_fg(state) }};" | |||
| href="/admin/taskinstance/?flt0_dag_id_equals={{ dag.dag_id }}&flt2_state_equals={{ state }}"> | |||
| href="{{ url_for('admin.index')}}taskinstance/?flt0_dag_id_equals={{ dag.dag_id }}&flt2_state_equals={{ state }}"> | |||
There was a problem hiding this comment.
This is much difficult than I though:
url_for('admin.taskinstance')}}does not work (template render exception)url_for('airflow.task_instances')}}works but the URL pointed is wrong (wants to go to/<urlprefix>/admin/airflow/object/taskinstance) and ends up in a 404
|
ho, thanks that will help a lot ! I was looking for |
gsemet
force-pushed
the
url_prefix
branch
2 times, most recently
from
44f16b7
to
517232c
Compare
|
PR updated after your recommendations. I have tested it on my home dev environment, which is not the labs where i needed these url_prefix changes. So, manual tests looks ok. I will stress it next week again when I go back to work |
gsemet
force-pushed
the
url_prefix
branch
2 times, most recently
from
28a83ac
to
90e63f2
Compare
gsemet
changed the title
|
-1 for the moment, I have encountered a bug when testing on our Kubernetes instance. Static files are not properly prefixed, while on my dev machine it works fine |
| app = Flask(__name__) | ||
| app.secret_key = configuration.get('webserver', 'SECRET_KEY') | ||
| app.config['LOGIN_DISABLED'] = not configuration.getboolean('webserver', 'AUTHENTICATE') | ||
| app = Flask(__name__, static_url_path=conf.get_url_prefix() + '/static') |
gsemet
changed the title
|
I have found the root cause of the static file url issue. The fact to use url_for for static files didn't worked because of bad initialization of the |
|
Hello. Here is the result of some addiotional tests on Kube 1.8:
Action: I would like to align both behavior
I will work on it asap. |
|
Hello. Fixed the experimental api url. i have also updated the documentation about reverse-proxying airflow |
|
As for writing unit tests, you can test out the webserver configuration by (1) configure |
|
I do not have any problem with any button. I however do all my test on Python 3, and this seems to be related to future package. |
|
@stibbons it works in PY3 perfectly fine, the error only shows up in PY2. The future library converts A kind of patchy way to work around this is instead of: return the native type: There may be a cleaner fix that i'm not aware of though. |
|
Something from the six package is the usual way of dealing with py2/py3 issues. @stibbons could you add a test case that exercises whatever code path was triggering the problem? |
|
I am on it, I do not see any problem here with Python 2.7, I'll add a unit test to cover the fact get_url_prefix returns a str in python 2.7.
|
|
I have found the issue and fixed it. It works in when the string is empty and when it is not, in python 2.7. So now should work in python 2.7, 3.5 and 3.6, at least. I have some difficulties to add unit tests, I'll try to do something this afternoon. |
gsemet
force-pushed
the
url_prefix
branch
3 times, most recently
from
ce7e712
to
eb8938b
Compare
|
I think the dask test issue has been fixed, so rebasing should help. |
Allows Airflow to support accessing easily the web UI from an endpoint such as `/airflow` and Flower with `/flower`. Adds 2 options in airflow.cfg: - web_server_url_prefix for the webservice - flower_url_prefix for Flower Allow mounting on any kind of HTTP endpoint, not only FQDN. Example in`airflow.cfg`: # Root URL to use for the web server web_server_url_prefix = /airflow ... # The root URL for Flower flower_url_prefix = /flower Updated documentation as well. Signed-off-by: Gaetan Semet <gaetan@xeberon.net>
|
I've rebased the pull request, still works fine. |
|
This change looks good to me. There is a weird add in the docs about Microsoft Azure which needs to be removed. Can you check that this change still works if someone messes up with trailing/slashes? If so I would recommend using Once those two issues are addressed, I am +1 on this. |
| @@ -1006,6 +1007,7 @@ def mark_success_url(self): | |||
| iso = self.execution_date.isoformat() | |||
| BASE_URL = configuration.get('webserver', 'BASE_URL') | |||
| return BASE_URL + ( | |||
| configuration.get_url_prefix() + | |||
There was a problem hiding this comment.
Should this be something using posixpath.join in case of trailing slashes ?
There was a problem hiding this comment.
Yes, it requires to aways write a backslash before, so this get url returns either nothing (no prefix) or a path that does not ends with a backslash.
Double slash on a file path is not really a problem, it is more on some web server that might be an issue. But I do not know if I can put a urljoin/ file join everywhere, especially in JavaScript file
| @@ -84,7 +84,7 @@ def timing(cls, stat, dt): | |||
| _/_/ |_/_/ /_/ /_/ /_/ \____/____/|__/ | |||
| """ | |||
|
|
|||
| BASE_LOG_URL = '/admin/airflow/log' | |||
| BASE_LOG_URL = conf.get_url_prefix() + '/admin/airflow/log' | |||
There was a problem hiding this comment.
Same here. Consider using posixpath.join
There was a problem hiding this comment.
I usually use os.path.join, is it better to use posixpath.join ?
There was a problem hiding this comment.
While we do not guarantee compatibility with windows per se, os.path.join is os dependent. posixpath.join would be consistent.
|
As with the other PR: Why is Secondly, I am not a fan of adding another config option for this, as I don't think it is required. Why not use the base_url and use a split? Furthermore please add some tests that cover this. |
|
I'll have a alternative PR soon, that will use MiddleWare as per documentation of wsgi. |
|
Hello. Will this WSGI configuration works on kubernetes and other system that does not use Apache/Nginx? In my instance, airflow is behind a Traefik reverse proxy, both running on our internal kubernetes cluster. |
This enables Airflow and Celery Flower to live below root. Draws on the work of Geatan Semet (@stibbons). This closes apache#2723 and closes apache#2818
This enables Airflow and Celery Flower to live below root. Draws on the work of Geatan Semet (@stibbons). This closes #2723 and closes #2818 Closes #2952 from bolkedebruin/AIRFLOW-1755
This enables Airflow and Celery Flower to live below root. Draws on the work of Geatan Semet (@stibbons). This closes apache#2723 and closes apache#2818 Closes apache#2952 from bolkedebruin/AIRFLOW-1755
Dear Airflow maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
JIRA
Description
I would like to submit this change that allows Airflow to support accessing easily the web UI from an endpoint such as
/airflowand Flower with/flower.For Flower, it is already possible to use the
flowerconfig.pyto convey theurl_prefixsetting, but I prefered aligning itI propose this change that add 2 new settings on
airflow.cfgand the CLI arguments:Tests
I am open to suggestion on how to add unit tests on this PR. Tests will be done on a preproduction cluster running Kubernetes.
Commits