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

Proposal for an admin REST and GraphQL Server #9

Merged
merged 5 commits into from
Nov 4, 2020

Conversation

gmcrobert
Copy link
Member

for Strimzi.

Signed-off-by: Graeme McRobert mcrobert@uk.ibm.com

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

I had a first quick pass. Anyway, it would be great to have one sentence per line. This is the way we follow for documentation as well because it simplifies the review (for how GitHub works) in order to allow us to comment on every single line and not the entire paragraph.

An API server will:
* provide a consolidated backend API for additional interfaces like a browser based UI or a CLI.
* be capable of supporting both REST and GraphQL interfaces.
* allow for more sophisticated APIs to be built on top of the existing APIs providing value-add to existing content.
Copy link
Member

Choose a reason for hiding this comment

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

what are the "existing" APIs it's referring to?

The GraphQL parser does not permit type re-definitions so, a name clash in the schema will create an error and the schema will be rejected. The proposal is to use the Java service interface to define a GraphQL module service which loads a schema for that module and a runtime wiring which is the definition that connects the nodes and properties to the implementation. The schema and wiring are then merged to give a single consolidated GraphQL API. This would allow a module to easily create both a REST interface and a GraphQL interface for the module and share a lot of the implementation between the two.

### Security
Vert.x networking is based on Netty and works in a very similar manner. When a request is received by the server, it is funneled through a pipe and that pipe has a number of interceptors or handlers that can inspect the contents at that point and ignore or modify it before allowing it to proceed through the pipe. Vert.x also creates a routing context that travels through the pipe with the request and is available for inspection and/or modification by all the handlers. A handler can decided whether the request should be passed on to the next downstream handler or the request is complete and the downstream handlers should be bypassed and they have access to the full request through the routing context. The handlers that are called for a particular request is defined by the Vert.x `Router` which maps patterns to particular handlers. To implement a security handler we add a handler that matches all paths, i.e. the root path (`/`) and ensure that it appears in the router at the top of the list of patterns. When the handler is called, it creates a generic security context and attaches the security context to the routing context making it accessible to downstream handlers.
Copy link
Member

Choose a reason for hiding this comment

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

talking about security, how you envisage authn and authz of users for calling REST or GraphQL API?

@gmcrobert
Copy link
Member Author

gmcrobert commented Jul 13, 2020

@ppatierno I have changed the file to have each sentence on a separate line. I will elaborate on the security stuff after I have put some more thought into it. Feel free to add some suggestions. Advice would be most welcome.

### The server
The proposal is to make a modular server using the Vert.x-Web toolkit which has strong support for creating both REST and GraphQL interfaces.
The server contains a single Vert.x verticle which listens for inbound traffic on 1 or more configurable ports.
Allowing multiple configurable ports will allow the server to expose different functionality on each port, the simplest example of this being exposing an HTTP API on one port and a separate HTTPS API on a different port.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume different functionality includes varying methods of authn and authz too? i.e. a mechanism to set different authed endpoints, one scram, one TLS etc?

Or is this handled by the security context piece in the Security section?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have rewritten this section to clarify the uses and have specifically cited the use of multiple authentication schemes as the main driver for needing to do this.

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

I left some questions for clarification. But LGTM overall. Great work, thanks.

### The server
The proposal is to make a modular server using the Vert.x-Web toolkit which has strong support for creating both REST and GraphQL interfaces.
The server contains a single Vert.x verticle which listens for inbound traffic on 1 or more configurable ports.
Allowing multiple configurable ports will allow the server to expose different functionality on each port, the simplest example of this being exposing an HTTP API on one port and a separate HTTPS API on a different port.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can add some examples for clarification what exactly does this mean and what could be the use-case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have provided the example of supporting multiple authentication schemes to demonstrate the use case. The idea behind this is to provide something similar to the advertised listeners in Kafka.

## Proposal

### The server
The proposal is to make a modular server using the Vert.x-Web toolkit which has strong support for creating both REST and GraphQL interfaces.
Copy link
Member

Choose a reason for hiding this comment

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

What is the value and the cost of supporting both REST and GraphQL? Not just for development, but also QE, maintenance etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not think it will add a huge amount of overhead supporting both. Both APIs will be retrieving similar data but in a slightly different way. This allows the collection and aggregation of the information to be done in a way that is common to both and the difference then becomes a small piece on top that formats the results slightly differently. Both apis still use a REST endpoint with the main difference being that the REST api has multiple resource paths and each path represents a resource whereas GraphQL has a single resource path and the different resources to be returned are defined in the payload so similar tools can be used to test both APIs. Both APIs will also be implemented using the Vert.x libraries so keeping the libraries in-sync should be straightforward.

Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal

@scholzj
Copy link
Member

scholzj commented Oct 7, 2020

I added it to the agenda for tomorrows community meeting to push others to review as well ...

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

My main comment here is about defining the URL structure of the API with respect to plugins from multiple vendors and supporting multiple API versions.

The server module itself does not contain any API implementations but defines a service interface using the Java SPI and will load modules that implement that interface using the Java Service Loader.
The implementation of the APIs that the server then exposes can be created independently and will be automatically included in the running server if they are available to the Service Loader.

The reason for the modular approach is that it allows the strimzi project to create a base set of functionality that can easily be extended by a downstream project.
Copy link
Member

Choose a reason for hiding this comment

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

While I don't have a problem with this in principal, I think we need to ensure that the URL structure of the API reflects the identity of the service and also its version. This is to prevent two different plugins (e.g. from different vendors) using the same name and also to provide API evolution between different versions of the same plugin (it's not clear to me whether a service could provide more than one API version).

Copy link
Member Author

Choose a reason for hiding this comment

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

@tombentley is it sufficient that the mount point cannot be empty (the root) and must be unique or were there other restrictions you had in mind? The versioning is a little more challenging because the world has moved away from versioning being present on the URI. A common way to version is to use the media type so, e.g. you would have application/vnd.strimzi.v1+json, application/vnd.strimzi.v2+json, etc and the type without versioning, i.e. application/json would always refer to the latest version. Do we want a registration object which allows an added service to specify its mount point, version and anything else that would be useful, e.g. a vendor identifier, but not include the version in the URI? This would of course imply that you couldn't have more than 1 version of the API loaded which makes sense as a true REST API has the same limitation. Handling different version is then the responsibility of the implementor of the API.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong preference for including the version in the URL and TBH I wasn't aware of a trend to use media types instead. That said, I think there is some value, for people how know Kubernetes' REST API, in doing something similar with api groups at least. So if it were me, I would be envisaging

  • GET / would return some description (presumably openapi) of the available endpoints
  • The common, strimzi provided plugins might live under /strimzi.io/... or even /core.strimzi.io/..., to allow for other Strimzi api groups in the future.
  • Plugins provided by some vendor might live under /example.com/...

The reason for the modular approach is that it allows the strimzi project to create a base set of functionality that can easily be extended by a downstream project.
Not only is it easy to extend the API but it can be done in isolation.
This removes the requirement of the extensions needing to track the strimzi source repositories.
For example, lets assume that strimzi only implements a Kafka administration API which provides a set of APIs based around the Kafka Admin Client.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example, lets assume that strimzi only implements a Kafka administration API which provides a set of APIs based around the Kafka Admin Client.
For example, let's assume that Strimzi only implements a Kafka administration API which provides a set of APIs based around the Kafka Admin Client.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will correct my colonial english ;-)

This removes the requirement of the extensions needing to track the strimzi source repositories.
For example, lets assume that strimzi only implements a Kafka administration API which provides a set of APIs based around the Kafka Admin Client.
A downstream project then would like to add a Schema Registry to the cluster and needs to add an API in support of the new functionality.
With the modular server, they can define the API and its implementation, bundle the new functionality into a JAR file and add the new JAR file to the classpath.
Copy link
Member

Choose a reason for hiding this comment

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

Would this really be classpath-based, or use java modules?

Copy link
Member Author

@gmcrobert gmcrobert Oct 12, 2020

Choose a reason for hiding this comment

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

When I originally wrote this, Strimzi had only just made the switch from Java 8 to Java 11. The update I did the other day I was trying to be agnostic of the two versions because I wasn't sure whether we were ready to exploit Java 11 as a community but I obviously failed. My preference is definitely use the Java 11 modules in which case this statement will need to be updated. If you agree that it should be Java 11 modules then I will update the document to be specific and correct.

For example, lets assume that strimzi only implements a Kafka administration API which provides a set of APIs based around the Kafka Admin Client.
A downstream project then would like to add a Schema Registry to the cluster and needs to add an API in support of the new functionality.
With the modular server, they can define the API and its implementation, bundle the new functionality into a JAR file and add the new JAR file to the classpath.
A secondary benefit of a modular approach is that it allows the strimzi user to configure the server to only contain the functionality that they would like to use providing for a leaner server.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A secondary benefit of a modular approach is that it allows the strimzi user to configure the server to only contain the functionality that they would like to use providing for a leaner server.
A secondary benefit of a modular approach is that it allows the strimzi user to configure the server to only contain the functionality that they would like to use, providing for a leaner server.

The Vert.x OpenAPI3 support will read an OpenAPI3 specification YAML file and allow the developer to map the original operations to handler methods.
It creates a Vert.x Router which can be mounted on a Vert.x HTTP/S server.
The module would implement the service interface defined by the server which will contain a method that the server can call to load the OpenAPI3 router and mount the router as a subrouter at a specified mount point to the root path.
So, for example, if the OpenAPI3 spec defined endpoints `GET /topics` and `POST /topics` and these were mounted on the root path at the mount point `/admin` then the server would listen for incoming requests of `GET /admin/topics` and `POST /admin/topics` and route them to the handlers defined in the module.
Copy link
Member

Choose a reason for hiding this comment

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

So with reference to my previous comment, I'm not sure if any plugin should be bindable to the root path. The root path could be used for discovery of the API provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

So if the response in the previous comment is sufficient.

@tombentley
Copy link
Member

Oh, one other thing: API should be spelt in upper case. "REST api" is a little jarring.

for Strimzi.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
per request for review purposes.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
Added more details in the security and client service sections.
Added a paragraph on the implementation of the business logic modules.
Expanded on the use of allowing multiple ports to be configured.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
@scholzj
Copy link
Member

scholzj commented Oct 29, 2020

@tombentley Is this now OK for you?

Vert.x networking is based on Netty and works in a very similar manner.
When a request is received by the server, it is funneled through a pipe and that pipe has a number of interceptors or handlers that can inspect the contents at that point and ignore or modify it before allowing it to proceed through the pipe.
Vert.x also creates a routing context that travels through the pipe with the request and is available for inspection and/or modification by all the handlers.
A handler can decided whether the request should be passed on to the next downstream handler or the request is complete and the downstream handlers should be bypassed and they have access to the full request through the routing context.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A handler can decided whether the request should be passed on to the next downstream handler or the request is complete and the downstream handlers should be bypassed and they have access to the full request through the routing context.
A handler can decide whether the request should be passed on to the next downstream handler or the request is complete and the downstream handlers should be bypassed and they have access to the full request through the routing context.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
@scholzj
Copy link
Member

scholzj commented Nov 4, 2020

Thanks for the proposal.

@scholzj scholzj merged commit 9853668 into strimzi:master Nov 4, 2020
chris-giblin pushed a commit to SeanRooooney/proposals that referenced this pull request Nov 13, 2020
* Proposal for an admin REST and GraphQL Server
for Strimzi.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Separate each sentence into its own line as

per request for review purposes.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Added further clarification.

Added more details in the security and client service sections.
Added a paragraph on the implementation of the business logic modules.
Expanded on the use of allowing multiple ports to be configured.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Corrections/amendments from review comments.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Fixed grammar error highlighted in review comments.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
chris-giblin pushed a commit to chris-giblin/proposals that referenced this pull request Dec 16, 2020
* Proposal for an admin REST and GraphQL Server
for Strimzi.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Separate each sentence into its own line as

per request for review purposes.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Added further clarification.

Added more details in the security and client service sections.
Added a paragraph on the implementation of the business logic modules.
Expanded on the use of allowing multiple ports to be configured.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Corrections/amendments from review comments.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Fixed grammar error highlighted in review comments.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
Signed-off-by: C. Giblin <cgi@zurich.ibm.com>
scholzj added a commit that referenced this pull request Feb 3, 2021
* feat: write Strimzi ui proposal (#6)

* feat: write Strimzi ui proposal

- initial proposal for a Strimzi ui
- covers high level implementation, goals, suggested deliverables and
points of discussion
- adds a template for future proposals, based on existing proposals in
the repo (happy to contribute this separately if desired)
- more detail to be added in following commits as required

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: address 1st round of comments

- remove template and readme (to be proposed in new PR)
- renumber proposal
- add additional links to mentioned technologies
- add partition and replica to first client UI delivery goal

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: 2nd round of discussion updates

- add topology diagram/clarify difference between backend and ui server

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: change capitalisation in topology legend

- changes capitalisation of Strimzi in topology legend

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: add proposed deployment, images folder

- move images to images folder
- add proposed deployment approach

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: add pseudo CR example, more deployment details

- adds a pseudo CR of a UI deployment for further discussion

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor:  update proposal given comments

- updates proposal for given comments, and to reflect the outcome of
the 16th July community call.

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* feat: add comments for browser support, a11y rulesets

- update proposal to cover suggested browser support, a11y rulesets

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* fix: spelling

- fixes spelling errors in last commit

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor:  rebase proposal

- changes from rebasing on latest proposals branch
- added links to strimzi-ui, and deeper dives

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* feat: add session management proposal (#1)

- add proposal for session management
- add sequence diagrams for session flow
- add session store proposal
- add pseudo CR for auth endpoints

Signed-off-by: nictownsend <47386186+nictownsend@users.noreply.github.com>

* refactor: update proposal

- updates proposal given recent discussions/developments

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: restore template, proposal updates

- restores template md file, removed in error
- updates proposal wording around security and affected projects

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: rebase post security proposal

- update document numbers/links based on new proposal number

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

* refactor: add license details

- adds license details for the key 3rd party tools referenced in
the proposal
- grammar/spelling fixes

Signed-off-by: Matthew Chirgwin <chirmatt@uk.ibm.com>

Co-authored-by: Matthew Chirgwin <CHIRMATT@uk.ibm.com>
Co-authored-by: nictownsend <47386186+nictownsend@users.noreply.github.com>
Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* Proposal for an admin REST and GraphQL Server (#9)

* Proposal for an admin REST and GraphQL Server
for Strimzi.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Separate each sentence into its own line as

per request for review purposes.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Added further clarification.

Added more details in the security and client service sections.
Added a paragraph on the implementation of the business logic modules.
Expanded on the use of allowing multiple ports to be configured.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Corrections/amendments from review comments.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>

* Fixed grammar error highlighted in review comments.

Signed-off-by: Graeme McRobert <mcrobert@uk.ibm.com>
Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* Proposal for Strimzi canary (#15)

* Proposal for Strimzi canary

Signed-off-by: Rob Shelly <rshelly@redhat.com>

* Some updates and fixes

Signed-off-by: Rob Shelly <rshelly@redhat.com>

* Updates after community discussion

Signed-off-by: Rob Shelly <rshelly@redhat.com>

* More community discussion updates

Signed-off-by: Rob Shelly <rshelly@redhat.com>

* Made some clarifications

Signed-off-by: Rob Shelly <rshelly@redhat.com>

* Added message scheme

Signed-off-by: Paolo Patierno <ppatierno@live.com>

* Fixed comments

Signed-off-by: Paolo Patierno <ppatierno@live.com>

Co-authored-by: Paolo Patierno <ppatierno@live.com>
Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* Updated index with Strimzi canary proposal (#17)

Signed-off-by: Paolo Patierno <ppatierno@live.com>
Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* Move Strimzi container images to Quay.io (#16)

* Move Strimzi container images to Quay.io

Signed-off-by: Jakub Scholz <www@scholzj.com>

* Handle README and mention more advanced features

Signed-off-by: Jakub Scholz <www@scholzj.com>

* Link to Docker Hub pricing

Signed-off-by: Jakub Scholz <www@scholzj.com>

* Be more clear which versions should be copied

Signed-off-by: Jakub Scholz <www@scholzj.com>

* Review comments: Clarify the 6 months retention which only applies for unused images

Signed-off-by: Jakub Scholz <www@scholzj.com>

* Rename file

Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* Add Kafka topic encryption proposal

Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* add details on message encryption, headers, policy

Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* incorporate vep's remarks, editing

Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* editing

Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* editing

Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* updates on integration aspects

Signed-off-by: C. Giblin <cgi@zurich.ibm.com>

* additional clarification on policy updates

* correct typos, remove section comment

Signed-off-by: C. Giblin <christopher.giblin@gmail.com>

* Fix proposal number and add a link to the index

Signed-off-by: Jakub Scholz <www@scholzj.com>

Co-authored-by: Matthew Chirgwin <43138971+matthew-chirgwin@users.noreply.github.com>
Co-authored-by: Matthew Chirgwin <CHIRMATT@uk.ibm.com>
Co-authored-by: nictownsend <47386186+nictownsend@users.noreply.github.com>
Co-authored-by: gmcrobert <mcrobert@uk.ibm.com>
Co-authored-by: robshelly <robshelly89@gmail.com>
Co-authored-by: Paolo Patierno <ppatierno@live.com>
Co-authored-by: Jakub Scholz <www@scholzj.com>
Co-authored-by: C. Giblin <cgi@zurich.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants