Add support for creating static DNS 'A' records in consommé - #4085
Add support for creating static DNS 'A' records in consommé#4085OneBlue wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The static-record name normalization currently accepts inputs that can never match real DNS questions, and the new UDP fast-path lacks a direct test despite existing test harness support in the same file.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds a “static DNS” fast-path to the net_consomme virtual networking stack so host code can register DNS records that are answered immediately (currently only A records), bypassing the normal resolver pipeline.
Changes:
- Introduces a static DNS record store and wire-format response builder for
Aqueries. - Adds a control-plane API (
ConsommeControl::add_dns_record) and internal message handling to register records at runtime. - Hooks UDP gateway DNS handling to check static records first and emit a response immediately on match.
File summaries
| File | Description |
|---|---|
| vm/devices/net/net_consomme/src/lib.rs | Exposes static DNS record types/errors and adds a control API + message wiring for registering records. |
| vm/devices/net/net_consomme/consomme/src/udp.rs | Adds the UDP DNS fast-path to answer matching queries from the static record store. |
| vm/devices/net/net_consomme/consomme/src/lib.rs | Adds the static_dns store to Consomme and an API to register records. |
| vm/devices/net/net_consomme/consomme/src/dns_records.rs | New module implementing record storage, query parsing, and A response construction with unit tests. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cb4bc6b9-d84b-4992-9b4f-c97a1e493f16
|
DNSSEC delenda est. |
There was a problem hiding this comment.
🟡 Not ready to approve
The static DNS implementation has correctness and robustness gaps (query validation and response sizing/MTU considerations) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (4)
vm/devices/net/net_consomme/src/lib.rs:324
- Doc comment is missing a space after
///, which breaks rustdoc formatting and is inconsistent with surrounding docs.
///Adds a static DNS record that will be returned directly
/// if the guest sends a matching query.
vm/devices/net/net_consomme/consomme/src/dns_records.rs:133
normalize_namerejects empty labels but doesn’t enforce the DNS max label length (63). As written, callers can add records that can never appear on the wire (and could lead to unexpectedly large stored names).
// Reject empty labels ("..").
if name.split('.').any(|label| label.is_empty()) {
return None;
}
Some(name.to_ascii_lowercase())
vm/devices/net/net_consomme/consomme/src/dns_records.rs:98
build_responsewill currently answer DNS packets even if the incoming message is already a response (QR=1). That can cause incorrect behavior if the guest sends DNS responses to port 53 (or otherwise misroutes traffic).
let packet = DnsPacket::new_checked(query).ok()?;
if packet.question_count() != 1 {
// Multiple or no question; let the query go through.
vm/devices/net/net_consomme/consomme/src/udp.rs:606
- Static DNS responses can grow without bound (one RR per matching stored record). A large record set for a single name could produce a UDP payload/frame that exceeds the guest MTU (and/or the internal send buffer), leading to dropped DNS replies and potentially large allocations when building the response. Consider bounding the number of answers (or total response size) based on
client.rx_mtu()/ a fixed maximum.
if let Some(response_data) = self.inner.static_dns.build_response(udp.payload()) {
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Static DNS interception is implemented only for UDP despite existing TCP DNS support, which can lead to behavior that contradicts the stated “DNS question from the guest” semantics (e.g., TCP retries bypass static records).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
vm/devices/net/net_consomme/consomme/src/udp.rs:603
- Static DNS records are only consulted for UDP DNS queries. The codebase already supports DNS over TCP (see
TcpConnection::new_dnsusingDnsTransport::Tcp), so clients that retry over TCP (e.g. after a truncated UDP response) will bypass the static-record path and hit the resolver instead, which seems to contradict the PR description (“DNS question from the guest”). Consider extending static record handling to the TCP DNS path as well, or explicitly documenting/renaming the API to indicate it only applies to UDP queries.
fn handle_dns(
&mut self,
frame: &EthernetRepr,
src_addr: IpAddress,
dst_addr: IpAddress,
udp: &UdpPacket<&[u8]>,
) -> Result<bool, DropReason> {
let flow = DnsFlow {
src: SocketAddr::new(src_addr.into(), udp.src_port()),
dst: SocketAddr::new(dst_addr.into(), udp.dst_port()),
gateway_mac: self.inner.state.params.gateway_mac,
client_mac: frame.src_addr,
transport: crate::dns_resolver::DnsTransport::Udp,
};
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces a user-visible DNS behavior change that should be reflected in the OpenVMM Guide’s Consomme backend documentation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
vm/devices/net/net_consomme/consomme/src/lib.rs:875
- This introduces a new guest-visible DNS behavior (static A-record responses bypassing the resolver). The Guide’s Consomme backend page currently describes DNS as always being forwarded to the host resolver; it should be updated to mention static records and how/when they take precedence (and any current limitations like only A records and no DNSSEC/AD).
/// Adds a static DNS record that will be returned directly
/// if the guest sends a matching query.
pub fn add_dns_record(
&mut self,
record_type: StaticDnsRecordType,
name: &str,
rdata: &[u8],
) -> Result<(), StaticDnsRecordError> {
self.static_dns.add(record_type, name, rdata)
}
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Static DNS responses are currently hard-capped to 512 bytes inside build_response, which unintentionally constrains DNS-over-TCP responses and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (4)
vm/devices/net/net_consomme/consomme/src/dns_records.rs:137
build_responseunconditionally capsmax_lento 512 bytes (MAX_DNS_UDP_RESPONSE_LEN). This makes themax_lenparameter misleading and also prevents larger static responses over DNS-over-TCP (the TCP path passesu16::MAX). Consider letting callers enforce the UDP 512-byte limit and only capping here to the DNS-over-TCP framing maximum (u16::MAX).
let budget = max_len.min(MAX_DNS_UDP_RESPONSE_LEN);
let mut total = DNS_HEADER_LEN + question.buffer_len();
vm/devices/net/net_consomme/consomme/src/tcp.rs:1026
- There is an extra space in the comment (
records is).
// rx path: feed guest data into the DNS handler for query extraction.
// Done before the tx path so that a query answered locally from static
// records is drained into tx_buffer within the same poll.
vm/devices/net/net_consomme/consomme/src/udp.rs:616
- The comment says static responses are limited to the MTU, but
StaticDnsRecords::build_responsecurrently also enforces the 512-byte DNS/UDP limit internally. Ifbuild_responseis changed to allow >512 (needed for TCP), this call site should explicitly enforce the UDP 512-byte cap (in addition to the MTU cap).
// Limit static DNS response sizes to the MTU.
let ip_header_len = if matches!(dst_addr, IpAddress::Ipv4(_)) {
IPV4_HEADER_LEN
} else {
IPV6_HEADER_LEN
};
vm/devices/net/net_consomme/consomme/src/lib.rs:868
- This adds a new user-facing Consomme capability (static DNS answers), but
Guide/src/reference/backends/consomme.md's DNS section does not mention static records or their current limitations (A-only, AD unset, etc.). Please update the Guide (or add a follow-up tracking issue) so consumers know the feature exists and how it behaves.
/// Adds a static DNS record that will be returned directly
/// if the guest sends a matching query.
pub fn add_dns_record(
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This change adds API support for registering "static" DNS records via the virtionet API. When a DNS question from the guest matches a static record, that record is returned immediately, bypassing the resolver.
This change only implements support for "A" records, but the API is designed to easily allow support for more DNS records types in the future.
This change also does not support DNSSEC, and always returns replies with the AD flag unset. This can be addressed in a followup if needed