Rust Wrapper for CRT endpoint resolver #317

sauraank · 2023-06-27T09:46:49Z

Implementation of rust wrapper for endpoint resolver in CRT. Added a few tests as well. Used those rust wrappers in endpoint.rs to support regions outside aws region.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

dannycjones

I didn't get a chance to review fully, dropping some comments here

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

passaro · 2023-07-05T09:47:31Z

mountpoint-s3-client/src/endpoint.rs

+        let endpoint_rule_engine = RuleEngine::new(allocator).unwrap();
+        let mut endpoint_request_context = RequestContext::new(allocator).unwrap();
+        endpoint_request_context
+            .add_string(allocator, OsStr::new("Region"), OsStr::new(region))


nit: you could use .as_ref() instead of OsStr::new(..)

Thanks. Does it provide any difference that I should be aware of?

No, new will just call as_ref.

passaro · 2023-07-05T09:56:57Z

mountpoint-s3-client/src/endpoint.rs

+        addressing_style: AddressingStyle,
+        allocator: &Allocator,
+    ) -> Result<Self, EndpointError> {
+        let endpoint_rule_engine = RuleEngine::new(allocator).unwrap();


I'm a little confused by all the unwraps. What are the error conditions here?

passaro · 2023-07-05T10:23:47Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+                .ok_or_last_error()?;
+            match aws_endpoints_resolved_endpoint_get_type(out_endpoint) {
+                aws_endpoints_resolved_endpoint_type::AWS_ENDPOINTS_RESOLVED_ERROR => {
+                    let mut out_error: aws_byte_cursor = Default::default();


Is this error string ever used?

Yes. I wanted to use this error string to get the error. But there is no method for CRT error to get the error other than from int.

passaro · 2023-07-05T10:26:35Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+            match aws_endpoints_resolved_endpoint_get_type(out_endpoint) {
+                aws_endpoints_resolved_endpoint_type::AWS_ENDPOINTS_RESOLVED_ERROR => {
+                    let mut out_error: aws_byte_cursor = Default::default();
+                    let err = Error::from(aws_endpoints_resolved_endpoint_get_error(out_endpoint, &mut out_error));


I believe aws_endpoints_resolved_endpoint_get_error is guaranteed to succeed (in this branch), so aren't you always return success here?

I want to get error out of out_error which this is not doing.

Yeah,aws_endpoints_resolved_endpoint_get_error always returns AWS_OP_SUCCESS here. In other words, the return value of aws_endpoints_resolved_endpoint_get_error is telling you whether that operation (get_error) succeeded or not, which it always will in this branch.

This is why we were talking about making the return type of this method Result<Result<ResolvedEndpoint, String>, Error>. The outer Result captures "internal" errors, like the JSON not parsing or the rules being invalid. The inner Result captures the effect of the actual resolver logic, which sometimes gives an endpoint but sometimes gives an error string.

The Uri struct has a bunch of examples of turning aws_byte_cursors into OsStrs, which you'll need to tweak slightly because you want to return an owned string here, not a reference.

Also, this case is leaking the aws_endpoints_resolved_endpoint right now; it needs a release.

passaro · 2023-07-05T10:27:46Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+                    Err(err)
+                }
+                aws_endpoints_resolved_endpoint_type::AWS_ENDPOINTS_RESOLVED_ENDPOINT => {
+                    let out_endpoint = out_endpoint.ok_or_last_error()?;


What is last_error here? How is it different from line 42?

passaro · 2023-07-05T12:46:00Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+    }
+
+    #[test]
+    fn test_force_path_style_setting() {


Has this been added?

passaro · 2023-07-05T13:13:43Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+            .add_string(
+                &new_allocator,
+                OsStr::new("Bucket"),
+                OsStr::new("mountpoint-o-o0f129d3877cfede5ed9w5rxpf30v7bhaa83yause10--op-s3"),


Maybe use an obviously fabricated string, e.g. "mountpoint-o-o000s3-bucket-test0000000000000000000000000--op-s3"

passaro · 2023-07-05T13:14:16Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+            .add_string(
+                &new_allocator,
+                OsStr::new("Bucket"),
+                OsStr::new("arn:aws:s3::accountID:accesspoint/mfzwi23gnjvgw.mrap"),


As above, maybe something like: "arn:aws:s3::accountID:accesspoint/s3-bucket-test.mrap"

passaro · 2023-07-05T13:18:45Z

mountpoint-s3/src/main.rs

@@ -480,7 +481,7 @@ fn create_client_for_bucket(
    addressing_style: AddressingStyle,
 ) -> Result<S3CrtClient, anyhow::Error> {
    const DEFAULT_REGION: &str = "us-east-1";
-
+    let allocator = Allocator::default();


Is this really necessary? Couldn't from_region just use the default allocator? I don't think we want to expose this outside the client.

yes. It can. In fact, I was making that change in my work for ARN support yesterday. Changing it here as well.

passaro · 2023-07-05T13:32:03Z

mountpoint-s3-client/src/endpoint.rs

+        let resolved_endpoint = endpoint_rule_engine
+            .resolve(endpoint_request_context)
+            .map_err(|_| EndpointError::UnsupportedRegion(region.to_owned()))?;


+1 on this: I do not understand the difference between UnresolvedEndpoint and UnsupportedRegion

jamesbornholt · 2023-07-06T02:19:25Z

mountpoint-s3-client/src/endpoint.rs

+    pub fn from_region(
+        region: &str,
+        addressing_style: AddressingStyle,
+        allocator: &Allocator,


don't think this needs an allocator argument, it should just use Allocator::default() like S3CrtClient::new() does.

jamesbornholt · 2023-07-06T02:20:48Z

mountpoint-s3-client/src/endpoint.rs

+        let resolved_endpoint = endpoint_rule_engine
+            .resolve(endpoint_request_context)
+            .map_err(|_| EndpointError::UnsupportedRegion(region.to_owned()))?;


I think UnsupportedRegion probably just shouldn't exist, unless we feel like we need to distinguish it from other errors somewhere? In general, we're not going to be able to map every possible error from the endpoint resolver -- there are too many.

jamesbornholt · 2023-07-06T02:22:45Z

mountpoint-s3-client/src/endpoint.rs

    UnsupportedRegion(String),
+    #[error("Endpoint could not be resolved")]
+    UnresolvedEndpoint(#[from] mountpoint_s3_crt::common::error::Error),


This should also include the error string we got back from the resolver, which will explain why we couldn't resolve an endpoint.

jamesbornholt · 2023-07-06T02:25:58Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+            match aws_endpoints_resolved_endpoint_get_type(out_endpoint) {
+                aws_endpoints_resolved_endpoint_type::AWS_ENDPOINTS_RESOLVED_ERROR => {
+                    let mut out_error: aws_byte_cursor = Default::default();
+                    let err = Error::from(aws_endpoints_resolved_endpoint_get_error(out_endpoint, &mut out_error));


Yeah,aws_endpoints_resolved_endpoint_get_error always returns AWS_OP_SUCCESS here. In other words, the return value of aws_endpoints_resolved_endpoint_get_error is telling you whether that operation (get_error) succeeded or not, which it always will in this branch.

This is why we were talking about making the return type of this method Result<Result<ResolvedEndpoint, String>, Error>. The outer Result captures "internal" errors, like the JSON not parsing or the rules being invalid. The inner Result captures the effect of the actual resolver logic, which sometimes gives an endpoint but sometimes gives an error string.

The Uri struct has a bunch of examples of turning aws_byte_cursors into OsStrs, which you'll need to tweak slightly because you want to return an owned string here, not a reference.

jamesbornholt · 2023-07-06T02:27:18Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+    }
+
+    /// Add the parameter to [RequestContext] whose value is in form of string
+    pub fn add_string(&mut self, allocator: &Allocator, name: &OsStr, value: &OsStr) -> Result<(), Error> {


nit: this is slightly easier to use if it's name: impl AsRef<OsStr>, value: impl AsRef<OsStr>. Inside the method, use name.as_ref() instead of just name. Same thing for name in add_boolean. You can see examples in methods like Header::has_header.

This way you don't have to do the OsStr::new(...) dance when you try to call it.

jamesbornholt · 2023-07-06T02:31:42Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+            match aws_endpoints_resolved_endpoint_get_type(out_endpoint) {
+                aws_endpoints_resolved_endpoint_type::AWS_ENDPOINTS_RESOLVED_ERROR => {
+                    let mut out_error: aws_byte_cursor = Default::default();
+                    let err = Error::from(aws_endpoints_resolved_endpoint_get_error(out_endpoint, &mut out_error));


Also, this case is leaking the aws_endpoints_resolved_endpoint right now; it needs a release.

Signed-off-by: Ankit Saurabh <[email protected]>

jamesbornholt · 2023-07-06T16:25:11Z

mountpoint-s3-client/src/endpoint.rs

-    UnsupportedRegion(String),
-    #[error("Endpoint could not be resolved")]
-    UnresolvedEndpoint(#[from] mountpoint_s3_crt::common::error::Error),
+    #[error("Endpoint could not be resolved. {0}")]


Suggested change

#[error("Endpoint could not be resolved. {0}")]

#[error("endpoint could not be resolved")]

(no need to include the nested error when it's a #[from], it gets captured automatically

jamesbornholt · 2023-07-06T16:25:40Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+#[derive(Debug, Error, PartialEq, Eq)]
+pub enum ResolverError {
+    /// The header was not found
+    #[error("Resolved Endpoint Error: {0}")]


Suggested change

#[error("Resolved Endpoint Error: {0}")]

#[error("endpoint not resolved: {0}")]

jamesbornholt · 2023-07-06T16:26:15Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

-                    Err(err)
+                    aws_endpoints_resolved_endpoint_get_error(out_endpoint, &mut out_error);
+                    let resolved_endpoint_error = std::str::from_utf8(aws_byte_cursor_as_slice(&out_error))
+                        .unwrap()


Suggested change

.unwrap()

.expect("endpoint errors are always valid UTF-8")

jamesbornholt · 2023-07-06T16:27:46Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

+    #[test_case("UseDualStack", "https://s3-bucket-test.s3.dualstack.us-east-1.amazonaws.com"; "Using Dual Stack (IPv6) option")]
+    #[test_case("Accelerate", "https://s3-bucket-test.s3-accelerate.amazonaws.com"; "Using transfer acceleration option")]
+    #[test_case("ForcePathStyle", "https://s3.us-east-1.amazonaws.com/s3-bucket-test"; "Addressing style set to Path style")]
+    #[test_case("InvalidMountOption", "https://s3-bucket-test.s3.us-east-1.amazonaws.com"; "Invalid mount option is ignored")]


Suggested change

#[test_case("InvalidMountOption", "https://s3-bucket-test.s3.us-east-1.amazonaws.com"; "Invalid mount option is ignored")]

#[test_case("InvalidOption", "https://s3-bucket-test.s3.us-east-1.amazonaws.com"; "Invalid option is ignored")]

jamesbornholt · 2023-07-06T16:29:52Z

mountpoint-s3-crt/src/s3/endpoint_resolver.rs

-    #[test]
-    fn test_fips_setting() {
+    #[test_case("s3-bucket-test", "cn-north-1", "https://s3-bucket-test.s3.cn-north-1.amazonaws.com.cn"; "regions outside aws partition")]
+    #[test_case("s3-bucket-test", "eu-west-1", "https://s3-bucket-test.s3.eu-west-1.amazonaws.com"; "regions within aws partition")]


Suggested change

#[test_case("s3-bucket-test", "eu-west-1", "https://s3-bucket-test.s3.eu-west-1.amazonaws.com"; "regions within aws partition")]

#[test_case("s3-bucket-test", "eu-west-1", "https://s3-bucket-test.s3.eu-west-1.amazonaws.com"; "regions within aws partition")]

#[test_case("s3-bucket-test", "us-gov-west-1", "https://s3-bucket-test.s3.us-gov-west-1.amazonaws.com"; "regions in aws-us-gov partition")]

Signed-off-by: Ankit Saurabh <[email protected]>

sauraank · 2023-07-06T19:08:21Z

Thanks @jamesbornholt @passaro for the review. Addressed all the recommendations.

jamesbornholt

LGTM. Let's do the rest in a separate change, like you were suggesting.

done

sauraank temporarily deployed to PR integration tests June 27, 2023 09:46 — with GitHub Actions Inactive

sauraank self-assigned this Jun 27, 2023

sauraank marked this pull request as draft June 27, 2023 10:28

sauraank temporarily deployed to PR integration tests June 27, 2023 11:10 — with GitHub Actions Inactive

sauraank marked this pull request as ready for review June 27, 2023 12:05

dannycjones reviewed Jun 27, 2023

View reviewed changes

dannycjones reviewed Jun 28, 2023

View reviewed changes

mountpoint-s3-crt/src/s3/endpoint_resolver.rs Outdated Show resolved Hide resolved

sauraank temporarily deployed to PR integration tests June 28, 2023 11:05 — with GitHub Actions Inactive

sauraank temporarily deployed to PR integration tests June 28, 2023 11:07 — with GitHub Actions Inactive

sauraank had a problem deploying to PR integration tests June 29, 2023 16:43 — with GitHub Actions Failure

sauraank force-pushed the endpointResolverWrapper branch from 38a497c to 5267aa4 Compare June 30, 2023 10:54

sauraank had a problem deploying to PR integration tests June 30, 2023 10:54 — with GitHub Actions Failure

sauraank temporarily deployed to PR integration tests June 30, 2023 10:54 — with GitHub Actions Inactive

sauraank requested a review from passaro July 5, 2023 09:28

passaro previously requested changes Jul 5, 2023

View reviewed changes

jamesbornholt reviewed Jul 6, 2023

View reviewed changes

Ankit Saurabh added 2 commits July 6, 2023 14:32

Added the error message and released the leaking pointer

3905f5e

Signed-off-by: Ankit Saurabh <[email protected]>

Consolidate tests with test cases for endpoint resolver

8356dd3

Signed-off-by: Ankit Saurabh <[email protected]>

sauraank temporarily deployed to PR integration tests July 6, 2023 14:11 — with GitHub Actions Inactive

Tested Invalid cases

3e8909f

Signed-off-by: Ankit Saurabh <[email protected]>

sauraank had a problem deploying to PR integration tests July 6, 2023 15:15 — with GitHub Actions Failure

sauraank had a problem deploying to PR integration tests July 6, 2023 15:16 — with GitHub Actions Failure

Added region failure test

797f4d7

Signed-off-by: Ankit Saurabh <[email protected]>

sauraank temporarily deployed to PR integration tests July 6, 2023 15:55 — with GitHub Actions Inactive

jamesbornholt reviewed Jul 6, 2023

View reviewed changes

Added the recommended changes

3c658c3

Signed-off-by: Ankit Saurabh <[email protected]>

sauraank temporarily deployed to PR integration tests July 6, 2023 19:05 — with GitHub Actions Inactive

jamesbornholt approved these changes Jul 6, 2023

View reviewed changes

jamesbornholt merged commit 2d69769 into awslabs:main Jul 6, 2023

sauraank deleted the endpointResolverWrapper branch September 1, 2023 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust Wrapper for CRT endpoint resolver #317

Rust Wrapper for CRT endpoint resolver #317

sauraank commented Jun 27, 2023 •

edited

Loading

dannycjones left a comment

passaro Jul 5, 2023

sauraank Jul 5, 2023 •

edited

Loading

passaro Jul 5, 2023

passaro Jul 5, 2023

passaro Jul 5, 2023

sauraank Jul 5, 2023

passaro Jul 5, 2023

sauraank Jul 5, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

passaro Jul 5, 2023

passaro Jul 5, 2023

passaro Jul 5, 2023

passaro Jul 5, 2023

passaro Jul 5, 2023

sauraank Jul 5, 2023

passaro Jul 5, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

jamesbornholt Jul 6, 2023

sauraank commented Jul 6, 2023

jamesbornholt left a comment

	#[error("Endpoint could not be resolved. {0}")]
	#[error("endpoint could not be resolved")]

	#[error("Resolved Endpoint Error: {0}")]
	#[error("endpoint not resolved: {0}")]

	#[test_case("InvalidMountOption", "https://s3-bucket-test.s3.us-east-1.amazonaws.com"; "Invalid mount option is ignored")]
	#[test_case("InvalidOption", "https://s3-bucket-test.s3.us-east-1.amazonaws.com"; "Invalid option is ignored")]

	#[test_case("s3-bucket-test", "eu-west-1", "https://s3-bucket-test.s3.eu-west-1.amazonaws.com"; "regions within aws partition")]
	#[test_case("s3-bucket-test", "eu-west-1", "https://s3-bucket-test.s3.eu-west-1.amazonaws.com"; "regions within aws partition")]
	#[test_case("s3-bucket-test", "us-gov-west-1", "https://s3-bucket-test.s3.us-gov-west-1.amazonaws.com"; "regions in aws-us-gov partition")]

Rust Wrapper for CRT endpoint resolver #317

Rust Wrapper for CRT endpoint resolver #317

Conversation

sauraank commented Jun 27, 2023 • edited Loading

dannycjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauraank Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauraank commented Jul 6, 2023

jamesbornholt left a comment

Choose a reason for hiding this comment

sauraank commented Jun 27, 2023 •

edited

Loading

sauraank Jul 5, 2023 •

edited

Loading