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

S3 Endpoint Resolver #300

Merged
merged 41 commits into from
Jun 11, 2023
Merged

S3 Endpoint Resolver #300

merged 41 commits into from
Jun 11, 2023

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Jun 2, 2023

Issue #, if available:

Description of changes:

  • Adds an opt-in aws-c-s3-endpoint-resolver.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

Merging #300 (49a3912) into main (f5837e5) will decrease coverage by 0.13%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #300      +/-   ##
==========================================
- Coverage   88.99%   88.87%   -0.13%     
==========================================
  Files          17       17              
  Lines        4936     4925      -11     
==========================================
- Hits         4393     4377      -16     
- Misses        543      548       +5     

see 2 files with indirect coverage changes

#include <aws/sdkutils/endpoints_rule_engine.h>

AWS_PUSH_SANE_WARNING_LEVEL
extern const char aws_s3_endpoint_resolver_partitions[];
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need it customer header? or can we have it in private header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@waahm7 waahm7 marked this pull request as ready for review June 5, 2023 17:24
@waahm7 waahm7 changed the title WIP | S3 Endpoint Resolver S3 Endpoint Resolver Jun 5, 2023
Copy link
Contributor

@graebm graebm left a comment

Choose a reason for hiding this comment

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

fix & ship

scripts/update_s3_endpoint_resolver_artifacts.py Outdated Show resolved Hide resolved
scripts/update_s3_endpoint_resolver_artifacts.py Outdated Show resolved Hide resolved
source/s3_endpoint_resolver/s3_endpoint_resolver.c Outdated Show resolved Hide resolved
tests/s3_endpoint_resolver_tests.c Show resolved Hide resolved
tests/s3_endpoint_resolver_tests.c Outdated Show resolved Hide resolved
)
if (AWS_ENABLE_S3_ENDPOINT_RESOLVER)
list(APPEND S3_SRC ${AWS_S3_ENDPOINT_RESOLVER_SRC})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how this works, just throw out my question:
So, this will only compile the source/s3_endpoint_resolver/*.c when the cmake config is set. What about the header file? Will Cmake not compile the header files as well?

Copy link
Contributor Author

@waahm7 waahm7 Jun 5, 2023

Choose a reason for hiding this comment

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

Yes, it will only include the source when the CMake configuration is set. The header file is always included to simplify things and we warn users not to use it unless it's enabled.

@waahm7 waahm7 merged commit 5d912b0 into main Jun 11, 2023
@waahm7 waahm7 deleted the s3-endpoint-generate branch June 11, 2023 22:22
@rvansa
Copy link

rvansa commented Nov 7, 2024

Hi @waahm7 , is there any information available how this should be used? I am trying to direct CRT to use a localstack endpoint and this seems to be relevant; I can't find any API that would accept struct aws_endpoints_rule_engine. Thanks!

@waahm7
Copy link
Contributor Author

waahm7 commented Nov 7, 2024

@rvansa This is just a helper to resolve s3 endpoints. You need to override the endpoint at https://github.com/awslabs/aws-c-s3/blob/main/include/aws/s3/s3_client.h#L815.

@rvansa
Copy link

rvansa commented Nov 8, 2024

@waahm7 Thanks, I found that it is overridden here; I had a bit of hope that this would be intended for a client-wide override as in Aws::S3::S3ClientConfiguration.endpointOverride. The problem with aws_s3_meta_request_options is that the override is not exposed in all API, e.g. when trying to use aws_s3_initiate_list_objects. But there's more problems with that - a quick hacking revealed that the virtual host style starts conflicting when I actually try to list the buckets.

I am wondering how mature CRT really is - from docs I got the impression that it is the high-performant basis of SDK, but even with Aws::S3Crt::S3CrtClient the code seems to use CURL. And after couple of hacks, getting CRT to talk to localstack instance I found that the XML parser can't parse simple XML response. Not speaking of not-implemented features like SSO token refresh.

@waahm7
Copy link
Contributor Author

waahm7 commented Nov 8, 2024

the code seems to use CURL.

We have our own HTTP client and don't use Curl. Curl is only used in some integration tests for sanity checks as it is easier to use and compare against.

this would be intended for a client-wide override

CRT S3 Client is designed to be used under the SDKs. The SDKs resolve the endpoint, construct an HTTP request, and pass that as a meta_request. The http_request normally has a host header which is used to connect to the endpoint. Overriding the host header or the endpoint_override should work, but there is no client-level override AFAIK. These knobs exist in the SDK, and the CRT S3 Client is a fairly low-level API.

This client is more suited for high-performance GetObject and PutObject operations only. For generic S3 usage like listObjects, the recommendation is normally to use an SDK. Any reasons you are trying to use aws-c-s3 directly over an SDK?

@DmitriyMusatkin
Copy link
Contributor

DmitriyMusatkin commented Nov 8, 2024

Just a couple things to note: aws_s3_initiate_list_objects is a private function (as its defined in a private header). it does support endpoint override, but in a different way from how meta requests do it.

As waahm7 mentioned, aws-c-s3 is not meant to be a full featured sdk, but rather a library for other sdks to implement on top of. You can implement any s3 api on top of aws-c-s3 by using default meta. request type, but that does require manual construction of http request. Some SDKs like java do it. Some SDKs, like cpp you mentioned, only bind out put and get - mostly because default was not quite ready yet when they integrated and there has not been any pressing performance reasons to integrate with non-data plane functions.

Note: this is library backing official AWS SDKs. We dont test against anything other than S3 and thus cant guarantee that the client will work against all third party implementations. With that said, we are happy to address any misses in the generic core functionality as long as they are not specific to 3p services.

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.

6 participants