-
Notifications
You must be signed in to change notification settings - Fork 131
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
lib: Use client ssl config to access error target #254
lib: Use client ssl config to access error target #254
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
2d548ac
to
7a99ec1
Compare
I'm not much of JS guy, so there's probably better ways to do some of this. |
@chancez could you describe the logic change made so i can have an easier time reviewing this PR? I'm not also great with this tech stack. |
First, error-target is the URL the CHP will request if there is an error processing a request, like if a route doesn't exist. Basically, for everything besides the error target, the CHP was using the node-http-proxy proxy object to handle sending requests to the various backends for each route. However, for the error-target, it was using it's own logic. This logic used a default HTTP client, and basically took the headers and body of the response and returned those as the response for the current request. This is problematic, because the HTTP client, using it's defaults, obviously cannot validate self-signed certificates. Instead of using it's our own HTTP client to make requests to the error target, it uses the proxy object from node-http-proxy to do the request, just like it does for any other routes. The proxy object is configured with the CAs to validate self-signed certificate, so it fixes the issue. In addition, it makes the handling of errors the same as handling other requests for these purposes. This simplifies the logic of returning the response as well, since it's not error prone to copying the data/headers from the upstream response. |
This sounds sensible to me @chancez, I fail to resolve the CI errors or understand this in depth enough to make this PR go through at this point. Should we request help? |
Yeah, I'm not sure. I've been running this in production for a few months, so anything to get it merged would be great. |
a637870
to
98caf88
Compare
Fixes usage of self-signed certs with JupyterHub with CHP. Original issue here: jupyterhub/zero-to-jupyterhub-k8s#1742 (comment)
98caf88
to
6a84567
Compare
Thanks for your patience on this one! |
Fixes usage of self-signed certs with Jupyterhub with CHP.
Original issue here:
jupyterhub/zero-to-jupyterhub-k8s#1742 (comment)