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 downstream_compliance_region hostcall #403

Merged
merged 2 commits into from
Jul 27, 2024
Merged

Conversation

ulyssa
Copy link
Contributor

@ulyssa ulyssa commented Jul 3, 2024

This PR implements the downstream_compliance_region hostcall, which will eventually be exposed in the SDK when the Compliance Regions GA happens. I've made it so that it currently always returns "none", which is what you will see in Compute when you aren't using Compliance Regions with your service.

I've also updated the signature of the component interface to be more similar to downstream-tls-ja3-md5, since it just returns a Vec<u8> and doesn't need to know the max-len. (Let me know if I'm misunderstanding how the component interfaces should work.)

@ulyssa ulyssa force-pushed the ulyssa/compliance-regions branch from df89966 to 5732ccb Compare July 8, 2024 19:52
@ulyssa ulyssa force-pushed the ulyssa/compliance-regions branch from 5732ccb to 9ab65ba Compare July 26, 2024 18:05
@ulyssa ulyssa requested a review from elliottt July 26, 2024 18:09
@ulyssa ulyssa marked this pull request as ready for review July 26, 2024 18:09
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.

Looks good, though the max_len param should be kept if downstream_compliance_region can return an arbitrary-length vector.

downstream-compliance-region: func(max-len: u64) -> result<list<u8>, error>;
downstream-compliance-region: func() -> result<list<u8>, error>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add max-len back in? It's to help with the adapter, as it's ensuring that the returned vector that will be placed in the guest-provided buffer will be large enough.

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 656a5a9.

Comment on lines 811 to 812
async fn downstream_compliance_region(&mut self) -> Result<Vec<u8>, types::Error> {
Ok(Vec::from(b"none"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add a check here that max_len >= 4?

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 656a5a9.

@kpfleming
Copy link
Contributor

Can we find a place to keep track of hardcoded values like this, so that as part of the discussion about making development and testing easier (the 'redesign fastly.toml' topic) we can provide ways to make these configurable in the future? "none" is certainly a valid and expected value, but customers should be able to test their code against the other values they could see in production.

cc @kailan

@ulyssa
Copy link
Contributor Author

ulyssa commented Jul 26, 2024

Can we find a place to keep track of hardcoded values like this, so that as part of the discussion about making development and testing easier (the 'redesign fastly.toml' topic) we can provide ways to make these configurable in the future?

For now I've made this part of the Session type in 656a5a9 so that it can eventually be part of some configuration and passed in to Session::new.

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.

Looks good to me!

@ulyssa ulyssa merged commit cc717db into main Jul 27, 2024
15 checks passed
@ulyssa ulyssa deleted the ulyssa/compliance-regions branch July 27, 2024 00:02
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