You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/04/06 03:23:53 UTC
[GitHub] [airflow] mik-laj opened a new pull request #8157: Reorder
middleware - ProxyFix and BaseUrl
mik-laj opened a new pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157
ProxyFix overrides the value of `environ["SCRIPT_NAME"]` variable, so it must be the first middleware.
When I have setup proxy fix , setup base url (`internal-client`) and I send proxy headers (``"HTTP_X_FORWARDED_PREFIX": "/proxy-prefix"``). The application expects to be available at: ``https://valid:445/proxy-prefix/debug``
Now correctly builds the URL: ``https://valid:445/proxy-prefix/internal-client/debug``
---
Make sure to mark the boxes below before creating PR: [x]
- [X] Description above provides context of the change
- [X] Unit tests coverage for changes (not needed for documentation changes)
- [X] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [X] Relevant documentation is updated including usage instructions.
- [X] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #8157: Reorder middleware -
ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-610301671
So the problem was you couldn't have a proxy prefix set like this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8157:
Reorder middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r408733737
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
Review comment:
On line 31 I enable proxy fix. Original IP address is `192.168.0.2`, header IP is "192.168.0.1". We use header IP here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-613988145
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=h1) Report
> Merging [#8157](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0dafdd0b9d635b4513b1413007337b19c3d96b17&el=desc) will **decrease** coverage by `0.36%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8157 +/- ##
==========================================
- Coverage 88.31% 87.94% -0.37%
==========================================
Files 935 940 +5
Lines 45170 45354 +184
==========================================
- Hits 39892 39888 -4
- Misses 5278 5466 +188
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/www/app.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy93d3cvYXBwLnB5) | `92.40% <100.00%> (ø)` | |
| [...flow/providers/apache/cassandra/hooks/cassandra.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2Nhc3NhbmRyYS9ob29rcy9jYXNzYW5kcmEucHk=) | `21.25% <0.00%> (-72.50%)` | :arrow_down: |
| [...w/providers/apache/hive/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2hpdmUvb3BlcmF0b3JzL215c3FsX3RvX2hpdmUucHk=) | `35.84% <0.00%> (-64.16%)` | :arrow_down: |
| [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `47.82% <0.00%> (-52.18%)` | :arrow_down: |
| [airflow/providers/redis/operators/redis\_publish.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvb3BlcmF0b3JzL3JlZGlzX3B1Ymxpc2gucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [airflow/providers/mongo/sensors/mongo.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbW9uZ28vc2Vuc29ycy9tb25nby5weQ==) | `53.33% <0.00%> (-46.67%)` | :arrow_down: |
| [airflow/providers/mysql/operators/mysql.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbXlzcWwvb3BlcmF0b3JzL215c3FsLnB5) | `55.00% <0.00%> (-45.00%)` | :arrow_down: |
| [airflow/providers/redis/sensors/redis\_key.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvc2Vuc29ycy9yZWRpc19rZXkucHk=) | `61.53% <0.00%> (-38.47%)` | :arrow_down: |
| [airflow/executors/celery\_executor.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvY2VsZXJ5X2V4ZWN1dG9yLnB5) | `50.67% <0.00%> (-37.84%)` | :arrow_down: |
| [...roviders/google/cloud/operators/postgres\_to\_gcs.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL29wZXJhdG9ycy9wb3N0Z3Jlc190b19nY3MucHk=) | `54.28% <0.00%> (-31.43%)` | :arrow_down: |
| ... and [60 more](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=footer). Last update [0dafdd0...be8c5e5](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8157:
Reorder middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r404637898
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,186 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
Review comment:
These are ctor tests. They don't make much sense if we have real tests that check behavior, not just code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8157:
Reorder middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r404763476
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
+ # Should respect HTTP_X_FORWARDED_PROTO, HTTP_X_FORWARDED_HOST, HTTP_X_FORWARDED_PORT,
+ # HTTP_X_FORWARDED_PREFIX
+ self.assertEqual(request.url, 'https://valid:445/proxy-prefix/debug')
Review comment:
This works identically to base_url. From a technical point of view, these two middleware modify one variable.
https://github.com/pallets/werkzeug/blob/master/src/werkzeug/middleware/proxy_fix.py#L167
https://github.com/pallets/werkzeug/blob/master/src/werkzeug/middleware/dispatcher.py#L64
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #8157: Reorder middleware -
ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-613988145
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=h1) Report
> Merging [#8157](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0dafdd0b9d635b4513b1413007337b19c3d96b17&el=desc) will **decrease** coverage by `0.36%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8157 +/- ##
==========================================
- Coverage 88.31% 87.94% -0.37%
==========================================
Files 935 940 +5
Lines 45170 45354 +184
==========================================
- Hits 39892 39888 -4
- Misses 5278 5466 +188
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/www/app.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy93d3cvYXBwLnB5) | `92.40% <100.00%> (ø)` | |
| [...flow/providers/apache/cassandra/hooks/cassandra.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2Nhc3NhbmRyYS9ob29rcy9jYXNzYW5kcmEucHk=) | `21.25% <0.00%> (-72.50%)` | :arrow_down: |
| [...w/providers/apache/hive/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2hpdmUvb3BlcmF0b3JzL215c3FsX3RvX2hpdmUucHk=) | `35.84% <0.00%> (-64.16%)` | :arrow_down: |
| [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `47.82% <0.00%> (-52.18%)` | :arrow_down: |
| [airflow/providers/redis/operators/redis\_publish.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvb3BlcmF0b3JzL3JlZGlzX3B1Ymxpc2gucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [airflow/providers/mongo/sensors/mongo.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbW9uZ28vc2Vuc29ycy9tb25nby5weQ==) | `53.33% <0.00%> (-46.67%)` | :arrow_down: |
| [airflow/providers/mysql/operators/mysql.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbXlzcWwvb3BlcmF0b3JzL215c3FsLnB5) | `55.00% <0.00%> (-45.00%)` | :arrow_down: |
| [airflow/providers/redis/sensors/redis\_key.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvc2Vuc29ycy9yZWRpc19rZXkucHk=) | `61.53% <0.00%> (-38.47%)` | :arrow_down: |
| [airflow/executors/celery\_executor.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvY2VsZXJ5X2V4ZWN1dG9yLnB5) | `50.67% <0.00%> (-37.84%)` | :arrow_down: |
| [...roviders/google/cloud/operators/postgres\_to\_gcs.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL29wZXJhdG9ycy9wb3N0Z3Jlc190b19nY3MucHk=) | `54.28% <0.00%> (-31.43%)` | :arrow_down: |
| ... and [60 more](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=footer). Last update [0dafdd0...be8c5e5](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on a change in pull request #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r408861604
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
Review comment:
Oh yes, not _quite_ sure how I missed that. The :coffee: obviously hadn't kicked in yet.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on a change in pull request #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r404758180
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
+ # Should respect HTTP_X_FORWARDED_PROTO, HTTP_X_FORWARDED_HOST, HTTP_X_FORWARDED_PORT,
+ # HTTP_X_FORWARDED_PREFIX
+ self.assertEqual(request.url, 'https://valid:445/proxy-prefix/debug')
Review comment:
Does this work in practice without base_url set, or are all the links and stylesheets pulled in at the wrong URLs?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj merged pull request #8157: Reorder middleware -
ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-613988145
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=h1) Report
> Merging [#8157](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0dafdd0b9d635b4513b1413007337b19c3d96b17&el=desc) will **decrease** coverage by `0.36%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8157 +/- ##
==========================================
- Coverage 88.31% 87.94% -0.37%
==========================================
Files 935 940 +5
Lines 45170 45354 +184
==========================================
- Hits 39892 39888 -4
- Misses 5278 5466 +188
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/www/app.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy93d3cvYXBwLnB5) | `92.40% <100.00%> (ø)` | |
| [...flow/providers/apache/cassandra/hooks/cassandra.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2Nhc3NhbmRyYS9ob29rcy9jYXNzYW5kcmEucHk=) | `21.25% <0.00%> (-72.50%)` | :arrow_down: |
| [...w/providers/apache/hive/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2hpdmUvb3BlcmF0b3JzL215c3FsX3RvX2hpdmUucHk=) | `35.84% <0.00%> (-64.16%)` | :arrow_down: |
| [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `47.82% <0.00%> (-52.18%)` | :arrow_down: |
| [airflow/providers/redis/operators/redis\_publish.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvb3BlcmF0b3JzL3JlZGlzX3B1Ymxpc2gucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [airflow/providers/mongo/sensors/mongo.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbW9uZ28vc2Vuc29ycy9tb25nby5weQ==) | `53.33% <0.00%> (-46.67%)` | :arrow_down: |
| [airflow/providers/mysql/operators/mysql.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbXlzcWwvb3BlcmF0b3JzL215c3FsLnB5) | `55.00% <0.00%> (-45.00%)` | :arrow_down: |
| [airflow/providers/redis/sensors/redis\_key.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvc2Vuc29ycy9yZWRpc19rZXkucHk=) | `61.53% <0.00%> (-38.47%)` | :arrow_down: |
| [airflow/executors/celery\_executor.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvY2VsZXJ5X2V4ZWN1dG9yLnB5) | `50.67% <0.00%> (-37.84%)` | :arrow_down: |
| [...roviders/google/cloud/operators/postgres\_to\_gcs.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL29wZXJhdG9ycy9wb3N0Z3Jlc190b19nY3MucHk=) | `54.28% <0.00%> (-31.43%)` | :arrow_down: |
| ... and [60 more](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=footer). Last update [0dafdd0...be8c5e5](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #8157: Reorder middleware -
ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-610302094
How do proxy fix and the Dispatcher middleware interact? Do we need both still?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-613988145
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=h1) Report
> Merging [#8157](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0dafdd0b9d635b4513b1413007337b19c3d96b17&el=desc) will **decrease** coverage by `0.36%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8157 +/- ##
==========================================
- Coverage 88.31% 87.94% -0.37%
==========================================
Files 935 940 +5
Lines 45170 45354 +184
==========================================
- Hits 39892 39888 -4
- Misses 5278 5466 +188
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/www/app.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy93d3cvYXBwLnB5) | `92.40% <100.00%> (ø)` | |
| [...flow/providers/apache/cassandra/hooks/cassandra.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2Nhc3NhbmRyYS9ob29rcy9jYXNzYW5kcmEucHk=) | `21.25% <0.00%> (-72.50%)` | :arrow_down: |
| [...w/providers/apache/hive/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2hpdmUvb3BlcmF0b3JzL215c3FsX3RvX2hpdmUucHk=) | `35.84% <0.00%> (-64.16%)` | :arrow_down: |
| [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `47.82% <0.00%> (-52.18%)` | :arrow_down: |
| [airflow/providers/redis/operators/redis\_publish.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvb3BlcmF0b3JzL3JlZGlzX3B1Ymxpc2gucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [airflow/providers/mongo/sensors/mongo.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbW9uZ28vc2Vuc29ycy9tb25nby5weQ==) | `53.33% <0.00%> (-46.67%)` | :arrow_down: |
| [airflow/providers/mysql/operators/mysql.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbXlzcWwvb3BlcmF0b3JzL215c3FsLnB5) | `55.00% <0.00%> (-45.00%)` | :arrow_down: |
| [airflow/providers/redis/sensors/redis\_key.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvc2Vuc29ycy9yZWRpc19rZXkucHk=) | `61.53% <0.00%> (-38.47%)` | :arrow_down: |
| [airflow/executors/celery\_executor.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvY2VsZXJ5X2V4ZWN1dG9yLnB5) | `50.67% <0.00%> (-37.84%)` | :arrow_down: |
| [...roviders/google/cloud/operators/postgres\_to\_gcs.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL29wZXJhdG9ycy9wb3N0Z3Jlc190b19nY3MucHk=) | `54.28% <0.00%> (-31.43%)` | :arrow_down: |
| ... and [60 more](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=footer). Last update [0dafdd0...be8c5e5](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #8157: Reorder middleware -
ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-613924597
Let me test this out on our platform -- we have it setup behind a proxy and at a non-/ path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #8157: Reorder middleware -
ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-610352293
The problem is visible in this line.
https://github.com/apache/airflow/pull/8157/files#diff-a7826a115d2c9ae9009798807f329e8dR180
The following address was previously built.
```
self.assertEqual(request.url, "https://valid:445/internal-client/debug")
```
Information from proxy headers was lost.
Sometimes it needs several, because the proxy ensures that one instance can be accessed from several addresses
The instance can be accessed from two addresses. For example:
* `http://domain.com/team-a/`
* `http://domain.com/my-airflow/`
Whereas base_url makes Airflow simply available in a subdirectory.
* `http://domain.com/airflow/`
When we combine these two matters, we want Airflow to be available at the following addresses:
* `http://domain.com/team-a/airflow/`
* `http://domain.com/my-airflow/airflow/`
In this PR I wanted to add missing tests for middleware, but it turned out that these proxies are not working properly and we need to change their order.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on a change in pull request #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r408707950
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
Review comment:
How come this one is using the X-Forwarded-* headers? ProxyFix is off by default isn't it? so shouldn't those headers be ignored here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on a change in pull request #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r408705254
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
+ # Should respect HTTP_X_FORWARDED_PROTO, HTTP_X_FORWARDED_HOST, HTTP_X_FORWARDED_PORT,
+ # HTTP_X_FORWARDED_PREFIX
+ self.assertEqual(request.url, 'https://valid:445/proxy-prefix/debug')
+
+ return Response("success")
+
+ flask_app.view_functions['debug'] = debug_view
+
+ new_environ = {
+ "PATH_INFO": "/debug",
+ "REMOTE_ADDR": "192.168.0.2",
+ "HTTP_HOST": "invalid:9000",
+ "HTTP_X_FORWARDED_FOR": "192.168.0.1",
+ "HTTP_X_FORWARDED_PROTO": "https",
+ "HTTP_X_FORWARDED_HOST": "valid",
+ "HTTP_X_FORWARDED_PORT": "445",
+ "HTTP_X_FORWARDED_PREFIX": "/proxy-prefix",
+ }
+ environ = create_environ(environ_overrides=new_environ)
+
+ response = Response.from_app(app, environ)
+
+ self.assertEqual(b"success", response.get_data())
+ self.assertEqual(response.status_code, 200)
+
+ @conf_vars({
+ ('webserver', 'base_url'): 'http://localhost:8080/internal-client',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_base_url_ignore_proxy_headers(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.2')
+ # Should respect HTTP_X_FORWARDED_PROTO, HTTP_X_FORWARDED_HOST, HTTP_X_FORWARDED_PORT,
Review comment:
```suggestion
# Should ignore HTTP_X_FORWARDED_PROTO, HTTP_X_FORWARDED_HOST, HTTP_X_FORWARDED_PORT,
```
I think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #8157: Reorder middleware -
ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-613929990
Looks good - couple of questions about the tests though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8157: Reorder
middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#issuecomment-613988145
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=h1) Report
> Merging [#8157](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/0dafdd0b9d635b4513b1413007337b19c3d96b17&el=desc) will **decrease** coverage by `0.36%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8157 +/- ##
==========================================
- Coverage 88.31% 87.94% -0.37%
==========================================
Files 935 940 +5
Lines 45170 45354 +184
==========================================
- Hits 39892 39888 -4
- Misses 5278 5466 +188
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/www/app.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy93d3cvYXBwLnB5) | `92.40% <100.00%> (ø)` | |
| [...flow/providers/apache/cassandra/hooks/cassandra.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2Nhc3NhbmRyYS9ob29rcy9jYXNzYW5kcmEucHk=) | `21.25% <0.00%> (-72.50%)` | :arrow_down: |
| [...w/providers/apache/hive/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2hpdmUvb3BlcmF0b3JzL215c3FsX3RvX2hpdmUucHk=) | `35.84% <0.00%> (-64.16%)` | :arrow_down: |
| [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `47.82% <0.00%> (-52.18%)` | :arrow_down: |
| [airflow/providers/redis/operators/redis\_publish.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvb3BlcmF0b3JzL3JlZGlzX3B1Ymxpc2gucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [airflow/providers/mongo/sensors/mongo.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbW9uZ28vc2Vuc29ycy9tb25nby5weQ==) | `53.33% <0.00%> (-46.67%)` | :arrow_down: |
| [airflow/providers/mysql/operators/mysql.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbXlzcWwvb3BlcmF0b3JzL215c3FsLnB5) | `55.00% <0.00%> (-45.00%)` | :arrow_down: |
| [airflow/providers/redis/sensors/redis\_key.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvc2Vuc29ycy9yZWRpc19rZXkucHk=) | `61.53% <0.00%> (-38.47%)` | :arrow_down: |
| [airflow/executors/celery\_executor.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvY2VsZXJ5X2V4ZWN1dG9yLnB5) | `50.67% <0.00%> (-37.84%)` | :arrow_down: |
| [...roviders/google/cloud/operators/postgres\_to\_gcs.py](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL29wZXJhdG9ycy9wb3N0Z3Jlc190b19nY3MucHk=) | `54.28% <0.00%> (-31.43%)` | :arrow_down: |
| ... and [60 more](https://codecov.io/gh/apache/airflow/pull/8157/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=footer). Last update [0dafdd0...be8c5e5](https://codecov.io/gh/apache/airflow/pull/8157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8157:
Reorder middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r408733737
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
Review comment:
On line 31 I enabled proxy fix. Original IP address is `192.168.0.2`, header IP is "192.168.0.1". We use header IP here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8157:
Reorder middleware - ProxyFix and BaseUrl
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8157: Reorder middleware - ProxyFix and BaseUrl
URL: https://github.com/apache/airflow/pull/8157#discussion_r408733899
##########
File path: tests/www/test_app.py
##########
@@ -17,41 +17,185 @@
# under the License.
import unittest
+from unittest import mock
-from werkzeug.middleware.proxy_fix import ProxyFix
+from werkzeug.routing import Rule
+from werkzeug.test import create_environ
+from werkzeug.wrappers import Response
-from airflow.settings import Session
from airflow.www import app as application
from tests.test_utils.config import conf_vars
class TestApp(unittest.TestCase):
- @conf_vars({('webserver', 'enable_proxy_fix'): 'False'})
- def test_constructor_no_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertFalse(isinstance(app.wsgi_app, ProxyFix))
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True'})
- def test_constructor_proxyfix(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 1)
- self.assertEqual(app.wsgi_app.x_proto, 1)
- self.assertEqual(app.wsgi_app.x_host, 1)
- self.assertEqual(app.wsgi_app.x_port, 1)
- self.assertEqual(app.wsgi_app.x_prefix, 1)
-
- @conf_vars({('webserver', 'enable_proxy_fix'): 'True',
- ('webserver', 'proxy_fix_x_for'): '3',
- ('webserver', 'proxy_fix_x_proto'): '4',
- ('webserver', 'proxy_fix_x_host'): '5',
- ('webserver', 'proxy_fix_x_port'): '6',
- ('webserver', 'proxy_fix_x_prefix'): '7'})
- def test_constructor_proxyfix_args(self):
- app, _ = application.create_app(session=Session, testing=True)
- self.assertTrue(isinstance(app.wsgi_app, ProxyFix))
- self.assertEqual(app.wsgi_app.x_for, 3)
- self.assertEqual(app.wsgi_app.x_proto, 4)
- self.assertEqual(app.wsgi_app.x_host, 5)
- self.assertEqual(app.wsgi_app.x_port, 6)
- self.assertEqual(app.wsgi_app.x_prefix, 7)
+
+ @conf_vars({
+ ('webserver', 'enable_proxy_fix'): 'True',
+ ('webserver', 'proxy_fix_x_for'): '1',
+ ('webserver', 'proxy_fix_x_proto'): '1',
+ ('webserver', 'proxy_fix_x_host'): '1',
+ ('webserver', 'proxy_fix_x_port'): '1',
+ ('webserver', 'proxy_fix_x_prefix'): '1'
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_proxy_fix(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
+ self.assertEqual(request.remote_addr, '192.168.0.1')
+ # Should respect HTTP_X_FORWARDED_PROTO, HTTP_X_FORWARDED_HOST, HTTP_X_FORWARDED_PORT,
+ # HTTP_X_FORWARDED_PREFIX
+ self.assertEqual(request.url, 'https://valid:445/proxy-prefix/debug')
+
+ return Response("success")
+
+ flask_app.view_functions['debug'] = debug_view
+
+ new_environ = {
+ "PATH_INFO": "/debug",
+ "REMOTE_ADDR": "192.168.0.2",
+ "HTTP_HOST": "invalid:9000",
+ "HTTP_X_FORWARDED_FOR": "192.168.0.1",
+ "HTTP_X_FORWARDED_PROTO": "https",
+ "HTTP_X_FORWARDED_HOST": "valid",
+ "HTTP_X_FORWARDED_PORT": "445",
+ "HTTP_X_FORWARDED_PREFIX": "/proxy-prefix",
+ }
+ environ = create_environ(environ_overrides=new_environ)
+
+ response = Response.from_app(app, environ)
+
+ self.assertEqual(b"success", response.get_data())
+ self.assertEqual(response.status_code, 200)
+
+ @conf_vars({
+ ('webserver', 'base_url'): 'http://localhost:8080/internal-client',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ @mock.patch("airflow.www.app.appbuilder", None)
+ def test_should_respect_base_url_ignore_proxy_headers(self):
+ app = application.cached_app(testing=True)
+ flask_app = next(iter(app.mounts.values()))
+ flask_app.url_map.add(Rule("/debug", endpoint="debug"))
+
+ def debug_view():
+ from flask import request
+
+ # Should respect HTTP_X_FORWARDED_FOR
Review comment:
```suggestion
# Should ignore HTTP_X_FORWARDED_FOR
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services