-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
enhancement(aws provider): Add ability to disable request signing #20973
base: master
Are you sure you want to change the base?
Conversation
This can be useful when sending anonymous requests to AWS S3. Potentially it could be useful in other situations as well (e.g. sending to AWS API compatible endpoints that don't support signing). I'd really have liked to introduce this as a new "strategy" for AWS authentication configuration since it is mutually exclusive with the others, but given the way AWS authentication configuration is currently implemented as an untagged enum, adding it to the default config enum seemed like the best option. If/when we refactor this to follow https://github.com/vectordotdev/vector/blob/master/docs/specs/configuration.md#polymorphism then we can move it. Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Datadog ReportBranch report: ✅ 0 Failed, 443 Passed, 0 Skipped, 4m 5.35s Total Time |
Signed-off-by: Jesse Szwedko <[email protected]>
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.
good for docs
match config.auth { | ||
AwsAuthentication::Default { sign, .. } => { | ||
assert!(!sign); | ||
} | ||
_ => panic!(), | ||
} |
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.
This could probably be more concise as a assert!(matches!(...))
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.
Mmm, true, though I like it as-is since it matches the other tests here.
.set_http_client(Some(SharedHttpClient::new(connector))) | ||
.set_sleep_impl(Some(SharedAsyncSleep::new(Arc::new(TokioSleep::new())))) | ||
.set_identity_cache(Some(auth.credentials_cache().await?)) | ||
.set_region(Some(region.clone())) | ||
.set_retry_config(Some(retry_config.clone())) |
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.
Why did these all become options? Somewhat confusing since the credentials provider doesn't appear to be one.
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.
Ah, good question, in hindsight I should have left a comment explaining this. This change was because I wanted to use set_credentials_provider
to pass in an Option
(since the credentials provider can be None
). This requires a &mut self
, and modifies in place, rather than a self
where it returns an updated self
. I updated all of the calls for consistency but I think I could leave the existing calls and just use set_credentials_provider
for the one where I need to pass an Option
if preferred.
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.
The builder has <option>
and set_<option>
methods for all configuration options. The ones with set_
take an Option
(and &mut self
) where the ones without do not (and take a self
).
Just thought of adding this extra information here in case it helps anyone / for anything .With respect to anonymous access to S3 (via S3 sink) for whose use case this unsigned request feature can be used, AWS does not allow multipart upload (for reason only known to AWS ) . S3 supports file uploads in chunks of 5 MB., and hence, only one 5 MB file can be uploaded in one go at max with unsigned request. There is no official document with this regards. Post my discussion in discord (link mentioned in issue description) , I was experimenting with batch uploads of files using AWS cli with anonymous access instead of relying on vector until this feature gets into main branch. And I just happened to stumble on this error. |
Hey @jszwedko Even before its merged to main branch , I have started testing it 😝 .So kindly excuse my impatience. For some reason, the logs are not getting delivered to my S3 bucket with the made changes. I have detailed my experiment below. Let me know if it's a bug or if I am going wrong somewhere in my steps. Pr-requisitesThe target
And the instance from where vector is being run has <MY_RESTRICTED_IP>. Steps to reproduce the issueStep -1 Building from source
Step -2 Drafting a dummy log generator and Anonymous access based S3 sink vector config
Step 3: Set the trace mode for checking out trace log entries for debuggingexport VECTOR_LOG=trace
export RUST_LOG=trace Step 4: Running vector
When vector is run I get (not some clear) errors which looks like below :-
I unset the trace for both RUST_LOG and VECTOR_LOG so that noise can be filtered out and I got below WARN level logs.
Further, I tested with aws cli with sample file just to make sure if my permissions were correct. The sample file got uploaded successfully. The command used for it is given below :-
Thought of putting these now here, so that if any issue exists, it can be corrected in the PR. |
@rams3sh Thanks for trying this out proactively! That's unfortunate to hear it doesn't seem to be working for you. I'll try to reproduce and see if I can figure it out this upcoming week. |
This can be useful when sending anonymous requests to AWS S3. Potentially it could be useful in other situations as well (e.g. sending to AWS API compatible endpoints that don't support signing).
I'd really have liked to introduce this as a new "strategy" for AWS authentication configuration since it is mutually exclusive with the others, but given the way AWS authentication configuration is currently implemented as an untagged enum, adding it to the default config enum seemed like the best option.
If/when we refactor this to follow
https://github.com/vectordotdev/vector/blob/master/docs/specs/configuration.md#polymorphism then we can move it.
Prompted by a user in discord: https://discord.com/channels/742820443487993987/1267892632319692802/1267892632319692802