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

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

Merged
merged 9 commits into from Apr 20, 2021

Conversation

develohpanda
Copy link
Contributor

@develohpanda develohpanda commented Apr 15, 2021

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 develohpanda added the PA-openapi-2-kong Package: OpenAPI 2 Kong label Apr 15, 2021
@develohpanda develohpanda self-assigned this Apr 15, 2021
@@ -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> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

describe('distinctByProperty()', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
});

describe('generatePlugin()', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@develohpanda develohpanda marked this pull request as ready for review April 15, 2021 06:12
@develohpanda develohpanda force-pushed the feature/root-request-validater-plugin branch from b203442 to b44a251 Compare April 16, 2021 03:57
Copy link
Contributor

@sonicyeti sonicyeti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!

@Tieske
Copy link
Member

Tieske commented Apr 16, 2021

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
Copy link
Contributor Author

develohpanda commented Apr 19, 2021

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
Copy link
Member

Tieske commented Apr 19, 2021

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

Base automatically changed from feature/o2k-request-plugins to develop April 20, 2021 01:04
@develohpanda develohpanda force-pushed the feature/root-request-validater-plugin branch from 8e50bdf to 07bd964 Compare April 20, 2021 01:06
@develohpanda develohpanda force-pushed the feature/root-request-validater-plugin branch from 07bd964 to a40f13f Compare April 20, 2021 01:11
@develohpanda develohpanda merged commit 60a3faa into develop Apr 20, 2021
@develohpanda develohpanda deleted the feature/root-request-validater-plugin branch April 20, 2021 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PA-openapi-2-kong Package: OpenAPI 2 Kong
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants