feat(cisco): model ASA DNS server groups and domain-lookup - #10133
Conversation
Replace the inherited IOS-style DNS grammar and extraction with an ASA-native model. Parse "dns domain-lookup", "dns server-group" (with name-server, domain-name, timeout, retries, poll-timer, and expire-entry-timer), "dns name-server", "dns-group", and "dns-group-map"/"dns-to-domain". DNS lookup is now tracked per interface instead of as a single global flag. DNS servers are modeled as DnsServerGroup objects (each holding NameServer entries and tuning parameters) keyed by group name, and the vendor-independent DNS server set is derived from all groups. Server group tuning values are validated against their documented ranges, and out-of-range values are dropped with a warning. Remove the IOS-only "domain", "ip domain", "ip domain-name", and "ip name-server" rules along with the now-unused dnsServers and dnsSourceInterface fields.
|
This change is |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10133 +/- ##
==========================================
+ Coverage 74.22% 74.23% +0.01%
==========================================
Files 3482 3484 +2
Lines 179024 179137 +113
Branches 21962 21975 +13
==========================================
+ Hits 132876 132983 +107
+ Misses 36289 36280 -9
- Partials 9859 9874 +15
🚀 New features to boost your workflow:
|
Assert the extracted DnsServerGroup objects as a set in the grammar test so DnsServerGroup and NameServer equals/hashCode are exercised, raising patch coverage above the target.
|
Happy to add the last 0.06% of the code coverage to make the test pass if necessary :) |
dhalperi
left a comment
dhalperi
left a comment
There was a problem hiding this comment.
@dhalperi reviewed 2 files and made 4 comments.
Reviewable status: 2 of 11 files reviewed, 4 unresolved discussions (waiting on Murt2005).
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_asa/AsaParser.g4 line 2315 at r2 (raw file):
; s_dns
create a new file called Asa_dns.4 for s_dns and move all this to it.
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_asa/AsaParser.g4 line 2322 at r2 (raw file):
| dns_name_server | dns_server_group | dns_null
we don't use dns_null catch-all rules -- that would be a rule like dns_null: DNS null_rest_of_line (or similar). If you want to ignore keywords, create one null rule per keyword.
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_asa/AsaParser.g4 line 2392 at r2 (raw file):
: DNS_GROUP_MAP NEWLINE dns_to_domain*
Please reread the docs about idiomatic grammar rules. this should be like dnsgm_dns_to_domain -- the dnsgm is for dns_group_map and dns_to_domain is the next token. This problem happens in here a lot.
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_asa/AsaParser.g4 line 2624 at r2 (raw file):
; s_ip_name_server
is the claim that all of these are not valid ASA syntax? I didn't see it called out.
Move s_dns and its related rules (dns server-group, dns-group, dns-group-map) out of AsaParser.g4 into Asa_dns.g4. Rename child rules to follow the <parent-abbrev>_<next-token> convention: dnsg_* becomes dnssg_* (dns server-group) and dns_to_domain becomes dnsgm_dns_to_domain (dns-group-map). Drop the dns_null catch-all rule; unmodeled dns subcommands now surface as normal unrecognized-line warnings instead of being silently swallowed. Update AsaControlPlaneExtractor context imports and listener methods to match the renamed rules. No parsing or extraction behavior changes. All tests pass locally.
|
Applied all your feedback. Moved the dns related grammar to
Yes, these are not valid ASA syntax. The commands
|
dhalperi
left a comment
dhalperi
left a comment
There was a problem hiding this comment.
Nice! One Q before I merge.
@dhalperi reviewed 10 files and all commit messages, made 3 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Murt2005).
projects/batfish/src/main/antlr4/org/batfish/grammar/cisco_asa/Asa_dns.g4 line 54 at r3 (raw file):
dnssg_timeout : TIMEOUT secs = dec NEWLINE
can these be more precise than dec? I see you have ranges in the extrator.
projects/batfish/src/main/java/org/batfish/grammar/cisco_asa/AsaControlPlaneExtractor.java line 4451 at r3 (raw file):
public void exitDnsgm_dns_to_domain(Dnsgm_dns_to_domainContext ctx) { // "dns-to-domain <group> <domain>" maps a domain to a DNS server group. Each domain maps to // exactly one group.
should we define and track references to these groupo?
Reviewing a vendor parsing PR by reading the diff misses the failures that matter: a submode rule that silently swallows the next global command, a removed rule that regresses configs no test covers, or extracted state that nothing ever reads. The skill drives a differential-probe workflow instead -- parse a hand-built config on the branch and on the merge base, then diff -- and points at the docs/ that already encode our conventions (parser_rule_conventions for LL(1) shape and rule naming, implementation_guide for the extract-vs-_null decision and Pattern 4 structure tracking, extraction/README for range validation and ParseWarning vs redFlag). The severity rubric is the load-bearing part, since the two easiest mistakes both produce confident wrong findings. CLI submodes are not indentation-sensitive, so a submode rule consuming a trailing global command is usually what the real device does -- fidelity, not a bug; and Batfish parses canonically-ordered show-run output, so an ordering that cannot be emitted is not a realistic input. Then: grep each new getter for callers outside the extractor, because a write-only field that no conversion or question reads cannot carry a correctness finding. Only after those two gates does a newly-surfaced parse warning get ranked, and it ranks low -- error recovery is per-line and the snapshot still converts. references/probe-harness.md carries the mechanics, including the trap that a javac failure prints no marker lines and so reads as empty output rather than as a compile error. Also gitignore Claude Code's local settings and state directories so project config (skills) can be shared while permission allowlists and transcripts stay local. ---- Prompt: ``` Use this to start a repo local code review skill. It should be able to produce both your and my insights on this PR, it should include reading or pulling the important parts of the docs/ about idiomatic grammar, parsing, extraction, converstion, reference tracking. It should include using local or fetching CLI reference manuals as appropriate, etc. ``` Written after reviewing #10133, where two of my three initial findings did not survive review: one described device-faithful submode behavior, and one corrupted a field nothing reads. Both gates in the rubric come from those corrections. A claim that probe runs need --nocache_test_results was also removed as false -- Bazel invalidates on the edited test source and testconfig, and a cached result still replays its log under --test_output=all; both verified before committing.
Narrow the dns server-group subcommand values from the generic dec rule to sized numeric rules so the parser rejects structurally-impossible values: timeout and retries use uint8, poll-timer and expire-entry-timer use uint16 (added a uint16 rule to Asa_common). The extractor keeps its range checks for the exact valid sub-ranges. Update asa_dns_bad_ranges to use 0 for expire-entry-timer, which parses as uint16 but is still out of range, so the extractor's range-drop path stays exercised. Track DNS server groups as a referenceable structure. Add the DNS_SERVER_GROUP structure type and DNS_GROUP / DNS_GROUP_MAP_DNS_TO_DOMAIN usages; define the structure at "dns server-group" and record references at "dns-group" and "dns-to-domain"; mark it concrete during conversion. This populates defined-structure, undefined-reference, and unused-structure analysis for DNS groups. Extend testDnsReferences to assert the group definitions and referrer counts. All tests pass locally.
Yes. I added a uint16 rule to
Good idea. Since a |
dhalperi
left a comment
dhalperi
left a comment
There was a problem hiding this comment.
@dhalperi reviewed 8 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Murt2005).
3 participants
Summary
Add ASA-native extraction for the Cisco ASA
dns domain-lookup,dns server-group(and itsname-server,domain-name,timeout,retries,poll-timer,expire-entry-timersub-commands),dns name-server,dns-group, anddns-group-map/dns-to-domaincommands to capture DNS-resolver state: which interfaces have name lookup enabled, the configured DNS server groups with their servers and tuning parameters, the default server group, and domain-to-group mappings. This is captured in the vendor-specificcisco_asamodel; interface references are recorded so DNS domain-lookup and name-server source interfaces are not reported as unused. It also removes the inherited IOS-style DNS grammar/extraction that never applied to real ASA configurations and was never tested.Changes
Add lexer tokens
DNS_GROUP,DNS_GROUP_MAP,DNS_TO_DOMAIN,EXPIRE_ENTRY_TIMER,MINUTES, andPOLL_TIMER, and removeDNSfrom thenull_singleignored alternation so top-leveldns ...dispatches to a real stanza.Add the
s_dnsgrammar rule dispatching todns_domain_lookup(dns domain-lookup <interface>),dns_name_server(dns name-server <ip>...), and adns_server_groupsubmode (name-server <ip>... [interface],domain-name,timeout,retries,poll-timer minutes,expire-entry-timer minutes), plus a closeddns_nullfallback for unmodeleddnsglobals. Adds_dns_group(dns-group <name>) and thes_dns_group_mapsubmode withdns_to_domain(dns-to-domain <group> <domain>). Remove the IOS-onlys_domain,s_ip_domain,s_ip_domain_name, ands_ip_name_serverrules and their stanza dispatch entries; keep the real ASA globaldomain-name(s_domain_name).Add two new VS model classes in
representation/cisco_asa:DnsServerGroup.java(name, orderedNameServerlist, and the group'sdomainName,timeoutSeconds,retries,pollTimerMinutes,expireEntryTimerMinutestuning parameters) andNameServer.java(server IP plus optional per-linesourceInterface), both withequals/hashCode/toString.Add DNS state to
AsaConfiguration.java: a per-interfacednsDomainLookupInterfacesset (DNS lookup is a per-interface setting, not a single global flag), an insertion-ordereddnsServerGroupsmap with agetOrCreateDnsServerGroupaccessor, adnsGroupMap(domain -> group), and a nullabledefaultDnsServerGroup. The vendor-independentConfiguration.dnsServersset is now derived from every group's name servers. Remove the now-unuseddnsServersanddnsSourceInterfacefields (the latter had no ASA equivalent).Add
DNS_DOMAIN_LOOKUP_INTERFACEandDNS_SERVER_GROUP_NAME_SERVER_INTERFACEstructure usages and remove the IOS-onlyDOMAIN_LOOKUP_SOURCE_INTERFACEandIP_DOMAIN_LOOKUP_INTERFACE. Populate the new handlers inAsaControlPlaneExtractor.java:exitDns_domain_lookup,exitDns_name_server(into the defaultDefaultDNSgroup),enter/exitDns_server_groupwith thednsg_*sub-command methods,exitS_dns_group, andexitDns_to_domain. Both the domain-lookup interface and each name-server source interface record anINTERFACEstructure reference (matching the existing source-interface patterns), and nameif logical names are referenced by raw text since nameif registers the interface as a structure. Server-group tuning values are validated against their documented ranges (timeout1-30,retries0-10,poll-timer/expire-entry-timer1-65535) viatoIntegerInSpace, so out-of-range values are warned and dropped rather than stored.Testing
Add two
cisco_asatestconfigs:asa_dns(per-interfacedns domain-lookupon three nameif interfaces, a fullDefaultDNSgroup with a multi-IPname-serverline carrying a trailing interface plus a secondname-serverline,domain-name/timeout/retries/poll-timer/expire-entry-timer, a secondInsideDNSgroup, a globaldns name-server,dns-group-map/dns-to-domainmappings,dns-group, and a globaldomain-name) andasa_dns_bad_ranges(out-of-range tuning values).Add unit tests confirming extraction works correctly: the per-interface domain-lookup set, server-group contents (ordered name servers with per-line source interface and all tuning parameters across both groups), the DefaultDNS unification of
dns name-server, the domain-to-group map and default group, rejection of out-of-range tuning values, the vendor-independent DNS server set and domain name after conversion, and a reference test asserting each DNS-referenced interface carries the expected referrer count.All tests pass locally, formatting (
google-java-format) passes, and the fullcisco_asagrammar suite runs clean.References: