Skip to content

Add support for creating static DNS 'A' records in consommé - #4085

Open
OneBlue wants to merge 11 commits into
microsoft:mainfrom
OneBlue:user/oneblue/dns-records
Open

Add support for creating static DNS 'A' records in consommé#4085
OneBlue wants to merge 11 commits into
microsoft:mainfrom
OneBlue:user/oneblue/dns-records

Conversation

@OneBlue

@OneBlue OneBlue commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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

@OneBlue
OneBlue requested a review from a team as a code owner July 29, 2026 21:35
Copilot AI review requested due to automatic review settings July 29, 2026 21:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 A queries.
  • 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.

Comment thread vm/devices/net/net_consomme/src/lib.rs Outdated
Comment thread vm/devices/net/net_consomme/consomme/src/dns_records.rs Outdated
Comment thread vm/devices/net/net_consomme/consomme/src/udp.rs Outdated
Comment thread vm/devices/net/net_consomme/src/lib.rs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cb4bc6b9-d84b-4992-9b4f-c97a1e493f16
@github-actions

Copy link
Copy Markdown

@jstarks

jstarks commented Jul 30, 2026

Copy link
Copy Markdown
Member

DNSSEC delenda est.

Copilot AI review requested due to automatic review settings July 30, 2026 18:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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_name rejects 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_response will 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.

Comment thread vm/devices/net/net_consomme/consomme/src/dns_records.rs Outdated
@OneBlue OneBlue closed this Jul 30, 2026
@OneBlue
OneBlue deleted the user/oneblue/dns-records branch July 30, 2026 19:21
@OneBlue OneBlue reopened this Jul 30, 2026
Copilot AI review requested due to automatic review settings July 30, 2026 20:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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_dns using DnsTransport::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.

@github-actions

Copy link
Copy Markdown

Copilot AI review requested due to automatic review settings July 30, 2026 20:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.

Copilot AI review requested due to automatic review settings July 30, 2026 21:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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_response unconditionally caps max_len to 512 bytes (MAX_DNS_UDP_RESPONSE_LEN). This makes the max_len parameter misleading and also prevents larger static responses over DNS-over-TCP (the TCP path passes u16::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_response currently also enforces the 512-byte DNS/UDP limit internally. If build_response is 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants