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

Authz headers handled and set as attributes for traces #350

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Bianco95
Copy link
Collaborator

@Bianco95 Bianco95 commented Jan 8, 2025

The SetupTelemetry function has been updated to dynamically create a service with a unique name depending on whether it is invoked by the VK or the InterLink API.
Additionally, information extracted from the AUTHZ headers is now included as attributes of the span associated with different HTTP calls.

@Bianco95 Bianco95 requested a review from dciangot January 8, 2025 15:46
@Bianco95 Bianco95 linked an issue Jan 8, 2025 that may be closed by this pull request
@@ -113,25 +109,24 @@ func SetupTelemetry(ctx context.Context) (*sdktrace.TracerProvider, error) {
Certificates: []tls.Certificate{cert},
RootCAs: certPool,
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: false,
InsecureSkipVerify: true, // #nosec
Copy link
Collaborator

Choose a reason for hiding this comment

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

why exactly? @Bianco95 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the CA used for the mTLS mechanism is not verified by a trusted authority, an error occurred when a trace is sent to the Tempo endpoint, specifically
2025/01/08 12:39:19 traces export: context deadline exceeded: rpc error: code = Unavailable desc = connection error: desc = "transport: authentication handshake failed: tls: failed to verify certificate: x509: certificate signed by unknown authority

Copy link
Collaborator

Choose a reason for hiding this comment

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

alright, only if you open an issue where we track that this value should be passed as a configuration parameter :) it can go for a later time, but we need to keep track

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this relates to telemetry configuration, I propose adding an environment variable, similar to how other telemetry parameters are handled. By default, insecureSkipVerify will be set to false, but users will have the option to override this and set it to true if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@dciangot dciangot self-requested a review January 11, 2025 14:39
@dciangot
Copy link
Collaborator

@Bianco95 can you document this env variable in the documenation of the monitoring as well?

@Bianco95
Copy link
Collaborator Author

Monitoring documentation has been properly updated

Copy link
Collaborator

@dciangot dciangot left a comment

Choose a reason for hiding this comment

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

Lgtm

@dciangot
Copy link
Collaborator

Although, the integration test seems loke not working... need to check it carefully

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.

Pass OAuth2 proxy authz headers to the plugin call
2 participants