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 tests for specifying multiple network interfaces in the S3 client #1009

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

dannycjones
Copy link
Contributor

Description of change

We currently can't do binary tests where they depend on feature flags. (https://docs.rs/assert_cmd/2.0.6/assert_cmd/cargo/index.html#limitations)

Instead, I'll focus on testing how interface names are specified to the S3 client and when that returns errors.

Currently, we'll fail S3 requests and not client creation. The tests document that, it is certainly an area that desires improvement.

I was tempted to mark these 'Linux-only' using a config flag but I'm able to run them on macOS, just need to install the package for ip commands.

Relevant issues: #815

Does this change impact existing behavior?

No, just new tests.

Does this change need a changelog entry in any of the crates?

No.


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).

/// This test demonstrates how the S3 client will fail today when configured with bad network interfaces.
/// The behavior isn't great, but this documents what happens.
///
/// TODO: How can we get to a point where S3 client creation fails when invalid interface names are provided?
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be possible if we could affect somehow the interface which is used for the request; then we could've verify validity of all provided interfaces at least on the initial ListObjectsV2, may be we can leverage that it's round-robin and do the initial ListObjectsV2 multiple times defined by NiCs number?

how does the error provided to mount-s3 output / logs looks like, when there is a typo in interface's name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how does the error provided to mount-s3 output / logs looks like, when there is a typo in interface's name?

Error: Failed to create S3 client

Caused by:
    0: initial ListObjectsV2 failed for bucket djonesoa-dub in region us-east-1
    1: Client error
    2: Unknown CRT error
    3: CRT error 1053: aws-c-io: AWS_IO_SOCKET_INVALID_OPTIONS, Invalid socket options.

You get something similar when it fails at file operation time (such as where one of the interfaces did exist) along with EIO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may be we can leverage that it's round-robin and do the initial ListObjectsV2 multiple times defined by NiCs number?

I think that could work. I was hoping not to mess too much with that right now since I'd prefer to implement this at the CRT level.

My thinking is that CRT could eagerly do the interface name to ID lookup, and propagate the bad network names immediately at client initialization.

@dannycjones dannycjones added this pull request to the merge queue Sep 13, 2024
Merged via the queue into awslabs:main with commit abae870 Sep 13, 2024
23 checks passed
@dannycjones dannycjones deleted the add-network-bind-tests branch September 13, 2024 11:17
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.

2 participants