#77 fix cname dedupe#78
Merged
Merged
Conversation
Signed-off-by: Matthew DeVenny <matt@codecargo.com>
There was a problem hiding this comment.
Pull request overview
This PR updates CargoWall’s summary-stage event deduplication so that when multiple connection events collapse to the same dedup key, the retained “first-seen” representative row is enriched with missing distinguishing fields (notably CNAME chain and auto-allowed type), preventing loss of drill-down metadata during API push and summary rendering.
Changes:
- Update
deduplicateStepEventsto track the representative event index per dedup key and backfillCNAMEChain/AutoAllowedTypefrom later duplicates when the representative is missing them. - Add focused unit tests covering CNAME-chain backfill, non-overwrite behavior, and auto-allowed-type backfill semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/summary.go | Enhances deduplication to preserve key metadata (CNAME chain / auto-allowed type) across same-key duplicates. |
| cmd/summary_test.go | Adds regression tests for the new backfill behavior to prevent metadata loss in future changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matthewdevenny
commented
Jul 8, 2026
Signed-off-by: Matthew DeVenny <matt@codecargo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #77
Why
cargowall summarydedupes connection events per step before the SaaS push, on a key of(process, dest, port, protocol, eventType)— and keeps the first-seen event wholesale. Transparent CNAME support (#63/#68/#73) attributes a CNAME-derived connection to its origin hostname (chain[0]), the samedesta direct connection to that origin reports. So a chain-bearing event and a chainless direct hit to the same origin collide on the dedup key, and whenever the chainless one sorts first, the chain-bearing event is discarded — the pushed/stored event has an emptycname_chaineven though the firewall logged the chain correctly (see #77 for the observedauth.docker.ioevidence). The same wholesale discard applies toauto_allowed_type, which is per-IP (CIDR pass ofGetAutoAllowedType) while the dedup key ignoresDstIPwhen a hostname is set — round-robin DNS can produce same-key duplicates where only a later one carries the type.What
cmd/summary.go—deduplicateStepEvents: theseenmap now stores the index of the retained representative (map[dedupKey]int). On a collision, instead of discarding the duplicate outright, backfill the distinguishing fields the representative is missing:CNAMEChain— filled when the representative has none and the duplicate carries one;AutoAllowedType— same fill-if-empty rule.Fill-if-empty only: the first-seen event stays the representative and an existing value is never overwritten. One row per destination is preserved (the merge approach preferred in summary: cname_chain dropped by deduplicateStepEvents (chainless duplicate to the same origin wins) #77, not the add-chain-to-key alternative, which would emit a redundant chainless row). No signature or caller changes —
auditEventToProtopicks up the now-populated fields via its existing non-empty guards.MatchedRuledeliberately gets no backfill: onlyconnection_late_allowedevents carry it, and they always do (HasAllow()⇒ non-empty rule), so witheventTypein the dedup key a same-key group is uniformly populated or uniformly empty — the backfill branch could never fire. Documented in a comment.cmd/summary_test.go: three new tests alongside the existing dedup group —ChainBackfilledFromLaterDuplicate— the summary: cname_chain dropped by deduplicateStepEvents (chainless duplicate to the same origin wins) #77 repro shape: chainless direct hit first, chain-bearing CNAME-derived duplicate second → one row, chain preserved, first-seen timestamp retained;ChainNotOverwritten— a later chainless duplicate doesn't clobber an existing chain;AutoAllowedTypeBackfilled— per-IP round-robin shape: type backfilled from the first duplicate that carries one, a later different type doesn't overwrite.Semantics note
For a merged row with mixed auto/non-auto duplicates, the auto-allow attribution now deterministically wins (previously the label was ordering-arbitrary, first-seen verbatim). This means
totalAutoAllowed/AutoAllowedConnectionscount a destination as auto-allowed if any connection to it was — intended, since at least one connection genuinely took the auto-allow path, and the alternative silently hides that attribution.Testing
TestSummary_DeduplicateStepEvents_*tests (7 existing + 3 new) and the fullcmdpackage pass on linux (golang:1.25container; the dev host is darwin and these files are//go:build linux).GOOS=linux go build ./...,go vet,staticcheck, andgofumpt -lare clean.🤖 Generated with Claude Code