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

chore: url validation & README to prevent gRPC footguns. #2130

Merged
merged 15 commits into from
May 3, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
extend README and url validation to avoid footguns.
  • Loading branch information
lizthegrey committed Apr 30, 2021
commit 3644ac407cdf0ba70067c5a97c7b97adf2a36270
50 changes: 40 additions & 10 deletions packages/opentelemetry-exporter-collector-grpc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ npm install --save @opentelemetry/exporter-collector-grpc
The CollectorTraceExporter in Node expects the URL to only be the hostname. It will not work with `/v1/trace`.

```js
const Graceful = require('node-graceful');
lizthegrey marked this conversation as resolved.
Show resolved Hide resolved

const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
const { CollectorTraceExporter } = require('@opentelemetry/exporter-collector-grpc');

const collectorOptions = {
serviceName: 'basic-service',
url: '<opentelemetry-collector-url>' // url is optional and can be omitted - default is localhost:4317
// url is optional and can be omitted - default is localhost:4317
url: '<collector-hostname>:<port>',
};

const provider = new BasicTracerProvider();
Expand All @@ -32,48 +35,70 @@ provider.addSpanProcessor(new SimpleSpanProcessor(exporter));

provider.register();

Graceful.on("exit", async () => {
lizthegrey marked this conversation as resolved.
Show resolved Hide resolved
await provider.shutdown();
});
```

By default, plaintext connection is used. In order to use TLS in Node.js, provide `credentials` option like so:

```js
const fs = require('fs');
// Must be 'grpc', _not_ '@grpc/grpc-js'
lizthegrey marked this conversation as resolved.
Show resolved Hide resolved
const grpc = require('grpc');
const Graceful = require('node-graceful');

const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
const { CollectorTraceExporter } = require('@opentelemetry/exporter-collector-grpc');

const collectorOptions = {
serviceName: 'basic-service',
url: '<opentelemetry-collector-url>', // url is optional and can be omitted - default is localhost:4317
credentials: grpc.credentials.createSsl(
fs.readFileSync('./ca.crt'),
fs.readFileSync('./client.key'),
fs.readFileSync('./client.crt')
)
// url is optional and can be omitted - default is localhost:4317
url: '<collector-hostname>:<port>',
credentials: grpc.credentials.createSsl(),
};

const provider = new BasicTracerProvider();
const exporter = new CollectorTraceExporter(collectorOptions);
provider.addSpanProcessor(new SimpleSpanProcessor(exporter));

provider.register();

Graceful.on("exit", async () => {
await provider.shutdown();
});
```

To see how to generate credentials, you can refer to the script used to generate certificates for tests [here](./test/certs/regenerate.sh)
To use mutual authentication, pass to the `createSsl()` constructor:

```js
credentials: grpc.credentials.createSsl(
fs.readFileSync('./ca.crt'),
fs.readFileSync('./client.key'),
fs.readFileSync('./client.crt')
),
```

To generate credentials for mutual authentication, you can refer to the script used to generate certificates for tests [here](./test/certs/regenerate.sh)

The exporter can be configured to send custom metadata with each request as in the example below:

```js
// Must be 'grpc', _not_ '@grpc/grpc-js'
const grpc = require('grpc');
const Graceful = require('node-graceful');

const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
const { CollectorTraceExporter } = require('@opentelemetry/exporter-collector-grpc');

const metadata = new grpc.Metadata();
// For instance, an API key or access token might go here.
metadata.set('k', 'v');

const collectorOptions = {
serviceName: 'basic-service',
url: '<opentelemetry-collector-url>', // url is optional and can be omitted - default is localhost:4317
// url is optional and can be omitted - default is localhost:4317
url: '<collector-hostname>:<port>',
metadata, // // an optional grpc.Metadata object to be sent with each request
};

Expand All @@ -82,6 +107,10 @@ const exporter = new CollectorTraceExporter(collectorOptions);
provider.addSpanProcessor(new SimpleSpanProcessor(exporter));

provider.register();

Graceful.on("exit", async () => {
await provider.shutdown();
});
```

Note, that this will only work if TLS is also configured on the server.
Expand All @@ -95,7 +124,8 @@ const { MeterProvider } = require('@opentelemetry/metrics');
const { CollectorMetricExporter } = require('@opentelemetry/exporter-collector-grpc');
const collectorOptions = {
serviceName: 'basic-service',
url: '<opentelemetry-collector-url>', // url is optional and can be omitted - default is localhost:55681
// url is optional and can be omitted - default is localhost:4317
url: '<collector-hostname>:<port>',
};
const exporter = new CollectorMetricExporter(collectorOptions);

Expand Down
3 changes: 3 additions & 0 deletions packages/opentelemetry-exporter-collector-grpc/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ export function onInit<ExportItem, ServiceRequest>(
config: CollectorExporterConfigNode
): void {
collector.grpcQueue = [];
if (collector.url.includes('/')) {
lizthegrey marked this conversation as resolved.
Show resolved Hide resolved
diag.warn('URL path cannot be set when using grpc');
}
const serverAddress = removeProtocol(collector.url);
const credentials: grpc.ChannelCredentials =
config.credentials || grpc.credentials.createInsecure();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,15 @@ const testCollectorMetricExporter = (params: TestParams) =>
const args = spyLoggerWarn.args[0];
assert.strictEqual(args[0], 'Headers cannot be set when using grpc');
});
it('should warn about path in url', () => {
const spyLoggerWarn = sinon.stub(diag, 'warn');
collectorExporter = new CollectorMetricExporter({
serviceName: 'basic-service',
url: address + '/v1/metrics',
});
const args = spyLoggerWarn.args[0];
assert.strictEqual(args[0], 'URL path cannot be set when using grpc');
});
});

describe('export', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,15 @@ const testCollectorExporter = (params: TestParams) =>
const args = spyLoggerWarn.args[0];
assert.strictEqual(args[0], 'Headers cannot be set when using grpc');
});
it('should warn about path in url', () => {
const spyLoggerWarn = sinon.stub(diag, 'warn');
collectorExporter = new CollectorTraceExporter({
serviceName: 'basic-service',
url: address + '/v1/trace',
});
const args = spyLoggerWarn.args[0];
assert.strictEqual(args[0], 'URL path cannot be set when using grpc');
lizthegrey marked this conversation as resolved.
Show resolved Hide resolved
});
});

describe('export', () => {
Expand Down