Skip to content

Conversation

@TeodorSAP
Copy link
Member

@TeodorSAP TeodorSAP commented Nov 21, 2025

Description

Changes proposed in this pull request (what was done and why):

  • Use OTel's oauth2client extension to support OAuth2 in the OTLP output
  • Adapt configuration builders and secret management accordingly
  • Implement UTs for the new configuration flow

⚠️ DISCLAIMER: GRPC output without TLS configuration is not supported by the OTel collector when OAuth2 is enabled.

Left TODO:

  • E2E tests
  • Documentation
  • CRD validation for GRPC without TLS scenario

Changes refer to particular issues, PRs or documents:

Traceability

  • The PR is linked to a GitHub issue.
  • The follow-up issues (if any) are linked in the Related Issues section.
  • If the change is user-facing, the documentation has been adjusted.
  • If a CRD is changed, the corresponding Busola ConfigMap has been adjusted.
  • The feature is unit-tested.
  • The feature is e2e-tested.

@TeodorSAP TeodorSAP requested a review from a team as a code owner November 21, 2025 15:27
@TeodorSAP TeodorSAP added area/logs LogPipeline area/metrics MetricPipeline area/traces TracePipeline do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 21, 2025
@github-actions github-actions bot modified the milestone: 1.53.0 Nov 21, 2025
@github-actions github-actions bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 21, 2025
@TeodorSAP TeodorSAP requested a review from a team as a code owner November 21, 2025 15:35
NHingerl
NHingerl previously approved these changes Nov 24, 2025
@skhalash skhalash modified the milestones: 1.53.0, 1.54.0 Nov 25, 2025
@NHingerl NHingerl added area/documentation Documentation changes kind/docs Categorizes issue or PR as related to a documentation change labels Dec 1, 2025
NHingerl
NHingerl previously approved these changes Dec 1, 2025
@NHingerl
Copy link
Contributor

NHingerl commented Dec 1, 2025

Must add this to DITA (probably) for takt 11b publication.
Let's discuss the release schedule in week 51.

@TeodorSAP TeodorSAP removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2025
@TeodorSAP TeodorSAP added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/docs Categorizes issue or PR as related to a documentation change labels Dec 2, 2025
.env Outdated
ENV_FLUENTBIT_EXPORTER_IMAGE="europe-docker.pkg.dev/kyma-project/prod/directory-size-exporter:v20250910-86122076"
ENV_FLUENTBIT_IMAGE="europe-docker.pkg.dev/kyma-project/prod/external/fluent/fluent-bit:4.1.1"
ENV_OTEL_COLLECTOR_IMAGE="europe-docker.pkg.dev/kyma-project/prod/kyma-otel-collector:0.139.0-main"
ENV_OTEL_COLLECTOR_CONTRIB_IMAGE="otel/opentelemetry-collector-contrib:0.139.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this ?

Copy link
Member Author

@TeodorSAP TeodorSAP Dec 4, 2025

Choose a reason for hiding this comment

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

This is now required, since the oidc OTel extension is used as part of the e2e tests (on the receiving/backend side), but we don't use it nor want to have it in the production-ready version (i.e. add it to OCC). Thus, we use the official opentelemetry-collector-contrib image for building tests' image only.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we move it to our registry. If we our tests get rate-limited by docker ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the version number.. will we keep it on the old version for ever or will we update it together with the otel upgrade or do we use latest?

namespace: default
key: tokenUrl
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a note about this: DISCLAIMER: GRPC output without TLS configuration is not supported by the OTel collector when OAuth2 is enabled. ?

Copy link
Contributor

@NHingerl NHingerl Dec 4, 2025

Choose a reason for hiding this comment

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

Is there a way to express this in active voice and more user-centric?

Like "If you want to use OAuth2 with gRPC, you must configure TLS."

Copy link
Member Author

@TeodorSAP TeodorSAP Dec 4, 2025

Choose a reason for hiding this comment

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

Yes, I will add it. The user will also get a straight-forward message from the CRD validation if they try configuring it without TLS. But might be good to add it to the documentation as well.

errorMsg: "Can define either both 'cert' and 'key', or neither",
field: "spec.output.otlp.tls",
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

we can add one more test where one of the 3 mandatory fields is missing then it rejects creation ?

errorMsg: "Can define either both 'cert' and 'key', or neither",
field: "spec.output.otlp.tls",
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

add more rejection tests see comment above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/documentation Documentation changes area/logs LogPipeline area/metrics MetricPipeline area/traces TracePipeline kind/feature Categorizes issue or PR as related to a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants