Skip to content
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

Track, Agree Upon, and Document Breaking Semantic Changes #2815

Closed
2 of 6 tasks
rylev opened this issue Sep 9, 2024 · 10 comments
Closed
2 of 6 tasks

Track, Agree Upon, and Document Breaking Semantic Changes #2815

rylev opened this issue Sep 9, 2024 · 10 comments
Milestone

Comments

@rylev
Copy link
Collaborator

rylev commented Sep 9, 2024

Below is a list of the breaking semantic changes to the Spin runtime that will be coming in Spin 3.0. Before we release, we should ensure that we are all of with these changes, and then we should document them.

@michelleN
Copy link
Member

@radu-matei @karthik2804 - are there documentation changes for the llm-factor breaking changes we need to add to this list?

@itowlson
Copy link
Contributor

itowlson commented Oct 8, 2024

@rylev and SDK folks: do you know if/how:

HttpRequestDenied used to be returned from wasi:http/outgoing-handler#handle but are now returned from wasi:http/types/future-incoming-response#get

would affect developers using the SDKs, as opposed to programming directly against wasi-http? It seems like it would still manifest as an error in whatever async "send request" wrapper they use, just internally it would be in a slightly different part of the promise/future/task/whatever. Does that sound right?

@itowlson
Copy link
Contributor

itowlson commented Oct 8, 2024

@rylev re "Change how incoming-request.authority is set #2723" - could you expand on what changes in the observable behaviour please? The "moral equivalent" #2684 says "The guest will now see incoming-request.authority as whatever is set in the incoming request's Host header" but I am having trouble parsing the second paragraph. E.g. is the specific change something like:

When an HTTP handler makes a self request, the incoming-request.authority seen by the receiving component is now the same authority as seen by the original HTTP handler. If the self-request sets the Host header then... it is discarded?

or more like

In an HTTP handler, the incoming-request.authority is now always set to the Host header of the incoming request. (Previously it was... something different?) For self requests, this is (always overridden? may be suppied by the requestor but will be delivered to the local app regardless?)

Neither of these feel right, and they are clearly incomplete anyway, sorry...!

Thanks!

@rylev
Copy link
Collaborator Author

rylev commented Oct 9, 2024

would affect developers using the SDKs, as opposed to programming directly against wasi-http? It seems like it would still manifest as an error in whatever async "send request" wrapper they use, just internally it would be in a slightly different part of the promise/future/task/whatever. Does that sound right?

For example, for users of the Rust SDK, this shouldn't really make a difference as both API points are wrapped up in the executor::outgoing_request_send function and so it should look the same to the Rust SDK user no matter where the error surfaces. I've not looked at other SDKs, but we'd had to see if any of them don't abstract this away from the users. Of course, for those using the wasi:http/outgoing-handler API directly, they will be exposed to this difference.

Re: the authority header:

You can see the full logic here - the logic changed a bit in #2747. The authority is determined by the host header or the URL authority (and they much match if both are present).

Specifically, for a "self" request, the host header is always set (meaning that the host header will be responsible for the authority the guest will see), and it is set here to the listener address of the running Spin server.

I'm actually not sure what the authority is set to in self requests in Spin 2.x - we should investigate that and document it.

Does this clarify things?

@itowlson
Copy link
Contributor

itowlson commented Oct 9, 2024

Thanks for the confirmation re the HTTP error. I'll double check other SDKs but I think they all hide this (and yep, understood for folks working at the WASI level).

Thanks for the link to the authority code. Unfortunately the low-level fragment hasn't really gotten me confident I have my head around what this means for application developers, but let me have another crack at some wording:

Spin now infers the authority property of an incoming request from either the request URL or the HTTP Host header. (If the request includes both, then they must agree.) This will not usually result in any change of behaviour for "normal" requests from HTTP clients.

(I recognise that "normal" is handwavey here!)

Spin now adds an HTTP Host header on self requests (those made using a path only). The value of the header is the listener address of the Spin HTTP server. Consequently, the component receiving the self request will always see the URL authority as that of the Spin HTTP server, just as if the request had come from an external client.

Does that correctly capture the effect at guest level of these changes? Thanks!

@rylev
Copy link
Collaborator Author

rylev commented Oct 11, 2024

These summaries seem largely good to me.

This will not usually result in any change of behaviour for "normal" requests from HTTP clients.

This part, however, doesn't really track. Previously the authority was always set to the Spin server's listener address (much like is done now for self requests), and now it's based on the host header. So unless the client was making a direct against the listener address (instead of requesting for example against a DNS name that ultimately gets routed to the listener), the authority will change.

@itowlson
Copy link
Contributor

Aha! This is the missing piece. Let me word it back to check I now have it right:

Spin now infers the authority property of an incoming HTTP request from either the request URL or the HTTP Host header. (If the request includes both, then they must agree.) This means that when a client makes the request through a DNS name, the authority will be that DNS name, rather than the Spin server's listener address.

(and, thus, modifying the text in self-requests about "as if the request had come from an external client")

Thanks!

@rylev
Copy link
Collaborator Author

rylev commented Oct 14, 2024

This means that when a client makes the request through a DNS name, the authority will be that DNS name, rather than the Spin server's listener address.

I think this is largely correct enough. There are more precise ways to formulate this, but for the purposes of change logs this might be the best balance between correctness and terseness.

@vdice
Copy link
Member

vdice commented Nov 4, 2024

By now we've had plenty of chance to disagree on the breaking changes linked above. @itowlson's docs PR appears to cover the documentation front. Are we good with fermyon/developer#1396 closing this issue once merged?

@rylev
Copy link
Collaborator Author

rylev commented Nov 5, 2024

From my side, we can close this.

@vdice vdice closed this as completed Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

4 participants