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

* extend README and url validation to avoid footguns. * deal with grpc(s) while here * fix tests by moving serverAddress checks earlier * un-private so util can access * Update packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de> * s#grpc#@grpc/grpc-js# * address review feedback. * fix lint and nullcheck * I hate prettier --fix * address review feedback. * my prettier config has diverged :( * address lint & review feedback. Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de>
open-telemetry · May 3, 2021 · cd54a45 · cd54a45
1 parent 082abf9
commit cd54a45
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 25 deletions.
diff --git a/packages/opentelemetry-exporter-collector-grpc/README.md b/packages/opentelemetry-exporter-collector-grpc/README.md
@@ -23,57 +23,74 @@ const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-g
 
 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();
 const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
-
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
 ```
 
 By default, plaintext connection is used. In order to use TLS in Node.js, provide `credentials` option like so:
 
 ```js
 const fs = require('fs');
-const grpc = require('grpc');
+const grpc = require('@grpc/grpc-js');
+
 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();
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
+```
+
+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 see how to generate credentials, you can refer to the script used to generate certificates for tests [here](./test/certs/regenerate.sh)
+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
-const grpc = require('grpc');
+const grpc = require('@grpc/grpc-js');
+
 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
 };
 
@@ -82,6 +99,9 @@ const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
 ```
 
 Note, that this will only work if TLS is also configured on the server.
@@ -95,20 +115,24 @@ 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);
 
 // Register the exporter
-const meter = new MeterProvider({
+const provider = new MeterProvider({
   exporter,
   interval: 60000,
-}).getMeter('example-meter');
+})
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
 
 // Now, start recording data
+const meter = provider.getMeter('example-meter');
 const counter = meter.createCounter('metric_name');
 counter.add(10, { 'key': 'value' });
-
 ```
 
 ## Running opentelemetry-collector locally to see the traces

diff --git a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts
@@ -26,6 +26,7 @@ import {
   ServiceClientType,
 } from './types';
 import { ServiceClient } from './types';
+import { validateAndNormalizeUrl } from './util';
 
 /**
  * Collector Metric Exporter abstract base class
@@ -41,13 +42,16 @@ export abstract class CollectorExporterNodeBase<
   grpcQueue: GRPCQueueItem<ExportItem>[] = [];
   metadata?: Metadata;
   serviceClient?: ServiceClient = undefined;
+  serverAddress: string;
   private _send!: Function;
 
   constructor(config: CollectorExporterConfigNode = {}) {
     super(config);
     if (config.headers) {
       diag.warn('Headers cannot be set when using grpc');
     }
+
+    this.serverAddress = validateAndNormalizeUrl(this.url);
     this.metadata = config.metadata;
   }
   private _sendPromise(

diff --git a/packages/opentelemetry-exporter-collector-grpc/src/util.ts b/packages/opentelemetry-exporter-collector-grpc/src/util.ts
@@ -21,6 +21,7 @@ import { globalErrorHandler } from '@opentelemetry/core';
 import { collectorTypes } from '@opentelemetry/exporter-collector';
 import * as path from 'path';
 import { CollectorExporterNodeBase } from './CollectorExporterNodeBase';
+import { URL } from 'url';
 import {
   CollectorExporterConfigNode,
   GRPCQueueItem,
@@ -32,7 +33,6 @@ export function onInit<ExportItem, ServiceRequest>(
   config: CollectorExporterConfigNode
 ): void {
   collector.grpcQueue = [];
-  const serverAddress = removeProtocol(collector.url);
   const credentials: grpc.ChannelCredentials =
     config.credentials || grpc.credentials.createInsecure();
 
@@ -52,12 +52,12 @@ export function onInit<ExportItem, ServiceRequest>(
 
       if (collector.getServiceClientType() === ServiceClientType.SPANS) {
         collector.serviceClient = new packageObject.opentelemetry.proto.collector.trace.v1.TraceService(
-          serverAddress,
+          collector.serverAddress,
           credentials
         );
       } else {
         collector.serviceClient = new packageObject.opentelemetry.proto.collector.metrics.v1.MetricsService(
-          serverAddress,
+          collector.serverAddress,
           credentials
         );
       }
@@ -105,6 +105,17 @@ export function send<ExportItem, ServiceRequest>(
   }
 }
 
-function removeProtocol(url: string): string {
-  return url.replace(/^https?:\/\//, '');
+export function validateAndNormalizeUrl(url: string): string {
+  const target = new URL(url);
+  if (target.pathname !== '/') {
+    diag.warn(
+      'URL path should not be set when using grpc, the path part of the URL will be ignored.'
+    );
+  }
+  if (target.protocol !== '' && !target.protocol?.match(/(http|grpc)s?/)) {
+    diag.warn(
+      'URL protocol should be http(s):// or grpc(s)://. Using grpc://.'
+    );
+  }
+  return target.host;
 }
diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorMetricExporter.test.ts
@@ -129,7 +129,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
           )
         : undefined;
       collectorExporter = new CollectorMetricExporter({
-        url: address,
+        url: 'grpcs://' + address,
         credentials,
         serviceName: 'basic-service',
         metadata: params.metadata,
@@ -166,7 +166,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
 
     describe('instance', () => {
       it('should warn about headers', () => {
-        // Need to stub/spy on the underlying logger as the "diag" instance is global
+        // Need to stub/spy on the underlying logger as the 'diag' instance is global
         const spyLoggerWarn = sinon.stub(diag, 'warn');
         collectorExporter = new CollectorMetricExporter({
           serviceName: 'basic-service',
@@ -178,6 +178,18 @@ 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 should not be set when using grpc, the path part of the URL will be ignored.'
+        );
+      });
     });
 
     describe('export', () => {

diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorTraceExporter.test.ts
@@ -125,7 +125,7 @@ const testCollectorExporter = (params: TestParams) =>
         : undefined;
       collectorExporter = new CollectorTraceExporter({
         serviceName: 'basic-service',
-        url: address,
+        url: 'grpcs://' + address,
         credentials,
         metadata: params.metadata,
       });
@@ -143,7 +143,7 @@ const testCollectorExporter = (params: TestParams) =>
 
     describe('instance', () => {
       it('should warn about headers when using grpc', () => {
-        // Need to stub/spy on the underlying logger as the "diag" instance is global
+        // Need to stub/spy on the underlying logger as the 'diag' instance is global
         const spyLoggerWarn = sinon.stub(diag, 'warn');
         collectorExporter = new CollectorTraceExporter({
           serviceName: 'basic-service',
@@ -155,6 +155,18 @@ 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 should not be set when using grpc, the path part of the URL will be ignored.'
+        );
+      });
     });
 
     describe('export', () => {