From 05aca23c1941775d1db7fc89243b2c30c970e0a4 Mon Sep 17 00:00:00 2001
From: Luiz Aoqui <luiz@hashicorp.com>
Date: Wed, 19 Oct 2022 12:13:20 -0400
Subject: [PATCH] Decode values from OTEL_RESOURCE_ATTRIBUTES (#2963)

* Decode values from OTEL_RESOURCE_ATTRIBUTES

The W3C spec specifies that values must be percent-encoded so when
reading the environment variable `OTEL_RESOURCE_ATTRIBUTES` the SDK
should decode them.

This is done by the `baggage` package, but its behaviour in case of
errors is slightly different from the current implementation of the SDK,
more specifically in cases where a key is missing a value. The SDK
returns a partial resource while the `bagage` package returns nil.

This may be considered a breaking change, so this commit fixes the
current implementation instead of using `baggage.Parse`.

* Add changelog entry for #2963

* Use otel.Handle on OTEL_RESOURCE_ATTRIBUTES decode error

* retain original value when decoding fails

* docs: update CHANGELOG

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
---
 CHANGELOG.md             |  1 +
 sdk/resource/env.go      | 11 ++++++++++-
 sdk/resource/env_test.go | 22 +++++++++++++++++++---
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4656e968b4a..e1da6302645 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Changed
 
+- Decode urlencoded values from the `OTEL_RESOURCE_ATTRIBUTES` environment variable. (#2963)
 - `sdktrace.TraceProvider.Shutdown` and `sdktrace.TraceProvider.ForceFlush` to not return error when no processor register. (#3268)
 - The `"go.opentelemetry.io/otel/exporters/prometheus".New` now also returns an error indicating the failure to register the exporter with Prometheus. (#3239)
 - The prometheus exporter will no longer try to enumerate the metrics it will send to prometheus on startup.
diff --git a/sdk/resource/env.go b/sdk/resource/env.go
index eb22d007922..1c349247b0a 100644
--- a/sdk/resource/env.go
+++ b/sdk/resource/env.go
@@ -17,9 +17,11 @@ package resource // import "go.opentelemetry.io/otel/sdk/resource"
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"os"
 	"strings"
 
+	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/attribute"
 	semconv "go.opentelemetry.io/otel/semconv/v1.12.0"
 )
@@ -88,7 +90,14 @@ func constructOTResources(s string) (*Resource, error) {
 			invalid = append(invalid, p)
 			continue
 		}
-		k, v := strings.TrimSpace(field[0]), strings.TrimSpace(field[1])
+		k := strings.TrimSpace(field[0])
+		v, err := url.QueryUnescape(strings.TrimSpace(field[1]))
+		if err != nil {
+			// Retain original value if decoding fails, otherwise it will be
+			// an empty string.
+			v = field[1]
+			otel.Handle(err)
+		}
 		attrs = append(attrs, attribute.String(k, v))
 	}
 	var err error
diff --git a/sdk/resource/env_test.go b/sdk/resource/env_test.go
index c50a0031c9b..62de6729e0a 100644
--- a/sdk/resource/env_test.go
+++ b/sdk/resource/env_test.go
@@ -43,7 +43,7 @@ func TestDetectOnePair(t *testing.T) {
 func TestDetectMultiPairs(t *testing.T) {
 	store, err := ottest.SetEnvVariables(map[string]string{
 		"x":             "1",
-		resourceAttrKey: "key=value, k = v , a= x, a=z",
+		resourceAttrKey: "key=value, k = v , a= x, a=z, b=c%2Fd",
 	})
 	require.NoError(t, err)
 	defer func() { require.NoError(t, store.Restore()) }()
@@ -51,12 +51,13 @@ func TestDetectMultiPairs(t *testing.T) {
 	detector := &fromEnv{}
 	res, err := detector.Detect(context.Background())
 	require.NoError(t, err)
-	assert.Equal(t, res, NewSchemaless(
+	assert.Equal(t, NewSchemaless(
 		attribute.String("key", "value"),
 		attribute.String("k", "v"),
 		attribute.String("a", "x"),
 		attribute.String("a", "z"),
-	))
+		attribute.String("b", "c/d"),
+	), res)
 }
 
 func TestEmpty(t *testing.T) {
@@ -102,6 +103,21 @@ func TestMissingKeyError(t *testing.T) {
 	))
 }
 
+func TestInvalidPercentDecoding(t *testing.T) {
+	store, err := ottest.SetEnvVariables(map[string]string{
+		resourceAttrKey: "key=%invalid",
+	})
+	require.NoError(t, err)
+	defer func() { require.NoError(t, store.Restore()) }()
+
+	detector := &fromEnv{}
+	res, err := detector.Detect(context.Background())
+	assert.NoError(t, err)
+	assert.Equal(t, NewSchemaless(
+		attribute.String("key", "%invalid"),
+	), res)
+}
+
 func TestDetectServiceNameFromEnv(t *testing.T) {
 	store, err := ottest.SetEnvVariables(map[string]string{
 		resourceAttrKey: "key=value,service.name=foo",