[stankiewicz]

Summary

Allows referencing secrets managed by Beam's Secret framework (e.g., GcpSecret / GoogleCloudSecretManager) inline inside --openTelemetryProperties values so sensitive tokens (such as OTLP exporter authentication headers) do not need to be passed in plaintext in PipelineOptions.

Details

• Updates SdkHarnessOptions.configureOpenTelemetry(PipelineOptions) to resolve inline ${...} placeholders (formatted as ${type:<secret_type>;<secret_param>:<value>}) via Secret.parseSecretOption(String) when populating JVM system properties on the worker, while preserving the unresolved placeholder in PipelineOptions.
• Example usage:

--openTelemetryProperties={"otel.exporter.otlp.metrics.headers":"api-key=${type:GcpSecret;version_name:projects/my-project/secrets/my-secret/versions/latest}"}

• Secret parameter values may contain : (such as ARNs or URLs), but cannot contain ; or }.
• Trims surrounding whitespace/newlines from fetched secret payloads and quotes regex replacement metacharacters ($ and \).
• Adds unit tests in SdkHarnessOptionsTest covering plain strings, single and multiple ${...} secret placeholders, parameter ordering, colons in parameter values, whitespace/newline trimming, $ and \ escaping, invalid secret specs / missing type parameter, and configureOpenTelemetry system property initialization.

[github-actions]

📋 Review Summary

This PR adds support for resolving Beam Secret placeholders (${type:<secret_type>;...}) in OpenTelemetry properties configured via SdkHarnessOptions. The implementation is clean, avoids mutating PipelineOptions (preventing secret leakage into serialized options), properly handles special regex characters and trailing whitespace, and includes comprehensive unit test coverage.

🔍 General Feedback

• The secret resolution logic in SdkHarnessOptions.resolveOpenTelemetryPropertyValue is well-scoped to only match ${type:...} placeholders, leaving other ${...} expressions intact.
• Good use of Matcher.quoteReplacement and .trim() when substituting secret values into property strings.
• Unit tests in SdkHarnessOptionsTest thoroughly cover plain values, single and multiple secret placeholders, special characters, invalid secret types, and non-mutation of PipelineOptions.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.07%. Comparing base (76f6e6d) to head (7b7344a).
⚠️ Report is 196 commits behind head on master.

Files with missing lines	Patch %	Lines
...org/apache/beam/sdk/options/SdkHarnessOptions.java	85.71%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #40579      +/-   ##
============================================
+ Coverage     56.16%   65.07%   +8.90%     
- Complexity     2288    13988   +11700     
============================================
  Files          1124     1729     +605     
  Lines        178336   108491   -69845     
  Branches       1489    11309    +9820     
============================================
- Hits         100170    70603   -29567     
+ Misses        75645    33680   -41965     
- Partials       2521     4208    +1687     

Flag	Coverage Δ
go	?
java	65.07% <85.71%> (-10.72%)	⬇️
python	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[shunping]

This pattern requires type: to be at the very beginning of the secret option string, but Secret.parseSecretOption doesn't actually enforce that restriction.

Have you considered making the regex more flexible to match the parser's behavior?

[github-actions]

Assigning reviewers:

R: @damccorm for label java.
Backup reviewer: Abacn (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

This pull request likely touches a core component ("core" label). Please review with scrutiny.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

[shunping]

LGTM, thanks!

[damccorm]

I wonder if this would be better as a generic function packaged in util alongside Secret? Right now, it is just used by OpenTelemetry, but I could imagine it being broadly useful, and it would be nice to make sure we do this consistently.

WDYT?

[damccorm]

Sorry, just missed mentioning this before the merge. It is fine to leave or move, I will defer to you

[stankiewicz]

I will move it in follow up pr, thanks for feedback

[shunping]

Yes, I agree. Moving this logic to a generic utility alongside Secret would be a great way to ensure we handle this consistently across IOs that accept a map of configuration, like icebergIO and kafkaIO.

This PR also highlights that using an inline secret string format is much more user-friendly than attempting to parse nested JSON. However, I am afraid this will confuse users since in some places we support JSON secret spec and others support inline secret string.

Do you consider it as a problem @damccorm ?

[damccorm]

I think the UX here is not particularly problematic (it mostly puts some work on us to handle it correctly, which I think is fine)