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

[confmap] Fix bug where expand didn't honor escaping #10560

Merged
merged 5 commits into from
Jul 10, 2024

Conversation

TylerHelmuth
Copy link
Member

Description

When we promoted confmap.unifyEnvVarExpansion to beta, we found that the new expansion logic in confmap wasn't handling escaping of $$ like it is supposed to. This PR fixes that bug, but adding escaping logic for $$.

@azunna1 this fixes the bug you mentioned in #10435 around the metricstransformprocessor:

  metricstransform:
    transforms:
      - include: '^k8s\.(.*)\.(.*)$$'
        match_type: regexp
        action: update
        new_name: 'kubernetes.$${1}.$${2}'
      - include: '^container_([0-9A-Za-z]+)_([0-9A-Za-z]+)_.*'
        match_type: regexp
        action: update
        new_name: 'container.$${1}.$${2}'

Testing

Added new unit tests explicitly for escaping logic

@TylerHelmuth TylerHelmuth requested review from a team and evan-bradley July 8, 2024 19:19
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.35%. Comparing base (8de544b) to head (b60ceb9).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10560      +/-   ##
==========================================
+ Coverage   92.33%   92.35%   +0.01%     
==========================================
  Files         393      395       +2     
  Lines       18647    18682      +35     
==========================================
+ Hits        17218    17253      +35     
  Misses       1069     1069              
  Partials      360      360              

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

@mx-psi mx-psi added the release:blocker The issue must be resolved before cutting the next release label Jul 9, 2024
@mx-psi mx-psi requested a review from codeboten July 10, 2024 07:49
@mx-psi mx-psi linked an issue Jul 10, 2024 that may be closed by this pull request
@codeboten codeboten merged commit 637b1f4 into open-telemetry:main Jul 10, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 10, 2024
@TylerHelmuth TylerHelmuth deleted the confmap-fix-escape-bug branch July 10, 2024 21:49
@danelson
Copy link

danelson commented Jul 11, 2024

@TylerHelmuth would you happen to know how I can identify if I am affected by this? I assumed something like the following would be problematic but it doesn't appear to be

transform/redact_sensitive_information:
  error_mode: ignore
  log_statements:
    - context: log
      statements:
        - replace_all_patterns(attributes, "value", "Bearer ey.*", "$$1<redacted>")
        - replace_pattern(body, "Bearer ey.*", "$$1<redacted>")

I have quite a few more uses of $$ being used in the transform processor and I was hoping to prove I wasn't affected. Worst case I guess I can just toggle off confmap.unifyEnvVarExpansion.

jvoravong added a commit to signalfx/splunk-otel-collector that referenced this pull request Jul 12, 2024
jvoravong added a commit to signalfx/splunk-otel-collector that referenced this pull request Jul 15, 2024
jvoravong added a commit to signalfx/splunk-otel-collector that referenced this pull request Jul 15, 2024
jvoravong added a commit to jvoravong/splunk-otel-collector that referenced this pull request Jul 15, 2024
jvoravong added a commit to jvoravong/splunk-otel-collector that referenced this pull request Jul 16, 2024
jvoravong added a commit to signalfx/splunk-otel-collector that referenced this pull request Jul 16, 2024
jvoravong added a commit to jvoravong/splunk-otel-collector that referenced this pull request Jul 16, 2024
@dmitryax
Copy link
Member

@danelson you likely will be affected. See #10713

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:blocker The issue must be resolved before cutting the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

metricstransform Regexp Config Causes Startup Failure in v0.104.0
6 participants