diff --git a/CHANGELOG.md b/CHANGELOG.md index ea0653c181d..59f25bdcbc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,11 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2 ### :boom: Breaking Changes +* fix(resources): update `OTEL_RESOURCE_ATTRIBUTES` parsing to match spec changes (open-telemetry/opentelemetry-specification#4856) [#6261](https://github.com/open-telemetry/opentelemetry-js/pull/6261) @jacksonweber + * **Important:** This fix is included in the "breaking changes" section because it can be breaking for some edge case usage of `OTEL_RESOURCE_ATTRIBUTES`: + * `export OTEL_RESOURCE_ATTRIBUTES=foo=bar,spam` will now be fully ignored, because the `spam` entry is invalid (missing `=`). Per spec, any parsing error results in ignoring the entire environment variable. + * `export OTEL_RESOURCE_ATTRIBUTES='wat=" spaces "'` will now result in `{"wat": "\" spaces \""}` *with* the double-quotes included in the value. Before this change the implementation included brittle double-quoting to allow leading and trailing whitespace in the value. To support leading or trailing whitespace now, you must percent-encode the whitespace. Internal whitespace still works without encoding, e.g. `export OTEL_RESOURCE_ATTRIBUTES='green=eggs and ham'`. + ### :rocket: Features ### :bug: Bug Fixes diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index dd4a23c44d0..dbf3d53db7b 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -764,16 +764,8 @@ describe('Node SDK', () => { }); describe('with a diag logger', () => { - // Local functions to test if a mocked method is ever called with a specific argument or regex matching for an argument. + // Local function to test if a mocked method is ever called with regex matching for an argument. // Needed because of race condition with parallel detectors. - const callArgsContains = ( - mockedFunction: Sinon.SinonSpy, - arg: any - ): boolean => { - return mockedFunction.getCalls().some(call => { - return call.args.some(callarg => arg === callarg); - }); - }; const callArgsMatches = ( mockedFunction: Sinon.SinonSpy, regex: RegExp @@ -832,7 +824,7 @@ describe('Node SDK', () => { describe('with a faulty environment variable', () => { beforeEach(() => { - process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=\\attribute'; + process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=' + 'x'.repeat(300); }); it('prints correct error messages when EnvDetector has an invalid variable', async () => { @@ -850,9 +842,9 @@ describe('Node SDK', () => { sdk.start(); assert.ok( - callArgsContains( + callArgsMatches( mockedLoggerMethod, - 'EnvDetector failed: Attribute value should be a ASCII string with a length not exceed 255 characters.' + /EnvDetector failed: Attribute value exceeds the maximum length of 255 characters/ ) ); await sdk.shutdown(); @@ -954,7 +946,7 @@ describe('Node SDK', () => { it('should configure service instance id via OTEL_RESOURCE_ATTRIBUTES env var', async () => { process.env.OTEL_RESOURCE_ATTRIBUTES = - 'service.instance.id=627cc493,service.name=my-service,service.namespace'; + 'service.instance.id=627cc493,service.name=my-service,service.namespace=default'; const sdk = new NodeSDK(); sdk.start(); diff --git a/packages/opentelemetry-resources/src/detectors/EnvDetector.ts b/packages/opentelemetry-resources/src/detectors/EnvDetector.ts index 1266506bae5..b8176620d2c 100644 --- a/packages/opentelemetry-resources/src/detectors/EnvDetector.ts +++ b/packages/opentelemetry-resources/src/detectors/EnvDetector.ts @@ -34,16 +34,6 @@ class EnvDetector implements ResourceDetector { // OTEL_RESOURCE_ATTRIBUTES contains key value pair separated by '='. private readonly _LABEL_KEY_VALUE_SPLITTER = '='; - private readonly _ERROR_MESSAGE_INVALID_CHARS = - 'should be a ASCII string with a length greater than 0 and not exceed ' + - this._MAX_LENGTH + - ' characters.'; - - private readonly _ERROR_MESSAGE_INVALID_VALUE = - 'should be a ASCII string with a length not exceed ' + - this._MAX_LENGTH + - ' characters.'; - /** * Returns a {@link Resource} populated with attributes from the * OTEL_RESOURCE_ATTRIBUTES environment variable. Note this is an async @@ -62,7 +52,7 @@ class EnvDetector implements ResourceDetector { const parsedAttributes = this._parseResourceAttributes(rawAttributes); Object.assign(attributes, parsedAttributes); } catch (e) { - diag.debug(`EnvDetector failed: ${e.message}`); + diag.debug(`EnvDetector failed: ${e instanceof Error ? e.message : e}`); } } @@ -77,78 +67,77 @@ class EnvDetector implements ResourceDetector { * Creates an attribute map from the OTEL_RESOURCE_ATTRIBUTES environment * variable. * - * OTEL_RESOURCE_ATTRIBUTES: A comma-separated list of attributes describing - * the source in more detail, e.g. “key1=val1,key2=val2”. Domain names and - * paths are accepted as attribute keys. Values may be quoted or unquoted in - * general. If a value contains whitespace, =, or " characters, it must - * always be quoted. + * OTEL_RESOURCE_ATTRIBUTES: A comma-separated list of attributes in the + * format "key1=value1,key2=value2". The ',' and '=' characters in keys + * and values MUST be percent-encoded. Other characters MAY be percent-encoded. + * + * Per the spec, on any error (e.g., decoding failure), the entire environment + * variable value is discarded. * * @param rawEnvAttributes The resource attributes as a comma-separated list * of key/value pairs. - * @returns The sanitized resource attributes. + * @returns The parsed resource attributes. + * @throws Error if parsing fails (caller handles by discarding all attributes) */ private _parseResourceAttributes(rawEnvAttributes?: string): Attributes { if (!rawEnvAttributes) return {}; const attributes: Attributes = {}; const rawAttributes: string[] = rawEnvAttributes.split( - this._COMMA_SEPARATOR, - -1 + this._COMMA_SEPARATOR ); + for (const rawAttribute of rawAttributes) { const keyValuePair: string[] = rawAttribute.split( - this._LABEL_KEY_VALUE_SPLITTER, - -1 + this._LABEL_KEY_VALUE_SPLITTER ); + + // Per spec: ',' and '=' MUST be percent-encoded in keys and values. + // If we get != 2 parts, there's an unencoded '=' which is an error. if (keyValuePair.length !== 2) { - continue; + throw new Error( + `Invalid format for OTEL_RESOURCE_ATTRIBUTES: "${rawAttribute}". ` + + `Expected format: key=value. The ',' and '=' characters must be percent-encoded in keys and values.` + ); } - let [key, value] = keyValuePair; - // Leading and trailing whitespaces are trimmed. - key = key.trim(); - value = value.trim().split(/^"|"$/).join(''); - if (!this._isValidAndNotEmpty(key)) { - throw new Error(`Attribute key ${this._ERROR_MESSAGE_INVALID_CHARS}`); + + const [rawKey, rawValue] = keyValuePair; + const key = rawKey.trim(); + const value = rawValue.trim(); + + if (key.length === 0) { + throw new Error( + `Invalid OTEL_RESOURCE_ATTRIBUTES: empty attribute key in "${rawAttribute}".` + ); } - if (!this._isValid(value)) { - throw new Error(`Attribute value ${this._ERROR_MESSAGE_INVALID_VALUE}`); + + let decodedKey: string; + let decodedValue: string; + try { + decodedKey = decodeURIComponent(key); + decodedValue = decodeURIComponent(value); + } catch (e) { + throw new Error( + `Failed to percent-decode OTEL_RESOURCE_ATTRIBUTES entry "${rawAttribute}": ${e instanceof Error ? e.message : e}` + ); } - attributes[key] = decodeURIComponent(value); - } - return attributes; - } - /** - * Determines whether the given String is a valid printable ASCII string with - * a length not exceed _MAX_LENGTH characters. - * - * @param str The String to be validated. - * @returns Whether the String is valid. - */ - private _isValid(name: string): boolean { - return name.length <= this._MAX_LENGTH && this._isBaggageOctetString(name); - } + if (decodedKey.length > this._MAX_LENGTH) { + throw new Error( + `Attribute key exceeds the maximum length of ${this._MAX_LENGTH} characters: "${decodedKey}".` + ); + } - // https://www.w3.org/TR/baggage/#definition - private _isBaggageOctetString(str: string): boolean { - for (let i = 0; i < str.length; i++) { - const ch = str.charCodeAt(i); - if (ch < 0x21 || ch === 0x2c || ch === 0x3b || ch === 0x5c || ch > 0x7e) { - return false; + if (decodedValue.length > this._MAX_LENGTH) { + throw new Error( + `Attribute value exceeds the maximum length of ${this._MAX_LENGTH} characters for key "${decodedKey}".` + ); } + + attributes[decodedKey] = decodedValue; } - return true; - } - /** - * Determines whether the given String is a valid printable ASCII string with - * a length greater than 0 and not exceed _MAX_LENGTH characters. - * - * @param str The String to be validated. - * @returns Whether the String is valid and not empty. - */ - private _isValidAndNotEmpty(str: string): boolean { - return str.length > 0 && this._isValid(str); + return attributes; } } diff --git a/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts b/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts index 229579a6943..9d1b2f50144 100644 --- a/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts +++ b/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import * as assert from 'assert'; import { envDetector } from '../../../src'; import { resourceFromDetectedResource } from '../../../src/ResourceImpl'; import { describeNode } from '../../util'; @@ -26,7 +27,7 @@ describeNode('envDetector() on Node.js', () => { describe('with valid env', () => { before(() => { process.env.OTEL_RESOURCE_ATTRIBUTES = - 'k8s.pod.name="pod-xyz-123",k8s.cluster.name="c1",k8s.namespace.name="default",k8s.deployment.name="deployment%20name"'; + 'k8s.pod.name=pod-xyz-123,k8s.cluster.name=c1,k8s.namespace.name=default,k8s.deployment.name=deployment%20name'; }); after(() => { @@ -44,8 +45,65 @@ describeNode('envDetector() on Node.js', () => { }); }); - describe('with invalid env', () => { - const values = ['k8s.deployment.name="with spaces"']; + describe('with unencoded spaces in values', () => { + before(() => { + process.env.OTEL_RESOURCE_ATTRIBUTES = + 'k8s.deployment.name=deployment name with spaces'; + }); + + after(() => { + delete process.env.OTEL_RESOURCE_ATTRIBUTES; + }); + + it('should treat spaces as spaces and keep the attribute', async () => { + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual( + resource.attributes?.['k8s.deployment.name'], + 'deployment name with spaces' + ); + }); + }); + + describe('with quoted values (no special handling)', () => { + before(() => { + process.env.OTEL_RESOURCE_ATTRIBUTES = + 'k8s.deployment.name="deployment name"'; + }); + + after(() => { + delete process.env.OTEL_RESOURCE_ATTRIBUTES; + }); + + it('should preserve quotes as literal characters in values', async () => { + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual( + resource.attributes?.['k8s.deployment.name'], + '"deployment name"' + ); + }); + }); + + describe('with other special characters in values', () => { + before(() => { + process.env.OTEL_RESOURCE_ATTRIBUTES = + 'k8s.deployment.name=deployment%3Bname%5Cwith%5Cdelims'; + }); + + after(() => { + delete process.env.OTEL_RESOURCE_ATTRIBUTES; + }); + + it('should decode percent-encoded chars and preserve the value', async () => { + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual( + resource.attributes?.['k8s.deployment.name'], + 'deployment;name\\with\\delims' + ); + }); + }); + + describe('with invalid env (invalid percent-encoding)', () => { + const values = ['k8s.deployment.name=%E0%A4%']; for (const value of values) { describe(`value: '${value}'`, () => { @@ -57,7 +115,7 @@ describeNode('envDetector() on Node.js', () => { delete process.env.OTEL_RESOURCE_ATTRIBUTES; }); - it('should return empty resource', async () => { + it('should discard entire env var and return empty resource', async () => { const resource = resourceFromDetectedResource(envDetector.detect()); assertEmptyResource(resource); }); @@ -71,4 +129,113 @@ describeNode('envDetector() on Node.js', () => { assertEmptyResource(resource); }); }); + + describe('with partially invalid env (unencoded = in value)', () => { + before(() => { + process.env.OTEL_RESOURCE_ATTRIBUTES = + 'k8s.pod.name=pod-xyz-123,k8s.deployment.name=bad=value,k8s.cluster.name=c1'; + }); + + after(() => { + delete process.env.OTEL_RESOURCE_ATTRIBUTES; + }); + + it('should discard entire env var on any error per spec', async () => { + const resource = resourceFromDetectedResource(envDetector.detect()); + // Per spec: on any error, the entire value SHOULD be discarded + assertEmptyResource(resource); + }); + }); + + describe('edge cases for input handling', () => { + afterEach(() => { + delete process.env.OTEL_RESOURCE_ATTRIBUTES; + }); + + it('allows spaces in keys (spec does not forbid)', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = 'key with spaces=value'; + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual(resource.attributes?.['key with spaces'], 'value'); + }); + + it('discards entire env var when key is empty', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = '=value'; + const resource = resourceFromDetectedResource(envDetector.detect()); + assertEmptyResource(resource); + }); + + it('discards entire env var when missing key/value separator', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = 'novalue'; + const resource = resourceFromDetectedResource(envDetector.detect()); + assertEmptyResource(resource); + }); + + it('rejects keys exceeding 255 characters', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = `${'k'.repeat(300)}=value`; + const resource = resourceFromDetectedResource(envDetector.detect()); + assertEmptyResource(resource); + }); + + it('rejects values exceeding 255 characters', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = `k=${'a'.repeat(300)}`; + const resource = resourceFromDetectedResource(envDetector.detect()); + assertEmptyResource(resource); + }); + + it('discards entire env var with invalid percent-encoding', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = 'k=%E0%A4%'; + const resource = resourceFromDetectedResource(envDetector.detect()); + assertEmptyResource(resource); + }); + + it('allows non-printable ASCII after decoding (spec allows)', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = 'k=%00'; + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual(resource.attributes?.['k'], '\0'); + }); + + it('properly decodes percent-encoded comma and equals', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = + 'key%3Dwith%3Dequals=value%2Cwith%2Ccommas'; + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual( + resource.attributes?.['key=with=equals'], + 'value,with,commas' + ); + }); + }); + + describe('service name and error handling', () => { + afterEach(() => { + delete process.env.OTEL_RESOURCE_ATTRIBUTES; + delete process.env.OTEL_SERVICE_NAME; + // restore if stubbed + const detectorWithAny = envDetector as any; + if (detectorWithAny._parseResourceAttributesBackup) { + detectorWithAny._parseResourceAttributes = + detectorWithAny._parseResourceAttributesBackup; + delete detectorWithAny._parseResourceAttributesBackup; + } + }); + + it('includes OTEL_SERVICE_NAME even when no attributes are set', async () => { + process.env.OTEL_SERVICE_NAME = 'svc-from-env'; + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual(resource.attributes?.['service.name'], 'svc-from-env'); + }); + + it('logs and continues when attribute parsing throws', async () => { + process.env.OTEL_RESOURCE_ATTRIBUTES = 'k=v'; + process.env.OTEL_SERVICE_NAME = 'svc'; + const detectorWithAny = envDetector as any; + detectorWithAny._parseResourceAttributesBackup = + detectorWithAny._parseResourceAttributes; + detectorWithAny._parseResourceAttributes = () => { + throw new Error('parse boom'); + }; + + const resource = resourceFromDetectedResource(envDetector.detect()); + assert.strictEqual(resource.attributes?.['service.name'], 'svc'); + }); + }); });