-
Notifications
You must be signed in to change notification settings - Fork 164
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
Conversation
Signed-off-by: Daniel Carl Jones <[email protected]>
43e5981
to
1cb0d37
Compare
/// 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? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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).