Skip to content

fix(dns): fold domain case as ASCII, matching the reverse-DNS parser#2723

Merged
chet merged 1 commit into
NVIDIA:mainfrom
chet:dns-normalize-domain-ascii
Jun 22, 2026
Merged

fix(dns): fold domain case as ASCII, matching the reverse-DNS parser#2723
chet merged 1 commit into
NVIDIA:mainfrom
chet:dns-normalize-domain-ascii

Conversation

@chet

@chet chet commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

normalize_domain folded domain case with to_lowercase (Unicode-aware), while arpa_qname_to_ip (the reverse-DNS parser from #2637) uses to_ascii_lowercase. DNS names are ASCII (RFC 1123) and case-insensitivity is ASCII-scoped (RFC 4343), so this aligns normalize_domain to the same ASCII primitive — the two now fold case identically, by construction.

  • to_lowercaseto_ascii_lowercase: no change for a valid ASCII domain; the two differ only on non-ASCII input, which is not valid in a DNS name.
  • The debug log moves to structured (logfmt) fields.
  • The test now covers the case folding it had skipped — its only input was already lowercase — with an uppercase and a mixed-case name.

Addresses CodeRabbit's open comment on #2691. Supports #2637; part of the #2630 (reverse DNS) effort.

Draft pending review.

DNS names are ASCII (RFC 1123) and their case-insensitivity is defined over ASCII (RFC 4343), so `normalize_domain` now folds case with `to_ascii_lowercase` -- the same primitive `arpa_qname_to_ip` already uses. For a valid (ASCII) domain the normalized result is unchanged; the two functions now agree by construction rather than one reaching for the Unicode-aware `to_lowercase`. The debug log moves to structured fields in the same pass.

The test now exercises the case folding it had skipped -- its only input was already lowercase -- covering an uppercase and a mixed-case name.

This supports NVIDIA#2637.

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet

chet commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@copy-pr-bot

copy-pr-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 56c27b30-0941-412f-aa6d-0117518b01e3

📥 Commits

Reviewing files that changed from the base of the PR and between f947966 and 6641f5f.

📒 Files selected for processing (1)
  • crates/api-db/src/dns/mod.rs

Summary by CodeRabbit

  • Refactor

    • Improved DNS domain normalization logic with enhanced structured logging for better observability.
  • Tests

    • Expanded test coverage for domain normalization with multiple input/output scenarios to ensure reliability across edge cases.

Walkthrough

normalize_domain is updated to use to_ascii_lowercase() instead of a generic lowercasing method, and its debug log is refactored from a formatted string to a structured tracing event with named input and normalized fields. The corresponding unit test is expanded from a single assertion to a value_scenarios!-driven parametric table.

Changes

normalize_domain: ASCII lowercasing, structured logging, and test coverage

Layer / File(s) Summary
ASCII lowercasing, structured debug log, and parametric unit tests
crates/api-db/src/dns/mod.rs
normalize_domain now calls to_ascii_lowercase() and emits a structured tracing::debug! with discrete input and normalized fields. test_normalize_domain_name is replaced by a value_scenarios! table exercising trailing-dot stripping and ASCII case folding across multiple input variants.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • NVIDIA/infra-controller#2691: Modifies the same normalize_domain function in crates/api-db/src/dns/mod.rs, introducing to_ascii_lowercase() and expanding test coverage via value_scenarios!, directly overlapping with this PR's scope.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and precisely identifies the core change: switching from Unicode-aware to ASCII case folding in the DNS domain normalization function to match the reverse-DNS parser behavior.
Description check ✅ Passed The description thoroughly explains the rationale (RFC compliance), the technical change (to_lowercase → to_ascii_lowercase), and test improvements, demonstrating clear understanding of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@chet chet marked this pull request as ready for review June 22, 2026 07:33
@chet chet requested a review from a team as a code owner June 22, 2026 07:33
@github-actions

Copy link
Copy Markdown

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
boot-artifacts-aarch64 3 0 0 3 0 0
boot-artifacts-x86_64 3 0 0 3 0 0
forge-admin-cli-x86_64 264 6 23 99 6 130
machine-validation-runner 704 34 183 258 35 194
machine_validation 704 34 183 258 35 194
nvmetal-carbide 704 34 183 258 35 194
TOTAL 2382 108 572 879 111 712

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@chet chet merged commit 2a03436 into NVIDIA:main Jun 22, 2026
55 checks passed
@chet chet deleted the dns-normalize-domain-ascii branch June 22, 2026 23:19
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.

2 participants