-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(http_client source): #23338 Enable AWS authentication for the http_client source. #23339
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
base: master
Are you sure you want to change the base?
feat(http_client source): #23338 Enable AWS authentication for the http_client source. #23339
Conversation
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.
Pull Request Overview
This PR adds AWS SigV4 signing support to the http_client source, allowing Vector to authenticate requests against AWS services (e.g., Amazon Managed Service for Prometheus).
- Introduces conditional AWS signing in the
http_clientutility with empty-body support. - Refactors shared signing logic into
create_signing_instructionsand exposessign_request_with_empty_body. - Updates HTTP sink config to streamline region and credentials provider setup and adds a changelog entry.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sources/util/http_client.rs | Wraps request signing and send in timeouts, injects AWS SigV4 code |
| src/sinks/http/config.rs | Simplifies AWS region/credentials resolution in HTTP sink config |
| src/aws/mod.rs | Refactors signing into helper functions and adds empty-body signer |
| src/aws/auth.rs | Adds region() helper on AwsAuthentication |
| changelog.d/23338.feature.md | Adds changelog entry for AWS auth feature |
Comments suppressed due to low confidence (1)
src/aws/auth.rs:372
- The new
region()helper onAwsAuthenticationcovers various enum variants. Adding unit tests for each variant would ensure region extraction remains correct under future changes.
pub fn region(&self) -> Option<Region> {
1720078 to
ffe54be
Compare
|
Please resolve the merge conflicts and we will take a look |
Thanks, merged master back into my branch - the reorg of the imports has broken git, I try to keep changes to existing code to a minimum in a PR. |
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.
Looks good. Found a few minor issues.
| } | ||
| } | ||
|
|
||
| async fn prepare_request(auth: Option<Auth>, request: Request<Body>) -> Request<Body> { |
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.
With a little bit of refactoring this can be:
async fn prepare_request(auth: Option<Auth>, request: Request<Body>) -> Request<Body> {
match auth {
#[cfg(feature = "aws-core")]
Some(Auth::Aws { auth, service: _ }) => sign_aws_request(auth, request).await,
_ => request,
}
}
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.
So, move the Some branch to a new function?
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.
Yes.
Co-authored-by: Pavlos Rontidis <[email protected]>
Co-authored-by: Pavlos Rontidis <[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.
Hi @johannesfloriangeiger, thanks again for this PR. Left some comments. The only real concern is that this is not tested anywhere in the codebase.
| let region = auth | ||
| .region() | ||
| .or(default_region) | ||
| .expect("Region must be specified"); |
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.
Replace all expect() calls (excluding those in test code) with proper error propagation.
| let default_region = crate::aws::region_provider(&ProxyConfig::default(), None) | ||
| .expect("Region provider must be available") | ||
| .region() | ||
| .await; | ||
| let region = auth | ||
| .region() | ||
| .or(default_region) | ||
| .expect("Region must be specified"); |
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.
Nit: introduce a helper and use it here and also while in config build()
pub async fn resolve_region_from_auth(
auth: &AwsAuthentication,
proxy_config: &ProxyConfig,
tls_config: Option<&TlsConfig>,
) -> crate::Result<Region> {
if let Some(region) = auth.region() {
return Ok(region);
}
region_provider(proxy_config, tls_config)?
.region()
.await
.ok_or_else(|| {
"AWS region must be specified either in auth config or via environment/config".into()
})
}| } | ||
| } | ||
|
|
||
| async fn prepare_request(auth: Option<Auth>, request: Request<Body>) -> Request<Body> { |
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.
Yes.
Summary
See title, enables AWS authentication for the http_client source.
Vector configuration
How did you test this PR?
Manually. Get yourself an AWS Account, create an Amazon Managed Service for Prometheus workspace, get the Endpoint - query URL and use it in the config above. Run Vector and see it working (and not failing with a 403 like the current version).
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.References
Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo nextest run --workspace(alternatively, you can runcargo test --all)git merge origin masterandgit push.Cargo.lock), pleaserun
cargo vdev build licensesto regenerate the license inventory and commit the changes (if any). More details here.