feat(aws_s3 sink): add option to use virtual addressing #21999

sam6258 · 2024-12-10T06:10:28Z

Summary

The following PR adds an option to enable virtual host style addressing to an s3 sink. The force_path_style option defaults to true for backwards compatibility and may eventually be desired to default to false as path style addressing will eventually be removed from aws s3.

This is my first Rust and first Vector PR, so please correct me on any contribution guidelines, style semantics, and code patterns!

Change Type

Bug fix
New feature
Non-functional (chore, refactoring, docs)
Performance

Is this a breaking change?

Yes
No

How did you test this PR?

I pointed an aws s3 sink at my object storage service and validated both path style and vhost style addressing works and also has the correct authority in access logs.

Does this PR include user facing changes?

Yes. Please add a changelog fragment based on our guidelines.
No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

Please read our Vector contributor resources.
If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

jszwedko

Thanks for this contribution @sam6258 !

Do you think it'd be possible to add an integration test that uses the virtual addressing style? It looks like localstack supports it.

jszwedko · 2024-12-10T11:10:52Z

Integration tests are defined here: https://github.com/vectordotdev/vector/blob/master/src/sinks/aws_s3/integration_tests.rs

website/cue/reference/components/sinks/base/aws_s3.cue

sam6258 · 2024-12-10T18:14:17Z

Do you think it'd be possible to add an integration test that uses the virtual addressing style?

@jszwedko working on this! My editor didn't let me know I broke integration tests in the call to create_service too. Is it possible to enable tests to run on this PR? I'm working on getting my dev env setup to run integration tests in the meantime.

pront · 2024-12-10T18:19:46Z

Do you think it'd be possible to add an integration test that uses the virtual addressing style?

@jszwedko working on this! My editor didn't let me know I broke integration tests in the call to create_service too. Is it possible to enable tests to run on this PR? I'm working on getting my dev env setup to run integration tests in the meantime.

I enabled the CI checks (because it requires approval by maintainers).

Local iterations should be much faster. This might be helpful: https://github.com/vectordotdev/vector/blob/master/scripts/integration/README.md

sam6258 · 2024-12-11T00:58:57Z

Adapted the integration tests for the new function call structure and added s3_healthchecks_vhost, but I'm getting a lot of test timeouts locally for some reason. Will dig into a bit, since I can't run all the tests I need, but I think the current commit should get us what we want.

jszwedko · 2024-12-11T09:49:02Z

src/sinks/aws_cloudwatch_logs/integration_tests.rs

@@ -467,7 +467,7 @@ async fn create_client_test() -> CloudwatchLogsClient {
    let endpoint = Some(cloudwatch_address());
    let proxy = ProxyConfig::default();

-    create_client::<CloudwatchLogsClientBuilder>(&auth, region, endpoint, &proxy, &None, &None)
+    create_client::<CloudwatchLogsClientBuilder>(&auth, region, endpoint, &proxy, &None, &None, true)


I think this means we will only test with virtual path style addressing. Could we make sure we have at least one test using each style?

force_path_style set to true should mean the previous behavior, not virtual host. Most of the components don't even use the value, they just needed the builder implemented with it.

Still digging into integration failures as it seems at least some might be related to changes in the PR

I got the integration tests working locally and pushed the changes. I had some incorrect values in the test client configs.

On a side note, I did not end up adding an integration test for virtual host style access. This proved more difficult than initially intended as it requires wildcard DNS which docker compose does not support easily. Localstack does support virtual host based lookups and I was able to set up the sdk to do so, but it fails on resolution of the actual bucket. I think wildcard DNS might be out of the scope of this PR.

What are your thoughts here?

Here is the branch I was playing with if interested https://github.com/sam6258/vector/tree/sam6258/vhost-buckets-test

Signed-off-by: Scott Miller <smiller1@coreweave.com>

sam6258 · 2024-12-12T19:12:05Z

@pront I fixed the CI check that was failing if you can kick off again, thanks!

pront · 2024-12-12T20:15:38Z

@pront I fixed the CI check that was failing if you can kick off again, thanks!

Is PR now ready for a final review? There has been difficulty with getting an IT running?

sam6258 · 2024-12-12T20:39:01Z

Yes, PR in my opinion is ready for final review. It passes existing tests, but I wasn't able to get a test to cover the new functionality per this comment. #21999 (comment)

pront

Thanks @sam6258

ADustyOldMuffin · 2024-12-13T21:38:39Z

Is there anything else we need to do to get this merged?

pront · 2024-12-13T23:13:08Z

Is there anything else we need to do to get this merged?

Leaving it this up to @jszwedko, due to this thread:
#21999 (comment)

jszwedko

Thanks @sam6258 ! I see that it'd be difficult to add an integration test due to the necessity of docker-compose supporting wildcard domains. In this case, I think we can just rely on the SDK to do the right thing.

I left one comment about changing the way the configuration is injected, but otherwise I think this looks good to go.

jszwedko · 2024-12-17T17:20:23Z

src/aws/mod.rs

@@ -124,6 +124,9 @@ pub trait ClientBuilder {

    /// Build the client using the given config settings.
    fn build(config: &SdkConfig) -> Self::Client;
+
+    /// Build the client using the given config settings and path style addressing.
+    fn build_and_force_path_style(config: &aws_types::SdkConfig) -> Self::Client;


I'm not wild about having an S3 specific method on this generic trait.

What would think of, instead of adding this method here, adding a field to

vector/src/common/s3.rs

Line 5 in 475e93f

pub(crate) struct S3ClientBuilder;

that controls the argument to force_path_style?

Yeah, I wasn't crazy about this either, but lacking rust skills here didn't know how else to achieve this. Would that look something like this?

pub(crate) struct S3ClientBuilder{ // field here? } impl ClientBuilder for S3ClientBuilder {

Yeah, I think it'd be something like that. Then you'll need to add &self to the build() method. I'm happy to play around with it if you get stuck. Hopefully the compiler pushes you in the right directions.

sam6258 requested review from a team as code owners December 10, 2024 06:10

github-actions bot added domain: sources Anything related to the Vector's sources domain: sinks Anything related to the Vector's sinks domain: external docs Anything related to Vector's external, public documentation labels Dec 10, 2024

sam6258 force-pushed the sam6258/vhost-buckets branch from 207f753 to 30feaa4 Compare December 10, 2024 06:13

sam6258 mentioned this pull request Dec 10, 2024

Providing a virtual hosted style s3 bucket endpoint causes the healthcheck to fail #17351

Open

sam6258 force-pushed the sam6258/vhost-buckets branch from 30feaa4 to 5ebb457 Compare December 10, 2024 06:16

jszwedko reviewed Dec 10, 2024

View reviewed changes

neko-dd approved these changes Dec 10, 2024

View reviewed changes

website/cue/reference/components/sinks/base/aws_s3.cue Show resolved Hide resolved

sam6258 changed the title ~~feat(aws_sdk): add option to use virtual addressing~~ feat(aws_s3): add option to use virtual addressing Dec 10, 2024

jszwedko reviewed Dec 11, 2024

View reviewed changes

sam6258 force-pushed the sam6258/vhost-buckets branch 2 times, most recently from 3e95f13 to a6a184e Compare December 12, 2024 08:45

pront changed the title ~~feat(aws_s3): add option to use virtual addressing~~ feat(aws_s3 sink): add option to use virtual addressing Dec 12, 2024

feat(aws_s3 sink): add option to use virtual addressing

d03f11b

Signed-off-by: Scott Miller <smiller1@coreweave.com>

sam6258 force-pushed the sam6258/vhost-buckets branch from a6a184e to d03f11b Compare December 12, 2024 19:11

pront approved these changes Dec 13, 2024

View reviewed changes

jszwedko reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws_s3 sink): add option to use virtual addressing #21999

feat(aws_s3 sink): add option to use virtual addressing #21999

sam6258 commented Dec 10, 2024 •

edited

Loading

jszwedko left a comment

jszwedko commented Dec 10, 2024

sam6258 commented Dec 10, 2024

pront commented Dec 10, 2024

sam6258 commented Dec 11, 2024

jszwedko Dec 11, 2024

sam6258 Dec 11, 2024

sam6258 Dec 11, 2024

sam6258 Dec 11, 2024

sam6258 commented Dec 12, 2024

pront commented Dec 12, 2024

sam6258 commented Dec 12, 2024 •

edited

Loading

pront left a comment

ADustyOldMuffin commented Dec 13, 2024

pront commented Dec 13, 2024

jszwedko left a comment

jszwedko Dec 17, 2024

sam6258 Dec 17, 2024 •

edited

Loading

jszwedko Dec 17, 2024

feat(aws_s3 sink): add option to use virtual addressing #21999

Are you sure you want to change the base?

feat(aws_s3 sink): add option to use virtual addressing #21999

Conversation

sam6258 commented Dec 10, 2024 • edited Loading

Summary

Change Type

Is this a breaking change?

How did you test this PR?

Does this PR include user facing changes?

Checklist

References

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko commented Dec 10, 2024

sam6258 commented Dec 10, 2024

pront commented Dec 10, 2024

sam6258 commented Dec 11, 2024

jszwedko Dec 11, 2024

Choose a reason for hiding this comment

sam6258 Dec 11, 2024

Choose a reason for hiding this comment

sam6258 Dec 11, 2024

Choose a reason for hiding this comment

sam6258 Dec 11, 2024

Choose a reason for hiding this comment

sam6258 commented Dec 12, 2024

pront commented Dec 12, 2024

sam6258 commented Dec 12, 2024 • edited Loading

pront left a comment

Choose a reason for hiding this comment

ADustyOldMuffin commented Dec 13, 2024

pront commented Dec 13, 2024

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko Dec 17, 2024

Choose a reason for hiding this comment

sam6258 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

jszwedko Dec 17, 2024

Choose a reason for hiding this comment

sam6258 commented Dec 10, 2024 •

edited

Loading

sam6258 commented Dec 12, 2024 •

edited

Loading

sam6258 Dec 17, 2024 •

edited

Loading