-
Notifications
You must be signed in to change notification settings - Fork 41
feat: Add Pushpin proxying support for local testing #497
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
Conversation
36b7715
to
1663446
Compare
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.
This overall looks good to me, but I'm not sure whether the body teeing behaviour actually matches what h2o is doing in practice, although teeing might be close enough.
lib/src/component/http_req.rs
Outdated
) -> Result<(), types::Error> { | ||
Err(Error::NotAvailable("Redirect to Fanout/GRIP proxy").into()) | ||
let req = self.session.request_parts(req_handle.into())?; |
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.
Something I learned last week is that technically this hostcall will accept invalid handles in Compute (ostenstibly because of old SDKs?). I'm not sure if it's really worth duplicating that behaviour here, though, but it's worth noting in case it comes up.
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, I think that's not hard to implement.
lib/src/session.rs
Outdated
/// This method must only be called once, *after* a channel has been opened with | ||
/// [`Session::set_downstream_response_sender`][set], and *before* the associated | ||
/// [oneshot::Receiver][receiver] has been dropped. | ||
/// | ||
/// This method will panic if: | ||
/// * the downstream response channel was never opened | ||
/// * the associated receiver was dropped prematurely | ||
/// | ||
/// [set]: struct.Session.html#method.set_downstream_response_sender | ||
/// [receiver]: https://docs.rs/tokio/latest/tokio/sync/oneshot/struct.Receiver.html |
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.
I'm not sure if the set_downstream_response_sender
ever actually existed, since I can't find it in the Git history. This (and the other references to it) should probably be updated.
/// This method must only be called once, *after* a channel has been opened with | |
/// [`Session::set_downstream_response_sender`][set], and *before* the associated | |
/// [oneshot::Receiver][receiver] has been dropped. | |
/// | |
/// This method will panic if: | |
/// * the downstream response channel was never opened | |
/// * the associated receiver was dropped prematurely | |
/// | |
/// [set]: struct.Session.html#method.set_downstream_response_sender | |
/// [receiver]: https://docs.rs/tokio/latest/tokio/sync/oneshot/struct.Receiver.html | |
/// This method must only be called once per downstream request, after which attempting | |
/// to send another response will trigger a panic. |
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, I'm going to do this in both places.
Thanks @ulyssa, I've applied your suggestions. Please review again =) |
For this I went with what would have to happen because the original request's body needs to be replayed in full to the proxied request. |
85f02d1
to
c967b69
Compare
Because of recent changes in the main branch, I did a rebase and merged the conflicts. |
2aed19c
to
42a1eaf
Compare
I made an additional push that makes the following changes:
|
I talked with Fred about this some last week, and it seems like there were some plans for sending bodies to |
Thanks! that sounds great =) I appreciate your thoroughness |
This PR adds code to the CLI to enable `fastly compute serve` with "enable Pushpin", a new mode that locates and runs Pushpin before starting Viceroy to run guest code. This is a companion PR to fastly/Viceroy#497, and will only function after that feature has landed. ## How to enable it: It can be invoked in one of two ways: 1. set a new flag `--experimental-enable-pushpin` 2. or, set a new flag in `fastly.toml`: ```toml [local_server.pushpin] enable = true ``` ## What it does: * `fastly compute serve` starts Pushpin before starting Viceroy, and detects its startup. It also sets up to shut it down when the CLI process ends. * `pushpin` must be started at this time, because routes are set based on the backends defined in the `fastly.toml` manifest file. * "kill process" is done differently between Windows and Unix, so build flags are used to include the right implementation of `killProcess`. * Pushpin routes are set up based on the `fastly.toml` manifest file. * A route is set up per backend. * The name of each backend is set as an `id=` condition. For example, a backend named `origin` will set `id=origin`. Viceroy handles Fanout handoff by forwarding a request for this backend to Pushpin and set the header `Pushpin-Route: origin`, allowing Pushpin to match the route. * If the backend definition's URL has a path segment, then it is set as a `replace_beg` configuration on the route. For example, if a backend is configured for `http://localhost:3000/realtime`, then this is set as `replace_beg=realtime`. Pushpin will forward a request for `/api/user` to this backend as `http://localhost:3000/realtime/api/user`. * `over_http` is set for every route target, as to always enable WebSocket-over-HTTP. * If the backend URL is HTTPS, then `ssl` is added. * If the backend definition sets `override_host`, then it will be set as the `host` value of the route target. * When starting Viceroy, `--local-pushpin-proxy-port=<port>` is appended as a command-line argument, which starts Viceroy in "enable Pushpin" mode. * Pushpin proxy runs by default on port `7677`, but this can be overridden: * using `proxy_port=<port>` under `local_server.pushpin` section of the `fastly.toml` file, or * using `--pushpin-proxy-port=<port>` * Pushpin's publishing endpoint runs by default on port `5561`, but this can be overridden: * using `publish_port=<port>` under `local_server.pushpin` section of the `fastly.toml` file, or * using `--pushpin-publish-port=<port>` * The `pushpin` binary is searched on the system path, but can be specified: * using `pushpin_path=/path/to/pushpin` under the `local_server.pushpin` section of the `fastly.toml` file, or * using `--pushpin-path=/path/to/pushpin`. * The CLI writes the config and routes to temporary files in the OS temporary directory. This is cleaned up as part of shutdown. * Pushpin runner places some runtime files into a temporary directory specified in pushpin.conf. We set this to an OS temporary directory, so that multiple instances can run if necessary. This is cleaned up as part of shutdown. * Pushpin runner places logs into a directory specified in pushpin.conf. This is placed in the ./pushpin-logs directory under the current project All Submissions: * [x] Have you followed the guidelines in our Contributing document? * [x] Have you checked to ensure there aren't other open [Pull Requests](https://github.com/fastly/cli/pulls) for the same update/change? ### New Feature Submissions: * [x] Does your submission pass tests? ### Changes to Core Features: * [x] Have you added an explanation of what your changes do and why you'd like us to include them? * [ ] Have you written new tests for your core changes, as applicable? * [x] Have you successfully run tests with your changes locally? ### User Impact * [x] What is the user impact of this change? ### Are there any considerations that need to be addressed for release? The new feature is opt-in using the `--experimental-enable-pushpin` flag, and does nothing if the new flag is not set.
This PR adds Viceroy code and hostcall implementations to support the hostcalls
fastly_http_req#redirect_to_grip_proxy
andfastly_http_req#redirect_to_grip_proxy_v2
.It adds a new runtime command-line flag,
--local-pushpin-port=<port>
that enables this feature.What does this PR do?
This pull request introduces the foundational support within Viceroy for proxying requests to a local Pushpin instance. This enables developers to locally test Fastly Compute services that use real-time features like HTTP streaming or WebSockets-over-HTTP, which are powered by Fanout/Pushpin.
Specifically, this change implements the host-side logic for the following ABI functions:
fastly_http_req#redirect_to_grip_proxy
andfastly_http_req#redirect_to_grip_proxy_v2
When a guest module calls one of these functions, Viceroy will now intercept the request and proxy it to a configured Pushpin server instead of handling it directly.
The configuration of Pushpin is performed by a corresponding update to the Fastly CLI, which will set up the appropriate routes and start Pushpin during the
fastly compute serve
command.Why is this change important?
Currently, there is no way to locally test Compute features that rely on Fanout. This is a significant gap in the local development workflow, forcing developers to deploy their services to test real-time functionality. By adding this proxying capability to Viceroy, we bridge that gap and create a much more complete and efficient local development experience.
How is this implemented?
A new
pushpin.rs
Module: A new module is introduced to encapsulate all the proxying logic.If the guest code calls
redirect_to_grip_proxy
(or_v2
), the request is replayed to the Pushpin instance, using the name of the backend as the route ID (sets thePushpin-route
header value to the backend name). For example, a request to/api/user
to a backend namedorigin
will be sent to Pushpin as/api/user
with the headerPushpin-route: origin
. Fastly CLI configures Pushpin with theaccept_pushpin_route=true
setting.During this forwarding, the original body of the request must be replayed from the beginning. To enable this, I've added a new
body_tee.rs
module, designed to allow forking a stream.For WebSocket upgrades, the underlying connection is then handed off to a
copy_bidirectional
task to stream data between the client and Pushpin. In order to allow upgrading the original request, code has been added at the beginning ofhandle_request
to save anon_upgrade
future.Details
NonHttpResponse
, has been added, which enables the receiver to signal tohandle_request
that the guest wants to return a non-http response, allowing the handler to perform handling specific to the non-http response type.DownstreamResponse
has been added that represents the response itself, which may be anHttp(Response<Body>)
or aRedirectToPushpin(PushpinRedirectInfo)
. The hostcall forredirect_to_grip_proxy
(and_v2
) sends thisRedirectToPushpin
response type, allowing the receiver to signalNonHttpResponse
to the handler.DownstreamResponse
enumeration has been renamed toDownstreamResponseState
to better reflect its name and to mirror other uses within Fastly.handle_request
adds code to handleNonHttpResponse
response types. If the wrapped response isRedirectToPushpin
, then if this feature is enabled,proxy_through_pushpin
is invoked, proxying the request to Pushpin and streaming the result, and upgrading to WebSocket if the response is101 Switching Protocols
.replace_beg
configuration, but that setup is performed in the CLI, not in Viceroy.