[RFC] [dns-server] reorganize per RFD 619

Cargo.toml

@@ -67,6 +67,7 @@ members = [
     "internal-dns/cli",
     "internal-dns/resolver",
     "internal-dns/types",
+    "internal-dns/types/versions",
     "ipcc",
     "key-manager",
     "live-tests",
@@ -229,6 +230,7 @@ default-members = [
     "internal-dns/cli",
     "internal-dns/resolver",
     "internal-dns/types",
+    "internal-dns/types/versions",
     "ipcc",
     "key-manager",
     "live-tests",
@@ -515,6 +517,7 @@ installinator-client = { path = "clients/installinator-client" }
 installinator-common = { path = "installinator-common" }
 internal-dns-resolver = { path = "internal-dns/resolver" }
 internal-dns-types = { path = "internal-dns/types" }
+internal-dns-types-versions = { path = "internal-dns/types/versions" }
 ipcc = { path = "ipcc" }
 ipnet = "2.9"
 itertools = "0.14.0"

clients/dns-service-client/src/lib.rs

@@ -26,12 +26,11 @@ progenitor::generate_api!(
     }
 );
 
-pub type DnsError = crate::Error<crate::types::Error>;
+pub use internal_dns_types::config::{
+    ERROR_CODE_BAD_UPDATE_GENERATION, ERROR_CODE_UPDATE_IN_PROGRESS,
+};
 
-pub const ERROR_CODE_UPDATE_IN_PROGRESS: &'static str = "UpdateInProgress";
-pub const ERROR_CODE_BAD_UPDATE_GENERATION: &'static str =
-    "BadUpdateGeneration";
-pub const ERROR_CODE_INCOMPATIBLE_RECORD: &'static str = "IncompatibleRecord";
+pub type DnsError = crate::Error<crate::types::Error>;
 
 /// Returns whether an error from this client should be retried
 pub fn is_retryable(error: &DnsError) -> bool {

dns-server-api/Cargo.toml

@@ -12,6 +12,7 @@ chrono.workspace = true
 dropshot.workspace = true
 dropshot-api-manager-types.workspace = true
 internal-dns-types.workspace = true
+internal-dns-types-versions.workspace = true
 omicron-workspace-hack.workspace = true
 schemars.workspace = true
 semver.workspace = true

dns-server-api/src/lib.rs

@@ -91,6 +91,8 @@
 
 use dropshot::{HttpError, HttpResponseOk, RequestContext};
 use dropshot_api_manager_types::api_versions;
+use internal_dns_types::config::ERROR_CODE_INCOMPATIBLE_RECORD;
+use internal_dns_types_versions::{latest, v1, v2};
 
 api_versions!([
     // WHEN CHANGING THE API (part 1 of 2):
@@ -127,52 +129,64 @@ pub trait DnsServerApi {
     #[endpoint(
         method = GET,
         path = "/config",
-        operation_id = "dns_config_get",
-        versions = VERSION_INITIAL..VERSION_SOA_AND_NS
+        versions = VERSION_SOA_AND_NS..
     )]
-    async fn dns_config_get_v1(
+    async fn dns_config_get(
         rqctx: RequestContext<Self::Context>,
-    ) -> Result<
-        HttpResponseOk<internal_dns_types::v1::config::DnsConfig>,
-        HttpError,
-    >;
+    ) -> Result<HttpResponseOk<latest::config::DnsConfig>, HttpError>;
 
     #[endpoint(
         method = GET,
         path = "/config",
         operation_id = "dns_config_get",
-        versions = VERSION_SOA_AND_NS..
+        versions = VERSION_INITIAL..VERSION_SOA_AND_NS
     )]
-    async fn dns_config_get_v2(
+    async fn dns_config_get_v1(
         rqctx: RequestContext<Self::Context>,
-    ) -> Result<
-        HttpResponseOk<internal_dns_types::v2::config::DnsConfig>,
-        HttpError,
-    >;
+    ) -> Result<HttpResponseOk<v1::config::DnsConfig>, HttpError> {
+        Self::dns_config_get(rqctx).await?.try_map(|config| {
+            config.try_into().map_err(
+                |v2::config::V2ToV1TranslationError::IncompatibleRecord| {
+                    HttpError::for_bad_request(
+                        None,
+                        ERROR_CODE_INCOMPATIBLE_RECORD.to_string(),
+                    )
+                },
+            )
+        })
+    }
 
     #[endpoint(
         method = PUT,
         path = "/config",
-        operation_id = "dns_config_put",
-        versions = VERSION_INITIAL..VERSION_SOA_AND_NS,
+        versions = VERSION_SOA_AND_NS..
     )]
-    async fn dns_config_put_v1(
+    async fn dns_config_put(
         rqctx: RequestContext<Self::Context>,
-        rq: dropshot::TypedBody<
-            internal_dns_types::v1::config::DnsConfigParams,
-        >,
+        rq: dropshot::TypedBody<latest::config::DnsConfigParams>,
     ) -> Result<dropshot::HttpResponseUpdatedNoContent, dropshot::HttpError>;
 
     #[endpoint(
         method = PUT,
         path = "/config",
         operation_id = "dns_config_put",
-        versions = VERSION_SOA_AND_NS..
+        versions = VERSION_INITIAL..VERSION_SOA_AND_NS,
     )]
-    async fn dns_config_put_v2(
+    async fn dns_config_put_v1(
         rqctx: RequestContext<Self::Context>,
-        rq: dropshot::TypedBody<
-            internal_dns_types::v2::config::DnsConfigParams,
-        >,
-    ) -> Result<dropshot::HttpResponseUpdatedNoContent, dropshot::HttpError>;
+        rq: dropshot::TypedBody<v1::config::DnsConfigParams>,
+    ) -> Result<dropshot::HttpResponseUpdatedNoContent, dropshot::HttpError>
+    {
+        let rq = rq.try_map(|params| {
+            params.try_into().map_err(
+                |v2::config::V1ToV2TranslationError::GenerationTooLarge| {
+                    HttpError::for_bad_request(
+                        None,
+                        ERROR_CODE_INCOMPATIBLE_RECORD.to_string(),
+                    )
+                },
+            )
+        })?;
+        Self::dns_config_put(rqctx, rq).await
+    }
 }

dns-server/Cargo.toml

@@ -21,6 +21,7 @@ hickory-resolver.workspace = true
 hickory-server.workspace = true
 http.workspace = true
 internal-dns-types.workspace = true
+internal-dns-types-versions.workspace = true
 omicron-common.workspace = true
 oxide-tokio-rt.workspace = true
 pretty-hex.workspace = true

dns-server/src/http_server.rs

@@ -6,14 +6,10 @@
 
 use crate::storage::{self, UpdateError};
 use dns_server_api::DnsServerApi;
-use dns_service_client::{
-    ERROR_CODE_BAD_UPDATE_GENERATION, ERROR_CODE_INCOMPATIBLE_RECORD,
-    ERROR_CODE_UPDATE_IN_PROGRESS,
-};
 use dropshot::RequestContext;
-use internal_dns_types::{
-    v1::{self, config::TranslationError as V1TranslationError},
-    v2::{self, config::TranslationError as V2TranslationError},
+use internal_dns_types::config::{
+    DnsConfig, DnsConfigParams, ERROR_CODE_BAD_UPDATE_GENERATION,
+    ERROR_CODE_UPDATE_IN_PROGRESS,
 };
 
 pub struct Context {
@@ -36,66 +32,9 @@ enum DnsServerApiImpl {}
 impl DnsServerApi for DnsServerApiImpl {
     type Context = Context;
 
-    async fn dns_config_get_v1(
-        rqctx: RequestContext<Context>,
-    ) -> Result<
-        dropshot::HttpResponseOk<v1::config::DnsConfig>,
-        dropshot::HttpError,
-    > {
-        let result = Self::dns_config_get(rqctx).await?;
-        match result.0.try_into() {
-            Ok(config) => Ok(dropshot::HttpResponseOk(config)),
-            Err(V2TranslationError::IncompatibleRecord) => {
-                Err(dropshot::HttpError::for_bad_request(
-                    None,
-                    ERROR_CODE_INCOMPATIBLE_RECORD.to_string(),
-                ))
-            }
-        }
-    }
-
-    async fn dns_config_get_v2(
-        rqctx: RequestContext<Context>,
-    ) -> Result<
-        dropshot::HttpResponseOk<v2::config::DnsConfig>,
-        dropshot::HttpError,
-    > {
-        Self::dns_config_get(rqctx).await
-    }
-
-    async fn dns_config_put_v1(
-        rqctx: RequestContext<Context>,
-        rq: dropshot::TypedBody<v1::config::DnsConfigParams>,
-    ) -> Result<dropshot::HttpResponseUpdatedNoContent, dropshot::HttpError>
-    {
-        let provided_config = match rq.into_inner().try_into() {
-            Ok(config) => config,
-            Err(V1TranslationError::GenerationTooLarge) => {
-                return Err(dropshot::HttpError::for_bad_request(
-                    None,
-                    ERROR_CODE_INCOMPATIBLE_RECORD.to_string(),
-                ));
-            }
-        };
-        Self::dns_config_put(rqctx, provided_config).await
-    }
-
-    async fn dns_config_put_v2(
-        rqctx: RequestContext<Context>,
-        rq: dropshot::TypedBody<v2::config::DnsConfigParams>,
-    ) -> Result<dropshot::HttpResponseUpdatedNoContent, dropshot::HttpError>
-    {
-        Self::dns_config_put(rqctx, rq.into_inner()).await
-    }
-}
-
-impl DnsServerApiImpl {
     async fn dns_config_get(
         rqctx: RequestContext<Context>,
-    ) -> Result<
-        dropshot::HttpResponseOk<v2::config::DnsConfig>,
-        dropshot::HttpError,
-    > {
+    ) -> Result<dropshot::HttpResponseOk<DnsConfig>, dropshot::HttpError> {
         let apictx = rqctx.context();
         let config = apictx.store.dns_config().await.map_err(|e| {
             dropshot::HttpError::for_internal_error(format!(
@@ -108,11 +47,14 @@ impl DnsServerApiImpl {
 
     async fn dns_config_put(
         rqctx: RequestContext<Context>,
-        params: v2::config::DnsConfigParams,
+        rq: dropshot::TypedBody<DnsConfigParams>,
     ) -> Result<dropshot::HttpResponseUpdatedNoContent, dropshot::HttpError>
     {
         let apictx = rqctx.context();
-        apictx.store.dns_config_update(&params, &rqctx.request_id).await?;
+        apictx
+            .store
+            .dns_config_update(&rq.into_inner(), &rqctx.request_id)
+            .await?;
         Ok(dropshot::HttpResponseUpdatedNoContent())
     }
 }

dns-server/tests/cross_version_test.rs

@@ -23,7 +23,7 @@ const TEST_ZONE: &'static str = "oxide.internal";
 // well.
 mod v1_client {
     use anyhow::Context;
-    use internal_dns_types::v1;
+    use internal_dns_types_versions::v1;
 
     use std::collections::HashMap;
 
@@ -116,8 +116,8 @@ mod v1_client {
 pub async fn cross_version_works() -> Result<(), anyhow::Error> {
     let test_ctx = init_client_server("cross_version_works").await?;
 
-    use internal_dns_types::v1::config::DnsRecord as V1DnsRecord;
-    use internal_dns_types::v2::config::DnsRecord as V2DnsRecord;
+    use internal_dns_types_versions::v1::config::DnsRecord as V1DnsRecord;
+    use internal_dns_types_versions::v2::config::DnsRecord as V2DnsRecord;
 
     let ns1_addr = Ipv6Addr::new(0xfd, 0, 0, 0, 0, 0, 0, 0x1);
     let ns1_name = format!("ns1.{TEST_ZONE}.");
@@ -163,7 +163,7 @@ pub async fn cross_version_works() -> Result<(), anyhow::Error> {
         Err(dns_service_client::Error::ErrorResponse(rv)) => {
             assert_eq!(
                 rv.message,
-                dns_service_client::ERROR_CODE_INCOMPATIBLE_RECORD
+                internal_dns_types::config::ERROR_CODE_INCOMPATIBLE_RECORD
             );
         }
         o => {

internal-dns/types/Cargo.toml

@@ -9,6 +9,7 @@ workspace = true
 [dependencies]
 anyhow.workspace = true
 chrono.workspace = true
+internal-dns-types-versions.workspace = true
 omicron-common.workspace = true
 omicron-workspace-hack.workspace = true
 omicron-uuid-kinds.workspace = true

internal-dns/types/src/config.rs

@@ -69,9 +69,19 @@ use omicron_uuid_kinds::{OmicronZoneUuid, SledUuid};
 use std::collections::BTreeMap;
 use std::net::{Ipv6Addr, SocketAddrV6};
 
-// "v2" types are the most recent, so we re-export them here for dependents that
+// Re-export the latest versions from the versions crate for dependents that
 // just want "latest".
-pub use crate::v2::config::*;
+pub use internal_dns_types_versions::latest::config::*;
+
+/// Error code used when a record type cannot be represented in an older API
+/// version (e.g., NS records in v1).
+pub const ERROR_CODE_INCOMPATIBLE_RECORD: &str = "IncompatibleRecord";
+
+/// Error code used when an update is already in progress.
+pub const ERROR_CODE_UPDATE_IN_PROGRESS: &str = "UpdateInProgress";
+
+/// Error code used when the provided generation number is stale.
+pub const ERROR_CODE_BAD_UPDATE_GENERATION: &str = "BadUpdateGeneration";
 
 /// Used to construct the DNS name for a control plane host
 #[derive(Clone, Debug, PartialEq, PartialOrd)]

internal-dns/types/src/lib.rs

@@ -7,29 +7,7 @@
 //!
 //! ## Organization
 //!
-//! Some types in this crate are exposed by its dependents as part of versioned
-//! HTTP interfaces, such as `dns-server`'s management interfaces. In those
-//! cases we may want to support multiple HTTP interface versions concurrently,
-//! and so this crate preserves old versions of select public items used in this
-//! way.
-//!
-//! The alternative here would be to require dependents of `internal-dns` to
-//! declare duplicate dependencies on `internal-dns` at different revisions.
-//! That would force dependents to take all of `internal-dns`' dependencies at
-//! versions of interest as transitive dependencies, and precludes maintenance
-//! that would otherwise be able to preserve API compatibility of the public
-//! types.
-//!
-//! `cargo xtask openapi` helps us check that we don't unintentionally break an
-//! existing committed version, which also helps us be confident that future
-//! maintenance on old versions' types does not introduce breaking changes.
-//!
-//! The top-level items here can be thought of as the "current" version, where
-//! versioned items (and their previous versions) are in the `vN` modules with
-//! their latest form re-exported as the "current" version.
-
-pub mod v1;
-pub mod v2;
+//! See [RFD 619](https://rfd.shared.oxide.computer/rfd/0619).
 
 pub mod config;
 pub mod diff;