-
Notifications
You must be signed in to change notification settings - Fork 67
fix: Remove Netty as an agent dependency #1206
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,16 +40,11 @@ val dependencyBoms = listOf( | |
| "com.google.protobuf:protobuf-bom:3.25.1", | ||
| "com.linecorp.armeria:armeria-bom:1.26.4", | ||
| "io.grpc:grpc-bom:1.59.1", | ||
| // netty-bom is a fix for CVE-2025-58056 (https://github.com/advisories/GHSA-fghv-69vj-qj49). | ||
| // Remove once https://github.com/aws/aws-sdk-java-v2/pull/6398 and https://github.com/aws/aws-sdk-java/pull/3192 | ||
| // are both merged and released, and we update the corresponding dependencies. | ||
| "io.netty:netty-bom:4.1.126.Final", | ||
| "io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha:$otelAlphaVersion", | ||
| "org.apache.logging.log4j:log4j-bom:2.21.1", | ||
| "org.junit:junit-bom:5.10.1", | ||
| "org.springframework.boot:spring-boot-dependencies:2.7.17", | ||
| "org.testcontainers:testcontainers-bom:1.19.3", | ||
| "software.amazon.awssdk:bom:2.30.17", | ||
| ) | ||
|
|
||
| val dependencySets = listOf( | ||
|
|
@@ -103,6 +98,9 @@ dependencies { | |
| for (dependency in dependencyLists) { | ||
| api(dependency) | ||
| } | ||
|
|
||
| api("software.amazon.awssdk:auth:2.33.11") | ||
| api("software.amazon.awssdk:aws-core:2.33.11") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does adding
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any UTs for Sigv4 work? Core does not include STS.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||
| } | ||
| } | ||
|
|
||
|
|
||
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 dependency cannot be removed. It is required for customers using the Sigv4 exporters: #1101
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 the build and Unit Test not fail after removing it? Do you know which API Agent uses?
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 dependency is managed by the AWS SDK and is used internally in the the AWS SDK authentication libraries to retrieve credentials from all sources. This is already well tested by upstream which is why we don't have unit tests here.
To validate this you have to just check one of these paths work:
https://docs.aws.amazon.com/sdkref/latest/guide/feature-assume-role-credentials.html#feature-assume-role-credentials-settings
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.
I tested this PR with this document. Both trace and log are working.
Exporting collector-less telemetry using AWS Distro for OpenTelemetry (ADOT) SDK
https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-OTLP-UsingADOT.html
May I ask why you believe the SigV4 projects need STS? Are you trying to obtain an AWS access token from STS? If so, which AWS account, user, or role are you using? I wasn’t able to find the code.
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.
Thank you, Luke! Yes, SigV4 requires STS. This request comes from users who rely on this feature but are unable to explicitly set or hardcode their credentials in their environment. Instead, these environments use the STS library to automatically retrieve and authenticate credentials on their behalf.
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.
see: #1101