-
Notifications
You must be signed in to change notification settings - Fork 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
Add UNIX socket support to notebook server. #4835
Conversation
ac28ad6
to
43611a8
Compare
0cd0176
to
71b2132
Compare
@minrk might you be able to help with a review/merging of this change? as a datapoint, we've been running all of our notebook servers at Twitter on this exact patch since August with no issues. |
@kwlzn Could you please update the pr with latest master. |
@lresende I'd be happy to do that as long as there's some form of stated commitment from the maintainers of this project to actually review and make an attempt to land the PR this time around vs ignoring it. let me know who would be willing to shepherd this on the |
* gnu/packages/python-xyz.scm (python-notebook): Add patch from upstream jupyter/notebook#4835
* gnu/packages/python-xyz.scm (python-notebook): Add patch from upstream jupyter/notebook#4835
This is very cool. This makes it very easy to layer security and not need a network stack; i.e.: start the notebook in a restrictive network namespace without access to the network and proxy the domain socket using nginx/openresty to easily add OIDC on top of it. |
* gnu/packages/python-xyz.scm (python-notebook): Add patch from upstream jupyter/notebook#4835 (python-requests-unixsocket) New variable
would love to get this upstreamed soon, let me know if any maintainers are willing to help if I update the PR. |
@kwlzn, I can review and help push this PR through. Apologies for the delay—the state of the classic notebook is in limbo right now (see this issue and the discussion in last week's Jupyter Server meeting). We're working on better communication here for the state+future of the notebook. In the meantime, you can ping me here for review on this PR as you update on master (I can help along the way too—just let me know what you need). This is likely something we'll want to port over the Jupyter Server as soon as possible. We'd love to have you submit this work over there too, but I'm happy to cherry-pick the commits myself, if you're short on time. Thank you, again, for all this great work! |
@kwlzn, we are all trying to get this merged, and that's why you are seeing the third person asking for help rebasing this without success. |
71b2132
to
8aad324
Compare
@Zsailer @kevin-bates thanks. PR is rebased and tests fine locally - I'll give it another self-review pass tomorrow after CI runs, but should be good for an initial look from one of you. |
@Zsailer also happy to port this to Jupyter Server, will take a look at that next. |
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.
Given the response on the issue this is resolving, this sounds like a useful feature - thank you. I just had some comments surrounding parameter co-existence (sock/port/browser), usability, and shutdown right now.
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.
Thanks Kris - just had a few more comments.
@kevin-bates this should be ready for another review. |
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.
Thank you for the updates. This looks good to me. I'd love to get another review prior to merge if possible.
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.
LGTM, thanks!
Great work, @kwlzn! Thanks for your patience here! |
thanks folks, appreciate it! @Zsailer now that this is finalized, I'll circle back to port to Jupyter Server when I can. |
Awesome! Thanks, @kwlzn! |
This change permits the notebook server to bind to a configurable UNIX socket for cases where UNIX filesystem permissions may be useful to help guard access to the notebook server instance (e.g. such as a shared shell machine). This is mainly useful when paired with SSH tunneling, which directly supports binding TCP ports to UNIX sockets via either of these two option forms:
running the notebook server against a test socket:
vs client:
testing lifecycle (which roundtrips a shutdown request through the server):
Closes #2503