-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
Fix to Werkzeug ProxyFix; expose ProxyFix configuration items #8117
Fix to Werkzeug ProxyFix; expose ProxyFix configuration items #8117
Conversation
superset/config.py
Outdated
@@ -111,6 +111,13 @@ | |||
|
|||
# Extract and use X-Forwarded-For/X-Forwarded-Proto headers? | |||
ENABLE_PROXY_FIX = False | |||
PROXY_FIX_CONFIG = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was validated to fix the issue with CSM, when ENABLE_PROXY_FIX
is set to True
.
FYI Airflow followup fix on: apache/airflow#5563 |
Thanks for putting in the work to fix this @ericandrewmeadows . Once this gets merged I think we can start picking cherries for a |
👍 - fair to enforce all on since the side effect is minimal. An aside: why don't we force line length to 79 max? I know it's an outdated standard set by punch cards...but most linters complain. |
@villebro - you're welcome. I just happened to come across the Airflow item after I solved it. I updated the docs - thanks again! |
LGTM, but I don't feel comfortable merging before @dpgaspar approves this, as he is more knowledgeable on this topic than I am. |
I figured - yeah - it may have just slipped under his radar. |
@dpgaspar - can this be merged? |
Few notes:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
Codecov Report
@@ Coverage Diff @@
## master #8117 +/- ##
=========================================
+ Coverage 66.06% 66.2% +0.13%
=========================================
Files 479 479
Lines 22930 22930
Branches 2524 2524
=========================================
+ Hits 15148 15180 +32
+ Misses 7648 7616 -32
Partials 134 134
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mistercrunch - Should be good to be merged.
…#8117) * Fix to werkzeug proxy; expose additional configuration items * Forced to all x-forwarded configurations ON; black done * added comments related to x_port after testing * Updated UPDATING.md * Removed accidental notebook; added *.ipynb to gitignore * Delete Untitled-checkpoint.ipynb
CATEGORY
Choose one
SUMMARY
Custom Security Manager appeared to be broken when behind an AWS ALB, but when I dug further, the Werkzeug version was bumped. This caused an invalid configuration for the proxy in that the host was not passed on. Because of this, along with the change of the library path, and the inclusion of the additional library bump in accordance with apache/airflow#5571, I have validated that the regression that prevented CSM from working in v0.33.0rc1 -> v0.34.0rc [1 & 2] has been resolved.
TEST PLAN
I deployed internally the fix, and validated that the port is not being returned in the header for the destination after hitting the root IP of the Superset instance.
ADDITIONAL INFORMATION
REVIEWERS