-
Notifications
You must be signed in to change notification settings - Fork 248
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
allow outbound http to components in same app by default #1710
Conversation
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 is great - thanks for making it happen!
My comments on the functional side of it are, I think, entirely suggestions or nits, not blockers. I did, though, get a bit lost in the sample - could we change / rationalise some of the naming there please? Thanks!
|
||
[[component]] | ||
id = "component-outbound-http" | ||
source = "component-outbound-http/target/wasm32-wasi/release/http_rust_outbound_http.wasm" |
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.
It would be great to make the difference between the outbound-http
and component-outbound-http
directories clearer. Maybe outbound-http-to-other-component
or outbound-http-to-same-app
?
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 think this might be because of trying to fit the "local component call" sample into the existing sample - if we're doing that then we should be careful not to lose the simplicity of the existing sample.
|
||
[[component]] | ||
id = "hello" | ||
source = "http-rust/target/wasm32-wasi/release/http_rust.wasm" |
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.
The http-rust
directory name isn't very helpful, and doesn't obviously relate to the hello
component when reading the code that makes the outbound request.
@@ -0,0 +1,19 @@ | |||
[package] | |||
name = "http-rust-outbound-http" |
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.
Another Cargo package already has this name - this has caused us problems in the past because when a user refers to the Rust SDK, Cargo scans the entire repo and complains about duplicate names.
|
||
let parsed = Url::parse(spin_full_url)?; | ||
|
||
let beforepath = Url::parse(&parsed[..Position::BeforePath]).unwrap(); |
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 did not know about Position
! Thank you!
id = "component-outbound-http" | ||
source = "component-outbound-http/target/wasm32-wasi/release/http_rust_outbound_http.wasm" | ||
[component.trigger] | ||
route = "/outbound-hello" |
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.
Maybe align the route to the component ID? The hello
and outbound-hello
components feel a bit too similar to me.
|
||
[[component]] | ||
id = "component-outbound-http" | ||
source = "component-outbound-http/target/wasm32-wasi/release/http_rust_outbound_http.wasm" |
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.
Suggest adding a comment highlighting that this component doesn't need an allowed_http_hosts
because it only sends requests to other components within the same app, and those are always allowed.
crates/outbound-http/src/lib.rs
Outdated
Ok(self.allowed_hosts.allow(&url)) | ||
} | ||
|
||
/// Check if the outbound request host and port are the same as origin | ||
fn is_same_origin(&self, url: &Url) -> bool { |
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.
Maybe
fn is_same_origin(&self, url: &Url) -> bool { | |
fn is_same_as_origin(&self, url: &Url) -> bool { |
? "Same origin" implies to me whether two things have the same origin, whereas you are interested in whether the URL (the destination) is the same as the origin.
crates/outbound-http/src/lib.rs
Outdated
@@ -31,8 +33,19 @@ impl OutboundHttp { | |||
/// If `None` is passed, the guest module is not allowed to send the request. | |||
fn is_allowed(&self, url: &str) -> Result<bool, HttpError> { | |||
let url = Url::parse(url).map_err(|_| HttpError::InvalidUrl)?; | |||
|
|||
if self.is_same_origin(&url) { |
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.
There is a possible sneaky gap here, if two applications are hosted on the same host but at different base URLs. I don't think that's possible with any current Spin implementation, so not a blocker (mentioning it mostly because this was the justification for have base
in the first place).
crates/outbound-http/src/lib.rs
Outdated
@@ -21,6 +21,8 @@ pub const ALLOWED_HTTP_HOSTS_KEY: MetadataKey<Vec<String>> = MetadataKey::new("a | |||
pub struct OutboundHttp { | |||
/// List of hosts guest modules are allowed to make requests to. | |||
pub allowed_hosts: AllowedHttpHosts, | |||
/// origin is the host of the component that created this outbound HTTP request. |
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 struct isn't associated with a specific HTTP request, so we could make this more explicit:
/// origin is the host of the component that created this outbound HTTP request. | |
/// During an incoming HTTP request, origin is set to the host of that incoming HTTP request. | |
/// This is used to automatically allow outbound requests to the same host. |
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.
that's really great wording thank you
@karthik2804 noted that Cloud might need a bit of work to support this behaviour. It would be good if that could happen before this goes into a Spin release, so as to avoid any "works locally but not in cloud" issues. |
This feature looks great! I have a question about how this works. From looking at the PR, component-to-component HTTP requests for the same app are enabled by default. This is a deviation from what Spin usually does as in that to allow something, some explicit configuration is needed. I am wondering if that is the desired behavior we would want? From the linked issue I see there is a proposed option for a I wanted to bring this up and see what people thought. |
32f8662
to
46fd5bc
Compare
Ok(res) | ||
} | ||
|
||
fn base_url(req: Request) -> Result<Url> { |
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.
Is it an option to provide a function like this in the SDKs, so users don't have to implement this in every component?
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.
Alternatively we could reserve some fake host name to represent "self", e.g. https://self.local
.
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.
https://my.ego
:-) Demo time "So here I call my ego" 👍🏻
I do think we should require e.g. |
crates/outbound-http/src/lib.rs
Outdated
let mut url = Url::parse(&req.uri).map_err(|_| HttpError::InvalidUrl)?; | ||
|
||
if url.host_str() == Some("self.local") { | ||
url.set_host(self.origin.host()) |
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.
Should we update the scheme here too? Thinking about testing on http://localhost
and then switching to https://my.spin-self-hosting.com
.
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.
yea right now the url here would have the same scheme specified in request made in the app so you'd have to update http://self.local
to https://self.local
before deploying and that seems like a paper cut. Let me try updating the scheme like you suggested.
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.
Updated the implementation so you don't need to use self.local
in the uri when making a request and instead you can use relative paths and the correct origin info will be added to the uri as you originally suggested in #1533
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.
self.local
is now self
because... one word is better than two!
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.
We may still want to allow http://self.local/abc
(in addition to just /abc
) for code that expects an absolute URL, like (I think?) the Go SDK.
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 seeing where the Go SDK requires an absolute URL. Could you point me in right direction?
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 was thinking that since we replace the default http client (here) it might break something in that path if you pass a relative URL, but I haven't tested.
919c1cc
to
6475e5c
Compare
|
||
func init() { | ||
spinhttp.Handle(func(w http.ResponseWriter, r *http.Request) { | ||
resp, err := spinhttp.Get("/hello") |
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.
If convenient, could you try this as just http.Get
(like here)?
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.
that worked as well
6475e5c
to
37e31d0
Compare
37e31d0
to
6cb693a
Compare
49f405c
to
8ba038b
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.
Looks fine - I left some style suggestions but nothing blocking. Thanks!
crates/outbound-http/src/lib.rs
Outdated
fn is_allowed(&self, url: &str) -> Result<bool, HttpError> { | ||
fn is_allowed(&mut self, url: &str) -> Result<bool, HttpError> { | ||
if url.starts_with('/') { | ||
if self.allowed_hosts.includes(AllowedHttpHost::host("self")) { |
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.
Rather than requiring callers to know how AllowedHttpHosts represents the self-link permission (and requiring them to construct an AllowedHttpHost just in order to ask), consider putting this into a AllowedHttpHosts::allow_relative_url
(or some better name) method.
if self.allowed_hosts.includes(AllowedHttpHost::host("self")) { | |
return self.allowed_hosts.allow_relative_url(); |
(and remove the includes
method)
crates/outbound-http/src/lib.rs
Outdated
@@ -49,7 +61,14 @@ impl outbound_http::Host for OutboundHttp { | |||
} | |||
|
|||
let method = method_from(req.method); | |||
let url = Url::parse(&req.uri).map_err(|_| HttpError::InvalidUrl)?; | |||
|
|||
let req_url: Url = if req.uri.starts_with('/') { |
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.
To avoid having the parse and error map on both branches, I wonder if this could be reorganised as something like
let req_url: Url = if req.uri.starts_with('/') { | |
let abs_url = if req.uri.starts_with('/') { | |
format!(...) | |
} else { | |
req.uri.to_string() | |
}; | |
let req_url = Url::parse(&abs_uri).map_err(...)?; |
crates/trigger-http/src/spin.rs
Outdated
if let Some(outbound_http_handle) = engine | ||
.engine | ||
.find_host_component_handle::<std::sync::Arc<outbound_http::OutboundHttpComponent>>( | ||
) |
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.
Oh rustfmt no
(no action for you, I am just sad at the tool)
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 don't think rustfmt
produced this, I think it gave up on this code for some reason...
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.
Looks like it gave up because the function call was too long. If you add use
s for Arc
and OutboundHttpComponent
it will happily reformat this.
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.
good tip! Moving the logic to its own function helped but I added the uses anyway for readability.
crates/trigger-http/src/spin.rs
Outdated
|
||
outbound_http_data.origin = format!("{}://{}", scheme, authority); | ||
} | ||
} |
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.
Two consecutive close braces at the same indent feels weird to me but I suppose this is another rustfmt special...
crates/trigger-http/src/spin.rs
Outdated
let EitherInstance::Component(instance) = instance else { | ||
unreachable!() | ||
}; | ||
|
||
if let Some(authority) = req.uri().authority() { |
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 has grown to a big enough block that it rather breaks up the flow of the function - consider pulling it out into a set_http_origin_from_request(&mut store, &req)
(or otherwise meaningfully named) function.
@@ -0,0 +1,2 @@ | |||
github.com/julienschmidt/httprouter v1.3.0 h1:U0609e9tgbseu3rBINet9P48AI/D3oJs4dN7jwJOQ1U= |
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.
Is it correct that we are adding these in this PR?
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.
good catch. removed!
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.
turns out we did need that go.sum file (integration tests failed and other examples include it) so I added this back in.
8ba038b
to
88f66a8
Compare
+ use special value `self` in allowed_http_hosts to allow components in the same spin app to make outbound http requests to each other + make requests with relative routes in app once self is set + update example for rust outbound http to demonstrate capability + adds e2e + resolves fermyon#957 + resolves fermyon#1533 Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
88f66a8
to
d31f03b
Compare
Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
d31f03b
to
c233253
Compare
to another component in the same Spin app using special value
self
inallowed_http_hosts