apollographql / rover

The CLI for Apollo GraphOS
https://rover.apollo.dev
Other
407 stars 85 forks source link

Improve `rover supergraph compose` version rollouts #1518

Open EverlastingBugstopper opened 1 year ago

EverlastingBugstopper commented 1 year ago

The "updating the default latest version" by default in rover supergraph compose has led to some confusion, especially due to breaking changes from Federation 2.2 to 2.3. Namely, if you update your composition pipeline before your router/gateway, due to the core spec being bumped from v0.2 to v0.3, routers crash on startup. Our docs recommend that you don't pin a version and to only do that if you need to. Unfortunately, due to breaking changes and the careful migration path that must be navigated each time (and generally just being surprised that the software changed under your feet), I think we should reconsider that advice.

Things that I think we should do to fix this up:

phase 1:

phase 2:

abernix commented 1 year ago

Worth noting that we do lightly-recommend pinning the version of Rover, even if we might not currently recommend pinning the version of the Router. I'm definitely curious if we should generally recommend pinning?

EverlastingBugstopper commented 1 year ago

I don't think we need to pin the router, but I definitely think we need to pin composition, because of the incompatible changes to the join spec. Auto updates have caused routers to crash on startup due to this.

EverlastingBugstopper commented 1 year ago

Unless we can commit to a platform without breaking changes between minor versions which seems out of the question?

carldunham commented 1 year ago

has led to some confusion

Yes, some confusion indeed. Namely, how on earth can a command-line tool exhibit breaking behavior with only a merge to its main branch? How is it our fault because we didn't add a configuration item for a plugin? What other plugins or hidden dependencies are there that we haven't seen? Wait, is Router or Gateway going to auto-update when we don't expect it?

Please understand that this incident has raised some serious concerns about Apollo in general. There are some basic assumptions about dependency stability that have been broken here.

michael-watson commented 1 year ago

We created some documentation on update order to help avoid runtime or composition errors. As a best practice, you should always pin your supergraph.yaml to a specific federation_version: major.minor.patch to ensure your composition pipeline doesn’t get out ahead of your router version.

To give some more details on the "why" behind the versioning story, a federated graph is a distributed system. For that reason, we follow major.minor.patch versioning with new features introduced with new minor federation versions. By setting federation_version: 2, rover supergraph compose is always going to use the latest version of federation for composition. If your router instance hasn't been upgraded to support that new federation version, it won't be able to use that supergraphSDL. Pinning the major.minor.patch is how you can ensure your router and composition are never out of sync. When you do want to upgrade, you should do it in this order:

  1. Upgrade Apollo Router - we have a support table to help with the versions (the gateway/router will never update itself, you have to do that)
  2. Upgrade composition in the federation_version (what is used in rover supergraph compose)
  3. Finally upgrade subgraph libraries and the Fed spec @link in subgraph schemas

New composition versions may output new runtime requirements @link specs marked for:EXECUTION like a new join spec version for:SECURITY, like @inaccessible, can emit new directives with no for:purpose, that the router can safely ignore. However, older router versions will refuse newer supergraph schemas with new runtime requirements they don’t know how to process. This is a safety measure of the router to ensure correct and secure execution.

We currently have a rover supergraph compose warning for this: Screenshot 2023-04-06 at 8 59 33 AM

Is there anything you think we should change about that warning? Maybe we should explicitly call out the major.minor.patch, we currently point at these docs: https://www.apollographql.com/docs/rover/commands/supergraphs#setting-a-composition-version