[HTTP] Versioned API router designs #151596

jloleysens · 2023-02-20T10:30:27Z

Summary

This PR contains the initial designs for our versioned router API. This contribution contains only types, any implementation will come in later PRs.

Previous PR #149943

jloleysens · 2023-02-20T10:47:40Z

A few additional points for discussion

How could/should we expose this to plugins?

If we want to give plugins the easiest access to the versioning toolkit, we should implement it such that the createVersionedRouter is wrapped behind a Core-exposed interface. Perhaps something like:

// ...really haven't given this much thought:
const versionedRouter = coreSetup.http.createVersionedRouter();
versionedRouter.post( ... );

Should we consider passing version directly to a route handler?

Note: consumers can implement this in the current design by doing something like:

const mySingleHandler = (version: '1' | '2', ctx, req, res) => { /* infer body type, switch/if-else and do stuff */ }

const versionedRoute = versionedRouter.post({ path: '/api/my-api/foo' });

versionedRoute
  .addVersion({ version: '1', validate: { /* some validation */ } }, async (ctx, req, res) => mySingleHandler('1', ctx, req, res))
  .addVersion({ version: '2', validate: { /* some other validation */ } }, async (ctx, req, res) => mySingleHandler('2', ctx, req, res))

...but a dedicated API designed to do this may be simpler to use.

In the current design we require a new handler for each version of a route. In certain cases it may be preferable for consumers to receive the version value via ctx. Design-wise: primary difference is we will require a single handler for all versions. The question is, how common is this case?

With a single handler things become a bit less ergonomic: consumers will need to switch/map this onto their appropriate handler and infer types of body, query and params.

I decided to keep the .get, .put etc. interface to keep things similar to the current IRouter

Taking @rudolf's point about familiarity of APIs further, the current design exposes a VersionedRouter with the same .get style registrars.

jloleysens · 2023-02-20T10:59:27Z

packages/core/versioning/core-version-http-server/src/example.ts

+ * Side Public License, v 1.
+ */
+
+import { schema } from '@kbn/config-schema';


Added this example as it is easier to follow than the doc comment version, we can remove this if we feel it is not helpful.

…fix'

jloleysens · 2023-02-20T11:01:14Z

packages/core/versioning/core-version-http-server/README.md

@@ -0,0 +1,13 @@
+# @kbn/core-version-http-server


Naming things, naming things... What to name things? Happy to change this. I chose to put this in a package outside core/http and follow the naming convention of -server. My guess is we will have the same thing later, but -browser.

rudolf

left some nits with thoughts on how we could future-proof the API even more, but these are all compatible changes we can make in the future so it doesn't have to block the PR

rudolf · 2023-02-20T10:59:07Z

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

+ * Options for a versioned route. Probably needs a lot more options like sunsetting
+ * of an endpoint etc.
+ */
+export interface AddVersionOpts<P, Q, B> {


this is probably not something that would often be used, but we could probably extend RouteConfigOptions to future proof the API so that e.g. changing the max body size or accepts content types doesn't require a new "parent" Router but can be done in a new version https://github.com/elastic/kibana/blob/main/packages/core/http/core-http-server/src/router/route.ts#L102

rudolf · 2023-02-20T11:07:24Z

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

+ * ```ts
+ * const versionedRoute = versionedRouter
+ *  .post({
+ *    path: '/api/my-app/foo',
+ *    options: { timeout: { payload: 60000 } },
+ *  })
+ *  // First version of the API, accepts { foo: string } in the body
+ *  .addVersion(
+ *    { version: '1', validate: { body: schema.object({ foo: schema.string() }) } },
+ *    async (ctx, req, res) => {
+ *      await ctx.fooService.create(req.body.foo);
+ *      return res.ok({ body: { foo: req.body.foo } });
+ *    }
+ *  )
+ *  // Second version of the API, accepts { fooName: string } in the body
+ *  .addVersion(
+ *    { version: '2', validate: { body: schema.object({ fooName: schema.string() }) } },
+ *    async (ctx, req, res) => {
+ *      await ctx.fooService.create(req.body.fooName);
+ *      return res.ok({ body: { fooName: req.body.fooName } });
+ *    }
+ *  );
+ * ```


nit: one of the cons of structuring an API by path instead of by version (our previous discussion) is that it becomes more awkward to make a breaking change to a path. So you have to create a new versionedRouter and it's no longer clear that it shares a "version history" with other paths/controllers.

So I wondered if we should allow a path to be specified in a version too. E.g. changing '/api/my-app/foo' -> '/api/my-app/foobar'

const versionedRoute = versionedRouter .post({ path: '/api/my-app/foo', options: { timeout: { payload: 60000 } }, }) // First version of the API, accepts { foo: string } in the body .addVersion( { version: '1', validate: { body: schema.object({ foo: schema.string() }) } }, async (ctx, req, res) => { await ctx.fooService.create(req.body.foo); return res.ok({ body: { foo: req.body.foo } }); } ) // Second version of the API, accepts { fooName: string } in the body and new path .addVersion( { version: '2', path: '/api/my-app/foobar', // <-- Version 2 makes a breaking change to the path validate: { body: schema.object({ fooName: schema.string() }) } }, async (ctx, req, res) => { await ctx.fooService.create(req.body.fooName); return res.ok({ body: { fooName: req.body.fooName } }); } );

This should not be a very common use case but it does happen.

That's a great point. I think it could definitely happen with parameters added/removed/changed in the path!

I'll incoroporate your changes, but leave this comment open for others!

A "breaking change to a path" is basicaly a new path so not sure it is a breaking change for the client. ('/api/my-app/foobar' is a new "v1" route). The example of JL(in example.ts) where he changes the parameter name from "name" to "id" is maybe an exception.

Good point @sebelga , I realise that example did not capture the nature of the issue correctly. That would be a change that is not visible to consumers of the API. I'll update to capture it correctly.

The breaking change in the example is in the body where we change foo to fooName. I was just hoping to show that you can change name of params if you wanted to.

'/api/my-app/foobar' is a new "v1" route

It could be modeled that way but doesn't have to be. In e.g. alerting we changed our terminology so what was previously called alerts now became rules and actions became alerts. To capture this change in domain language they might want to release a new version of the API where many paths would be different. Having two completely different endpoints both use alerts with a different meaning would be confusing so here the new version helps users navigate the change.

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

…http_toolkit.ts Co-authored-by: Ahmad Bamieh <[email protected]>

sebelga · 2023-02-20T16:00:11Z

Can you confirm I understand correctly the mechanism

We declare version "1" on first release
New version is released (2) but we don't need any data change so we don't declare any new route
For the next version (3) we need a change so we declare a version "3" on the router? We skip version "2" declaration is that correct?

TinaHeiligers

I think I'm missing something subtle in allowing path params to change. I left some comments/questions to clarify.

TinaHeiligers · 2023-02-20T21:58:58Z

packages/core/versioning/core-version-http-server/src/example.ts

+  .addVersion(
+    {
+      version: '2',
+      path: '/api/my-app/foo/{id?}', // Update the path to something new


path: '/api/my-app/foo/{id?}', // Update the path to something new
If routers also have to be backward compatible, then would it be better to accept both optional arguments? For example:

Version 2's path could be:
path: '/api/my-app/foo/{nameorid?}',
where nameorid = name | id | undefined

If someone still uses the "old" path with a name as the query param, and the route has changed only to accept an optional id, would we not consider that a breaking change and, therefore, not permitted?

My understanding is that we want public HTTP API's to be as stable as possible and any iterations on the route declaration should still work with older versions. Maybe I'm missing some subtlety here.

If someone still uses the "old" path with a name as the query param, and the route has changed only to accept an optional id, would we not consider that a breaking change and, therefore, not permitted?

Yes, that is a good point. I've updated the example to reflect this as a breaking change.

My understanding is that we want public HTTP API's to be as stable as possible and any iterations on the route declaration should still work with older versions. Maybe I'm missing some subtlety here.

Also a good point :). My reasoning was that we don't want breaking changes such that a later version removes an earlier version. But I think there are nuanced cases of breaking changes that we should consider allowing if they bring code/functionality to a place that is more maintainable.

CC @rudolf

Maybe the example I shared in #151596 (comment) is helpful.

We want any given version of the API to be stable, but we don't want to hold back teams. If they deem it necessary / beneficial to make a breaking change they should create a new version for that so that they're unblocked.

make a breaking change they should create a new version

Creating a new version for a new path makes me far more comfortable!

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts

jloleysens · 2023-02-21T09:33:57Z

New version is released (2) but we don't need any data change so we don't declare any new route

Would it be possible get a code example of a change like this? If I understand correctly: the intention is to not release new versions (only patch versions) for non-breaking changes.

[UPDATE]
I've updated the code example, so hopefully it is a bit clearer now.

jloleysens · 2023-02-21T09:58:10Z

CC @sebelga (forgot to add a ping)

sebelga · 2023-02-21T11:08:37Z

packages/core/versioning/core-version-http-server/src/example.ts

+  .addVersion(
+    {
+      version: '2',
+      path: '/api/my-app/foo/{id?}/{name?}', // Update "fooId" => "id", this is not a breaking change!


Why is it not a breaking change? Our code won't be able to access params.fooId, the value will now be under params.id right?

breaking changes refer to changes which break clients, we can always adapt our code to deal with "internal" breaking changes like this. So a client will still construct exactly the same query whether we map that path variable to id or fooId

sebelga · 2023-02-21T11:12:53Z

I'm still confused on what a route version maps to. Can you confirm that we won't declare them incrementally ("v1", "v2", "v3"...) but based on a release cadence (we might have "v1", "v7" route version but not the intermediary ones) ?

rudolf · 2023-02-21T16:20:02Z

I'm still confused on what a route version maps to. Can you confirm that we won't declare them incrementally ("v1", "v2", "v3"...) but based on a release cadence (we might have "v1", "v7" route version but not the intermediary ones) ?

A route version is introduced by a plugin team any time they think it's necessary to make a breaking change. For public APIs this cadence should be extremely slow, we can't remove APIs in < 18 months. But for internal APIs we should feel free to introduce a new version as often as it helps remove technical debt or support new features.

But api versions don't ever correspond to stack release versions.

TinaHeiligers · 2023-02-21T21:06:38Z

packages/core/versioning/core-version-http-server/src/example.ts

@@ -21,26 +21,62 @@ const versionedRouter = vtk.createVersionedRouter({ router });
 // @ts-ignore unused variable
 const versionedRoute = versionedRouter
  .post({
-    path: '/api/my-app/foo/{name?}',
+    path: '/api/my-app/foo/{fooId?}/{name?}', // <= design mistake, {name?} should be a query parameter, but it has been released...


Aside: What would the path be if fooId isn't defined? /api/my-app/foo/undefined/name or /api/my-app/foo//name or something else?

TinaHeiligers · 2023-02-21T22:10:48Z

api versions don't ever correspond to stack release versions

Decoupling api versions from stack release versions is what's tripping a lot of folks up, even though that was communicated early on. Maybe we didn't make the message clear enough or there are too many folks who don't have all the history. @rudolf is there some way we can communicate this more widely?

TinaHeiligers

We could iterate on the design forever but we have to make a start somewhere. The design looks good enough to me to start sharing.
Great work!

TinaHeiligers

LGTM

elasticmachine · 2023-02-22T11:32:13Z

Pinging @elastic/kibana-core (Team:Core)

jloleysens · 2023-02-22T11:33:20Z

Thanks for the early review @sebelga @rudolf @TinaHeiligers . I am going to merge this as our first iteration, we can continue updating it in following PRs.

kibana-ci · 2023-02-22T12:33:44Z

💚 Build Succeeded

Buildkite Build
Commit: 661d5f6

Metrics [docs]

✅ unchanged

History

💚 Build #109320 succeeded fe9b46f
💚 Build #109119 succeeded 2c16429
💚 Build #108967 succeeded 8b02ffd
💚 Build #108920 succeeded e110070
💔 Build #108907 failed 9d09f48

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jloleysens

sebelga · 2023-02-22T15:14:53Z

A route version is introduced by a plugin team any time they think it's necessary to make a breaking change. For public APIs this cadence should be extremely slow, we can't remove APIs in < 18 months. But for internal APIs we should feel free to introduce a new version as often as it helps remove technical debt or support new features.

Cool got it 👍

The piece of the puzzle that I was missing was that each browser client (per plugin) hardcodes their latest version when bundling. And that version is unique for each plugin.

## Summary Implements the designs from #151596 * Move `packages/versioning/*` into `packages/core/http` to follow existing structure more closely * Implements the first iteration of the versioned router as a wrapper/layer around the existing router * Adds some integration tests * Future work needed! Once we have a the versioned spec we should implement it in this wrapper layer * Validation is a little bit tricky because of when the `CoreKibanaResponse` object is instantiated, the approach taken here is to replace body, params, query on the route-level's request object Closes #149286 --------- Co-authored-by: kibanamachine <[email protected]>

jloleysens added 2 commits February 20, 2023 10:35

added initial package: core-version-http-server

649c587

added initial version of types

9d09f48

jloleysens self-assigned this Feb 20, 2023

ran bootstrap

02f41b2

jloleysens commented Feb 20, 2023

View reviewed changes

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

bd02291

…fix'

jloleysens commented Feb 20, 2023

View reviewed changes

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

e110070

rudolf approved these changes Feb 20, 2023

View reviewed changes

Bamieh reviewed Feb 20, 2023

View reviewed changes

packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts Outdated Show resolved Hide resolved

jloleysens and others added 3 commits February 20, 2023 14:51

Update packages/core/versioning/core-version-http-server/src/version_…

5b80d9c

…http_toolkit.ts Co-authored-by: Ahmad Bamieh <[email protected]>

updated comment

f70b66e

incoroporate rudolfs feedback and fix some typos

8b02ffd

TinaHeiligers reviewed Feb 20, 2023

View reviewed changes

updated example with some nuanced cases

313dd75

update doc comments with @experimental

2c16429

jloleysens added the discuss label Feb 21, 2023

sebelga reviewed Feb 21, 2023

View reviewed changes

jloleysens added 2 commits February 21, 2023 17:13

remove additional options for now and simplify example

c352f4c

updated doc comment

fe9b46f

TinaHeiligers reviewed Feb 21, 2023

View reviewed changes

TinaHeiligers self-requested a review February 21, 2023 22:39

TinaHeiligers approved these changes Feb 21, 2023

View reviewed changes

jloleysens marked this pull request as ready for review February 22, 2023 11:32

Merge branch 'main' into versioned-api-router-designs

661d5f6

jloleysens enabled auto-merge (squash) February 22, 2023 11:33

jloleysens merged commit acf7d01 into elastic:main Feb 22, 2023

kibanamachine added the backport:skip This commit does not require backporting label Feb 22, 2023

jloleysens deleted the versioned-api-router-designs branch February 22, 2023 15:53

TinaHeiligers mentioned this pull request Feb 23, 2023

[http] Prevent access to internal-only APIs when running in serverless #151940

Closed

This was referenced Feb 24, 2023

Versioned API Router #149286

Closed

[HTTP] Versioned browser client designs #152091

Closed

jloleysens mentioned this pull request Mar 6, 2023

[HTTP] Versioned API browser client designs #152194

Closed

jloleysens mentioned this pull request Mar 23, 2023

[HTTP] Versioned router implementation #153543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP] Versioned API router designs #151596

[HTTP] Versioned API router designs #151596

jloleysens commented Feb 20, 2023 •

edited

Loading

jloleysens commented Feb 20, 2023 •

edited

Loading

jloleysens Feb 20, 2023

jloleysens Feb 20, 2023

rudolf left a comment

rudolf Feb 20, 2023

rudolf Feb 20, 2023

jloleysens Feb 20, 2023

jloleysens Feb 20, 2023

sebelga Feb 20, 2023

jloleysens Feb 21, 2023

jloleysens Feb 21, 2023

rudolf Feb 21, 2023

sebelga commented Feb 20, 2023 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Feb 20, 2023

jloleysens Feb 21, 2023

rudolf Feb 21, 2023

TinaHeiligers Feb 21, 2023

jloleysens commented Feb 21, 2023 •

edited

Loading

jloleysens commented Feb 21, 2023

sebelga Feb 21, 2023

rudolf Feb 21, 2023 •

edited

Loading

sebelga commented Feb 21, 2023

rudolf commented Feb 21, 2023

TinaHeiligers Feb 21, 2023 •

edited

Loading

TinaHeiligers commented Feb 21, 2023

TinaHeiligers left a comment

TinaHeiligers left a comment

elasticmachine commented Feb 22, 2023

jloleysens commented Feb 22, 2023

kibana-ci commented Feb 22, 2023

sebelga commented Feb 22, 2023

[HTTP] Versioned API router designs #151596

[HTTP] Versioned API router designs #151596

Conversation

jloleysens commented Feb 20, 2023 • edited Loading

Summary

jloleysens commented Feb 20, 2023 • edited Loading

A few additional points for discussion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga commented Feb 20, 2023 • edited Loading

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Feb 21, 2023 • edited Loading

jloleysens commented Feb 21, 2023

Choose a reason for hiding this comment

rudolf Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

sebelga commented Feb 21, 2023

rudolf commented Feb 21, 2023

TinaHeiligers Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers commented Feb 21, 2023

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

elasticmachine commented Feb 22, 2023

jloleysens commented Feb 22, 2023

kibana-ci commented Feb 22, 2023

💚 Build Succeeded

Metrics [docs]

History

sebelga commented Feb 22, 2023

jloleysens commented Feb 20, 2023 •

edited

Loading

jloleysens commented Feb 20, 2023 •

edited

Loading

sebelga commented Feb 20, 2023 •

edited

Loading

jloleysens commented Feb 21, 2023 •

edited

Loading

rudolf Feb 21, 2023 •

edited

Loading

TinaHeiligers Feb 21, 2023 •

edited

Loading