Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

https://github.com/oxidecomputer/dropshot/compare/v0.16.4\...HEAD[Full list of commits]

* https://github.com/oxidecomputer/dropshot/pull/1475[#1475] Added `ClientSpecifiesVersionInHeader::on_missing` to provide a default version when the header is missing, intended for use when you're not in control of all clients

== 0.16.4 (released 2025-09-04)

https://github.com/oxidecomputer/dropshot/compare/v0.16.3\...v0.16.4[Full list of commits]
Expand Down
70 changes: 51 additions & 19 deletions dropshot/src/versioning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@ pub trait DynamicVersionPolicy: std::fmt::Debug + Send + Sync {
) -> Result<Version, HttpError>;
}

/// Implementation of `DynamicVersionPolicy` where the client must specify a
/// Implementation of `DynamicVersionPolicy` where the client specifies a
/// specific semver in a specific header and we always use whatever they
/// requested
/// requested.
///
/// An incoming request will be rejected with a 400-level error if:
///
Expand All @@ -125,17 +125,22 @@ pub trait DynamicVersionPolicy: std::fmt::Debug + Send + Sync {
/// [`ClientSpecifiesVersionInHeader::new()`], which implies that the client
/// is trying to use a newer version of the API than this server supports.
///
/// By default, incoming requests will also be rejected with a 400-level error
/// if the header is missing. To override this behavior, supply a default
/// version via [`Self::on_missing`].
///
/// If you need anything more flexible (e.g., validating the provided version
/// against a fixed set of supported versions), you'll want to impl
/// `DynamicVersionPolicy` yourself.
#[derive(Debug)]
pub struct ClientSpecifiesVersionInHeader {
name: HeaderName,
max_version: Version,
on_missing: Option<Version>,
}

impl ClientSpecifiesVersionInHeader {
/// Make a new `ClientSpecifiesVersionInHeader` policy
/// Make a new `ClientSpecifiesVersionInHeader` policy.
///
/// Arguments:
///
Expand All @@ -148,7 +153,25 @@ impl ClientSpecifiesVersionInHeader {
name: HeaderName,
max_version: Version,
) -> ClientSpecifiesVersionInHeader {
ClientSpecifiesVersionInHeader { name, max_version }
ClientSpecifiesVersionInHeader { name, max_version, on_missing: None }
}

/// If the header is missing, use the provided version instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we update this comment to dissuade people from using this if they control the clients (and maybe explicitly say not for Oxide internal services?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, and bolded.

///
/// By default, the policy will reject requests with a missing header. Call
/// this function to use the provided version instead.
///
/// Typically, the provided version should either be a fixed supported
/// version (for backwards compatibility with older clients), or the newest
/// supported version (in case clients are generally kept up-to-date but not
/// all clients send the header).
///
/// Using this function is not recommended if you control all clients—in
/// that case, arrange for clients to send the header instead. In
/// particular, **at Oxide, do not use this function for internal APIs.**
pub fn on_missing(mut self, version: Version) -> Self {
self.on_missing = Some(version);
self
}
}

Expand All @@ -159,13 +182,25 @@ impl DynamicVersionPolicy for ClientSpecifiesVersionInHeader {
_log: &Logger,
) -> Result<Version, HttpError> {
let v = parse_header(request.headers(), &self.name)?;
if v <= self.max_version {
Ok(v)
} else {
Err(HttpError::for_bad_request(
match (v, &self.on_missing) {
(Some(v), _) => {
if v <= self.max_version {
Ok(v)
} else {
Err(HttpError::for_bad_request(
None,
format!(
"server does not support this API version: {}",
v
),
))
}
}
(None, Some(on_missing)) => Ok(on_missing.clone()),
(None, None) => Err(HttpError::for_bad_request(
None,
format!("server does not support this API version: {}", v),
))
format!("missing expected header {:?}", self.name),
)),
}
}
}
Expand All @@ -175,17 +210,12 @@ impl DynamicVersionPolicy for ClientSpecifiesVersionInHeader {
fn parse_header<T>(
headers: &http::HeaderMap,
header_name: &HeaderName,
) -> Result<T, HttpError>
) -> Result<Option<T>, HttpError>
where
T: FromStr,
<T as FromStr>::Err: std::fmt::Display,
{
let v_value = headers.get(header_name).ok_or_else(|| {
HttpError::for_bad_request(
None,
format!("missing expected header {:?}", header_name),
)
})?;
let Some(v_value) = headers.get(header_name) else { return Ok(None) };

let v_str = v_value.to_str().map_err(|_| {
HttpError::for_bad_request(
Expand All @@ -197,10 +227,12 @@ where
)
})?;

v_str.parse::<T>().map_err(|e| {
let v = v_str.parse::<T>().map_err(|e| {
HttpError::for_bad_request(
None,
format!("bad value for header {:?}: {}: {}", header_name, e, v_str),
)
})
})?;

Ok(Some(v))
}
34 changes: 34 additions & 0 deletions dropshot/tests/integration-tests/versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,40 @@ async fn test_versions() {
}
}

#[tokio::test]
async fn test_versions_on_missing() {
let logctx = common::create_log_context("test_versions_on_missing");
let server = ServerBuilder::new(api(), (), logctx.log.clone())
.version_policy(VersionPolicy::Dynamic(Box::new(
ClientSpecifiesVersionInHeader::new(
VERSION_HEADER_NAME.parse().unwrap(),
Version::new(1, 4, 0),
)
.on_missing(Version::new(1, 1, 0)),
)))
.start()
.unwrap();

let server_addr = server.local_addr();
let url = format!("http://{}/demo", server_addr);
let client = reqwest::Client::new();

// Missing header => should resolve to 1.1.0.
let request = client.request(Method::GET, &url);
let response = request.send().await.unwrap();
assert_eq!(response.status(), StatusCode::OK);
let body: String = response.json().await.unwrap();
assert_eq!(body, HANDLER2_MSG);

// Now add a version header.
let request =
client.request(Method::GET, &url).header(VERSION_HEADER_NAME, "1.4.0");
let response = request.send().await.unwrap();
assert_eq!(response.status(), StatusCode::OK);
let body: String = response.json().await.unwrap();
assert_eq!(body, HANDLER4_MSG);
}

/// Test that the generated OpenAPI spec only refers to handlers in that version
/// and types that are used by those handlers.
#[test]
Expand Down