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

OTLP exporters should not percent decode the key when parsing HEADERS env vars #5623

Open
pellared opened this issue Jul 16, 2024 · 3 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed pkg:exporter:otlp Related to the OTLP exporter package

Comments

@pellared
Copy link
Member

The OTLP exporters SHOULD NOT percent decode the header keys when parsing OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_TRACES_HEADERS, OTEL_EXPORTER_OTLP_METRICS_HEADERS, OTEL_EXPORTER_OTLP_LOGS_HEADERS env vars. Moreover, only valid header keys should be parsed.

Current behavior:

name, err := url.PathUnescape(n)
if err != nil {
global.Error(err, "escape header key", "key", n)
continue
}

The specification says https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#specifying-headers-via-environment-variables:

The OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_TRACES_HEADERS, OTEL_EXPORTER_OTLP_METRICS_HEADERS, OTEL_EXPORTER_OTLP_LOGS_HEADERS environment variables will contain a list of key value pairs, and these are expected to be represented in a format matching to the W3C Baggage, except that additional semi-colon delimited metadata is not supported, i.e.: key1=value1,key2=value2. All attribute values MUST be considered strings.

From W3C Baggage spec https://www.w3.org/TR/baggage/#key:

A token which identifies a value in the baggage. token is defined in RFC7230, Section 3.2.6. Leading and trailing whitespaces (OWS) are allowed and are not considered to be a part of the key.

NOTE
Though the baggage header is a [UTF-8] encoded [UNICODE] string, key is limited to the ASCII code points allowed by the definition of token in [RFC7230]. This is due to the implementation details of stable implementations prior to the writing of this specification.

There is nothing about percent encoding. Only the values are supposed to be percent decoded.

Notice that the implementation should also make sure that only valid key characters are used.

@pellared pellared added bug Something isn't working pkg:exporter:otlp Related to the OTLP exporter package labels Jul 16, 2024
@pellared pellared changed the title OTLP exporters should not percent decode the key when parsing header env vars OTLP exporters should not percent decode the key when parsing HEADERS env vars Jul 16, 2024
@pellared pellared added help wanted Extra attention is needed good first issue Good for newcomers labels Jul 31, 2024
@zhihali
Copy link
Contributor

zhihali commented Aug 10, 2024

I will open a PR for it...

@zhihali
Copy link
Contributor

zhihali commented Aug 20, 2024

the OTLPlog should be updated as well, I have raised an issue related #5722, and will implement a fix soon

MrAlias added a commit that referenced this issue Aug 21, 2024
… HEADERS env vars (#5705)

Bugfix #5623

As stated in the issue, we need to avoid parsing the key and instead
implement a validation check for it. I've added some unit tests to
verify this fix.

However, I noticed a comment at the top of this file:
```
// Code created by gotmpl. DO NOT MODIFY.
// source: internal/shared/otlp/envconfig/envconfig.go.tmpl
```
It seems that `internal/shared/otlp/envconfig/envconfig.go.tmpl` is the
source template for this file. Since this template matches
`exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go`,
I updated the template to maintain consistency. I’m not entirely sure if
this approach is correct, so please confirm if this is the right course
of action.

---------

Co-authored-by: Fools <54661071+Charlie-lizhihan@users.noreply.github.com>
Co-authored-by: Damien Mathieu <42@dmathieu.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@pellared
Copy link
Member Author

pellared commented Sep 5, 2024

@zhihali, this is for all OTLP exporters. I see that OTLP trace and metric exporters are already fixed. Therefore, only OTLP log exporters need bugfixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed pkg:exporter:otlp Related to the OTLP exporter package
Projects
Status: Low priority
Development

Successfully merging a pull request may close this issue.

2 participants