Skip to content

Commit

Permalink
[dns-server] convert DNS server API into a trait (#6079)
Browse files Browse the repository at this point in the history
Straightforward, and resulted in some nice cleanup.
  • Loading branch information
sunshowers authored Jul 15, 2024
1 parent d993746 commit 8fc8312
Show file tree
Hide file tree
Showing 15 changed files with 248 additions and 243 deletions.
13 changes: 13 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ members = [
"dev-tools/releng",
"dev-tools/xtask",
"dns-server",
"dns-server-api",
"end-to-end-tests",
"gateway-cli",
"gateway-test-utils",
Expand Down Expand Up @@ -119,6 +120,7 @@ default-members = [
# hakari to not work as well and build times to be longer.
# See omicron#4392.
"dns-server",
"dns-server-api",
# Do not include end-to-end-tests in the list of default members, as its
# tests only work on a deployed control plane.
"gateway-cli",
Expand Down Expand Up @@ -279,6 +281,7 @@ derive-where = "1.2.7"
diesel = { version = "2.1.6", features = ["postgres", "r2d2", "chrono", "serde_json", "network-address", "uuid"] }
diesel-dtrace = { git = "https://github.com/oxidecomputer/diesel-dtrace", branch = "main" }
dns-server = { path = "dns-server" }
dns-server-api = { path = "dns-server-api" }
dns-service-client = { path = "clients/dns-service-client" }
dpd-client = { path = "clients/dpd-client" }
dropshot = { git = "https://github.com/oxidecomputer/dropshot", branch = "main", features = [ "usdt-probes" ] }
Expand Down
1 change: 1 addition & 0 deletions dev-tools/openapi-manager/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ anyhow.workspace = true
atomicwrites.workspace = true
camino.workspace = true
clap.workspace = true
dns-server-api.workspace = true
dropshot.workspace = true
fs-err.workspace = true
indent_write.workspace = true
Expand Down
10 changes: 10 additions & 0 deletions dev-tools/openapi-manager/src/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ use openapiv3::OpenAPI;
/// All APIs managed by openapi-manager.
pub fn all_apis() -> Vec<ApiSpec> {
vec![
ApiSpec {
title: "Internal DNS".to_string(),
version: "0.0.1".to_string(),
description: "API for the internal DNS server".to_string(),
boundary: ApiBoundary::Internal,
api_description:
dns_server_api::dns_server_api::stub_api_description,
filename: "dns-server.json".to_string(),
extra_validation: None,
},
ApiSpec {
title: "Nexus internal API".to_string(),
version: "0.0.1".to_string(),
Expand Down
15 changes: 15 additions & 0 deletions dns-server-api/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
[package]
name = "dns-server-api"
version = "0.1.0"
edition = "2021"
license = "MPL-2.0"

[lints]
workspace = true

[dependencies]
chrono.workspace = true
dropshot.workspace = true
omicron-workspace-hack.workspace = true
schemars.workspace = true
serde.workspace = true
160 changes: 160 additions & 0 deletions dns-server-api/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

//! Dropshot API for configuring DNS namespace.
//!
//! ## Shape of the API
//!
//! The DNS configuration API has just two endpoints: PUT and GET of the entire
//! DNS configuration. This is pretty anti-REST. But it's important to think
//! about how this server fits into the rest of the system. When changes are
//! made to DNS data, they're grouped together and assigned a monotonically
//! increasing generation number. The DNS data is first stored into CockroachDB
//! and then propagated from a distributed fleet of Nexus instances to a
//! distributed fleet of these DNS servers. If we accepted individual updates to
//! DNS names, then propagating a particular change would be non-atomic, and
//! Nexus would have to do a lot more work to ensure (1) that all changes were
//! propagated (even if it crashes) and (2) that they were propagated in the
//! correct order (even if two Nexus instances concurrently propagate separate
//! changes).
//!
//! This DNS server supports hosting multiple zones. We could imagine supporting
//! separate endpoints to update the DNS data for a particular zone. That feels
//! nicer (although it's not clear what it would buy us). But as with updates to
//! multiple names, Nexus's job is potentially much easier if the entire state
//! for all zones is updated at once. (Otherwise, imagine how Nexus would
//! implement _renaming_ one zone to another without loss of service. With
//! a combined endpoint and generation number for all zones, all that's necessary
//! is to configure a new zone with all the same names, and then remove the old
//! zone later in another update. That can be managed by the same mechanism in
//! Nexus that manages regular name updates. On the other hand, if there were
//! separate endpoints with separate generation numbers, then Nexus has more to
//! keep track of in order to do the rename safely.)
//!
//! See RFD 367 for more on DNS propagation.
//!
//! ## ETags and Conditional Requests
//!
//! It's idiomatic in HTTP use ETags and conditional requests to provide
//! synchronization. We could define an ETag to be just the current generation
//! number of the server and honor standard `if-match` headers to fail requests
//! where the generation number doesn't match what the client expects. This
//! would be fine, but it's rather annoying:
//!
//! 1. When the client wants to propagate generation X, the client would have
//! make an extra request just to fetch the current ETag, just so it can put
//! it into the conditional request.
//!
//! 2. If some other client changes the configuration in the meantime, the
//! conditional request would fail and the client would have to take another
//! lap (fetching the current config and potentially making another
//! conditional PUT).
//!
//! 3. This approach would make synchronization opt-in. If a client (or just
//! one errant code path) neglected to set the if-match header, we could do
//! the wrong thing and cause the system to come to rest with the wrong DNS
//! data.
//!
//! Since the semantics here are so simple (we only ever want to move the
//! generation number forward), we don't bother with ETags or conditional
//! requests. Instead we have the server implement the behavior we want, which
//! is that when a request comes in to update DNS data to generation X, the
//! server replies with one of:
//!
//! (1) the update has been applied and the server is now running generation X
//! (client treats this as success)
//!
//! (2) the update was not applied because the server is already at generation X
//! (client treats this as success)
//!
//! (3) the update was not applied because the server is already at a newer
//! generation
//! (client probably starts the whole propagation process over because its
//! current view of the world is out of date)
//!
//! This way, the DNS data can never move backwards and the client only ever has
//! to make one request.
//!
//! ## Concurrent updates
//!
//! Given that we've got just one API to update the all DNS zones, and given
//! that might therefore take a minute for a large zone, and also that there may
//! be multiple Nexus instances trying to do it at the same time, we need to
//! think a bit about what should happen if two Nexus do try to do it at the same
//! time. Spoiler: we immediately fail any request to update the DNS data if
//! there's already an update in progress.
//!
//! What else could we do? We could queue the incoming request behind the
//! in-progress one. How large do we allow that queue to grow? At some point
//! we'll need to stop queueing them. So why bother at all?
use std::{
collections::HashMap,
net::{Ipv4Addr, Ipv6Addr},
};

use dropshot::{HttpError, HttpResponseOk, RequestContext};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

#[dropshot::api_description]
pub trait DnsServerApi {
type Context;

#[endpoint(
method = GET,
path = "/config",
)]
async fn dns_config_get(
rqctx: RequestContext<Self::Context>,
) -> Result<HttpResponseOk<DnsConfig>, HttpError>;

#[endpoint(
method = PUT,
path = "/config",
)]
async fn dns_config_put(
rqctx: RequestContext<Self::Context>,
rq: dropshot::TypedBody<DnsConfigParams>,
) -> Result<dropshot::HttpResponseUpdatedNoContent, dropshot::HttpError>;
}

#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
pub struct DnsConfigParams {
pub generation: u64,
pub time_created: chrono::DateTime<chrono::Utc>,
pub zones: Vec<DnsConfigZone>,
}

#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
pub struct DnsConfig {
pub generation: u64,
pub time_created: chrono::DateTime<chrono::Utc>,
pub time_applied: chrono::DateTime<chrono::Utc>,
pub zones: Vec<DnsConfigZone>,
}

#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
pub struct DnsConfigZone {
pub zone_name: String,
pub records: HashMap<String, Vec<DnsRecord>>,
}

#[allow(clippy::upper_case_acronyms)]
#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
#[serde(tag = "type", content = "data")]
pub enum DnsRecord {
A(Ipv4Addr),
AAAA(Ipv6Addr),
SRV(SRV),
}

#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
#[serde(rename = "Srv")]
pub struct SRV {
pub prio: u16,
pub weight: u16,
pub port: u16,
pub target: String,
}
1 change: 1 addition & 0 deletions dns-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ anyhow.workspace = true
camino.workspace = true
chrono.workspace = true
clap.workspace = true
dns-server-api.workspace = true
dns-service-client.workspace = true
dropshot.workspace = true
http.workspace = true
Expand Down
29 changes: 0 additions & 29 deletions dns-server/src/bin/apigen.rs

This file was deleted.

9 changes: 2 additions & 7 deletions dns-server/src/dns_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
//! The facilities here handle binding a UDP socket, receiving DNS messages on
//! that socket, and replying to them.
use crate::dns_types::DnsRecord;
use crate::storage;
use crate::storage::QueryError;
use crate::storage::Store;
use anyhow::anyhow;
use anyhow::Context;
use dns_server_api::DnsRecord;
use pretty_hex::*;
use serde::Deserialize;
use slog::{debug, error, info, o, trace, Logger};
Expand Down Expand Up @@ -234,12 +234,7 @@ fn dns_record_to_record(
Ok(aaaa)
}

DnsRecord::SRV(crate::dns_types::SRV {
prio,
weight,
port,
target,
}) => {
DnsRecord::SRV(dns_server_api::SRV { prio, weight, port, target }) => {
let tgt = Name::from_str(&target).map_err(|error| {
RequestError::ServFail(anyhow!(
"serialization failed due to bad SRV target {:?}: {:#}",
Expand Down
50 changes: 0 additions & 50 deletions dns-server/src/dns_types.rs

This file was deleted.

Loading

0 comments on commit 8fc8312

Please sign in to comment.