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

Export Otel configuration in shell steps: OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS #155

Conversation

cyrille-leclerc
Copy link
Contributor

@cyrille-leclerc cyrille-leclerc commented Aug 12, 2021

Export Otel configuration in shell steps OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS...

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Tested with https://github.com/cyrille-leclerc/opentelemetry-maven-extension

@cyrille-leclerc cyrille-leclerc requested a review from v1v August 12, 2021 11:42
@cyrille-leclerc cyrille-leclerc changed the title Export Otel configuration in shell steps OTEL_EXPORTER_OTLP_ENDPOINT Export Otel configuration in shell steps: OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS Aug 12, 2021
Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

is it possible to mask those env variables to avoid exposing them?

For instance:

@cyrille-leclerc
Copy link
Contributor Author

cyrille-leclerc commented Aug 16, 2021

Great idea @v1v. I propose to do it as a subsequent milestone. In the meantime, people can disable the feature if needed. Would that work for you?

Copy link
Member

@v1v v1v left a comment

Choose a reason for hiding this comment

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

@@ -71,6 +75,8 @@

private transient OpenTelemetrySdkProvider openTelemetrySdkProvider;

private boolean dontExportOtelConfigurationAsEnvironmentVariables;
Copy link
Member

@v1v v1v Aug 17, 2021

Choose a reason for hiding this comment

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

I propose to do it as a subsequent milestone. In the meantime, people can disable the feature if needed. Would that work for you?

What do you think to set it false meantime? Then, whoever enables it will know the security implications

Suggested change
private boolean dontExportOtelConfigurationAsEnvironmentVariables;
private boolean dontExportOtelConfigurationAsEnvironmentVariables = false;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to enable this by default as I feel it's not a security problem in most cases because it's just telemetry.

false is the default value as the config attribute is "Don't export Otel Configuration as Environment Variables". Due to the way Jenkins XStream works, there would be some work to make it true by default, I would define the opposite variable instead: exportOtelConfigurationAsEnvironmentVariables.

@v1v v1v self-requested a review August 17, 2021 12:59
…BearerTokenAuthentication.java

Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
@cyrille-leclerc
Copy link
Contributor Author

FYI I'm trying to mask credentials in the console: https://groups.google.com/g/jenkinsci-dev/c/FMl_plHvPv8

@cyrille-leclerc
Copy link
Contributor Author

@v1v I followed your recommendation and I disabled the feature by default to ensure there is no surprise.

@cyrille-leclerc cyrille-leclerc merged commit 021085f into jenkinsci:master Aug 19, 2021
@cyrille-leclerc cyrille-leclerc deleted the export-otel-config-as-environment-variables-in-shell-scripts branch August 19, 2021 21:39
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.

3 participants