test(config,mgmt,k8s-intf): generate valid configurations and property-test the validator

.github/workflows/README.md

@@ -63,6 +63,11 @@ Production artifacts are produced via nix builds in a separate CI workflow.
 - `ci:+miri` - Run Miri checks
 - `ci:+wasm` - Run the WASM build check
 - `ci:+concurrency` - Run Shuttle and Loom tests
+- `ci:+debug-images` - Also build and push the core viewer, DAP debugger, and
+  syscall tracer images. They are built on main, in the merge queue, and on
+  dispatch regardless, and `ci:+merge-ready` turns them on too, since
+  `ci-gate` treats that label as enabling every gate; this is for when the
+  build itself needs debugging
 - `ci:+cross` - Build all cross-platform containers
 - `ci:+cross/full` - Also run the workspace test suite under qemu-user, on the
   two aarch64 musl legs. Gated like every other job, so the merge queue and
@@ -104,13 +109,92 @@ If those queue failures stop being rare, the phasing is worth revisiting.
 - Coverage: `debug` by default; `fuzz` on deep runs
 - Miri: required on deep runs; opt-in on pull requests with `ci:+miri`
 - Containers: debug/release for dataplane and FRR; release for validator
+- Debug images (core viewer, DAP debugger, syscall tracer): deep runs only,
+  or on a pull request with `ci:+debug-images`
 - VLAB configurations: spine-leaf fabric mode, L2VNI/L3VNI VPC modes,
   with gateway enabled
 
 ### Artifacts
 
 - Container images pushed to GitHub Container Registry (GHCR)
 - Release containers published on tag pushes via `just push`
+- `ghcr.io/githedgehog/dataplane/core-viewer` opens a core file from the lab.
+  It carries gdb plus the unstripped binaries and sources for the matching
+  `ghcr.io/githedgehog/dataplane` build.
+  Pull the tag matching the build the core came from; symbols only line up with
+  the exact version and profile that produced it.
+  The entrypoint takes the core as its only argument:
+
+  ```console
+  docker run --rm -it -v /path/to/cores:/cores \
+    ghcr.io/githedgehog/dataplane/core-viewer:TAG /cores/core.1234
+  ```
+
+- `ghcr.io/githedgehog/dataplane/dev-debugger` debugs a live dataplane from an
+  editor. It carries bugstalker, which understands Rust's std collections and
+  enum layouts, and listens for a Debug Adapter Protocol client on port 4711.
+  Publish the port and point the editor's DAP client at it:
+
+  ```console
+  docker run --rm -p 127.0.0.1:4711:4711 ghcr.io/githedgehog/dataplane/dev-debugger:TAG
+  ```
+
+  Connecting does not by itself start anything. In remote-DAP mode bugstalker
+  waits for the client's `launch` request to name the program, so the editor
+  has to send `program`, and any dataplane arguments as `args`. A request
+  without `program` is rejected with `launch: missing arguments.program`.
+  For VS Code, in `.vscode/launch.json`. `type` has to match whatever debug
+  type the BugStalker extension you installed registers -- it is not a name we
+  choose, and it differs between extensions, so check the one you have rather
+  than copying this field blind:
+
+  ```json
+  {
+    "type": "bs",
+    "request": "launch",
+    "name": "dataplane (container)",
+    "debugServer": 4711,
+    "program": "/bin/dataplane",
+    "args": []
+  }
+  ```
+
+  For `nvim-dap`, where the first line names the adapter itself, so `type = "bs"`
+  below is our own label rather than an extension's:
+
+  ```lua
+  dap.adapters.bs = { type = "server", host = "127.0.0.1", port = 4711 }
+  dap.configurations.rust = {
+    {
+      type = "bs",
+      request = "launch",
+      name = "dataplane (container)",
+      program = "/bin/dataplane",
+      args = {},
+    },
+  }
+  ```
+
+- `ghcr.io/githedgehog/dataplane/syscall-tracer` records what the dataplane
+  asks the kernel for, as JSON, using lurk.
+  It carries the same stripped binaries the release image ships, since nothing
+  here symbolizes, so it is smaller than the other two -- though not by as much
+  as that suggests: like them it ships the source tree, which the entrypoint
+  makes the working directory. Only the debug symbols and the debuggers
+  themselves are absent.
+
+  ```console
+  docker run --rm ghcr.io/githedgehog/dataplane/syscall-tracer:TAG > trace.jsonl
+  ```
+
+  The stream is one JSON object per line, except that tracing child threads
+  makes lurk announce each one with a bare `Attaching to child <pid>` line.
+  Filter those out if the consumer needs strict JSONL:
+
+  ```console
+  jq -R 'fromjson? // empty' < trace.jsonl
+  ```
+
 - Coverage reports from each `coverage/<profile>` job, kept for 7 days:
   - `coverage-html-<profile>.tar.gz` - `llvm-cov` HTML report, including the
     per-branch counts that Codecov does not render. Unpack and open
@@ -121,6 +205,53 @@ If those queue failures stop being rare, the phasing is worth revisiting.
   Both upload unarchived, so they download as the named file rather than
   wrapped in a zip.
 
+### Debugging locally
+
+The published images debug what CI built. To debug what you are building, `just
+debug` builds the matching image and runs a workspace binary or a single test
+inside it, at whatever `profile`, `platform`, `instrument`, and `sanitize` you
+pass. Symbols only line up when those match the build the problem appeared in,
+which is the whole reason to go through the image rather than a system gdb.
+
+```console
+just debug                             # pick from a list
+just debug bugstalker                  # pick, then wait for an editor
+just debug lurk dataplane              # trace syscalls, streams JSON, runs to exit
+just debug gdb dataplane               # gdbserver on 2345, waits for a client
+just profile=fuzz debug gdb test_parse_interface args
+just debug-list                        # print the same list without running anything
+```
+
+Name nothing and everything is offered through `skim`, which the dev shell
+provides. Name a filter matching one test and it runs without asking; name one
+matching several and those are offered. A filter matching nothing is an error
+rather than a guess, and so is an ambiguous one when there is no terminal to
+ask at, which is what makes this safe to call from a script.
+
+The third argument narrows which archive is searched, so
+`just debug gdb some_test args` builds only `args`' tests. It is worth passing:
+the default builds every test in the workspace, which is a long wait if all you
+wanted was to pick from a short list.
+
+`gdb` and `bugstalker` block until you disconnect and interrupt them; that is
+the point. `gdb` prints the `target remote` line to use. `bugstalker` prints a
+`.zed/debug.json` entry ready to paste, because in remote-DAP mode it takes the
+program from the client's launch request rather than from its own command line,
+so connecting an editor is only half of it. The `tcp_connection` field in that
+entry is what stops the editor spawning a second debugger of its own.
+
+A test runs with its package directory as the working directory, the way
+nextest runs it, so relative paths behave the same as under `just test`.
+
+To open a core file:
+
+```console
+just inspect-core /path/to/core.1234
+```
+
+Pass the same build settings that produced the binary that dumped
+(`just profile=release inspect-core ...`), for the same reason.
+
 ---
 
 ## Linting and Validation Workflows for Pull Requests

.github/workflows/dev.yml

@@ -68,6 +68,7 @@ jobs:
     outputs:
       container_profiles: "${{ steps.container-profiles.outputs.value }}"
       parallel: "${{ steps.parallel.outputs.value }}"
+      container_targets: "${{ steps.container-targets.outputs.value }}"
       profiles: "${{ steps.profiles.outputs.value }}"
       concurrency: "${{ steps.concurrency.outputs.value }}"
       cross: "${{ steps.cross.outputs.value }}"
@@ -132,6 +133,16 @@ jobs:
           on-value: '["debug", "release", "fuzz"]'
           off-value: '["debug"]'
 
+      # Debug images are opt-in on pull requests because of their size.
+      - id: "container-targets"
+        uses: *gate
+        with:
+          labels: "debug-images"
+          # Keep this on one line: `ci-gate` writes the value to GITHUB_OUTPUT
+          # with a plain printf, which a multi-line value would corrupt.
+          on-value: '["frr.dataplane", "dataplane", "dataplane-core-viewer", "dataplane-dev-debugger", "dataplane-syscall-tracer", "validator"]'
+          off-value: '["frr.dataplane", "dataplane", "validator"]'
+
       # Lab jobs require release images but not other release/fuzz checks.
       - id: "container-profiles"
         uses: *gate
@@ -425,10 +436,7 @@ jobs:
       fail-fast: false
       max-parallel: ${{ fromJSON(needs.plan.outputs.parallel) }}
       matrix:
-        nix-target:
-          - frr.dataplane
-          - dataplane
-          - validator
+        nix-target: "${{ fromJSON(needs.plan.outputs.container_targets) }}"
         # TODO: enable cfi and safe-stack on release when possible
         profile: "${{ fromJSON(needs.plan.outputs.container_profiles) }}"
         exclude:
@@ -443,6 +451,17 @@ jobs:
         with:
           recipe: "ci::push-container"
           recipe_args: "${{ matrix.nix-target }} ${{ matrix.profile }} ${{ needs.version.outputs.version }}"
+
+      # A debug image that builds is not a debug image that works.  Two of
+      # these shipped with entrypoints that could not do what the README
+      # documents, and building them said nothing about it.
+      - name: "smoke"
+        if: "${{ startsWith(matrix.nix-target, 'dataplane-') }}"
+        uses: *just
+        with:
+          recipe: "ci::smoke-container"
+          recipe_args: "${{ matrix.nix-target }} ${{ matrix.profile }}"
+
       - *verify-clean-tree
       - *tmate
 

CLAUDE.md

@@ -4,3 +4,21 @@ Follow the [development guide](development/README.md).
 
 If you make a design decision or do a code review, try to cite the section of the
 development guide you are following.
+
+## Commit messages
+
+A commit message signposts the diff; it does not restate it. Rationale belongs where it
+constrains the code -- in the code, its doc comments, or the development guide -- because a
+second copy in the message is a second thing to keep correct, and the two will drift.
+
+A message carries what the diff cannot:
+
+- the "why" of the commit,
+- non-obvious process facts,
+
+It does not re-summarize the change, narrate how it was made, or repeat an argument the
+commit already puts in the tree. For a docs commit, the file is the content; the message
+points at it.
+
+Keep `git log` scannable: a few lines by default, longer only where the diff is opaque and
+the message is what makes the commit reviewable.

ci.just

@@ -78,6 +78,10 @@ cross platform libc +args:
 cross-test platform libc:
     NEXTEST_PROFILE=cross-qemu just {{ _lab }} platform={{ platform }} libc={{ libc }} profile=debug test
 
+# Verify a debug image's entrypoint actually does its job.
+smoke-container target profile:
+    just {{ _lab }} profile={{ profile }} platform=x86-64-v3 smoke-container {{ target }}
+
 # Publish both content-derived and discoverable per-commit tags.
 [script]
 push-container target profile version:

config/Cargo.toml

@@ -5,6 +5,10 @@ license.workspace = true
 publish.workspace = true
 version.workspace = true
 
+[features]
+# Generators for the configuration types, for property tests in this crate and downstream.
+bolero = ["dep:bolero", "lpm/bolero"]
+
 [dependencies]
 # internal
 common = { workspace = true }
@@ -16,6 +20,7 @@ net = { workspace = true }
 
 # external
 arc-swap = { workspace = true }
+bolero = { workspace = true, optional = true, default-features = false, features = ["alloc"] }
 chrono = { workspace = true, features = ["alloc", "std"] }
 derive_builder = { workspace = true, features = [] }
 ipnet = { workspace = true }

config/src/converters/k8s/config/expose.rs

@@ -491,7 +491,8 @@ mod test {
                 "10.0.4.0/24".parse::<Prefix>().unwrap(),
             ),
         ]);
-        let expose_gen = k8s_intf::bolero::expose::LegalValueExposeGenerator::new(&subnets);
+        // One expose at a time here, so any slot will do.
+        let expose_gen = k8s_intf::bolero::expose::AnyExposeGenerator::new(0, &subnets);
         bolero::check!()
             .with_generator(expose_gen)
             .for_each(|k8s_expose| {
@@ -549,6 +550,7 @@ mod test {
                     })
                     .unwrap_or(vec![]);
                 k8s_nots.sort();
+                k8s_nots.dedup();
                 let k8s_subnets = k8s_expose
                     .ips
                     .as_ref()
@@ -565,7 +567,13 @@ mod test {
                     })
                     .unwrap_or(vec![]);
                 k8s_ips.extend(k8s_subnets);
+                // Sorted *and* deduplicated, because the conversion collects into a set-like
+                // structure: a prefix written twice in one expose means the same as writing it once,
+                // and comes out once. This only came up when the generators started producing
+                // repeats -- the previous ones drew each prefix from a uniqueness-preserving
+                // generator, so the question never arose.
                 k8s_ips.sort();
+                k8s_ips.dedup();
 
                 let k8s_as = k8s_expose.r#as.as_ref().map(|r#as| {
                     let mut ret = r#as
@@ -574,6 +582,7 @@ mod test {
                         .map(|r#as| r#as.cidr.as_ref().unwrap().clone())
                         .collect::<Vec<_>>();
                     ret.sort();
+                    ret.dedup();
                     ret
                 });
 
@@ -584,6 +593,7 @@ mod test {
                         .map(|r#as| r#as.not.as_ref().unwrap().clone())
                         .collect::<Vec<_>>();
                     ret.sort();
+                    ret.dedup();
                     ret
                 });
 

config/src/converters/k8s/config/peering.rs

@@ -91,14 +91,20 @@ mod test {
     use k8s_intf::bolero::peering::{
         LegalValuePeeringsGenerator, LegalValuePeeringsPeeringGenerator,
     };
+    use k8s_intf::bolero::{AddressFamily, NatFlavour};
     use lpm::prefix::Prefix;
 
     use crate::converters::k8s::config::{SubnetMap, VpcSubnetMap};
 
     #[test]
     fn test_vpc_manifest_conversion() {
         let subnets = SubnetMap::new(); // Let this be empty since we are test subnet conversion elsewhere
-        let generator = LegalValuePeeringsPeeringGenerator::new(&subnets);
+        // any flavour, one family, a couple of exposes: this is testing the conversion, not the
+        // rules the generators satisfy
+        let flavours = NatFlavour::all();
+        let generator =
+            // One manifest at a time here, so vpc zero: there is nothing to keep it disjoint from.
+            LegalValuePeeringsPeeringGenerator::new(&subnets, &flavours, AddressFamily::V4, 3, 0);
         bolero::check!()
             .with_generator(generator)
             .for_each(|peering| {
@@ -169,7 +175,11 @@ mod test {
                 ]),
             ),
         ]);
-        let generator = LegalValuePeeringsGenerator::new(&subnets).unwrap();
+        let flavours = NatFlavour::all();
+        let families = AddressFamily::all();
+        let groups = vec!["gwgroup-0".to_string()];
+        let generator =
+            LegalValuePeeringsGenerator::new(&subnets, &flavours, &families, 3, &groups).unwrap();
         bolero::check!()
             .with_generator(generator)
             .for_each(|peering| {

config/src/errors.rs

@@ -75,8 +75,33 @@ pub enum ConfigError {
     #[error("Invalid ACL configuration: {0}")]
     InvalidAcl(String),
     // NAT-specific
-    #[error("Mismatched prefixes sizes for static NAT: {0:?} and {1:?}")]
+    /// The two sides of a static NAT expose cover different numbers of address-port pairs.
+    //
+    // The sizes are rendered with `Debug` because `PrefixWithPortsSize` is a 145-bit bnum type
+    // with no `Display`, and Debug pads it out to a run of digits that reads as gibberish. Hence
+    // leading with what to change and leaving the numbers to the end.
+    #[error(
+        "Mismatched sizes for static NAT: the exposed prefixes and the range they translate to \
+         must cover the same number of address-port pairs (they cover {0:?} and {1:?})"
+    )]
     MismatchedPrefixSizes(PrefixWithPortsSize, PrefixWithPortsSize),
+    /// The two sides of a port-forwarding expose have prefixes of different lengths.
+    ///
+    /// Distinct from [`ConfigError::MismatchedPrefixSizes`], which compares addresses times ports.
+    /// That product can match while the lengths do not -- a `/32` carrying 100 ports and a `/30`
+    /// carrying 25 both come to 100 -- so reporting it as a size mismatch would name two numbers
+    /// that are equal. A port-forwarding rule maps addresses one for one, so it is the lengths
+    /// that have to agree.
+    #[error(
+        "Mismatched prefix lengths for port forwarding: /{private} exposed and /{public} \
+         translated to; a rule maps addresses one for one, so the two must be the same length"
+    )]
+    MismatchedPrefixLengths { private: u8, public: u8 },
+    #[error(
+        "Mismatched port range sizes for port forwarding: {private} ports exposed and {public} \
+         translated to; a rule maps ports one for one, so the two must be the same size"
+    )]
+    MismatchedPortRangeSizes { private: usize, public: usize },
     #[error("Peering {0} has manifests using incompatible NAT modes")]
     IncompatibleNatModes(String),
     #[error("Vpc {0} has a peering with no exposes")]