feat(dns): add HTTPS, SVCB, and TLSA record support - #246
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The TLSA flag parsers currently accept out-of-spec values (e.g., --usage 0..=255 instead of 0..=3), which can allow invalid input through and lead to avoidable API validation failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds first-class CLI support for managing modern DNS record types (HTTPS, SVCB, TLSA) in the gddy dns command family, aligning the Rust CLI’s record modeling and request payloads with the updated v3 Domains API schema.
Changes:
- Expand recognized writable/listable record types to include
HTTPS,SVCB, andTLSA, and update help text accordingly. - Add TLSA-specific flags (
--usage,--selector,--matching-type) and an HTTPS/SVCB--parametersflag, plus pre-flight validation for type-specific options. - Update v3 record construction and write-path tests to ensure HTTPS parameters and TLSA fields serialize into request bodies correctly.
File summaries
| File | Description |
|---|---|
| rust/src/dns/set/write.rs | Adds write-path tests verifying HTTPS parameters and TLSA fields are sent in the POST body. |
| rust/src/dns/set/mod.rs | Wires in new TLSA/SVCB validation before executing dns set. |
| rust/src/dns/records.rs | Extends supported types, adds new flags/options, validates type-specific fields, and maps TLSA/HTTPS/SVCB fields into v3 DnsRecord. |
| rust/src/dns/list.rs | Updates dns list --type help text to include HTTPS/SVCB/TLSA. |
| rust/src/dns/delete.rs | Updates dns delete --type help text to include HTTPS/SVCB/TLSA. |
| rust/src/dns/add.rs | Wires in new TLSA/SVCB validation before executing dns add. |
Review details
Suppressed comments (2)
rust/src/dns/records.rs:170
- The clap range for
--selectorallows 0..=255, but TLSA selector is defined as 0-1. Restricting the range provides immediate user feedback and prevents avoidable API validation errors.
/// TLSA selector, 0-1 (RFC 6698 §2.1.2; TLSA only; required for TLSA). 0
/// full certificate, 1 SubjectPublicKeyInfo.
#[arg(long = "selector", value_name = "N", value_parser = clap::value_parser!(i64).range(0..=255), required_if_eq("record_type", "TLSA"))]
pub(super) selector: Option<i64>,
rust/src/dns/records.rs:175
- The clap range for
--matching-typeallows 0..=255, but TLSA matching type is defined as 0-2. Tighten the range so invalid inputs are rejected at parse time.
/// TLSA matching type, 0-2 (RFC 6698 §2.1.3; TLSA only; required for
/// TLSA). 0 exact match, 1 SHA-256, 2 SHA-512.
#[arg(long = "matching-type", value_name = "N", value_parser = clap::value_parser!(i64).range(0..=255), required_if_eq("record_type", "TLSA"))]
pub(super) matching_type: Option<i64>,
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
TLSA records now omit data (using certificateData instead), but downstream conflict/duplicate detection and per-record reporting still assume data, leading to incorrect behavior and confusing output for TLSA operations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
rust/src/dns/records.rs:133
--priorityis documented as “MX, SRV, HTTPS, and SVCB only”, but it can currently be provided for any record type and will be sent to the API (since there’s no validation like the CAA/TLSA/SVCB guards). This makes the CLI accept invalid combinations and can lead to confusing API-side validation errors.
Consider adding a validation guard (similar to validate_caa_fields) that rejects --priority unless the record type is one of MX/SRV/HTTPS/SVCB, and call it from both dns add and dns set before any network calls.
rust/src/dns/records.rs:137
--port/--protocolare described here as “SRV and TLSA only”, but there’s no reverse validation to prevent them being set for other record types (and they will be serialized into the v3 request body). That mismatch between help text and behavior can produce hard-to-understand validation failures downstream.
Consider adding a validation helper that rejects SRV/TLSA-only fields (--port, --protocol, and also the SRV-only --service/--weight) unless record_type is SRV (and TLSA where applicable).
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The CLI now documents several flags as type-specific (e.g., --priority, --port, --protocol) but still accepts and forwards them for unrelated record types without validation, creating an inconsistent and potentially confusing public interface.
Review details
Suppressed comments (1)
rust/src/dns/records.rs:149
- The CLI help text now documents
--priority/--port/--protocolas only applying to specific record types, but there’s no validation to reject these flags for other types (andv3_recordwill still include any provided values in the request body). This can lead to confusing UX (flags appear accepted but are ignored or rejected server-side). Consider adding a small validation helper (similar tovalidate_caa_fields/validate_tlsa_fields/validate_svcb_fields) that rejectspriorityunless type is MX/SRV/HTTPS/SVCB and rejectsport/protocolunless type is SRV/TLSA (and ideally also gateweight/serviceto SRV).
/// Record priority (MX, SRV, HTTPS, and SVCB only). For HTTPS/SVCB, 0
/// means AliasMode.
#[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..=65535))]
pub(super) priority: Option<i64>,
/// Service port (SRV and TLSA only).
#[arg(long, value_name = "PORT", value_parser = clap::value_parser!(i64).range(1..=65535))]
pub(super) port: Option<i64>,
/// Record weight (SRV only).
#[arg(long, value_name = "N", value_parser = clap::value_parser!(i64).range(0..=65535))]
pub(super) weight: Option<i64>,
/// Service protocol, e.g. _tcp (SRV and TLSA only).
#[arg(long, value_name = "PROTO")]
pub(super) protocol: Option<String>,
/// Service type (SRV only).
#[arg(long, value_name = "SERVICE")]
pub(super) service: Option<String>,
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
gddy dns add/set/delete/list now recognize HTTPS, SVCB, and TLSA record types. TLSA's certificate association fields (usage/selector/matching-type) get dedicated flags, with the certificate data carried by the existing --data flag; HTTPS/SVCB get a --parameters flag for SvcParams, and reuse the existing --priority for SvcPriority.
RFC 6698 defines usage 0-3, selector 0-1, and matching-type 0-2; values outside those ranges aren't supported by the API and would otherwise round-trip to a 422 instead of failing fast at parse time.
v3_record() moves TLSA's value into certificateData since data isn't used for that type, but conflict diagnosis, delete/set reporting, and exact-duplicate detection still read data directly, so TLSA records showed "(no data)" and dodged duplicate detection. Adds a record_value helper that checks certificateData when data is absent, and switches every one of those read sites to use it.
96e06a4 to
ca49c3a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
--data already carries type-specific meaning (CA domain for CAA, target hostname for HTTPS/SVCB); overloading it with TLSA's hex certificate association data too was confusing. TLSA now has its own --cert-data flag, with --data required for every other writable type and rejected for TLSA.
There was a problem hiding this comment.
🟡 Changes recommended
record_value() currently prefers data over certificateData, which can misreport TLSA values if the API returns both fields.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
record_value fell back to data only when it was absent, so a TLSA record with both fields populated (not something v3_record builds, but not guaranteed absent from the API either) would surface the wrong value. Check the record's type and prefer certificate_data for TLSA.
There was a problem hiding this comment.
🟡 Changes recommended
dns list now accepts TLSA but its default output still projects data, which will render TLSA values as empty unless it also includes (or maps to) certificateData.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
TLSA records carry their value in certificateData, not data (absent for them), so dns list --type TLSA rendered a blank value column by default. Add certificateData to list's default field projection.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are consistent across command paths, add clear preflight validation, and include targeted unit/integration tests for the new record types and TLSA value handling.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
DNS treats a TLSA record's RDATA as one blob (RFC 6698 SS2.1) -- dig/zone-file output shows "<usage> <selector> <matching-type> <hex>". The v3 API splits that into four JSON fields, which is that API's own modeling choice, not something DNS does. dns list now reconstructs the presentation-format string into data for TLSA rows, leaving the original usage/selector/matchingType/certificateData fields in place alongside it -- supersedes the previous fix of just adding certificateData to the default field list, which left the value column blank and buried a large, rarely-useful column ahead of ttl.
There was a problem hiding this comment.
🔵 Needs a closer look
dns set’s no-op/duplicate handling compares only the “value” string, which can incorrectly skip updates (and/or misreport duplicates) when TLSA or other type-specific fields change while the value remains the same.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
rust/src/dns/conflicts.rs:119
- The exact-duplicate detection uses
record_value(r) == Some(desired_data). For TLSA,record_valueis onlycertificateData, but the record’s effective RDATA also includesusage/selector/matchingType(and potentiallyport/protocol). This can report an "exact duplicate" when the cert data matches but the other TLSA fields differ, which is misleading for users troubleshootingDUPLICATE_RECORD.
rust/src/dns/set/mod.rs:193 dns set's no-op detection only compares the existing record'srecord_value()againstreq.value. For TLSA (and also HTTPS/SVCB/MX/SRV/CAA where options contribute to the wire record), this can incorrectly skip a replace when the "value" string is unchanged but other fields differ (e.g. updating TLSA--usage/--selector/--matching-typewith the same--cert-data, or updating HTTPS/SVCB--parameters). This leaves the old record in place even though the user requested a change.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
describe_duplicate_record's exact-match check and apply_replace's no-op check both compared only the primary value string, so a record type whose identity spans several fields -- CAA's flag/tag, SRV's priority/weight/port, TLSA's usage/selector/matchingType, HTTPS/SVCB's priority/parameters -- could be misjudged: two TLSA records with the same certificate data but different usage looked like an exact duplicate, and a dns set that changed only usage while keeping the same --cert-data was silently skipped as a no-op. Adds same_content(), which compares two v3 records via JSON minus ttl/recordId, and threads RecordOptions through describe_duplicate_record and apply_replace so both build the actual desired record and compare against it instead of just the value.
|
Addressed Copilot's "duplicate handling" note from the review on 167b266 (it was only in the review body's "Suppressed comments" section, not posted as an inline thread — my review-loop polling only checks GraphQL Fixed in d137e8b: added |
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness gaps around TTL-based no-op detection and TLSA/HTTPS/SVCB validation/output that can cause skipped updates or misleading output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
rust/src/dns/records.rs:268
validate_svcb_fieldsrejects--parametersfor non-HTTPS/SVCB types, but it doesn't enforce that HTTPS/SVCB records provide--priorityeven though the API schema describespriorityas required for HTTPS/SVCB. This leads to avoidable round-trips where the server rejects the request. Consider validating--priorityis present whenrecord_typeis HTTPS or SVCB.
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
merged_tlsa_data substituted 0 -- a real, meaningful usage/selector/ matching-type value, not a sentinel -- for a missing numeric field, which could fabricate presentation-format RDATA that was never actually returned. Falls back to the certificate data alone when any of the three numeric fields is absent. apply_replace's no-op check compares same_content, which deliberately ignores ttl, so a `dns set --ttl <new>` that changed nothing else was silently skipped. Adds a separate ttl check against the raw --ttl input (not v3_record's defaulted ttl), so an explicit ttl change still triggers a replace, while an omitted --ttl still doesn't force one just because the CLI's default differs from the existing record.
There was a problem hiding this comment.
🔵 Needs a closer look
same_content currently masks JSON serialization failures by falling back to Null, which can cause incorrect duplicate/no-op decisions.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
rust/src/dns/records.rs:387
same_contentsilently treats JSON serialization failures asValue::Null(to_value(...).unwrap_or(Null)), which can incorrectly classify records as equal and cause wrong duplicate/no-op decisions. Prefer returningfalse(or otherwise surfacing the error) if either record can’t be serialized, rather than falling back toNull.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Every other multi-field type (CAA, SRV, MX, HTTPS, SVCB) reuses --data for its primary value and splits the rest across sibling flags -- TLSA having a dedicated --cert-data flag was inconsistent with that pattern rather than a necessity. v3_record() already routed --data's value into certificateData for TLSA internally, so this only touches the CLI-argument layer: removes --cert-data and validate_tlsa_values, and makes --data unconditionally required again.
There was a problem hiding this comment.
🔵 Needs a closer look
same_content can silently treat serialization failures as equality (via Value::Null fallback), which can incorrectly skip a real replace/duplicate detection path.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
rust/src/dns/records.rs:341
same_contentcurrently falls back toValue::Nullifserde_json::to_valuefails, which can silently treat unrelated records as equal (and skip a requested change). Sincetypes::DnsRecordshould always be serializable, it’s safer to fail loudly here.
rust/src/dns/records.rs:290- The
record_valuedoc comment says exact-duplicate detection uses this helper, but exact-duplicate/no-op detection now compares full records viasame_content(and no longer callsrecord_value). Updating the comment will prevent future readers from assumingrecord_valueis part of the duplicate check logic.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
same_content fell back to Value::Null if serialization failed, so two unrelated records that both failed to serialize would compare equal and silently skip a real change. DnsRecord's fields are all plain String/Option/newtype, so this should never actually happen -- expect it instead of masking a real bug as a false "no difference". Also fixes record_value's doc comment, which still claimed exact- duplicate/no-op detection reads through it; that moved to same_content in an earlier commit.
|
Addressed two more suppressed-comment findings from the review on 0ca6ce3 (my improved polling now catches these — see comment above about the review-loop gap): Fixed in 18dbb66:
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes are coherent, well-tested with focused regressions for the new record types and the corrected no-op/duplicate behavior, and align with the stated PR requirements.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
gddy dns add/set/delete/listnow recognizeHTTPS,SVCB, andTLSArecord types (DEVEX-1105).--dataflag for its certificate association data (hex-encoded) — same pattern as CAA/SRV/MX/HTTPS/SVCB, which all also split their RDATA across--data(the primary value) plus sibling flags. Adds dedicated--usage/--selector/--matching-typeflags for the rest, clap-validated to their actual RFC 6698 ranges (0-3/0-1/0-2).--parametersflag for SvcParams;--priority(already existing) doubles as SvcPriority.dns listre-merges TLSA's split fields (usage/selector/matchingType/certificateData) back into a singledatastring matching DNS's actual RDATA presentation format (RFC 6698 — same asdig/zone-file output), since the v3 API'sdatafield is the one exception that goes unused for TLSA on the wire. The original four fields stay available alongside it.dns add's "exact value already exists" message,dns set's replace-vs-no-op decision) now compares full record content, not just the primary value — fixes a pre-existing gap (not new to this PR) that affected every multi-field type: CAA (flag/tag), SRV (priority/weight/port), and now TLSA (usage/selector/matchingType)/HTTPS/SVCB (priority/parameters) too. Also fixesdns set --ttl <new>being silently skipped as a no-op when only the TTL changed.Rebased onto
mainnow that #244 (spec drift resync, which this depends on for the real TLSA/HTTPS/SVCB API fields) has merged.Test plan
cargo check --workspacecargo clippy --workspace -- -D warningscargo test --workspace(773 passed)cargo fmt --check./rust/scripts/check-module-size.sh