adds x_header configuration option for use behind proxies #299
Conversation
|
I set it up as to not change current behavior (through a feature flag) but it probably makes sense to set it to true by default and maybe even forget the flag. |
|
This looks like a very specific solution. I wonder if we could introduce something more generic instead. Notebook has a |
|
Looking at Notebook now on your suggestion and it looks like they already have the xheaders flag. The tornado_settings option won't work because it's used for the Application and not the httpServer. It seems notebook actually has an xheaders config: It looks like the tornado settings are used for the app and not the server: |
|
I see. Thanks for checking :-) I'd appreciate if someone else also has a look at this before merging. |
There was a problem hiding this comment.
@vlaurenzano - thank you for the contribution - this will be useful. I had a few comments, mostly along the lines of modeling this after the Notebook functionality.
Since we're in the process of building the 2.1.0 release, it would be great to get this in soon. I'd like to start cutting the release Monday or Tuesday next week. (8/13-14)
kernel_gateway/gatewayapp.py
Outdated
| @@ -174,6 +174,16 @@ def allow_origin_default(self): | |||
| def expose_headers_default(self): | |||
| return os.getenv(self.expose_headers_env, '') | |||
|
|
|||
| x_headers_env = 'KG_X_HEADERS' | |||
| x_headers = Unicode(config=True, | |||
There was a problem hiding this comment.
Since it sounds like this is providing the identical functionality to that provided in Notebook, then I suggest we use the same option name (and variable): trust_xheaders. This fits the boolean datatype better as well, since x_headers sounds more like a noun.
I also suggest using the Bool traitlet rather than Unicode. This should eliminate the need for strtobool and import.
There was a problem hiding this comment.
The reason I used unicode and not Bool is because it is coming from an env variable and it didn't say in the documentation whether it would cast from a string or how. There is also a CBool, for casting bool, however it seems like it only works for integers. I'll look into it and update it with Bool or CBool if appropriate.
Agree about the name, I didn't know it existed in notebook prior to implementing.
There was a problem hiding this comment.
Sounds good. Once the updates are in, we'll merge. Thanks.
There was a problem hiding this comment.
I updated the x_headers name to trust_xheaders. I ended up using both CBool and strtobool. Strtbool is necessary when the config is set as an environment variable. Since CBool only uses the python bool() function to cast, any non empy string value would make a positive value, such as 'false', 'no'... etc
kernel_gateway/gatewayapp.py
Outdated
|
|
||
| @default('x_headers') | ||
| def x_headers_default(self): | ||
| return os.getenv(self.x_headers_env, 'false') |
There was a problem hiding this comment.
It may be a good idea to handle various formats for the env value, so something like this may work more reliably...
return bool(os.getenv(self.x_headers_env, 'false').lower() == 'true')
kernel_gateway/gatewayapp.py
Outdated
| @@ -505,6 +515,7 @@ def init_http_server(self): | |||
| """ | |||
| ssl_options = self._build_ssl_options() | |||
| self.http_server = httpserver.HTTPServer(self.web_app, | |||
| xheaders=bool(strtobool(self.x_headers)), | |||
There was a problem hiding this comment.
Per above, I think we can eliminate the casts (and import) by using the traitlets - as Notebook did.
|
Squashed and merged - thanks again! |
This passes the xheaders configuration option to tornado. The option informs tornado to use the forwarded X-Real-Ip header if given. Useful for when the kernelgateway is behind a proxy and you need the original ip (for logging for instance).