Respect request validator plugin defined at the document root and hierarchy of plugin definitions #3293

develohpanda · 2021-04-15T06:01:35Z

Rules

The rules for which plugin to use and where to place it in the generated spec are the following:

all plugins on a route get derived from path -> operation
all plugins on a service get derived from root

There is one special case to do with the request-validator plugin. This is:

request validator plugin on a route gets derived from root -> path -> operation

Example

This means, if a plugin (eg. x-kong-plugin-key-auth) exists on the root, path and operation with different config at each level, in the generated declarative config, the:

service entity will have its plugin derived from the root
route entity will have its plugin derived from the operation (because operation takes precedence over path)

If the request validator plugin (x-kong-plugin-request-validator) exists on the root, in the generated declarative config, the:

service entity will have it's plugin derived from the root (or be a default config if nothing is user defined)
route entity will have its plugin derived from the root

Similarly, if the request validator plugin (x-kong-plugin-request-validator) exists on the root and operation, in the generated declarative config, the:

service entity will have it's plugin derived from the root (or be a default config if nothing is user defined)
route entity will have its plugin derived from the operation

Fixes INS-610, fixes INS-619

develohpanda · 2021-04-15T06:02:23Z

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

@@ -147,3 +147,20 @@ export function joinPath(p1: string, p2: string): string {

  return `${p1}/${p2}`;
 }
+
+// Select first unique instance of an array item depending on the property selector
+export function distinctByProperty<T>(arr: Array<T>, propertySelector: (item: T) => any): Array<T> {


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

develohpanda · 2021-04-15T06:02:33Z

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

@@ -237,4 +238,40 @@ describe('common', () => {
      expect(result.pathname).toBe('/api/v1');
    });
  });
+
+  describe('distinctByProperty()', () => {


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

develohpanda · 2021-04-15T06:03:59Z

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

-    });
-  });
-
-  describe('generatePlugin()', () => {


Is only exported for this test, which is covered by a lot of upstream tests already.

develohpanda · 2021-04-15T06:05:06Z

packages/openapi-2-kong/src/declarative-config/__tests__/services.test.js

@@ -67,6 +67,332 @@ describe('services', () => {
      ]);
    });

+    it('generates routes with request validator plugin from operation over path over global', async () => {


sonicyeti

LGTM!!!

Tieske · 2021-04-16T23:06:44Z

in the Declarative Config only the first server is used

I discussed with @sonicyeti that all servers (that differ only by host or port, differences by protocol or path should be an error) should be included as targets in the upstream. Just mentioning it here, since I do not know what the impact is exactly.

Note: root and server plugins are treated similarly because in the Declarative Config only the first server is used. However, if the same plugin is defined at the root and server, the config will be derived from the server.

I do not get the distinction between servers and root because servers essentially is a property of root so that is always a 1-on-1 relation ship. So imo adding anything to servers doesn't make sense, everything should be defined at root.

Expanding on that;

currently only the top-level servers is used, and servers defined on path or operation level are ignored.
we create 1 service and 1 upstream per spec. But technically a service is derived from servers (as in the accompanying upstream.
if we ever support servers on operation and path levels, those would become additional service and upstream entities. And also here; servers is a property of the operation and/or path, so also a 1-on-1 relationship.

develohpanda · 2021-04-19T03:51:53Z

in the Declarative Config only the first server is used

I discussed with @sonicyeti that all servers (that differ only by host or port, differences by protocol or path should be an error) should be included as targets in the upstream. Just mentioning it here, since I do not know what the impact is exactly.

Right, so its a requirement for all servers to follow that rule (which we should enforce in the generator logic), therefore allowing for a single upstream with multiple targets and a single service to be created in the Declarative Config.

if we ever support servers on operation and path levels, those would become additional service and upstream entities

My running understanding/assumption was that each server in the OAS root should map to an individual upstream and an individual service in the Declarative Config. Meaning, multiple servers in the OAS root resulting in multiple upstreams/services in the generated config (going by how the K8s generation works right now, which isn't a great example either). But according to this bullet point, it says that servers being defined at a different level is what controls whether new entities are created.

For instance, if an operation contains a servers property with multiple servers, a new service/upstream would be created in the DC but each server in that property (operation.servers) can only differ by host or port and each server will be created as a target in the new upstream.

technically a service is derived from servers (as in the accompanying upstream.

Essentially, you're saying a service/upstream is derived from the servers property on a parent (be it root, path or operation) and the array items contained within, not per entry.

If I am understanding your explanation correctly, I can see why plugins on a server in the OAS don't actually make sense and shouldn't be there (and that the K8s generator is incorrect).

Tieske · 2021-04-19T20:08:58Z

If I am understanding your explanation correctly, I can see why plugins on a server in the OAS don't actually make sense and shouldn't be there (and that the K8s generator is incorrect).

@develohpanda that understanding is correct

…rvice

develohpanda added the PA-openapi-2-kong Package: OpenAPI 2 Kong label Apr 15, 2021

develohpanda self-assigned this Apr 15, 2021

vercel bot temporarily deployed to Preview April 15, 2021 06:06 Inactive

develohpanda commented Apr 15, 2021

View reviewed changes

develohpanda marked this pull request as ready for review April 15, 2021 06:12

develohpanda requested review from Tieske and dimitropoulos April 15, 2021 06:12

develohpanda force-pushed the feature/o2k-request-plugins branch from e535aa5 to e41e047 Compare April 16, 2021 02:56

develohpanda force-pushed the feature/root-request-validater-plugin branch from b203442 to b44a251 Compare April 16, 2021 03:57

vercel bot temporarily deployed to Preview April 16, 2021 03:57 Inactive

vercel bot temporarily deployed to Preview April 16, 2021 04:37 Inactive

develohpanda requested a review from sonicyeti April 16, 2021 04:40

sonicyeti approved these changes Apr 16, 2021

View reviewed changes

vercel bot temporarily deployed to Preview April 19, 2021 06:37 Inactive

develohpanda force-pushed the feature/o2k-request-plugins branch from 037c6ad to d3e6d59 Compare April 19, 2021 08:16

develohpanda force-pushed the feature/root-request-validater-plugin branch from a0a779f to 8e50bdf Compare April 19, 2021 08:18

vercel bot temporarily deployed to Preview April 19, 2021 08:18 Inactive

Base automatically changed from feature/o2k-request-plugins to develop April 20, 2021 01:04

develohpanda force-pushed the feature/root-request-validater-plugin branch from 8e50bdf to 07bd964 Compare April 20, 2021 01:06

vercel bot temporarily deployed to Preview April 20, 2021 01:06 Inactive

develohpanda added 7 commits April 20, 2021 13:11

feat: use root request validator plugin

07cb749

test: add test to validate hierarchy

77299af

chore: rename

2577bea

feat: consume path plugins on operation, consume global plugins on se…

1f6368e

…rvice

chore: comments

389a98c

test: add tests to ensure correct hierarchy

4f4cc5b

chore: simplify

6133ff3

develohpanda added 2 commits April 20, 2021 13:11

feat: place plugins on service and route entities according to the rules

8b63b51

fix: remove support for plugins defined on a server entry

a40f13f

develohpanda force-pushed the feature/root-request-validater-plugin branch from 07bd964 to a40f13f Compare April 20, 2021 01:11

vercel bot temporarily deployed to Preview April 20, 2021 01:11 Inactive

develohpanda merged commit 60a3faa into develop Apr 20, 2021

develohpanda deleted the feature/root-request-validater-plugin branch April 20, 2021 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect request validator plugin defined at the document root and hierarchy of plugin definitions #3293

Respect request validator plugin defined at the document root and hierarchy of plugin definitions #3293

develohpanda commented Apr 15, 2021 •

edited

Loading

develohpanda Apr 15, 2021

develohpanda Apr 15, 2021

develohpanda Apr 15, 2021

develohpanda Apr 15, 2021

sonicyeti left a comment

Tieske commented Apr 16, 2021

develohpanda commented Apr 19, 2021 •

edited

Loading

Tieske commented Apr 19, 2021

Respect request validator plugin defined at the document root and hierarchy of plugin definitions #3293

Respect request validator plugin defined at the document root and hierarchy of plugin definitions #3293

Conversation

develohpanda commented Apr 15, 2021 • edited Loading

Rules

Example

develohpanda Apr 15, 2021

Choose a reason for hiding this comment

develohpanda Apr 15, 2021

Choose a reason for hiding this comment

develohpanda Apr 15, 2021

Choose a reason for hiding this comment

develohpanda Apr 15, 2021

Choose a reason for hiding this comment

sonicyeti left a comment

Choose a reason for hiding this comment

Tieske commented Apr 16, 2021

develohpanda commented Apr 19, 2021 • edited Loading

Tieske commented Apr 19, 2021

develohpanda commented Apr 15, 2021 •

edited

Loading

develohpanda commented Apr 19, 2021 •

edited

Loading