Prioritize plugins correctly if the same type of plugin exists at different levels in OAS (#2132)

packages/openapi-2-kong/src/common.js

@@ -86,14 +86,12 @@ export const HttpMethod = {
   trace: 'TRACE',
 };
 
-export type HttpMethodKeys = $Values<typeof HttpMethod>;
-
 export function isHttpMethodKey(key: string): boolean {
   const uppercaseKey = key.toUpperCase();
   return Object.values(HttpMethod).some(m => m === uppercaseKey);
 }
 
-export function getMethodAnnotationName(method: HttpMethodKeys): string {
+export function getMethodAnnotationName(method: HttpMethodType): string {
   return `${method}-method`.toLowerCase();
 }
 

packages/openapi-2-kong/src/kubernetes/__tests__/index.test.js

@@ -489,12 +489,8 @@ describe('index', () => {
         keyAuthPluginDoc('s2'),
         dummyPluginDoc('s3'),
         ingressDoc([keyAuthName('g0')], 'api-0.insomnia.rest', 'my-api-s0'),
-        ingressDoc([keyAuthName('g0'), keyAuthName('s1')], 'api-1.insomnia.rest', 'my-api-s1'),
-        ingressDoc(
-          [keyAuthName('g0'), keyAuthName('s2'), dummyName('s3')],
-          'api-2.insomnia.rest',
-          'my-api-s2',
-        ),
+        ingressDoc([keyAuthName('s1')], 'api-1.insomnia.rest', 'my-api-s1'),
+        ingressDoc([keyAuthName('s2'), dummyName('s3')], 'api-2.insomnia.rest', 'my-api-s2'),
       ]);
     });
 
@@ -522,14 +518,9 @@ describe('index', () => {
         keyAuthPluginDoc('p2'),
         dummyPluginDoc('p3'),
         ingressDoc([keyAuthName('g0')], 'api.insomnia.rest', 'my-api-s0', '/no-plugin'),
+        ingressDoc([keyAuthName('p1')], 'api.insomnia.rest', 'my-api-s0', '/plugin-0'),
         ingressDoc(
-          [keyAuthName('g0'), keyAuthName('p1')],
-          'api.insomnia.rest',
-          'my-api-s0',
-          '/plugin-0',
-        ),
-        ingressDoc(
-          [keyAuthName('g0'), keyAuthName('p2'), dummyName('p3')],
+          [keyAuthName('p2'), dummyName('p3')],
           'api.insomnia.rest',
           'my-api-s0',
           '/plugin-1',
@@ -573,14 +564,14 @@ describe('index', () => {
           '/path',
         ),
         ingressDocWithOverride(
-          [keyAuthName('g0'), keyAuthName('m1')],
+          [keyAuthName('m1')],
           'put-method',
           'api.insomnia.rest',
           'my-api-s0',
           '/path',
         ),
         ingressDocWithOverride(
-          [keyAuthName('g0'), keyAuthName('m2'), dummyName('m3')],
+          [keyAuthName('m2'), dummyName('m3')],
           'post-method',
           'api.insomnia.rest',
           'my-api-s0',

packages/openapi-2-kong/src/kubernetes/__tests__/plugins.test.js

@@ -10,11 +10,14 @@ import {
   getServerPlugins,
   normalizeOperationPlugins,
   normalizePathPlugins,
+  distinctByProperty,
+  prioritizePlugins,
 } from '../plugins';
 import { HttpMethod } from '../../common';
 import {
   dummyPluginDoc,
   keyAuthPluginDoc,
+  pluginDocWithName,
   pluginDummy,
   pluginKeyAuth,
 } from './util/plugin-helpers';
@@ -594,4 +597,145 @@ describe('plugins', () => {
       ]);
     });
   });
+
+  describe('prioritizePlugins', () => {
+    it('should return empty array if no plugins', () => {
+      const global = [];
+      const server = [];
+      const path = [];
+      const operation = [];
+
+      const result = prioritizePlugins(global, server, path, operation);
+      expect(result).toHaveLength(0);
+    });
+
+    it('should return all plugins if no two plugins are the same type', () => {
+      const p1 = pluginDocWithName('p1', 'custom-plugin-1');
+      const p2 = pluginDocWithName('p2', 'custom-plugin-2');
+      const p3 = pluginDocWithName('p3', 'custom-plugin-3');
+      const p4 = pluginDocWithName('p4', 'custom-plugin-4');
+
+      const global = [p1];
+      const server = [p2];
+      const path = [p3];
+      const operation = [p4];
+
+      const result = prioritizePlugins(global, server, path, operation);
+      expect(result).toEqual([...operation, ...path, ...server, ...global]);
+    });
+
+    it('should return operation plugin if same type exists in path, server and global', () => {
+      const pluginType = 'custom-plugin';
+      const p1 = pluginDocWithName('p1', pluginType);
+      const p2 = pluginDocWithName('p2', pluginType);
+      const p3 = pluginDocWithName('p3', pluginType);
+      const p4 = pluginDocWithName('p4', pluginType);
+
+      const global = [p1];
+      const server = [p2];
+      const path = [p3];
+      const operation = [p4];
+
+      const result = prioritizePlugins(global, server, path, operation);
+      expect(result).toEqual([...operation]);
+    });
+
+    it('should return path plugin if same type exists in server and global', () => {
+      const pluginType = 'custom-plugin';
+      const p1 = pluginDocWithName('p1', pluginType);
+      const p2 = pluginDocWithName('p2', pluginType);
+      const p3 = pluginDocWithName('p3', pluginType);
+
+      const global = [p1];
+      const server = [p2];
+      const path = [p3];
+      const operation = [];
+
+      const result = prioritizePlugins(global, server, path, operation);
+      expect(result).toEqual([...path]);
+    });
+
+    it('should return server plugin if same type exists in global', () => {
+      const pluginType = 'custom-plugin';
+      const p1 = pluginDocWithName('p1', pluginType);
+      const p2 = pluginDocWithName('p2', pluginType);
+
+      const global = [p1];
+      const server = [p2];
+      const path = [];
+      const operation = [];
+
+      const result = prioritizePlugins(global, server, path, operation);
+      expect(result).toEqual([...server]);
+    });
+
+    it('should prioritize as expected', () => {
+      const typeG = 'custom-plugin-1';
+      const typeS = 'custom-plugin-2';
+      const typeP = 'custom-plugin-3';
+      const typeO = 'custom-plugin-4';
+
+      // Note: the variable naming below is [applied-to][resolved-from]
+      // IE: po implies it is applied to a path, but that plugin type should be resolved by the operation
+      // Therefore, the result should only contain gg, ss, pp and oo. The others are duplicates
+      // of the same plugin type and should be filtered out due to prioritization.
+      const gg = pluginDocWithName('gg', typeG);
+
+      const gs = pluginDocWithName('gs', typeS);
+      const ss = pluginDocWithName('ss', typeS);
+
+      const gp = pluginDocWithName('gp', typeP);
+      const sp = pluginDocWithName('sp', typeP);
+      const pp = pluginDocWithName('pp', typeP);
+
+      const go = pluginDocWithName('go', typeO);
+      const so = pluginDocWithName('so', typeO);
+      const po = pluginDocWithName('po', typeO);
+      const oo = pluginDocWithName('oo', typeO);
+
+      const global = [gg, gs, gp, go];
+      const server = [ss, sp, so];
+      const path = [pp, po];
+      const operation = [oo];
+
+      const result = prioritizePlugins(global, server, path, operation);
+      expect(result).toEqual([oo, pp, ss, gg]);
+    });
+  });
+
+  describe('distinctByProperty()', () => {
+    it('returns empty array if no truthy items', () => {
+      expect(distinctByProperty([], i => i)).toHaveLength(0);
+      expect(distinctByProperty([undefined], i => i)).toHaveLength(0);
+      expect(distinctByProperty([null, undefined, ''], i => i)).toHaveLength(0);
+    });
+
+    it('should remove objects with the same property selector - removes 2/4', () => {
+      const item1 = { name: 'a', value: 'first' };
+      const item2 = { name: 'a', value: 'second' };
+      const item3 = { name: 'b', value: 'third' };
+      const item4 = { name: 'b', value: 'fourth' };
+      const items = [item1, item2, item3, item4];
+
+      // distinct by the name property
+      const filtered = distinctByProperty(items, i => i.name);
+
+      // Should remove item2 and item4
+      expect(filtered).toEqual([item1, item3]);
+    });
+
+    it('should remove objects with the same property selector - removes none', () => {
+      const item1 = { name: 'a', value: 'first' };
+      const item2 = { name: 'a', value: 'second' };
+      const item3 = { name: 'b', value: 'third' };
+      const item4 = { name: 'b', value: 'fourth' };
+      const items = [item1, item2, item3, item4];
+
+      // distinct by the value property
+      const filtered = distinctByProperty(items, i => i.value);
+
+      // Should remove no items
+      expect(filtered).toEqual(items);
+    });
+  });
 });

packages/openapi-2-kong/src/kubernetes/__tests__/util/plugin-helpers.js

@@ -17,6 +17,14 @@ export const pluginDummy = {
   },
 };
 
+export const pluginDocWithName = (name: string, pluginType: string) => ({
+  apiVersion: 'configuration.konghq.com/v1',
+  kind: 'KongPlugin',
+  metadata: {
+    name,
+  },
+  plugin: pluginType,
+});
 export const keyAuthPluginDoc = (suffix: string) => ({
   apiVersion: 'configuration.konghq.com/v1',
   config: {

packages/openapi-2-kong/src/kubernetes/index.js

@@ -2,8 +2,7 @@
 
 import { getMethodAnnotationName, getName, parseUrl } from '../common';
 import urlJoin from 'url-join';
-import { flattenPluginDocuments, getPlugins } from './plugins';
-import type { HttpMethodKeys } from '../common';
+import { flattenPluginDocuments, getPlugins, prioritizePlugins } from './plugins';
 import { pathVariablesToWildcard, resolveUrlVariables } from './variables';
 
 export function generateKongForKubernetesConfigFromSpec(
@@ -19,7 +18,7 @@ export function generateKongForKubernetesConfigFromSpec(
   // Initialize document collections
   const ingressDocuments = [];
 
-  const methodsThatNeedKongIngressDocuments: { [HttpMethodKeys]: boolean } = {};
+  const methodsThatNeedKongIngressDocuments: Set<HttpMethodType> = new Set<HttpMethodType>();
 
   // Iterate all global servers
   plugins.servers.forEach((sp, serverIndex) => {
@@ -27,8 +26,8 @@ export function generateKongForKubernetesConfigFromSpec(
     plugins.paths.forEach(pp => {
       // Iterate methods
       pp.operations.forEach(o => {
-        // Collect plugins for document
-        const pluginsForDoc = [...plugins.global, ...sp.plugins, ...pp.plugins, ...o.plugins];
+        // Prioritize plugins for doc
+        const pluginsForDoc = prioritizePlugins(plugins.global, sp.plugins, pp.plugins, o.plugins);
 
         // Identify custom annotations
         const annotations: CustomAnnotations = {
@@ -38,7 +37,7 @@ export function generateKongForKubernetesConfigFromSpec(
         const method = o.method;
         if (method) {
           annotations.overrideName = getMethodAnnotationName(method);
-          methodsThatNeedKongIngressDocuments[method] = true;
+          methodsThatNeedKongIngressDocuments.add(method);
         }
 
         // Create metadata
@@ -59,7 +58,7 @@ export function generateKongForKubernetesConfigFromSpec(
     });
   });
 
-  const methodDocuments = Object.keys(methodsThatNeedKongIngressDocuments).map(
+  const methodDocuments = Array.from(methodsThatNeedKongIngressDocuments).map(
     generateK8sMethodDocuments,
   );
   const pluginDocuments = flattenPluginDocuments(plugins);
@@ -74,7 +73,7 @@ export function generateKongForKubernetesConfigFromSpec(
   }: KongForKubernetesResult);
 }
 
-function generateK8sMethodDocuments(method: HttpMethodKeys): KubernetesMethodConfig {
+function generateK8sMethodDocuments(method: HttpMethodType): KubernetesMethodConfig {
   return {
     apiVersion: 'configuration.konghq.com/v1',
     kind: 'KongIngress',

packages/openapi-2-kong/src/kubernetes/plugins.js

@@ -197,3 +197,32 @@ export function normalizeOperationPlugins(operationPlugins: OperationPlugins): O
   const pluginsExist = operationPlugins.some(o => o.plugins.length);
   return pluginsExist ? operationPlugins : [blankOperation];
 }
+
+export function prioritizePlugins(
+  global: Array<KubernetesPluginConfig>,
+  server: Array<KubernetesPluginConfig>,
+  path: Array<KubernetesPluginConfig>,
+  operation: Array<KubernetesPluginConfig>,
+): Array<KubernetesPluginConfig> {
+  // Order in priority: operation > path > server > global
+  const plugins: Array<KubernetesPluginConfig> = [...operation, ...path, ...server, ...global];
+
+  // Select first of each type of plugin
+  return distinctByProperty(plugins, p => p.plugin);
+}
+
+export function distinctByProperty<T>(arr: Array<T>, propertySelector: (item: T) => any): Array<T> {
+  const result: Array<T> = [];
+  const set = new Set();
+
+  for (const item of arr.filter(i => i)) {
+    const selector = propertySelector(item);
+    if (set.has(selector)) {
+      continue;
+    }
+
+    set.add(selector);
+    result.push(item);
+  }
+  return result;
+}

packages/openapi-2-kong/types/k8plugins.flow.js

@@ -1,9 +1,7 @@
 // @flow
 
-import type { HttpMethodKeys } from '../src/common';
-
 declare type OperationPlugins = Array<{
-  method: ?HttpMethodKeys,
+  method: ?HttpMethodType,
   plugins: Array<KubernetesPluginConfig>,
 }>;
 

packages/openapi-2-kong/types/kubernetes-config.flow.js

@@ -1,7 +1,5 @@
 // @flow
 
-import type { HttpMethodKeys } from '../src/common';
-
 declare type K8sAnnotations = {
   [string]: string,
 };
@@ -40,7 +38,7 @@ declare type KubernetesMethodConfig = {
     name: string,
   },
   route: {
-    methods: Array<HttpMethodKeys>,
+    methods: Array<HttpMethodType>,
   },
 };
 

packages/openapi-2-kong/types/openapi3.flow.js

@@ -206,3 +206,16 @@ declare type OpenApi3Spec = {
   externalDocs?: OA3ExternalDocs,
   'x-kong-name'?: string,
 };
+
+const HttpMethod = {
+  get: 'GET',
+  put: 'PUT',
+  post: 'POST',
+  delete: 'DELETE',
+  options: 'OPTIONS',
+  head: 'HEAD',
+  patch: 'PATCH',
+  trace: 'TRACE',
+};
+
+declare type HttpMethodType = $Values<typeof HttpMethod>;