Alex/kaido-core
SHA256
Watch
1
0
Fork
You've already forked kaido-core
 0
Code Issues 10 Pull requests 1 Releases Activity Actions

Remove JSON from config.db #83

Merged
Alex merged 117 commits from 77-config-db-no-json into main 2026-11-08 13:29:53 +01:00
Conversation 2 Commits 117 Files changed 332 +41828 -30072
Alex commented 2026-11-08 13:22:30 +01:00
Owner

Problem

  • config.db stored structured configuration as JSON in body columns, so diffs and merges could not see single fields.
  • factory_profile and the Kaido-Journal commit trailer also held JSON.

Change

  • Every structured value has its own column. KeaJSON stays as the only named JSON text exception.
  • Dormant recovery units are removed (recovery_config, /system/recovery, router.recovery).
  • Kaido-Journal uses flat key/value trailers; factory_profile moved to state.db.
  • Concurrent drafts merge set lists per entry.
  • Fixes found during validation: restart without a mgmt mapping, QoS capture ordering, nil-safe PUT handlers, section removal in merges, read-only DHCP check on warm start, removal of empty marked firewall tables.
  • Test tooling: go-test-shards.py plans shards by available workers, so slow packages use all processes.

API change

  • frr.bfd and frr.ospf are removed.

Validation

  • Builder VM on a56d425d: go test ./... 9173 pass, 0 fail, 189 skip; make check including -race, lint, UI and nocgo checks pass; check-config-json passes.
  • make e2e-toml acceptance: main, variants and api without deviation; JSON audit of four databases without findings.
  • After a56d425d: go vet and go test on internal/service, internal/state and internal/state/historytest; 145 isolated kernel tests with 140 pass, 5 skip, 0 fail.

Known limits

  • check-build exits 2 on two environment-dependent tests.
  • Four browser specs fail (dhcpDnsActivation, externalNotifications, keepChanges, startupTemplate); four need an agent and are skipped.
  • E2E scripts for vrf, firewall, reverse proxy, ipsec and frr stop on environment or script configuration.
  • Kea native tests and five kernel tests need environments that were not available.

Findings kept out of this change

  • None of these concern JSON in config.db; the owner decided to track them separately.
  • #84: warm or cold start classification depends on the clock.
  • #85: absent VPN or router advertisement section leaves the runtime in place.
  • #86: QoS ifb device remains after a restart during activation.
  • A complete import keeps entries from other parallel drafts without a notice. The owner decided to keep this behaviour.

Closes #77

🤖 Generated with Claude Code

## Problem - `config.db` stored structured configuration as JSON in `body` columns, so diffs and merges could not see single fields. - `factory_profile` and the `Kaido-Journal` commit trailer also held JSON. ## Change - Every structured value has its own column. `KeaJSON` stays as the only named JSON text exception. - Dormant recovery units are removed (`recovery_config`, `/system/recovery`, `router.recovery`). - `Kaido-Journal` uses flat key/value trailers; `factory_profile` moved to `state.db`. - Concurrent drafts merge set lists per entry. - Fixes found during validation: restart without a `mgmt` mapping, QoS capture ordering, nil-safe PUT handlers, section removal in merges, read-only DHCP check on warm start, removal of empty marked firewall tables. - Test tooling: `go-test-shards.py` plans shards by available workers, so slow packages use all processes. ## API change - `frr.bfd` and `frr.ospf` are removed. ## Validation - Builder VM on a56d425d: `go test ./...` 9173 pass, 0 fail, 189 skip; `make check` including `-race`, lint, UI and nocgo checks pass; `check-config-json` passes. - `make e2e-toml` acceptance: main, variants and api without deviation; JSON audit of four databases without findings. - After a56d425d: `go vet` and `go test` on `internal/service`, `internal/state` and `internal/state/historytest`; 145 isolated kernel tests with 140 pass, 5 skip, 0 fail. ## Known limits - `check-build` exits 2 on two environment-dependent tests. - Four browser specs fail (dhcpDnsActivation, externalNotifications, keepChanges, startupTemplate); four need an agent and are skipped. - E2E scripts for vrf, firewall, reverse proxy, ipsec and frr stop on environment or script configuration. - Kea native tests and five kernel tests need environments that were not available. ## Findings kept out of this change - None of these concern JSON in `config.db`; the owner decided to track them separately. - #84: warm or cold start classification depends on the clock. - #85: absent VPN or router advertisement section leaves the runtime in place. - #86: QoS ifb device remains after a restart during activation. - A complete import keeps entries from other parallel drafts without a notice. The owner decided to keep this behaviour. Closes #77 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Alex added 116 commits 2026-11-08 13:22:30 +01:00 
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Subsystem ETags now hash table content through dolt_hashof_table, so they
no longer depend on version and updated_at columns. Content revisions move
out of the firewall helpers for use by every module.

Add a schema check comparing all body columns with a shrinking list, and an
audit that scans all text cells of config.db for JSON documents. A build
tag runs the audit whenever a store closes, which covers a complete test
run. Schema version tests follow the embedded files.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Restoring a backup now assigns each interface to a real target or, on a
full restore, discards it together with its dependents. Discarded
configuration stays in the backup file and no longer lives on the router.

Remove RouterState.Recovery, the recovery_config table, the three
/system/recovery routes, the __unassigned__ restore target, router.recovery
in TOML and the UI for keeping and assigning units. Discard keeps its
behaviour and its tests. Router state is no longer recursive, so the MCP
schema needs no reference for it.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop the /system/recovery routes and the recovery state section from the
API description, and describe the restore interface mapping.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rules dropped from zones, zone policies and NAT because of an affected
ifname set, policies of a removed zone and rules of a removed gateway
group disappeared without an entry in the items shown for the discard
decision. Rules without a comment are named by their ID.

The older-schema restore test now replaces the migration rows, since
rewriting all of them to one version violates the unique key.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Subsystem ETags now read inside a primed read transaction like Capture.
dolt_hashof_table with one argument serializes the catalog the connection
already holds, so a pooled connection on main could return an old value after
another connection wrote.

Move the audit into internal/configjsonaudit, which product binaries do not
link. It now covers the working set of every branch with that branch's own
tables and every commit reachable from a branch or tag, also for database
files kept from end-to-end runs. A failed audit, an unwritable log and a log
without a completed audit no longer count as passed. make check-config-json
runs the Go suite with the audit and evaluates the log; make e2e-toml keeps
config.db and checks the copies with cmd/config-json-audit.

Prove content dependence on a relational table and correct the comments for
tables that still carry version and updated_at columns. The latest schema
version helper moves to test code.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dolt_hashof_db() reads the catalog a connection already holds, so a pooled
connection could report a stale hash after another connection wrote. Both
content hash functions now read a table first in the same read transaction.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the logging_config and mirroring_config JSON bodies with typed
tables, so edits merge by cell and history shows single fields.

Optional structs keep their cells behind a configured column, so clearing
one and reverting restores the values. Session names become row ids
because the model has no other identity.

Subsystem ETags now also hash the domain's config_sections row. A change
of presence alone left the ETag unchanged and let stale edits through.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The journal document holds null or [] for the session list and compares
them exactly, so a nil list that read back as empty made a revert fail
with a conflict, turned every repeated save into a change and added an
empty list to the TOML export. A sessions_set column keeps them apart;
sessions win over it, and GET keeps answering with an array.

Cover absent values in revert, redo and snapshot restore, two new
sessions in two drafts, and match draft conflicts by error instead of
by text. Comments on kept cells now name their real use, a single-cell
diff: reverts run through the journal.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Interfaces and their mappings move from JSON bodies to typed tables, with
one table per optional struct and ordered child tables for addresses and
members. The id of a row is the configured name, so the separate stored
identity and its copy between branches are gone. References stay plain
names without foreign keys because a target may be a native link or a
service-owned device.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The id of configured interfaces and logical mappings is now the name, so the
API description and the tests say so. Add manager tests for a removed parent
against a child change and for the same new interface in two drafts, and name
test files and tests after their content.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the JSON bodies of system_config and system_access_config with
typed tables, so drafts merge by field, by sysctl key and by account.
Lists stay ordered by sequence; flags keep an explicit false, a cleared
domain and empty lists apart from nil.

Clearing a password hash, an account, all accounts or the access domain
deletes the secret from the working state in the same write, while
earlier commits keep it. The access domain gets its own ETag key because
clearing a domain without accounts changes only its presence flag.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the JSON bodies of dns_config and dns_forwarders with typed
tables. Service promotion sets the enabled cell of dns_settings; the
DHCP side of promotion is unchanged.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The system_access ETag key made POST /config/apply hand out an ETag it never
checked, so two clients could overwrite each other without a 412. The domain
has no ETag again.

The replay test covers the fallback to the package content with two rules in
one gap. The bounded prune boundary case for the JSON store moves from
interfaces, which have no body any more, to gateways.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Server, relay, pools, static leases and DHCP options move to typed tables.
Promoting the Kea service sets one cell, so no json_set on config.db remains.
kea_json stays the text the user entered.

ReconcileAll reads scoped parts of one table together, so child lists cost
one SELECT per table instead of one per pool.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Journal documents take the request's forwarder order while the database
  reads them by domain, so reverting a commit from an unsorted request always
  conflicted on @order. Documents now hold the read order.
- Restore did not validate DNS, so an archive naming one zone twice stored two
  rows that the resolver refuses and every later change trips on.
- Cover duplicate zones on the manager and model level.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each RA interface and QoS policy now has its own row, with DNS lists,
qdiscs, CAKE options, classes and filters in child tables. Interfaces and
policies read in name order and the journal holds them in that order, so a
request's order records nothing and a revert sees what the database reads.
Classes and filters keep their order in a sequence column.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Pools, leases, interfaces and logical interfaces read sorted, so InReadOrder
  now brings them into that order in every journal document. A request in
  another order no longer makes the revert of its commit conflict.
- Cover the exchange of unique pool and lease values through the manager and
  the masking of kea_json in the export.
- Correct the comment on reads outside a reconcile scope.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Static routes, policy rules, gateways, monitors, tests, groups, members
and default gateways get their own tables, written cell by cell. Default
gateways leave the routing_dynamic document, so the gateways ETag follows
them. Route ids are assigned before the journal sees a route; sets read
in a fixed order and the journal holds them in that order.

The edit revision of a static route now derives from its content, which
ends the stored row versions: the versioned table list, the capture and
restore helpers and the session fields are gone. Only the row identities
used for draft ownership remain.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reverting a commit compared a missing path with an object without
values strictly, so such a revert ended in a conflict, and journal
history dropped every absent-to-empty object change, so a section that
keeps the empty object could not be restored at all.

- journal: Revert and MatchesBefore use Equivalent; a field of an absent
  object resolves to nil and is restored by materializing the parent. A
  value changed in between remains a conflict.
- history: an empty object is hidden only in auxiliary domains and in
  the first save, which creates all tables.
- drafts: an empty object the editing branch stores as nothing is not
  part of the package.
- InReadOrder keeps the last entry of a repeated QoS policy or RA
  interface, like the store does.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A route without an id found its stored row by destination and table. That
key is not unique: failover routes share both, so an unchanged request gave
one route the identity of another and staged a change on every save.

- A route without a usable id takes the stored row with the same values,
  the same key the kernel uses. An edited route takes the row of its
  destination and table only when no other route could claim it.
- stableIDs hands the stored ids of one natural key out in order.
- Cover restores without route ids and a later revert, stale revisions on
  the command bus, two drafts that remove different routes, and DHCP parts
  without values through revert, redo and snapshot restore.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the shared helpers and table registers of the dynamic routing mapper.
BFD and OSPF move to their own tables, including their child lists; the
routing_dynamic document keeps only FRR and BGP until they have rows.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A commit revert wrote a removed field back below an object that a newer
draft had removed. The result was an incomplete object, and the removal
was silently undone. Repeated DHCP pools and leases also stayed in the
journal document although the database keeps only the last one, so a
draft differed from its own branch for good.

- journal: RevertCommit takes the document right after the commit and
  creates an absent object only if the commit left it without values;
  everything else conflicts. Revert never creates one.
- state: commit reverts pass that document.
- InReadOrder keeps the last pool of an interface and the last lease of
  a MAC, like the store does.
- tests: conflicts and restores below absent objects, a revert whose
  goal the draft already reached is a no-op without a package, the
  staged no-op for an unstored section, repeated DHCP entries, and the
  first-save and auxiliary branches of storageArtifact.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move the BGP settings, confederation peers and neighbors from the
remaining routing_dynamic document into their own tables. RPKI, advanced
settings and address families stay in the document for now. Neighbor
passwords live only in their column, and clearing BGP removes them.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The remaining document of dynamic routing now holds only FRR. Clearing RPKI
or BGP removes the cache servers with their key paths, and removing an address
family takes its lists with it through the foreign key.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
routing.frr keeps its settings, accept filters, prefix lists, access lists,
route maps, AS path lists and community lists in their own tables. Only the
BFD, OSPF and static fields stay in the remaining document. Clearing the
configuration empties config and raw_config in the same write.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
routing.frr.bfd, .ospf and .static get tables of their own, and with them
nothing of dynamic routing stays in a JSON document. saveRoutingDynamicRows
is now the only save path, so a save without change leaves the routing ETag
alone.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cover draft merges and conflicts, activation, rollback and restart,
reverts and snapshot restores with absent values and list order, journal
paths, redaction of commits with secrets, and the TOML import and export
round trip with masked secrets.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the pppoe_interfaces document with columns plus child tables for
LCP, IPv6 and prefix delegation. The id is the link name, so a removed link
takes its password and child rows with it. The journal sees links in read
order, and the pruning tests use their own table instead of this module.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tunnels, peers and connections become sets read in key order, every other
list keeps its order in a sequence column. Secrets live in the row of their
owner, so emptying a value or removing its owner leaves none in the working
state; journal and export masking stay as they are. Key files and assets stay
where they are, only their references are stored.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Caddy, its sites and HAProxy with frontends, routes, backends and servers
move to typed tables, so drafts merge by cell and order is one sequence
per row. The Caddyfile and haproxy.cfg stay verbatim text columns. Clearing
a proxy removes its native file, and clearing Caddy also removes its sites,
whose headers and key references may be secret.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the ha_config body with one row per optional part and child
tables for every list. Unsetting a part clears its credentials in the
same save, and whether HA is set at all is one cell in config_sections.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A tunnel stores one peer per public key, while the journal tells named
peers apart by name. Two drafts or a revert could therefore give two peers
one key and lose one without a message, so tunnel validation now rejects a
repeated key. Child rows without an owner are an error instead of a panic.
Tests cover rotating and renaming named peers in merges and reverts.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moving a site and moving it back ends with other sequence values, which the
table hash includes, so the ETag does not return. The test now checks that a
move and its reverse each move the ETag and keep the values.

The JSON audit treats HTTP header values and DHCP option data as user text:
headers such as Report-To carry JSON, and string options may hold any text.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Webhooks and their event and resource lists move to typed tables keyed by the
model id, and monitoring becomes one row with a configured flag, so drafts
merge by cell. Removing a hook removes its token in the same save. A hook
keeps the creation time it was first stored with, because the journal and
TOML do not carry it and otherwise every activation would rewrite the row.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Server listeners, feature switches, telemetry, logging and managed
assets get their own tables and auxiliary_runtime goes away. The early
reader keeps its place before the store opens: it reads main read-only
through the same load as the store, and a file without the tables holds
no settings. Clearing the assets or the server domain leaves no asset
content or key file path in the working set.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A value equal to the generated id of a repetition failed the save. String
lists now use the same id generation as the routing lists. A test pins the HA
peer credential on a full configuration: one cell changes and a repeated pin
commits nothing.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The commit journal is now a set of flat trailers with percent-escaped
values, so a commit message holds no JSON document. The commit list,
revert, redo and file history read those trailers.

The factory profile describes the installation, so it moves to one row
per value in state.db. It follows a factory reset and a database
restore, and config.db no longer holds a JSON body column.

The JSON audit also checks commit trailers. Only the named raw text
columns and comment trailers may hold a JSON document.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A revert of a commit in server, features, telemetry, agent_logging or
  config_assets staged nothing and reported success. Every domain of
  AuxiliarySettings now comes from the reverted document.
- Test reverts over boot defaults, asset metadata and order, mixed commits,
  and the refusal of commits with redacted values.
- ParseKey accepts several base64 spellings of one key, so the repeated peer
  check compares decoded keys.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Record the factory profile with FULL synchronization, so a power
  failure cannot keep a seeded configuration without its profile.
- Take a recorded profile back on a context of its own after a failed
  configuration write.
- Check commit summaries and tag messages in the JSON audit, comment
  text excluded, including messages that do not parse as trailers.
- Describe the stored form of the factory profile as it is.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Delete getSingleton, setSingleton, the JSON list and sync helpers and
  the pruning they alone used.
- The schema check now rejects every body column, without a list of
  remaining tables.
- Remove the config_sections rows of domains that keep their presence
  in their own tables; a missing row reads as not present, so ETags
  stay the same.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the separate schema files with 041_configuration.sql, which
holds the final tables, foreign keys and seed rows. Tables that a later
file dropped are gone. The recorded schema version is 41.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The second cherry-pick of an identical new row has nothing to commit, which
the activation path already handles as a contained package. The tests now
check that result and that a draft changing another row beside it still merges.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AssembleRouterState returns nothing before the first whole-state save, and
SaveLoggingConfig, SaveMirroringConfig and SaveReverseProxyConfig do not set
that marker. No production path relies on those section saves alone.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Hash its tables through contentETag in the tests, and read earlier commits
through a history view because a commit cannot be checked out.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A write refuses a dirty working set, so the fixture failed before any audit
case ran.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Validation rejects default gateways in an isolated routing context.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The inventory lost exactly the 301 recovery options, 1471 to 1170.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dolt_reset --hard rewrites every table, which took most of an HA switchover
with the relational schema. Restarting the stage branch usually has nothing
to drop.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A switchover runs several full commits on both nodes; three seconds failed
on timing alone.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An empty list of a type that can hold secrets holds none, yet it made the
commit protected and impossible to revert.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The alias followed the order of the request while pools read back sorted, so
reverting such a commit reported a conflict at the alias.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The subsystem comparison ignores list order and absent versus empty lists. An
activation with only such a change returned no transaction, left its package
open and never reached main.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also read the columns of all tables in one query.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tests reviewed a foreign package without the right to do so and hit the
permission check, which runs before the conflict check. The PPPoE case for one
struct now sets the same field, since different fields of a new struct merge.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A pool of an enabled server needs a range. The applied configuration also
touches the firewall, so the tests register that applier too.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two inserts into the same gap are a documented conflict. Two appended rules
break the cherry-pick and still merge through the package content.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A full PPPoE section needs its password, which makes the commit protected. An
empty list over an absent section stores nothing and records no draft.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Loading a snapshot drops empty sections.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each subtest builds its own manager and uses no shared hook, so the subtests
can overlap.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DoltLite does not scale inside one process. One table test takes 49 s serially,
44 s with two subtests at a time and 68 s with eight, while its CPU time grows
from 51 s to 312 s, nearly all of it inside the library.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A switch named like a secret, such as password_authentication, and a list whose
type could hold a secret but whose values hold none made a commit protected and
impossible to revert. A list with a free map, such as HTTP headers, stays protected.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Staging stores one row per key, so a revert that gave two peers one key dropped a
peer without a message before the review could see it.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two logical interfaces need two targets. A snapshot restore reads an empty section
as absent.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
They take about half of the 91 minutes the state package needs serially, and go
test runs packages in separate processes. DoltLite does not scale inside one
process: four processes finish one table test in 54 s instead of 49 s, while eight
subtests at a time in one process take 68 s.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- routing.frr.bfd and routing.frr.ospf duplicated routing.bfd and
  routing.ospf. Remove the model fields and their eight tables. Requests
  that still send these names now get the existing unknown field rejection.
- The renderer took the canonical sections through those fields. Pass the
  routing state to SetFRRConfig instead and select the BFD and OSPF daemons
  from the configuration text.
- Validate OSPF values in ValidateRoutingFRR, since the composed FRR check
  that covered them is gone.
- routing.frr.static stays: staticd routes with their own UI editor, separate
  from the kernel routes in routing.static.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Add an egress class with a leaf_qdisc to the example values and regenerate
  example-router.toml. The generator writes non-empty values of the recursive
  field as they are, so it needs no change.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Nothing sets firewall, interfaces or logical_interfaces in config_sections.
  The ETag reads a missing row as not present, so ETags stay the same.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The journal knows DHCP pools and leases by id, so undoing a key change does
not overlap with a later member that took the freed key. Staging keeps one
row per key and silently dropped a member. A revert now conflicts when any
list of the read-order form would lose a member.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The subsystem comparison ignores the order of named lists, so a move such as
HAProxy servers reached main while no applier ran and the service kept the
old order. Activation now also derives subsystems from the journal document.
An absent value becoming an empty one and newly assigned DHCP or DNS ids stay
without runtime work.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Embedded structs, types with their own MarshalJSON and fields of type any fell
through the check by JSON name, so a secret in them stayed in clear text and the
commit stayed reversible. Embedded fields are now visited in place and opaque
types count as carrying a secret unless empty. Known marshalers that only write
their fields, switches and secret-free lists stay reversible, and bodies without
a schema are still judged by field name.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An unkeyed list marked itself sensitive as soon as a field with a secret name
held false, true or an empty list, and an empty object counted as a value, so
for example a site with empty request headers could not be reverted.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A refused revert target always became a conflict and HTTP 409, including a field
the user can correct and an abandoned request. A field error now answers as a bad
request and a canceled context keeps its own meaning.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The inventory called routing.frr.static a deprecated alias. It holds the
routes of FRR staticd with their own dashboard editor and is separate from
the kernel routes in routing.static.

The inventory file was edited to match the generator, not regenerated.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The HAProxy gates compare with SemanticEqual, which sorts named lists, so a
move of frontends or backends never reached haproxy.cfg although they are
rendered in list order. The gates now also compare the rendered file.

A move of servers stays without reload: the renderer sorts them by name, so the
file does not change. The Caddy gates stay as they are: CaddySite has no key
field, so SemanticEqual keeps the order of sites and upstreams. Tests through
the real applier pin both.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The strict decoder of the request does not reach into the custom unmarshaller
of RoutingState, so routing.frr.ospf was answered with 200 and dropped. The
unmarshaller now refuses unknown fields itself, which gives routing.frr.bfd,
routing.frr.ospf and any other unknown routing field a 400. The legacy
routing.frr.bgp location and all model fields stay accepted.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A field error from staging went through writeRoutingApplyError, which reported
every failure as an unavailable FRR. A multi-line OSPF value through
PUT /routing/frr/ospf looked like an outage and invited retries. Field errors
now map to 400 for all routing PUTs; other failures keep the 503.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A DHCP/DNS validation failure and a canceled or timed out request both ended as
a logged 500 when preparing a revert. They now give 400 and 503, as apply and
import do for validation. The OpenAPI description lists both answers.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The substring search also matched router ospf6 and a neighbor description, and
missed a bfd section on the first line.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A nested list with repeated keys under several parents appeared once per parent.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Read order breaks ties between forwarders of one domain by id. With the ids
already gone, a different input order counted as a move and caused a needless
DNS apply.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two drafts adding or removing different entries of these lists conflicted
on @order. Lists the database reads in identity order are now sets, found
by path, so routing/frr/static stays ordered: static routes, WireGuard
tunnels and peers, IPsec connections, webhooks, RA interfaces and QoS
policies. Sets also merge when both drafts create the list and when one
draft removes all it saw while another one adds a member.

QoS classes take effect in order and use the firewall order merge.

Two drafts adding one logical entry under different identities conflict:
routes with one destination and table, peers with one public key, webhooks
with one name. A package records the same-place members its author saw, so
failover routes of one author stay valid.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The custom unmarshaller of RoutingState refused unknown fields on every path,
so a field that differs between versions broke loading stored states, draft
metadata, backups, restore and HA replicas. It reads them tolerantly again.
POST /config/apply now checks the routing document of its request body, so
routing.frr.bfd, routing.frr.ospf and other unknown routing fields still get a
400. The complete import keeps its own field check.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An edit is a field change below a member, so it escaped the check for one logical
entry under two identities: a route edited onto the destination of another draft's
new route gave two rows without a message in one order and a conflict in the other.
The same held for webhook names and peer keys.

The check now runs after the checks of a package, on the set as the package leaves
it, and looks at every member whose entry changes. Alongside is recorded from the
document the author saved and also on edits, so failover routes the author saw or
made in that action stay valid, and a package holding two routes to one place no
longer conflicts with another package holding one of them. A member keeping its
entry takes nothing, so editing the metric of a route never meets a route placed
beside it.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A destination written 10.0.0.1 and one written 10.0.0.1/32 counted as different
places, so the check for one logical entry did not see them meet. On a list two
packages create, a package holding two routes to one place also conflicted with a
package holding one of them in one order only; members it holds itself no longer
count against it.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Removing every member of a section that holds nothing else, such as ra or qos,
makes the section read as absent. A draft adding or removing a member there then
got a false conflict once the removal was confirmed. Such a change now merges like
one below a list that went away, and the section is created again when a member
needs it. A member of another set that is missing still conflicts, and so does an
edit of a member that went away.

The tests also cover removing every member and adding one for the other set lists,
and reverting one draft after a set merge.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
validateRevertDependencies can return a field the user can correct, for example
from the firewall compiler. It was turned into a conflict without the field, which
answered 409 instead of 400. It now takes the same way as the other revert target
checks.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The agent runs boot reconciliation on the recovery server too. That server had
no readiness gate, so recording the active recovery on the gate panicked and a
failed first installation killed the agent instead of offering recovery.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Capture runs before the interface applier, so a new VLAN of the same candidate
has no link yet. CAKE ingress on it failed the capture, and any other policy on
it failed the mixed baseline check once another image existed. Such a source
has no native state to preserve, so capture now skips it. A baseline policy on a
missing link is still refused.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The alias follows the order pools read back from the database, interface order, since the commit that derived it so. The test still expected the order of the request.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A request that leaves a list out keeps it, so these tests staged no removal at all and failed on an empty selection or a list that stayed. They now send an empty list as the API does. New tests pin that contract and cover a new member confirmed before the removal of every member.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two drafts moving peers onto one key now conflict on the logical entry, and a revert onto a key in use is refused before staging. The tests still expected the error of the target validator, which these cases no longer reach. The draft test now covers both orders and the conflict reason; the revert test checks the refusal's reason.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The merge helper checked only ids or names. It now also checks that main
holds the values each draft wrote and lacks what it removed, and that all
orders have the same effect on main.

A revert of a merged section removal restores the exact route beside the
route of the other draft when that draft confirmed first. When it
confirmed later, the revert is refused and leaves main and the drafts
alone. The review of a merged route list is compared with the end state.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A template needs no mapping named mgmt, and none of the end-to-end profiles
has one. The restart after the first installation still asked for a
management repair, which fails without that mapping. Interfaces stayed
unadopted, DHCP readiness of the provisioned WAN failed as a consequence and
the API never left 503.

Repair management only when the configuration maps it, on adoption and on
the startup retry. A direct repair without a mapping remains an error.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The acceptance matched zone policies by position, because they have no
name or id. The router reads them sorted by (from, to), and the compiler
orders the dispatch by specificity and pair, so the position carries no
meaning. Any profile not sorted that way reported false differences.

Rules, in contrast, were matched by id alone, so a changed evaluation
order passed unnoticed. Rule and NAT lists of the firewall now compare
their order as well.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A warm start without a mapped management interface skipped every
interface check and still counted interfaces as succeeded, so the
provisioned WAN DHCP readiness was always satisfied.

Check link, client and lease proof of every enabled DHCP interface and
start only a missing client, also beside a static management interface.
Restore a missing static address or a down link additively; a missing
static link is logged and does not hold the API. A failed interface
apply without a mapping now behaves like a cold start instead of
reporting a management recovery.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comparing the orders with each other let a side effect common to all
orders pass. Every order now has to change main by exactly the expected
effect of its test.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Starting a dead udhcpc runs deconfig, which flushes every IPv4 address of
the link, so a warm start could remove a valid lease and leave the router
without connectivity. A failed DHCP check also held the API at 503 until
the next agent restart, which could lock out a remote administrator.

- Leave a running client and its link alone.
- Leave a dead client alone on a link with any IPv4 address, and warn
  that the lease will not renew. Do the same for DHCPv6 and a global
  IPv6 address beside a running IPv4 client.
- Start a client only on a link without an IPv4 address, clearing a stale
  PID file first, and do not wait for a lease.
- Log every DHCP check problem instead of failing the startup. The
  provisioned readiness now gates only activations and a mapped
  management repair; a cold boot still waits and holds.
- Remove the waiting and the lease proof from this path, and the
  operations the exported test seam no longer needs.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A client start on a warm start could still flush a link, and each exception
to it added a case that needed its own rule. A router that was running before
the restart now keeps its state whatever the check finds.

- Start nothing, kill nothing, remove no PID file, raise no link and change no
  address for enabled DHCP interfaces outside the management repair.
- Log one warning per interface for a dead IPv4 or IPv6 client and for a link
  that is missing or unreadable. A problem on one interface does not stop the
  check of the others, and the check never holds readiness.
- Replace EnsureRestartDHCP with CheckRestartDHCP, which returns the warnings.
- Remove the client clearing, the start and link repair of this path, and the
  exported DHCP operations. The provider constructor keeps only a running check.

Refs #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The firewall compiler adds kaido_ip_options to every applied ruleset, so
the kernel snapshot holds it beside the authored table.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A table with an empty base chain holds no agent rule, so an agent that
did not know it from its configuration treated it as foreign and never
removed it, leaving a DROP policy in the kernel. Read the ownership
marker for such tables. An unreadable marker keeps the table foreign.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also cover a foreign table holding a marker of that name outside the
desired configuration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Packages with few but slow tests ran in three processes because shards
were sized by test count. Plan twice as many shards as workers and split
the largest history test by value kind.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kaido-reviewer requested changes 2026-11-08 13:26:09 +01:00
Dismissed
kaido-reviewer left a comment

Revision

  • Reviewed head: f5451bc757ddf9ffee5fa1f6028fd4eaecdde4e6e90357919a0bfbda2fbe790c

Scope

  • In depth: a56d425d..f5451bc7 (2ae344ca, 0c7a1f11, 9fa06d44, 7b7415b4, 6c49a608, merge f5451bc7).
  • Rest of the branch: only whether high or medium findings from the earlier static review rounds remain open without a recorded reason. No fresh full review.

Findings

Commits in depth

No high or medium findings.

Low: new "never adopted" assertion cannot detect adoption

  • internal/service/gateway_output_selectors_kernel_test.go:211 discards the ApplyManaged error before assertEmptyDrop(collision).
  • If firewallTableMarkerReader treated an invalid marker as owned, the table would stay in the snapshot, managedFirewallSnapshot (gatewayOutputTableOwned) would return an error, the apply would stop and the assertion would still pass.
  • The new warning is not asserted either. Assert that the apply succeeds and the table stays.

Earlier findings open without a recorded reason

Each one still holds in the code at this revision. No reason was found in the task notes, this PR or issue #77. Each needs a fix, or a recorded decision or follow-up issue.

High (round 2E H3): a real cold start can open the API early, depending on the wall clock

  • internal/state/startup.go:144, internal/state/manager.go:1190.
  • The cold attempt saves the boot ID. If interfaces fails there, the retry runs as a warm start.
  • On a board whose clock is behind at boot, metadata from the previous boot is not before btime, so the retry runs the read-only check instead of an activation.
  • Without a mgmt mapping, startupInterfacesAdopted then bypasses the provisioned WAN DHCP gate, although interfaces were never activated in this boot.
  • The logic is confirmed from code; the trigger (wrong clock) is plausible.

Medium (round 2 M1, round 2B M2): an absent VPN or RA section leaves runtime state in place

  • internal/service/appliers.go:656-660, :703-705 (WireGuard), :923-927, :959-961 (RA).
  • ExtractDesired returns nil, Apply does nothing, and the tunnel or RA keeps running after the configuration dropped it, for example on reverting the commit that created the first entry.
  • withoutFields (internal/drafts/sets.go:93-95) makes the outcome depend on commit order for a merged section removal.

Medium (round 2 M4): IFB leftover after a restart during a pending activation

  • internal/service/qos_transition.go:365, internal/service/qos.go:93-94, :182, :206.
  • With a.native nil and s.managed empty, a source created only by the candidate is not cleaned up, and ifb-<vlan> stays with its CAKE root.
  • removeStaleIfbInterfaces is reachable only with scoped=false, and no production caller passes that.
  • 50439042 fixed the capture ordering, not this path.

Medium (round 2B M1): a complete import can leave entries that the imported file does not contain

  • internal/state/complete_import.go:120, internal/drafts/sets.go:74-98.
  • After a concurrent confirmation, the rebased import only removes what its author saw. The router then keeps entries absent from the file, and the review shows no hint.
  • Round 2B left open whether this is intended; no decision is recorded.

Checked without findings

  • Merge f5451bc7: tree identical to the automatic merge of both parents (c88c82ad), so no conflict resolution; the combined diff is empty.
  • 0c7a1f11, 9fa06d44: marked runs only for otherwise unowned tables, and unreadable or invalid markers stay foreign. Removing the connection release leaks nothing, because FreshConn does not request a lasting socket and CloseLasting is a no-op there.
  • 2ae344ca: filters only the reserved generated table.
  • 7b7415b4: covers the earlier gap (the DHCP check beside a mgmt mapping); warning, repair count and repair result are asserted, including the applier without a DHCP check.
  • 6c49a608, shards: rest[index::count] partitions all non-serial tests, and the serial kernel/integration group stays one appended process. Below SHARD_MIN_TESTS the package stays one process, as before.
  • 6c49a608, split tests: the three tests use the same three pair generators as the former merged map, with the same per-pair assertions, so all former pairs stay covered.
  • Other earlier high and medium findings are fixed in code or superseded by the read-only warm-start DHCP check (a56d425d), whose commit message records the reason.

Validation limits

  • Static review of code, tests and existing evidence only. No tests, builds, checks, browser sessions or router actions were run.
  • Recorded evidence covers a56d425d (full run) and post-a56d425d package and kernel runs as stated in the PR description. I did not verify those results or any run at this exact head.
## Revision - Reviewed head: `f5451bc757ddf9ffee5fa1f6028fd4eaecdde4e6e90357919a0bfbda2fbe790c` ## Scope - In depth: `a56d425d..f5451bc7` (2ae344ca, 0c7a1f11, 9fa06d44, 7b7415b4, 6c49a608, merge f5451bc7). - Rest of the branch: only whether high or medium findings from the earlier static review rounds remain open without a recorded reason. No fresh full review. ## Findings ### Commits in depth No high or medium findings. **Low: new "never adopted" assertion cannot detect adoption** - `internal/service/gateway_output_selectors_kernel_test.go:211` discards the `ApplyManaged` error before `assertEmptyDrop(collision)`. - If `firewallTableMarkerReader` treated an invalid marker as owned, the table would stay in the snapshot, `managedFirewallSnapshot` (`gatewayOutputTableOwned`) would return an error, the apply would stop and the assertion would still pass. - The new warning is not asserted either. Assert that the apply succeeds and the table stays. ### Earlier findings open without a recorded reason Each one still holds in the code at this revision. No reason was found in the task notes, this PR or issue #77. Each needs a fix, or a recorded decision or follow-up issue. **High (round 2E H3): a real cold start can open the API early, depending on the wall clock** - `internal/state/startup.go:144`, `internal/state/manager.go:1190`. - The cold attempt saves the boot ID. If `interfaces` fails there, the retry runs as a warm start. - On a board whose clock is behind at boot, metadata from the previous boot is not before `btime`, so the retry runs the read-only check instead of an activation. - Without a `mgmt` mapping, `startupInterfacesAdopted` then bypasses the provisioned WAN DHCP gate, although interfaces were never activated in this boot. - The logic is confirmed from code; the trigger (wrong clock) is plausible. **Medium (round 2 M1, round 2B M2): an absent VPN or RA section leaves runtime state in place** - `internal/service/appliers.go:656-660`, `:703-705` (WireGuard), `:923-927`, `:959-961` (RA). - `ExtractDesired` returns nil, `Apply` does nothing, and the tunnel or RA keeps running after the configuration dropped it, for example on reverting the commit that created the first entry. - `withoutFields` (`internal/drafts/sets.go:93-95`) makes the outcome depend on commit order for a merged section removal. **Medium (round 2 M4): IFB leftover after a restart during a pending activation** - `internal/service/qos_transition.go:365`, `internal/service/qos.go:93-94`, `:182`, `:206`. - With `a.native` nil and `s.managed` empty, a source created only by the candidate is not cleaned up, and `ifb-<vlan>` stays with its CAKE root. - `removeStaleIfbInterfaces` is reachable only with `scoped=false`, and no production caller passes that. - 50439042 fixed the capture ordering, not this path. **Medium (round 2B M1): a complete import can leave entries that the imported file does not contain** - `internal/state/complete_import.go:120`, `internal/drafts/sets.go:74-98`. - After a concurrent confirmation, the rebased import only removes what its author saw. The router then keeps entries absent from the file, and the review shows no hint. - Round 2B left open whether this is intended; no decision is recorded. ### Checked without findings - **Merge f5451bc7:** tree identical to the automatic merge of both parents (`c88c82ad`), so no conflict resolution; the combined diff is empty. - **0c7a1f11, 9fa06d44:** `marked` runs only for otherwise unowned tables, and unreadable or invalid markers stay foreign. Removing the connection release leaks nothing, because `FreshConn` does not request a lasting socket and `CloseLasting` is a no-op there. - **2ae344ca:** filters only the reserved generated table. - **7b7415b4:** covers the earlier gap (the DHCP check beside a `mgmt` mapping); warning, repair count and repair result are asserted, including the applier without a DHCP check. - **6c49a608, shards:** `rest[index::count]` partitions all non-serial tests, and the serial kernel/integration group stays one appended process. Below `SHARD_MIN_TESTS` the package stays one process, as before. - **6c49a608, split tests:** the three tests use the same three pair generators as the former merged map, with the same per-pair assertions, so all former pairs stay covered. - Other earlier high and medium findings are fixed in code or superseded by the read-only warm-start DHCP check (a56d425d), whose commit message records the reason. ## Validation limits - Static review of code, tests and existing evidence only. No tests, builds, checks, browser sessions or router actions were run. - Recorded evidence covers a56d425d (full run) and post-a56d425d package and kernel runs as stated in the PR description. I did not verify those results or any run at this exact head. 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kaido-reviewer approved these changes 2026-11-08 13:29:31 +01:00
kaido-reviewer left a comment

Revision

  • Reviewed head: 766bb20fe8f9eafd90538bef1f81b51542a1c7b4cfb2fc64c4fb698b3663e5be
  • In depth: f5451bc7..766bb20f (one test file). Rest of the branch: only the status of the findings from review 42.

Findings

No new high, medium or low findings.

Low finding from review 42: addressed

  • internal/service/gateway_output_selectors_kernel_test.go:206-220 now asserts the refusal and the warning instead of discarding the error.
  • The refusal is real: ApplyManaged reaches refreshGatewayOutputMapsLocked (firewall.go:577), which checks all kernel tables through gatewayOutputTableOwned, and an invalid marker returns the asserted error. This also explains why the suggestion in review 42 (assert that the apply succeeds) did not apply.
  • Adoption is now detectable: if marker validation accepted the lookalike, the apply would not return this error and the test would fail; a deleted or changed table still fails assertEmptyDrop.
  • The warning assertion ties the case to firewallTableMarkerReader, using a fresh service with its own log buffer.
  • Remaining gap, not blocking: a reader that warned and then returned true would pass, but it has the same outcome, because managedFirewallSnapshot refuses with the same error and the table stays.

Earlier findings: recorded, out of scope by owner decision

  • #84 (open) matches H3: clock-dependent warm or cold classification, startup.go:144, manager.go:1190.
  • #85 (open) matches the VPN/RA nil finding: appliers.go:656-660, :703-705, :923-927, :959-961. It does not repeat the commit-order note on withoutFields; this omits a detail and does not misstate the defect.
  • #86 (open) matches the QoS ifb leftover: qos_transition.go:365, qos.go:206.
  • Complete import: the decision to keep the behaviour is recorded in the PR section "Findings kept out of this change".

Validation limits

  • Static review of code, tests and existing evidence only. No tests, builds, checks, browser sessions or router actions were run.
  • The kernel test result is reported by the implementer for the state before the final gofmt-only import reorder. I did not verify that run, and no run at this exact head is recorded. Statically the reorder has no behavioural effect.
## Revision - Reviewed head: `766bb20fe8f9eafd90538bef1f81b51542a1c7b4cfb2fc64c4fb698b3663e5be` - In depth: `f5451bc7..766bb20f` (one test file). Rest of the branch: only the status of the findings from review 42. ## Findings No new high, medium or low findings. ### Low finding from review 42: addressed - `internal/service/gateway_output_selectors_kernel_test.go:206-220` now asserts the refusal and the warning instead of discarding the error. - The refusal is real: `ApplyManaged` reaches `refreshGatewayOutputMapsLocked` (`firewall.go:577`), which checks all kernel tables through `gatewayOutputTableOwned`, and an invalid marker returns the asserted error. This also explains why the suggestion in review 42 (assert that the apply succeeds) did not apply. - Adoption is now detectable: if marker validation accepted the lookalike, the apply would not return this error and the test would fail; a deleted or changed table still fails `assertEmptyDrop`. - The warning assertion ties the case to `firewallTableMarkerReader`, using a fresh service with its own log buffer. - Remaining gap, not blocking: a reader that warned and then returned true would pass, but it has the same outcome, because `managedFirewallSnapshot` refuses with the same error and the table stays. ### Earlier findings: recorded, out of scope by owner decision - #84 (open) matches H3: clock-dependent warm or cold classification, `startup.go:144`, `manager.go:1190`. - #85 (open) matches the VPN/RA nil finding: `appliers.go:656-660`, `:703-705`, `:923-927`, `:959-961`. It does not repeat the commit-order note on `withoutFields`; this omits a detail and does not misstate the defect. - #86 (open) matches the QoS ifb leftover: `qos_transition.go:365`, `qos.go:206`. - Complete import: the decision to keep the behaviour is recorded in the PR section "Findings kept out of this change". ## Validation limits - Static review of code, tests and existing evidence only. No tests, builds, checks, browser sessions or router actions were run. - The kernel test result is reported by the implementer for the state before the final gofmt-only import reorder. I did not verify that run, and no run at this exact head is recorded. Statically the reorder has no behavioural effect.
Alex merged commit 881cd352f4 into main 2026-11-08 13:29:53 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
kaido-reviewer
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
Alex/kaido-core!83
No description provided.