Skip to content

Minimal client API versioning #2116

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.

Sign up for GitHub

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

Jump to bottom
Merged
pcapriotti merged 6 commits into from
Feb 16, 2022
Merged

Minimal client API versioning #2116

pcapriotti merged 6 commits into from
Feb 16, 2022

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Feb 9, 2022 •
edited
Loading

This PR implements the bare minimum interface for client API version negotiation.

The changes in the nginz chart are based on https://github.com/zinfra/cailleach/pull/879 and result in the following diff.

Tracked by https://wearezeta.atlassian.net/browse/FS-440.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If HTTP endpoint paths have been added or renamed, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.
    • If public end-points have been changed or added: does nginz need un upgrade?

fisx
fisx reviewed Feb 9, 2022
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

it just occurred to me that the nginz changes are a lot or work but pretty straight-forward. just add a (/v(\d+))? in front of every path and make them all regexps. but everybody except me probably already thought of that. :) https://xkcd.com/564/

@pcapriotti
Copy link
Contributor Author

pcapriotti commented Feb 9, 2022

it just occurred to me that the nginz changes are a lot or work but pretty straight-forward. just add a (/v(\d+))? in front of every path and make them all regexps. but everybody except me probably already thought of that. :) https://xkcd.com/564/

Yep, that's what I've been doing.

@fisx
Copy link
Contributor

fisx commented Feb 14, 2022

You should also have a flag in the server config(s) that keeps supported versions from being advertised, so we can allow on-prem backends to upgrade without forcing them to make their network infrastructure federation-ready:

  removeSupportedApiVersions:
  - 1

(not sure about the name)

@pcapriotti pcapriotti force-pushed the pcapriotti/api-version-endpoint branch from 823780f to 5b84cc4 Compare February 15, 2022 09:56
@pcapriotti
Copy link
Contributor Author

pcapriotti commented Feb 15, 2022

You should also have a flag in the server config(s) that keeps supported versions from being advertised, so we can allow on-prem backends to upgrade without forcing them to make their network infrastructure federation-ready:

I don't follow. Version 1 can be supported without having to enable federation or make any changes to the infrastructure. This change has not much to do with federation, really.

@pcapriotti pcapriotti force-pushed the pcapriotti/api-version-endpoint branch from 5b84cc4 to 9ca8c66 Compare February 15, 2022 11:49
@pcapriotti pcapriotti marked this pull request as ready for review February 15, 2022 13:22
@pcapriotti
Add GET /api-version endpoint
94ef176
@pcapriotti
Add version middleware
ac6b039 
This rewrites requests from `/vN/path` to `/path`.
@pcapriotti
Install version middleware on every service
5a1a3dc
@pcapriotti
Use wai-extra for rewriting WAI requests
82eb65c 
Simply changing the `pathInfo` field is not enough, because some Wai
applications or middleware are relying on the `rawPathInfo` field.
@pcapriotti
Add CHANGELOG entries
c510d47
@pcapriotti
Add version prefixes to nginz chart
a6c6d32
@pcapriotti pcapriotti force-pushed the pcapriotti/api-version-endpoint branch from 9ca8c66 to a6c6d32 Compare February 15, 2022 15:14
@pcapriotti pcapriotti merged commit d605a82 into develop Feb 16, 2022
@pcapriotti pcapriotti deleted the pcapriotti/api-version-endpoint branch February 16, 2022 08:35
@pcapriotti pcapriotti mentioned this pull request Feb 16, 2022
Merged
2 tasks
fisx
fisx reviewed Feb 16, 2022
View reviewed changes
libs/wire-api/src/Wire/API/VersionInfo.hs
import Data.Schema
import Imports

vinfoSchema :: ValueSchema NamedSwaggerDoc v -> ValueSchema NamedSwaggerDoc [v]
Copy link
Contributor

@fisx fisx Feb 16, 2022

Choose a reason for hiding this comment

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

Why is this not in Wire.API.Routes.Version?

Copy link
Contributor Author

@pcapriotti pcapriotti Feb 16, 2022

Choose a reason for hiding this comment

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

VersionInfo is more general, and can be used for other API version lists, whereas Wire.API.Routes.Version is specific for the client API.

Copy link
Contributor

@fisx fisx Feb 16, 2022

Choose a reason for hiding this comment

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

What about Version, then? Will there be a type Version for the federation server-to-server API as well?

Copy link
Contributor Author

@pcapriotti pcapriotti Feb 24, 2022

Choose a reason for hiding this comment

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

Yes, that was the idea, as well as for other APIs that need to be versioned separately.

services/brig/src/Brig/API/Public.hs
import qualified Wire.API.Routes.Public.LegalHold as LegalHoldAPI
import qualified Wire.API.Routes.Public.Spar as SparAPI
import qualified Wire.API.Routes.Public.Util as Public
import Wire.API.Routes.Version
Copy link
Contributor

@fisx fisx Feb 16, 2022

Choose a reason for hiding this comment

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

qualify this, and call versionSwagger swaggerDoc?

Copy link
Contributor Author

@pcapriotti pcapriotti Feb 16, 2022

Choose a reason for hiding this comment

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

I don't like to create name clashes on purpose and resolve them with qualified imports. It makes everything more awkward and I don't see the benefit.

Copy link
Contributor

@fisx fisx Feb 16, 2022

Choose a reason for hiding this comment

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

I won't insist, but I think we agreed at some point (we should document these things) that imports should either be explicit or qualified, but not neither. Doesn't have to happen now, since we're not consistent, but can you get on board with the general idea?

Copy link
Contributor Author

@pcapriotti pcapriotti Feb 16, 2022

Choose a reason for hiding this comment

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

Well, I disagree with the general idea, but if it's something everyone else agrees on, I'll comply.

@fisx fisx mentioned this pull request Feb 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fisx fisx fisx left review comments

+1 more reviewer

@mdimjasevic mdimjasevic mdimjasevic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pcapriotti @fisx @mdimjasevic