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
[talisman] Enforcing HTTP for status checks #8214
[talisman] Enforcing HTTP for status checks #8214
Conversation
if app.config["TALISMAN_ENABLED"]: | ||
talisman_config = app.config.get("TALISMAN_CONFIG") | ||
Talisman(app, **talisman_config) | ||
talisman.init_app(app, **app.config["TALISMAN_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.
We don't need to use get(...)
here since the config key exists in config.py.
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.
Makes sense. Also, this code would have broken before if there was no config anyway because we were doing **
on a None
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.
@etr2460 correct.
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.
a couple questions, otherwise lgtm
if app.config["TALISMAN_ENABLED"]: | ||
talisman_config = app.config.get("TALISMAN_CONFIG") | ||
Talisman(app, **talisman_config) | ||
talisman.init_app(app, **app.config["TALISMAN_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.
Makes sense. Also, this code would have broken before if there was no config anyway because we were doing **
on a None
@@ -222,9 +222,11 @@ def is_feature_enabled(feature): | |||
if conf.get("ENABLE_FLASK_COMPRESS"): | |||
Compress(app) | |||
|
|||
|
|||
talisman = Talisman() |
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.
is there a cost to always initializing this, even if the config var isn't set?
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.
@@ -629,16 +630,19 @@ class DashboardAddView(DashboardModelView): # noqa | |||
appbuilder.add_view_no_menu(DashboardAddView) | |||
|
|||
|
|||
@talisman(force_https=False) |
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.
Assuming these are needed so that some internal health check passes with TLS being handled by a LB or reverse proxy... 👍
(cherry picked from commit 762edf4)
CATEGORY
Choose one
SUMMARY
The
/health
and/ping
checks probably should use HTTP when Flask-Talisman is enabled.TEST PLAN
CI.
ADDITIONAL INFORMATION
REVIEWERS
to: @etr2460 @mistercrunch