RequestDecorator modernisation (#683)

* Extract RequestDecorator into separate object and provide backwards compatibility with a deprecation notice * Hide fields delegated to requester decorator. Simplify loading decorator - it is not updated, only loaded once at the start. Reflect optionality of decorator in createDruidRequester. * Fix typo * Update docs.
allegro · Dec 1, 2020 · 3fd7d74 · 3fd7d74
1 parent cd4a0a7
commit 3fd7d74
Show file tree

Hide file tree

Showing 7 changed files with 172 additions and 97 deletions.
diff --git a/docs/example/request-decoration/config.yaml b/docs/example/request-decoration/config.yaml
@@ -6,10 +6,9 @@ clusters:
     type: druid
     host: the.druid.host:8082 # This happens to be my Docker machine
 
-    requestDecorator: './druid-request-decorator.js'
-
-    # The are being read by the druidRequestDecorator
-    decoratorOptions:
-      myUsername: Aladdin
-      myPassword: OpenSesame
+    requestDecorator:
+      path: './druid-request-decorator.js'
+      options:
+        username: Aladdin
+        password: OpenSesame
 
diff --git a/docs/example/request-decoration/druid-request-decorator.js b/docs/example/request-decoration/druid-request-decorator.js
@@ -1,32 +1,35 @@
 exports.version = 1;
 
-// logger - is just a collection of functions that you should use instead of console to have your logs included with the Swiv logs
+// logger - is just a collection of functions that you should use instead of console
 // params - is an object with the following keys:
-//   * options: the decoratorOptions part of the cluster object
+//   * options: the options field from the requestDecorator property
 //   * cluster: Cluster - the cluster object
 exports.druidRequestDecoratorFactory = function (logger, params) {
-  var options = params.options;
-  var myUsername = options.myUsername; // pretend we store the username and password
-  var myPassword = options.myPassword; // in the config
+  const options = params.options;
+  const username = options.username; // pretend we store the username and password
+  const password = options.password; // in the config
 
-  if (!myUsername) throw new Error('must have username');
-  if (!myPassword) throw new Error('must have password');
+  if (!username) {
+    throw new Error("must have username");
+  }
+  if (!password) {
+    throw new Error("must have password");
+  }
 
-  logger.log("Decorator init for username: " + myUsername);
+  logger.log("Decorator init for username: " + username);
 
-  var auth = "Basic " + Buffer(myUsername + ":" + myPassword).toString('base64');
+  const auth = "Basic " + Buffer.from(`${username}:${password}`).toString("base64");
 
   // decoratorRequest: DecoratorRequest - is an object that has the following keys:
   //   * method: string - the method that is used (POST or GET)
   //   * url: string -
   //   * query: Druid.Query -
   return function (decoratorRequest) {
-
-    var decoration = {
+    const decoration = {
       headers: {
         "Authorization": auth,
-        "X-I-Like": "Koalas"
-      }
+        "X-I-Like": "Koalas",
+      },
     };
 
     // This can also be async if instead of a value of a promise is returned.

diff --git a/docs/security.md b/docs/security.md
@@ -47,9 +47,25 @@ For example if you wanted your users to only see the data for "United States" yo
 
 Turnilo can authenticate to a Druid server via request decoration. You can utilize it as follows:
 
-In the config add a key of `druidRequestDecorator` that point to a relative js file.
+In the config add a key of `druidRequestDecorator` with property `path` that point to a relative js file.
 
-`druidRequestDecorator: './druid-request-decorator.js'`
+```yaml
+druidRequestDecorator: 
+    path: './druid-request-decorator.js'
+```
+
+You can also pass parameters to your decorator using `options` field. Content of this field will be read as json and passed
+to your `druidRequestDecoratorFactory` under `options` key in second parameter.
+
+```yaml
+druidRequestDecorator: 
+    path: './druid-request-decorator.js'
+    options:
+        keyA: valueA
+        keyB:
+          - firstElement
+          - secondElement
+```
 
 Then the contract is that your module should export a function `druidRequestDecorator` that has to return a decorator.
 
@@ -61,41 +77,24 @@ Here is an example decorator:
 ```javascript
 exports.version = 1;
 
-// logger - is just a collection of functions that you should use instead of console to have your logs included with the Turnilo logs
-// options - is an object with the following keys:
-//   * cluster: Cluster - the cluster object
 exports.druidRequestDecoratorFactory = function (logger, params) {
-  var options = params.options; // The options will be an array and hence access values with index position
-  var myUsername = options[0].myUsername; // pretend we store the username and password
-  var myPassword = options[0].myPassword; // in the config
-
-  if (!myUsername) throw new Error('must have username');
-  if (!myPassword) throw new Error('must have password');
-
-  logger.log("Decorator init for username: " + myUsername);
+  const options = params.options;
+  const username = options.username;
+  const password = options.password;
 
-  var auth = "Basic " + Buffer(myUsername + ":" + myPassword).toString('base64');
+  const auth = "Basic " + Buffer.from(`${username}:${password}`).toString("base64");
 
-  // decoratorRequest: DecoratorRequest - is an object that has the following keys:
-  //   * method: string - the method that is used (POST or GET)
-  //   * url: string -
-  //   * query: Druid.Query -
-  return function (decoratorRequest) {
-
-    var decoration = {
+  return function () {
+    return {
       headers: {
-        "Authorization": auth,
-        "X-I-Like": "Koalas"
-      }
+        "Authorization": auth
+      },
     };
-
-    // This can also be async if instead of a value of a promise is returned.
-    return decoration;
   };
 };
 ```
 
-You can find this example, with an example config, in the [./example](./example/request-decoration) folder.
+You can find this example with additional comments and example config in the [./example](./example/request-decoration) folder.
 
 This would result in all Druid requests being tagged as:
 

diff --git a/src/common/models/cluster/cluster.ts b/src/common/models/cluster/cluster.ts
@@ -18,7 +18,8 @@
 import { BackCompat, BaseImmutable, Property } from "immutable-class";
 import { External } from "plywood";
 import { URL } from "url";
-import { isTruthy, verifyUrlSafeName } from "../../utils/general/general";
+import { RequestDecorator, RequestDecoratorJS } from "../../../server/utils/request-decorator/request-decorator";
+import { isNil, isTruthy, verifyUrlSafeName } from "../../utils/general/general";
 
 export type SourceListScan = "disable" | "auto";
 
@@ -37,8 +38,7 @@ export interface ClusterValue {
   guardDataCubes?: boolean;
 
   introspectionStrategy?: string;
-  requestDecorator?: string;
-  decoratorOptions?: any;
+  requestDecorator?: RequestDecorator;
 }
 
 export interface ClusterJS {
@@ -56,8 +56,7 @@ export interface ClusterJS {
   guardDataCubes?: boolean;
 
   introspectionStrategy?: string;
-  requestDecorator?: string;
-  decoratorOptions?: any;
+  requestDecorator?: RequestDecoratorJS;
 }
 
 function ensureNotNative(name: string): void {
@@ -131,8 +130,7 @@ export class Cluster extends BaseImmutable<ClusterValue, ClusterJS> {
       validate: [BaseImmutable.ensure.number, ensureNotTiny]
     },
     { name: "introspectionStrategy", defaultValue: Cluster.DEFAULT_INTROSPECTION_STRATEGY },
-    { name: "requestDecorator", defaultValue: null },
-    { name: "decoratorOptions", defaultValue: null },
+    { name: "requestDecorator", defaultValue: null, immutableClass: RequestDecorator },
     { name: "guardDataCubes", defaultValue: Cluster.DEFAULT_GUARD_DATA_CUBES }
   ];
 
@@ -144,6 +142,15 @@ export class Cluster extends BaseImmutable<ClusterValue, ClusterJS> {
       const oldHost = oldHostParameter(cluster);
       cluster.url = Cluster.HTTP_PROTOCOL_TEST.test(oldHost) ? oldHost : `http://${oldHost}`;
     }
+  }, {
+    condition: cluster => typeof cluster.requestDecorator === "string" || !isNil(cluster.decoratorOptions),
+    action: cluster => {
+      console.warn(`Cluster ${cluster.name} : requestDecorator as string and decoratorOptions fields are deprecated. Use object with path and options fields`);
+      cluster.requestDecorator = {
+        path: cluster.requestDecorator,
+        options: cluster.decoratorOptions
+      };
+    }
   }];
 
   public type = "druid";
@@ -163,8 +170,7 @@ export class Cluster extends BaseImmutable<ClusterValue, ClusterJS> {
 
   // Druid
   public introspectionStrategy: string;
-  public requestDecorator: string;
-  public decoratorOptions: any;
+  public requestDecorator: RequestDecorator;
 
   public getTimeout: () => number;
   public getSourceListScan: () => SourceListScan;

diff --git a/src/server/utils/cluster-manager/cluster-manager.ts b/src/server/utils/cluster-manager/cluster-manager.ts
@@ -22,21 +22,12 @@ import { DruidRequestDecorator } from "plywood-druid-requester";
 import { Logger } from "../../../common/logger/logger";
 import { Cluster } from "../../../common/models/cluster/cluster";
 import { noop } from "../../../common/utils/functional/functional";
+import { DruidRequestDecoratorModule } from "../request-decorator/request-decorator";
 import { properRequesterFactory } from "../requester/requester";
 
 const CONNECTION_RETRY_TIMEOUT = 20000;
 const DRUID_REQUEST_DECORATOR_MODULE_VERSION = 1;
 
-export interface RequestDecoratorFactoryParams {
-  options: any;
-  cluster: Cluster;
-}
-
-export interface DruidRequestDecoratorModule {
-  version: number;
-  druidRequestDecoratorFactory: (logger: Logger, params: RequestDecoratorFactoryParams) => DruidRequestDecorator;
-}
-
 // For each external we want to maintain its source and whether it should introspect at all
 export interface ManagedExternal {
   name: string;
@@ -75,13 +66,12 @@ export class ClusterManager {
   public initialConnectionEstablished: boolean;
   public introspectedSources: Record<string, boolean>;
   public version: string;
-  public requester: PlywoodRequester<any>;
   public managedExternals: ManagedExternal[] = [];
   public onExternalChange: (name: string, external: External) => Promise<void>;
   public onExternalRemoved: (name: string, external: External) => Promise<void>;
   public generateExternalName: (external: External) => string;
-  public requestDecoratorModule: DruidRequestDecoratorModule;
 
+  private requester: PlywoodRequester<any>;
   private sourceListRefreshInterval = 0;
   private sourceListRefreshTimer: NodeJS.Timer = null;
   private sourceReintrospectInterval = 0;
@@ -102,9 +92,7 @@ export class ClusterManager {
     this.onExternalChange = options.onExternalChange || emptyResolve;
     this.onExternalRemoved = options.onExternalRemoved || emptyResolve;
     this.generateExternalName = options.generateExternalName || getSourceFromExternal;
-
-    this.updateRequestDecorator();
-    this.updateRequester();
+    this.requester = this.initRequester();
 
     this.managedExternals.forEach(managedExternal => {
       managedExternal.external = managedExternal.external.attachRequester(this.requester);
@@ -159,40 +147,41 @@ export class ClusterManager {
     return this.onExternalRemoved(managedExternal.name, managedExternal.external);
   }
 
-  private updateRequestDecorator(): void {
+  private initRequester(): PlywoodRequester<any> {
+    const { cluster } = this;
+    const druidRequestDecorator = this.loadRequestDecorator();
+
+    return properRequesterFactory({
+      cluster,
+      verbose: this.verbose,
+      concurrentLimit: 5,
+      druidRequestDecorator
+    });
+  }
+
+  private loadRequestDecorator(): DruidRequestDecorator | undefined {
     const { cluster, logger, anchorPath } = this;
-    if (!cluster.requestDecorator) return;
+    if (!cluster.requestDecorator) return undefined;
+    let module: DruidRequestDecoratorModule;
 
-    var requestDecoratorPath = path.resolve(anchorPath, cluster.requestDecorator);
+    const requestDecoratorPath = path.resolve(anchorPath, cluster.requestDecorator.path);
     logger.log(`Loading requestDecorator from '${requestDecoratorPath}'`);
     try {
-      this.requestDecoratorModule = require(requestDecoratorPath);
+      module = require(requestDecoratorPath) as DruidRequestDecoratorModule;
     } catch (e) {
-      throw new Error(`error loading druidRequestDecorator module from '${requestDecoratorPath}': ${e.message}`);
+      logger.error(`error loading druidRequestDecorator module from '${requestDecoratorPath}': ${e.message}`);
+      return undefined;
     }
 
-    if (this.requestDecoratorModule.version !== DRUID_REQUEST_DECORATOR_MODULE_VERSION) {
-      throw new Error(`druidRequestDecorator module '${requestDecoratorPath}' has incorrect version`);
-    }
-  }
-
-  private updateRequester() {
-    const { cluster, logger, requestDecoratorModule } = this;
-
-    var druidRequestDecorator: DruidRequestDecorator = null;
-    if (cluster.type === "druid" && requestDecoratorModule) {
-      logger.log(`Cluster '${cluster.name}' creating requestDecorator`);
-      druidRequestDecorator = requestDecoratorModule.druidRequestDecoratorFactory(logger, {
-        options: cluster.decoratorOptions,
-        cluster
-      });
+    if (module.version !== DRUID_REQUEST_DECORATOR_MODULE_VERSION) {
+      logger.error(`druidRequestDecorator module '${requestDecoratorPath}' has incorrect version`);
+      return undefined;
     }
 
-    this.requester = properRequesterFactory({
-      cluster,
-      verbose: this.verbose,
-      concurrentLimit: 5,
-      druidRequestDecorator
+    logger.log(`Cluster '${cluster.name}' creating requestDecorator`);
+    return module.druidRequestDecoratorFactory(logger.addPrefix("DruidRequestDecoratorFactory"), {
+      options: cluster.requestDecorator.options,
+      cluster
     });
   }