Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nexus] put DNS servers in DNS, so you can DNS while you DNS #5033

Merged
merged 18 commits into from
Feb 12, 2024
Merged

[nexus] put DNS servers in DNS, so you can DNS while you DNS #5033

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e85e82d
wip
hawkw Feb 1, 2024
afda438
wip 2
hawkw Feb 1, 2024
76f24be
wip
hawkw Feb 1, 2024
4dba40e
plumbing
hawkw Feb 6, 2024
29ed273
wip
hawkw Feb 7, 2024
9ea07c6
at least panic in a prettier way
hawkw Feb 7, 2024
66d0fbc
wip
hawkw Feb 8, 2024
8eb0b98
OMG IT WORKS
hawkw Feb 8, 2024
7c6d77f
cleanup
hawkw Feb 8, 2024
fce2953
remove `upsert_service`
hawkw Feb 8, 2024
e0d3bb3
delete DnsWatcher test
hawkw Feb 8, 2024
05065b2
update EXPECTORATE
hawkw Feb 9, 2024
9a3f15d
[wip] remove services table
hawkw Feb 9, 2024
9527064
Revert "[wip] remove services table"
hawkw Feb 9, 2024
cde9680
use `ServiceName::InternalDns` rather than const
hawkw Feb 9, 2024
586cbe0
remove println
hawkw Feb 10, 2024
11a63a0
curse you, github web editor!
hawkw Feb 10, 2024
54c992a
Update nexus/src/app/background/init.rs
hawkw Feb 12, 2024
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
12 changes: 6 additions & 6 deletions dev-tools/omdb/tests/env.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ task: "dns_propagation_internal"


task: "dns_servers_external"
watches list of external DNS servers stored in CockroachDB
watches list of external DNS servers stored in internal DNS


task: "dns_servers_internal"
watches list of internal DNS servers stored in CockroachDB
watches list of internal DNS servers stored in internal DNS


task: "external_endpoints"
Expand Down Expand Up @@ -147,11 +147,11 @@ task: "dns_propagation_internal"


task: "dns_servers_external"
watches list of external DNS servers stored in CockroachDB
watches list of external DNS servers stored in internal DNS


task: "dns_servers_internal"
watches list of internal DNS servers stored in CockroachDB
watches list of internal DNS servers stored in internal DNS


task: "external_endpoints"
Expand Down Expand Up @@ -224,11 +224,11 @@ task: "dns_propagation_internal"


task: "dns_servers_external"
watches list of external DNS servers stored in CockroachDB
watches list of external DNS servers stored in internal DNS


task: "dns_servers_internal"
watches list of internal DNS servers stored in CockroachDB
watches list of internal DNS servers stored in internal DNS


task: "external_endpoints"
Expand Down
8 changes: 4 additions & 4 deletions dev-tools/omdb/tests/successes.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,11 +251,11 @@ task: "dns_propagation_internal"


task: "dns_servers_external"
watches list of external DNS servers stored in CockroachDB
watches list of external DNS servers stored in internal DNS


task: "dns_servers_internal"
watches list of internal DNS servers stored in CockroachDB
watches list of internal DNS servers stored in internal DNS


task: "external_endpoints"
Expand Down Expand Up @@ -313,7 +313,7 @@ task: "dns_servers_internal"
task: "dns_propagation_internal"
configured period: every 1m
currently executing: no
last completed activation: iter 5, triggered by a dependent task completing
last completed activation: iter 4, triggered by a dependent task completing
started at <REDACTED TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
attempt to propagate generation: 1

Expand Down Expand Up @@ -341,7 +341,7 @@ task: "dns_servers_external"
task: "dns_propagation_external"
configured period: every 1m
currently executing: no
last completed activation: iter 5, triggered by a dependent task completing
last completed activation: iter 4, triggered by a dependent task completing
started at <REDACTED TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
attempt to propagate generation: 2

Expand Down
2 changes: 1 addition & 1 deletion internal-dns/src/names.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl ServiceName {

/// Returns the DNS name for this service, ignoring the zone part of the DNS
/// name
pub(crate) fn dns_name(&self) -> String {
pub fn dns_name(&self) -> String {
match self {
ServiceName::Clickhouse
| ServiceName::ClickhouseKeeper
Expand Down
5 changes: 1 addition & 4 deletions nexus/src/app/background/dns_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ impl BackgroundTask for DnsConfigWatcher {
mod test {
use crate::app::background::common::BackgroundTask;
use crate::app::background::dns_config::DnsConfigWatcher;
use crate::app::background::init::test::read_internal_dns_zone_id;
use crate::app::background::init::test::write_test_dns_generation;
use assert_matches::assert_matches;
use async_bb8_diesel::AsyncRunQueryDsl;
Expand Down Expand Up @@ -197,9 +196,7 @@ mod test {

// Now write generation 2, activate again, and verify that the update
// was sent to the watcher.
let internal_dns_zone_id =
read_internal_dns_zone_id(&opctx, &datastore).await;
write_test_dns_generation(&datastore, internal_dns_zone_id).await;
write_test_dns_generation(&opctx, &datastore).await;
assert_eq!(watcher.borrow().as_ref().unwrap().generation, 1);
let value = task.activate(&opctx).await;
assert_eq!(watcher.borrow().as_ref().unwrap().generation, 2);
Expand Down
214 changes: 34 additions & 180 deletions nexus/src/app/background/dns_servers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,15 @@
use super::common::BackgroundTask;
use futures::future::BoxFuture;
use futures::FutureExt;
use internal_dns::names::ServiceName;
use internal_dns::resolver::Resolver;
use nexus_db_model::DnsGroup;
use nexus_db_model::ServiceKind;
use nexus_db_queries::context::OpContext;
use nexus_db_queries::db::DataStore;
use omicron_common::api::external::DataPageParams;
use serde::Serialize;
use serde_json::json;
use std::net::{SocketAddr, SocketAddrV6};
use std::num::NonZeroU32;
use std::sync::Arc;
use std::net::SocketAddr;
use tokio::sync::watch;

// This constraint could be relaxed by paginating through the list of servers,
// but we don't expect to have this many servers any time soon.
const MAX_DNS_SERVERS: usize = 10;

#[derive(Debug, Clone, Eq, PartialEq, Serialize)]
pub struct DnsServersList {
pub addresses: Vec<SocketAddr>,
Expand All @@ -31,20 +24,17 @@ pub struct DnsServersList {
/// Background task that keeps track of the latest list of DNS servers for a DNS
/// group
pub struct DnsServersWatcher {
datastore: Arc<DataStore>,
dns_group: DnsGroup,
hawkw marked this conversation as resolved.
Show resolved Hide resolved
resolver: Resolver,
last: Option<DnsServersList>,
tx: watch::Sender<Option<DnsServersList>>,
rx: watch::Receiver<Option<DnsServersList>>,
}

impl DnsServersWatcher {
pub fn new(
datastore: Arc<DataStore>,
dns_group: DnsGroup,
) -> DnsServersWatcher {
pub fn new(dns_group: DnsGroup, resolver: Resolver) -> DnsServersWatcher {
let (tx, rx) = watch::channel(None);
DnsServersWatcher { datastore, dns_group, last: None, tx, rx }
DnsServersWatcher { dns_group, last: None, tx, rx, resolver }
}

/// Exposes the latest list of DNS servers for this DNS group
Expand Down Expand Up @@ -75,58 +65,38 @@ impl BackgroundTask for DnsServersWatcher {
};

// Read the latest service configuration for this DNS group.
let service_kind = match self.dns_group {
DnsGroup::Internal => ServiceKind::InternalDns,
DnsGroup::External => ServiceKind::ExternalDns,
let service_name = match self.dns_group {
DnsGroup::Internal => ServiceName::InternalDns,
DnsGroup::External => ServiceName::ExternalDns,
};

let pagparams = DataPageParams {
marker: None,
limit: NonZeroU32::try_from(
u32::try_from(MAX_DNS_SERVERS).unwrap(),
)
.unwrap(),
direction: dropshot::PaginationOrder::Ascending,
let result = self.resolver.lookup_all_socket_v6(service_name).await;
let addresses = match result {
Err(error) => {
warn!(
&log,
"failed to lookup DNS servers";
"error" => format!("{:#}", error)
);
return json!({
"error":
format!(
"failed to read list of DNS servers: {:#}",
error
)
});
}
Ok(addresses) => {
// TODO(eliza): it would be nicer if `Resolver` had a method
// returning an iterator instead of a `Vec`, so we didn't
// have to drain the Vec and then collect it into a new
// one...
Comment on lines +90 to +93
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be very doable, though it doesn't need to be part of this PR. The lookup_all_ipv6 method is a convenience function within omicron, acting on an iterator type internally, which we could totally return.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i might do that, although i didn't think it was particularly important to minimize allocation overhead in the background task --- this code doesn't seem particularly hot.

Copy link
Contributor

Choose a reason for hiding this comment

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

The lookup methods on Resolver could definitely use a fresh pass. But agreed, it's not a blocker here

addresses.into_iter().map(SocketAddr::V6).collect()
}
};

let result = self
.datastore
.services_list_kind(opctx, service_kind, &pagparams)
.await;

if let Err(error) = result {
warn!(
&log,
"failed to read list of DNS servers";
"error" => format!("{:#}", error)
);
return json!({
"error":
format!(
"failed to read list of DNS servers: {:#}",
error
)
});
}

let services = result.unwrap();
if services.len() >= MAX_DNS_SERVERS {
warn!(
&log,
"found {} servers, which is more than MAX_DNS_SERVERS \
({}). There may be more that will not be used.",
services.len(),
MAX_DNS_SERVERS
);
}

let new_config = DnsServersList {
addresses: services
.into_iter()
.map(|s| SocketAddrV6::new(*s.ip, *s.port, 0, 0).into())
.collect(),
};
let new_addrs_dbg = format!("{:?}", new_config);
let new_config = DnsServersList { addresses };
let new_addrs_dbg = format!("{new_config:?}");
let rv =
serde_json::to_value(&new_config).unwrap_or_else(|error| {
json!({
Expand Down Expand Up @@ -177,119 +147,3 @@ impl BackgroundTask for DnsServersWatcher {
.boxed()
}
}

#[cfg(test)]
mod test {
use crate::app::background::common::BackgroundTask;
use crate::app::background::dns_servers::DnsServersList;
use crate::app::background::dns_servers::DnsServersWatcher;
use crate::app::background::dns_servers::MAX_DNS_SERVERS;
use assert_matches::assert_matches;
use async_bb8_diesel::AsyncRunQueryDsl;
use diesel::ExpressionMethods;
use diesel::QueryDsl;
use nexus_db_model::DnsGroup;
use nexus_db_queries::context::OpContext;
use nexus_db_queries::db::model::Service;
use nexus_db_queries::db::model::ServiceKind;
use nexus_test_utils_macros::nexus_test;
use std::net::Ipv6Addr;
use std::net::SocketAddrV6;
use uuid::Uuid;

type ControlPlaneTestContext =
nexus_test_utils::ControlPlaneTestContext<crate::Server>;

#[nexus_test(server = crate::Server)]
async fn test_basic(cptestctx: &ControlPlaneTestContext) {
let nexus = &cptestctx.server.apictx().nexus;
let datastore = nexus.datastore();
let opctx = OpContext::for_tests(
cptestctx.logctx.log.clone(),
datastore.clone(),
);

// Verify the initial state.
let mut task =
DnsServersWatcher::new(datastore.clone(), DnsGroup::Internal);
let watcher = task.watcher();
assert_matches!(*watcher.borrow(), None);

// The datastore from the ControlPlaneTestContext is initialized with
// one DNS server.
let _ = task.activate(&opctx).await;
assert_matches!(*watcher.borrow(), Some(DnsServersList {
ref addresses
}) if addresses.len() == 1);

// If we add another server, we should see it.
{
use nexus_db_queries::db::schema::service::dsl;
diesel::insert_into(dsl::service)
.values(Service::new(
Uuid::new_v4(),
Uuid::new_v4(),
Some(Uuid::new_v4()),
SocketAddrV6::new(Ipv6Addr::LOCALHOST, 1, 0, 0),
ServiceKind::InternalDns,
))
.execute_async(
&*datastore.pool_connection_for_tests().await.unwrap(),
)
.await
.unwrap();
}

let _ = task.activate(&opctx).await;
assert_matches!(*watcher.borrow(), Some(DnsServersList {
ref addresses
}) if addresses.len() == 2);

// If we add MAX_DNS_SERVERS more servers, we should see
// MAX_DNS_SERVERS.
{
use nexus_db_queries::db::schema::service::dsl;
let new_services = (0..u16::try_from(MAX_DNS_SERVERS).unwrap())
.map(|i| {
Service::new(
Uuid::new_v4(),
Uuid::new_v4(),
Some(Uuid::new_v4()),
SocketAddrV6::new(Ipv6Addr::LOCALHOST, i + 2, 0, 0),
ServiceKind::InternalDns,
)
})
.collect::<Vec<_>>();

diesel::insert_into(dsl::service)
.values(new_services)
.execute_async(
&*datastore.pool_connection_for_tests().await.unwrap(),
)
.await
.unwrap();
}

let _ = task.activate(&opctx).await;
assert_matches!(*watcher.borrow(), Some(DnsServersList {
ref addresses
}) if addresses.len() == MAX_DNS_SERVERS);

// Now delete all the servers and try again.
{
use nexus_db_queries::db::schema::service::dsl;
diesel::delete(
dsl::service.filter(dsl::kind.eq(ServiceKind::InternalDns)),
)
.execute_async(
&*datastore.pool_connection_for_tests().await.unwrap(),
)
.await
.unwrap();
}
let _ = task.activate(&opctx).await;
assert_matches!(*watcher.borrow(), Some(DnsServersList {
ref addresses
}) if addresses.is_empty());
}
}
Loading
Loading