Config generator coverage by daniel-noland · Pull Request #1663 · githedgehog/dataplane

daniel-noland · 2026-07-24T22:50:43Z

No description provided.

coderabbitai · 2026-07-24T22:50:50Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 524daec2-254b-46ca-9093-6f7737df6aa5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

Extends the k8s-intf bolero generators and config property tests to improve end-to-end coverage of validated GatewayAgent configs, including cross-field/cross-entity constraints (peerings↔groups, expose overlap rules, serialization boundaries, and negative “reject” paths).

Changes:

Add generators for RFC1123-compliant k8s names, port list/ranges, routable/non-reserved CIDRs, and a shared prefix pool to prevent expose overlap by construction.
Rework spec/peering generation to produce validation-legal configs (group references, default-expose constraints, unique VPC peering pairs) and add peering ACL generation.
Add hostile “mutated” generators plus new property tests for YAML/JSON round-trips and validation/rejection behavior.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
k8s-intf/src/bolero/support.rs	Adds `K8sName`, port generators, routable CIDR generators, and `PrefixPool`, plus targeted property tests.
k8s-intf/src/bolero/spec.rs	Updates spec generation to be validation-legal (groups first, shared prefix pool, default-expose constraints, communities sizing, agent_version/config generation).
k8s-intf/src/bolero/peering.rs	Adds gwgroup-aware peering generation, optional default expose support, pool-backed expose generation, and optional ACL generation.
k8s-intf/src/bolero/mutate.rs	Introduces hostile “legal-then-corrupt-one-thing” GatewayAgent generator for rejection-path coverage.
k8s-intf/src/bolero/mod.rs	Exposes new `acl` and `mutate` generator modules.
k8s-intf/src/bolero/gwgroups.rs	Improves gateway group generation (member uniqueness, bounded collisions) and provides a groups table generator with names for peerings.
k8s-intf/src/bolero/gateway.rs	Generates additional gateway fields (`flow_table_capacity`, profiling) to improve coverage.
k8s-intf/src/bolero/expose.rs	Adds default-expose constructor and pool-backed expose generation; switches to routable prefixes for validation-legal shapes.
k8s-intf/src/bolero/crd.rs	Adds property tests for YAML/JSON round-trips, generator non-vacuity, and encoding agreement.
k8s-intf/src/bolero/acl.rs	Adds peering-relative ACL generator with optional “independent_of_exposes” mode to avoid cross-coupling.
config/src/converters/k8s/config/peering.rs	Updates peering converter property test to pass gwgroup names and validate ACL/group invariants post-conversion.
config/src/converters/k8s/config/gateway_config.rs	Adds property tests that validate legal configs, verify fan-out behavior, ensure mutated configs are rejected, and check flow table capacity conversion.

    fn generate<D: Driver>(driver: &mut D) -> Option<Self> {
        let gmember = GatewayAgentGroupsMembers {
-            name: driver.produce::<String>()?,
+            name: driver.produce::<K8sName>()?.take(),
            priority: driver.gen_u32(Bound::Included(&0), Bound::Included(&10))?,
            vtep_ip: driver.produce::<UnicastIpv4Addr>()?.to_string(),
        };


+    fn generate<D: Driver>(&self, d: &mut D) -> Option<Self::Output> {
+        let mut groups = std::collections::BTreeMap::new();
+        let name_gen = K8sNameGenerator::new(1, 12);
+        let mut names = Vec::with_capacity(self.count);
+        for i in 0..self.count {
+            // Suffix with the index so group names are distinct without a retry loop;
+            // `GwGroupTable::add_group` rejects duplicates.
+            let name = format!("{}-{i}", name_gen.generate(d)?);
+            groups.insert(
+                name.clone(),
+                d.produce::<LegalValue<GatewayAgentGroups>>()?.take(),
+            );
+            names.push(name);
+        }
+        Some((groups, names))
    }


+        // A group of N members occupies ranks `0..N`, and validation requires a community at
+        // every rank it finds.  Sizing the table to the largest group a generated spec can hold
+        // keeps that satisfied without coupling to how many members were actually drawn.


Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

k8s-intf/src/bolero/gwgroups.rs:73

confidence: 8
tags: [logic]

This duplicate-name repair loop can break the RFC 1123 / 63-char constraint that motivated using `K8sName` in the first place: if `member.name` is already 63 chars and collides, `push('x')` makes it 64+ chars.

If this field is intended to stay RFC1123-valid, prefer making names unique with a bounded suffix while keeping `len <= 63` (truncate + append `-<n>`), rather than unbounded growth.

        while !names.insert(member.name.clone()) {
            member.name.push('x');
        }

**k8s-intf/src/bolero/gwgroups.rs:133**
* ```yaml
confidence: 9
tags: [docs]

The doc comment for LegalValueGroupsTableGenerator says it returns a sorted list of group names, but names is currently returned in generation order (which is not guaranteed to be sorted because the random prefix differs per entry).

Either sort names before returning, or update the docstring to avoid claiming sortedness.

        Some((groups, names))

+        let (cidr, vpc_subnet) = match shape {
+            0 => (Some(prefix), None),
+            1 if !subnet_names.is_empty() => (None, Some(choose(d, &subnet_names)?.clone())),
+            // Falls through to "any address" when the VPC has no subnets to reference.
+            _ => (None, None),
+        };


Four generator primitives the config generators need, none of which existed: `K8sName` produces an RFC 1123 DNS label. `bolero`'s `String` generator produces arbitrary Unicode, which is not a legal k8s object name, so every CRD field naming an object was violating the "never produce an illegal value" half of the `TypeGenerator` contract in `development/code/property-testing.md`. `generate_port_range`/`generate_port_list` produce the textual port forms the converters parse, covering both the single-port and range spellings. `RoutableV4CidrGenerator`/`RoutableV6CidrGenerator` produce prefixes that do not overlap a reserved block. `VpcExpose` validation rejects a prefix overlapping one -- overlap, not containment, so a `/0` is illegal -- which the existing `UniqueV4CidrGenerator` happily produces. Reserved blocks are skipped whole rather than stepped through, since `240.0.0.0/4` alone spans 2^28 `/32`s. `PrefixPool` hands out mutually non-overlapping prefixes from one shared allocator, for the manifest-level non-overlap rule that cannot be satisfied by generating each expose independently. It uses interior mutability so it can be shared by `&` through a generator tree, since `ValueGenerator::generate` takes `&self`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`TypeGenerator for GatewayAgentGroups` drew a member count in `0..=10` and then pushed exactly one member, so no generated group ever had more than one. That left `GwGroup::add_member`'s duplicate-name and duplicate-address checks unexercised, and the rank-to-community lookup in `validate_gw_groups` only ever reached rank 0. It also spent a driver draw on a value it discarded, which degrades shrinking. Generate the requested number of members, repairing name and address collisions in place (lengthen, walk) rather than redrawing, so every value the driver produced still contributes. Also move these impls under `LegalValue` and draw names from `K8sName`. They were bare `TypeGenerator` impls on the CRD types while restricting priority to `0..=10` -- narrower than "all legal values" -- and simultaneously drawing names from `produce::<String>()`, which is wider: arbitrary Unicode is not a legal k8s name. They were on the wrong side of both rules at once. `LegalValueGroupsTableGenerator` builds the whole `spec.groups` map and returns the group names alongside it, which a later commit needs so that peerings can reference a group that exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The peerings generator set `acl: None` with a FIXME, so the ACL converter -- the largest single piece of the intake path -- was covered only by hand-written examples. An ACL rule is only legal relative to its peering, which is why this is a `ValueGenerator` carrying the two VPC names and the subnet map as borrowed context rather than a `TypeGenerator` on the CRD type: - `from`/`to` must name the peering's two VPCs, or be blank to be inferred, so the generator enumerates the legal shapes and knows which VPC each resolves to; both spellings of blank (absent and empty string) are covered; - a `vpcSubnet` reference in `match.src` resolves against the from-side VPC's subnets and one in `match.dst` against the to-side VPC's, so each side must draw from the map its own resolved VPC. Ports are only emitted for TCP and UDP, and one address family is chosen per rule, because `AclPattern::validate` rejects ports on other protocols and rejects a rule whose `src` and `dst` disagree on IP version. The side names come from the generated map rather than from the chosen pair, since `VpcPeering::try_from` derives `left`/`right` by iterating it and so orders them lexicographically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Four fields were pinned to `None`, two of them with FIXMEs, leaving converter code unreachable from any generated input: - `flowTableCapacity`, whose converter has a fallible `u32` -> `NonZero<usize>` narrowing. Zero is illegal, so it is clamped away here and left to the hostile generators; - `profiling` and `agentVersion`, which were simply never populated; - `config.fabricBFD`, which the converter fans out over every underlay BGP neighbor after the underlay has already been built. That post-hoc mutation is invisible to a per-subtree converter test, so it needs to come from the top. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`LegalValue<GatewayAgentSpec>` produced specs that converted but could not validate, so no generated input had ever reached `ExternalConfig::validate`. The blocker was `gatewayGroup`, drawn from `produce::<String>()` while the group table used generated names, so `check_peering_gwgroups_exist` rejected almost every spec. "Legal" meant "convertible", which is strictly weaker than what the system means by legal. Fixing that exposed six more rules that are not local to any one subtree, so each is now upheld by whichever generator owns the relevant scope: - a peering's `gatewayGroup` must name a group in the table, so the group table is generated first and its names handed down; - `Vpc::check_peering_count` rejects a VPC pair peering twice, so pair selection moves up to the spec generator, which draws pairs without replacement; - `VpcManifest::validate` allows at most one default expose per manifest and `Peering::validate` forbids one on both sides of a peering, so default placement is decided by the caller that owns both sides; - `VpcRouteTable::validate` allows a VPC only one default destination across all its peerings, and a default expose on one side is a default destination for the other, so the spec generator tracks which VPCs have been offered one; - that same rule requires overlapping destinations to share a gateway group, and a default expose reaches the route table as `0.0.0.0/0`, which overlaps every v4 destination. So a spec can have default exposes or a diversity of gateway groups, but not both; each shape is generated half the time; - `VpcManifest::validate` rejects overlapping exposes, so addresses come from one `PrefixPool` shared by the whole spec, making non-overlap hold by construction. Two restrictions are deliberate and documented at the call sites, since lifting them needs context the generators do not have yet: ACL address matches and `scope: flow` (coupled to the exposes via `manifest_coverage_set`), and NAT-mode exposes and `vpcSubnet` references (coupled to prefix sizing and to the VPC subnet generator). Both remain covered at the conversion level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The top-level conversion test stopped at `try_from`, so every rule in `ExternalConfig::validate` -- and everything it delegates to, notably the overlay and its peerings -- went unfuzzed. Now that generated specs are legal by the system's own definition, take the property all the way to `ValidatedGwConfig`. This is the test that keeps `LegalValue` honest: a cross-reference bug in a generator now fails here instead of hiding as silent under-coverage. Two narrower properties for the fields the previous commits unpinned: `fabricBFD` must reach every underlay BGP neighbor or none of them, and `flowTableCapacity` must survive the `u32` -> `NonZero<usize>` narrowing intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Nothing fed YAML or JSON text into `GatewayAgentSpec`, yet that parse is the real trust boundary in both intake modes: k8s-less reads YAML from a watched file via `load_crd_from_file`, and the k8s watcher parses JSON off the API server. Both land in the same kopium-generated types, whose integer widths `build.rs` rewrites textually (`i64` -> `u64`, `i32` -> `u32`, with specific fields patched back). Nothing checked that those rewrites round-trip. Round-trip each encoding, and additionally require the two to agree: a field that survives one but not the other would make the k8s and k8s-less modes disagree about the same configuration. Also assert the spec generator is not vacuous. A property test passes just as easily against a generator that has quietly degenerated -- as `GatewayAgentGroups` had, drawing a member count in `0..=10` and emitting exactly one -- and nothing detects that from the outside. So require each interesting shape (multi-member groups, ACLs, default exposes, both `fabricBFD` settings, and the rest) to actually show up in the sample. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Every generator so far produces values the system should accept, which leaves the rejection paths -- the roughly thirty `FromK8sConversionError` and `ConfigError` variants a malformed CRD can provoke -- covered only by hand-written cases. Those paths matter: config arrives from outside the dataplane, so a malformed `GatewayAgent` must be refused cleanly rather than panicking or half-applying. `Mutated` takes a legal `GatewayAgent` and applies exactly one of 23 named corruptions: dangling references, duplicate VNIs and internal ids, malformed CIDRs and protocols, inverted port ranges, a three-sided peering, a zero generation, the wrong namespace, and so on. The mutation is carried in the generated value rather than applied by a loop in the test body, so `bolero` can shrink to the smallest config that still exhibits a problem and the failure names the mutation responsible. The property is deliberately coarse: rejected, never panicking. Asserting a specific error variant per mutation would pin down the current phrasing of every guard and break on harmless refactors, while asserting only "does not panic" would pass even if a dangling reference were silently accepted. A mutation that finds nothing to corrupt reports itself as a no-op and is required to still validate, rather than being resampled -- resampling would bias the distribution and complicate shrinking. One such case is load-bearing: `Vpc::try_from` ignores `subnets` entirely, and the CIDRs are parsed only by the overlay converter's `extract_subnets`, which runs only when the spec has both VPCs and peerings. So a malformed subnet CIDR on an unpeered VPC is accepted as-is today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

k8s-intf/src/bolero/gwgroups.rs:73

confidence: 8
tags: [logic]

The duplicate-name repair can push `member.name` beyond the 63-character RFC1123 DNS-label limit. Since member names are initially generated via `K8sName`, this can silently reintroduce illegal k8s-style names (and makes the generator’s guarantees inconsistent under collisions). Consider generating a collision suffix while keeping the total length ≤ 63 instead of unconditionally `push('x')`.

        while !names.insert(member.name.clone()) {
            member.name.push('x');
        }

</details>

+/// Validation rejects a prefix that *overlaps* a reserved block, not merely one contained in it,
+/// so every prefix shorter than this necessarily spans reserved space and cannot be legal.  Very
+/// short prefixes that happen to dodge every reserved block (`8.0.0.0/7`, say) are legal but not
+/// generated; expressing "everything" is what a `default` expose is for.


Copilot AI review requested due to automatic review settings July 24, 2026 22:50

Copilot started reviewing on behalf of daniel-noland July 24, 2026 22:51 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

daniel-noland force-pushed the config-generator-coverage branch from 8fdfc99 to 9da39cd Compare July 24, 2026 23:17

Copilot AI review requested due to automatic review settings July 24, 2026 23:17

Copilot started reviewing on behalf of daniel-noland July 24, 2026 23:17 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

daniel-noland and others added 8 commits July 24, 2026 18:36

Copilot AI review requested due to automatic review settings July 25, 2026 00:38

daniel-noland force-pushed the config-generator-coverage branch from 9da39cd to d5b2370 Compare July 25, 2026 00:39

Copilot started reviewing on behalf of daniel-noland July 25, 2026 00:39 View session

daniel-noland added ci:+vlab Enable VLAB tests ci:+cross/full labels Jul 25, 2026

Copilot AI reviewed Jul 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Config generator coverage#1663

Config generator coverage#1663
daniel-noland wants to merge 8 commits into
mainfrom
config-generator-coverage

daniel-noland commented Jul 24, 2026

Uh oh!

coderabbitai Bot commented Jul 24, 2026 •

edited

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

daniel-noland commented Jul 24, 2026

Uh oh!

coderabbitai Bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jul 24, 2026 •

edited

Loading