Make queue-proxy's reverse proxy handler extendable #16097
Conversation
This builds on knative#13133 to make it possible to adjust settings on the *httputil.ReverseProxy that queue-proxy uses, or to replace it entirely with any http.Handler, using the out-of-tree extension pattern.
|
Hi @mbaynton. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
dprotaso
left a comment
There was a problem hiding this comment.
Interesting use of the extension point - left a comment on order of operations
pkg/queue/sharedmain/main.go
Outdated
| d.ProxyHandler = buildProxyHandler(logger, env, d.Transport) | ||
|
|
||
| // allow extensions to read d and return modified context, transport, and proxy handler. | ||
| for _, opts := range opts { |
There was a problem hiding this comment.
There's one subtle regression. Prior if the integrator set d.Transport it would be used when building the proxy. This isn't the case anymore.
I think what you want is to set d.ProxyHandler to buildProxyHandler after iterating through the options - but only if ProxyHandler remains nil
There was a problem hiding this comment.
Indeed, good catch.
I think it's nicer if Options functions receive an incoming non-nil ProxyHandler -- this allows both fine-tuning options of the httputil.ReverseProxy and taking whatever handler knative wants to provide and wrapping it. But this does create an ordering problem since we configure a Transport onto the httputil.ReverseProxy that we provide to Options, and also Options can override the Transport.
What do you think about the new commit, which always resets the httputil.ReverseProxy's transport to the one set by the Options functions after iterating them?
There was a problem hiding this comment.
What do you think about the new commit, which always resets the httputil.ReverseProxy's transport to the one set by the Options functions after iterating them?
I think that works. Thanks for the change.
There was a problem hiding this comment.
Awesome. Very helpful for us to have this in, thank you for checking it out!
There was a problem hiding this comment.
No worries - consider adding yourself as an adopter here - https://github.com/knative/community/issues/new?template=ADOPTERS.yaml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16097 +/- ##
==========================================
- Coverage 80.15% 80.05% -0.11%
==========================================
Files 214 214
Lines 16877 16916 +39
==========================================
+ Hits 13528 13542 +14
- Misses 2987 3017 +30
+ Partials 362 357 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
If an integrator customizes the Transport in an Options function, we want to apply the provided Transport to our default httputil.ReverseProxy regardless of which Option sets the Transport and regardless of whether or not other Option functions replace our default http.Handler. Add some tests for this since the interactions beteween Options.Transport and Options.ProxyHandler can be subtle.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, mbaynton The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This builds on #13133 to make it possible to adjust settings on the *httputil.ReverseProxy that queue-proxy uses, or to replace it entirely with any http.Handler, using the out-of-tree extension pattern introduced by that PR.
In general this allows additions to the queue-proxy main server's request handler chain, which greatly expands what can be done via the out-of-tree extension model started in 13133. Many potential use cases for this form of out-of-tree extending exist, most probably not as involved as the below. For example someone might want to do their own customization to the
httputil.ReverseProxy, or add handlers that perform custom header rewrites. In case you are curious, I fully explain my specific use case that motivated the change at the bottom.Proposed Changes
http.Handlerto theDefaultsstruct that queue proxyOptionfunctions can manipulateRelease Note
My specific use case
We operate a service that allows users to publish and run their web application code. Because it leads to much better publishing performance than image building/pushing/pulling, our architecture allows the end user applications to be built (and updated, during dev/test cycles) in the same container that serves the application. As a result, we can experience states such as:
We have been handling this by running our own small http server behind queue-proxy to serve things like loading splash pages and error pages when the user's application is unavailable. To reduce total number of proxy hops, this server and the user application take turns listening on the main container port, rather than having our server be another reverse proxy. But we obviously do mishandle some requests with this method as there are transition states where no server is listening.
With these changes, I am able to keep the number of proxy hops the same by inheriting all the functionality that the rest of knative requires from queue-proxy, but also customize it a little bit to do the loading splash page and error page serving when I know based on external state that attempting proxying will not work.