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

Add get_addr_dest_{ip,port} hostcalls #402

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Conversation

ulyssa
Copy link
Contributor

@ulyssa ulyssa commented Jul 3, 2024

This adds the get_addr_dest_ip and get_addr_dest_port hostcalls which will be part of the upcoming SDK release. They allow getting the IP and port used to connect to a backend from the ResponseHandle.

There is a difference in behaviour here worth calling out: in Compute, this is only currently supported when using .set_pass(true) on the Request. Since Viceroy doesn't have any concept of caching/direct pass at the moment, I wasn't sure whether I should add enough code to check whether the guest code had at least requested direct pass or not, and limit support so that things match what applications deployed to Compute will see.

@ulyssa ulyssa requested review from elliottt and acw July 3, 2024 21:19
@ulyssa ulyssa force-pushed the ulyssa/backend-ip-addr branch from 416f605 to 6a1d87a Compare July 3, 2024 21:41
Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your idea to encode enough information to check if set_pass had been called -- it would be great if this was a pretty faithful reproduction of production behavior.

}

impl Connection {
fn metadata(&self) -> ConnMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a mild preference for returning a &ConnMetadata here and pushing the clone to the callsite, but given that this isn't exported beyond this module I could go either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10b6237!

@ulyssa
Copy link
Contributor Author

ulyssa commented Jul 4, 2024

I like your idea to encode enough information to check if set_pass had been called -- it would be great if this was a pretty faithful reproduction of production behavior.

Done in 270928a. I've added a CacheOverride struct like what we have in fastly-shared with a from_abi method similar to what's in Compute/used to exist in fastly-shared. I then put it into the Extensions for the Request so that I can populate the direct_pass field that I've added to ConnMetadata.

Copy link
Contributor

@acw acw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for addressing @elliottt's comments!

@ulyssa ulyssa merged commit c5c4e35 into main Jul 8, 2024
7 checks passed
@ulyssa ulyssa deleted the ulyssa/backend-ip-addr branch July 8, 2024 19:48
GeeWee pushed a commit to GeeWee/Viceroy that referenced this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants