support for specifying version numbers in API endpoints

[davepacheco]

Supersedes #862 (and is based on it). Fixes #869.

Also based on #1127. This includes a test that uses that option to make sure that operations have the same names in different versions, even when they have different handler function names.

[davepacheco]

I think this one is nearly ready to go (but after #1127). The one change I'm leaning towards is that I think versions = A..B ought to not include B, for consistency with Rust's range syntax. I think people will be confused to discover that it actually means the thing that Rust denotes with A..=B.

[davepacheco]

@jgallagher and @leftwo raised the excellent point that earlier versions of this PR would cause a Dropshot server to happily serve requests for API versions newer than the server was built with. For now, we can make this the responsibility of the DynamicVersionPolicy impl. Dropshot ships with only one impl, ClientSpecifiesVersionInHeader. I've changed this one to require a max version and reject requests newer than that.

Stepping back: my goal for this work is to provide a minimal base for us to go prototype versioning in some real consumers. I think it's pretty likely that as we do that, we will find changes we want to make to this. I wouldn't be surprised if we wind up with some common impls of DynamicVersionPolicy that we want to fold back into Dropshot. But I don't want to spend too much energy now trying to guess what those things are -- let's see what shakes out of the consumer impls.

[ahl]

very nice

[ahl]

I think we want

Suggested change

	span,
	input.span(),

Maybe? In particular I'm thinking of this test:

dropshot/tests/fail/bad_version_backwards.stderr

error: semver range (from ... until ...) has the endpoints out of order
  --> tests/fail/bad_version_backwards.rs:17:16
   |
17 |     versions = "1.2.3".."1.2.2"
   |    

Should we be underlining both?

[davepacheco]

This doesn't quite work:

error[E0277]: the trait bound `proc_macro2::Span: ToTokens` is not satisfied
   --> dropshot_endpoint/src/metadata.rs:354:25
    |
353 |                     return Err(syn::Error::new_spanned(
    |                                ----------------------- required by a bound introduced by this call
354 |                         input.span(),
    |                         ^^^^^^^^^^^^ the trait `ToTokens` is not implemented for `proc_macro2::Span`
    |
    = help: the following other types implement trait `ToTokens`:
              &'a T
              &'a mut T
              Abstract
              AndAnd
              AndEq
              AngleBracketedGenericArguments
              Arm
              As
            and 308 others
note: required by a bound in `syn::Error::new_spanned`
   --> /Users/dap/.cargo/registry/src/index.crates.io-6f17d22bba15001f/syn-2.0.79/src/error.rs:189:27
    |
189 |     pub fn new_spanned<T: ToTokens, U: Display>(tokens: T, message: U) -> Self {
    |                           ^^^^^^^^ required by this bound in `Error::new_spanned`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `dropshot_endpoint` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

I poked around a bit but did not immediately see how to turn that span into something that impls ToTokens (besides what the code is already doing).

[ahl]

what do Semver::parse() errors look like? Are those more useful than something like "{} is not of the form MAJOR.MINOR.PATCH"?

[davepacheco]

I think the messages it produces are okay. Here are a few examples:

dropshot/dropshot_endpoint/src/endpoint.rs

Lines 1070 to 1085 in e747169

	versions = "one dot two dot three",
	},
	quote! {
	pub async fn handler_xyz(
	_rqctx: RequestContext<()>,
	) -> Result<HttpResponseOk<()>, HttpError> {
	Ok(())
	}
	},
	);
	assert!(!errors.is_empty());
	assert_eq!(
	errors.get(0).map(ToString::to_string),
	Some(
	"expected semver: unexpected character 'o' while \
	parsing major version number"

dropshot/dropshot_endpoint/src/endpoint.rs

Lines 1094 to 1109 in e747169

	versions = "1.2",
	},
	quote! {
	pub async fn handler_xyz(
	_rqctx: RequestContext<()>,
	) -> Result<HttpResponseOk<()>, HttpError> {
	Ok(())
	}
	},
	);
	assert!(!errors.is_empty());
	assert_eq!(
	errors.get(0).map(ToString::to_string),
	Some(
	"expected semver: unexpected end of input while parsing \
	minor version number"

[ahl]

Do we definitely want to allow users to specify the version number header (i.e. in ClientSpecifiesVersionInHeader)? Or is this flexibility they don't need? If we allow this to be configurable, we'll need a similar sort of configuration when generating clients.

We could document (in OpenAPI) this optional header for all API endpoints; that seems quite annoying. Alternatively we could invent an extension to tell progenitor if there's a version header and--if so--what to call it.

[davepacheco]

Do we definitely want to allow users to specify the version number header (i.e. in ClientSpecifiesVersionInHeader)? Or is this flexibility they don't need? If we allow this to be configurable, we'll need a similar sort of configuration when generating clients.

We could document (in OpenAPI) this optional header for all API endpoints; that seems quite annoying. Alternatively we could invent an extension to tell progenitor if there's a version header and--if so--what to call it.

We discussed this offline and concluded:

• We'll pick a name that ought to be fine for all of our own stuff (probably api-version).
• Progenitor will use this name by default, but the behavior will be configurable so that you can have it not specify anything or use a different header name.
• Dropshot will make it easy to use the same header name. I'm being vague about the specifics here because I expect this part of the Dropshot interface to potentially change a bit when we start incorporating it into consumers so I plan to flesh this out when we do that.

[davepacheco]

I considered leaving this open until I'd done the consumer-side workflow. But with all the other work going on in Dropshot, I don't think it makes sense to maintain this as a feature branch. Nor is it worth a compile-time feature to exclude it. I'm going to just mark it as experimental for now (in documentation) and land it.

[davepacheco]

I did want to at least verify that Omicron could be made to compile easily with this change, and here it is:
oxidecomputer/omicron#7050

It's not easy to get CI to run because of the local patches required but I'm going to manually check clippy and cargo nextest run --no-fail-fast.

[davepacheco]

I've run into a few flakes like oxidecomputer/omicron#7051 but I believe they're unrelated.