merge: upstream/main into wildcards (NetworkNeighbor IPAddresses + cleanup)#31
Open
entlein wants to merge 12 commits into
Open
merge: upstream/main into wildcards (NetworkNeighbor IPAddresses + cleanup)#31entlein wants to merge 12 commits into
entlein wants to merge 12 commits into
Conversation
… pods Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
fix(cleanup): enhance pod deletion logic and add tests for standalone pods
Runtime counterpart to spec §5.7 (v0.0.2). Profile entries are matched disjunctively — any single entry hit means match. Each entry MAY be: - a literal IP (canonicalised by net.ParseIP, IPv4 ↔ ::ffff:v4 unified) - a CIDR (a.b.c.d/n, validated by net.ParseCIDR) - the '*' sentinel for any IP (sugar for 0.0.0.0/0 ∪ ::/0) Public API: CompileIP([]string) *IPMatcher // compile-once for hot paths (*IPMatcher).Match(string) bool // amortised match MatchIP([]string, string) bool // cold-path convenience The compiled-matcher pattern eliminates per-call allocs from net.ParseCIDR on the hot path — node-agent's nn.* CEL functions call this on every network event captured by the eBPF tracer. Tests cover: - literal equality (incl. IPv4/IPv6 canonicalisation, ::ffff: mapping) - CIDR membership (incl. /32 == literal, edge addresses) - '*' sentinel (and rejection of empty/garbage observations) - 0.0.0.0/0 + ::/0 RFC-aligned alternatives (do NOT cross address families) - malformed entry skipping (admission validates, runtime defends) - mixed-list disjunction Benchmarks (arm64, hot-path compiled form): Literal : 12.73 ns/op 0 allocs CIDR : 18.48 ns/op 0 allocs 10-entry miss : 59.10 ns/op 0 allocs All well under the README target of <200 ns/op for IP literal/CIDR.
…psis
Runtime counterpart to spec §5.8 (v0.0.2). Profile entries are matched
disjunctively against an observed FQDN. Each entry's labels are walked
left-to-right with positional token semantics:
literal label : byte-equality (case-insensitive)
leading '*' : matches EXACTLY ONE label (RFC 4592)
trailing '*' : matches ONE OR MORE labels (project extension)
'⋯' anywhere : matches EXACTLY ONE label (DynamicLabel)
'**' : reserved/recursive — admission rejects, runtime drops
Trailing dot is normalised on both sides (entry MAY or MAY NOT have it).
Empty inner labels (e.g. 'foo..bar') are treated as malformed and skipped.
Public API:
CompileDNS([]string) *DNSMatcher
(*DNSMatcher).Match(string) bool
MatchDNS([]string, string) bool // cold-path convenience
Tests cover:
- literal equality (case-insensitive, exact-label-count)
- trailing-dot normalisation in all four combinations
- RFC 4592 leading '*' (exactly one label)
- mid '⋯' DynamicLabel (the kubernetes service-FQDN case)
- trailing '*' extension (one or more, never zero)
- disjunction across entry list
- malformed input rejection ('**', empty labels, lone '*')
- compiled-form reuse contract
Benchmarks (arm64, hot-path compiled form):
Literal : 47.68 ns/op 1 alloc (label-split)
Leading wildcard : 53.20 ns/op 1 alloc
Deep name (10 lbl): 144.9 ns/op 1 alloc
Long mixed list : 103.3 ns/op 1 alloc
All under the README target of <600 ns/op for DNS wildcards.
Adds IPAddresses []string to the NetworkNeighbor schema (internal +
v1beta1) as the v0.0.2 list-form replacement for the deprecated singular
IPAddress field. Each entry MAY be a literal IP, a CIDR, or '*'.
Hand-edited generated code (deepcopy, conversion, protobuf) because
the codegen pipeline isn't run as part of build; verified by a
new round-trip test that pins the protobuf wire contract for field
number 9.
Validation lives in two places:
pkg/registry/file/networkmatch/validate.go
- ValidateIPEntry : literal | CIDR | '*'
- ValidateDNSEntry : literal | leading-* | trailing-* | mid-⋯
rejects '**', empty inner labels, mid-* without ⋯
pkg/registry/softwarecomposition/networkneighborhood/strategy.go
REST Validate() walks every neighbor in containers/initContainers/
ephemeralContainers × egress/ingress and surfaces a field.Invalid
error per malformed entry. Apiserver translates this into a 400
so the user gets immediate feedback at write time.
Runtime matchers (MatchIP/MatchDNS) keep their tolerance of malformed
entries — a misconfigured profile that slips past admission won't kill
the detection path.
New tests:
- protobuf round-trip (field 9 survives Marshal → Unmarshal)
- ValidateIPEntry / ValidateDNSEntry edge cases
- REST strategy Validate rejects malformed entries with the right
field paths
Five findings, all legit, all fixed: - ValidateUpdate now also runs validateNetworkProfileEntries, so malformed ipAddresses / dnsNames can't land via PUT after a clean POST. New TestValidateUpdate_NetworkProfileEntries pins this. - validateNeighborList now also validates the deprecated singular IPAddress field while it remains supported. Bypass via the old form is closed. - TestValidate_NetworkProfileEntries asserts each error's field path (multiset, order-insensitive) instead of just the count. Field-path contract is now pinned: if validation starts emitting errors on the wrong path, downstream tooling that surfaces these to users will catch it here first. New case for deprecated-singular path. - TestNetworkNeighbor_IPAddresses_EmptyOmitted now checks Marshal errors with t.Fatalf instead of swallowing them via _. - README links to spec-v0.0.2 (was v0.0.1) and DNS normalisation wording matches runtime: trailing dot stripped, labels lowercased (was incorrectly described as 'appended').
Three findings, two fixed, one skipped: - validateNetworkProfileEntries now iterates an ordered []struct of (groupName, items) pairs rather than a map. Go map iteration is non-deterministic and admission errors flow to clients via the apiserver — stable ordering keeps error messages reproducible. - TestValidateUpdate_NetworkProfileEntries asserts each error's field path (multiset, order-insensitive). Same pattern already applied to TestValidate_NetworkProfileEntries; this closes the parallel gap. Skipped: - README.md "canonicalisation" → "canonicalization" (Trivial nit). Project already uses British spelling consistently across other docs; changing one occurrence creates inconsistency without fixing anything. Tests pass on the new ordering — note the test uses an order-insensitive multiset comparison, so the deterministic-order fix is a separate concern from the test (the test would pass with either ordering, but the runtime is now stable for downstream consumers).
Closes a parity gap with IPAddress (which round-1 already validates). Now both deprecated singular fields receive the same admission grammar check while they remain supported. Test extended with a deprecated-DNS path case mirroring the existing IPAddress one.
Reverses the round-2 skip on the CodeRabbit nitpick (PR #30, README line 17). Files added in this branch now use American spelling (canonicalize/normalize/behavior) matching the Go ecosystem convention that 'canonicalisation' originally diverged from. Files touched: - pkg/registry/file/networkmatch/README.md - pkg/registry/file/networkmatch/match_ip.go (comment) - pkg/registry/file/networkmatch/match_dns.go (comment) - pkg/registry/file/networkmatch/match_ip_test.go (comment) No behaviour change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds wildcard-aware network neighbor matching, implementing IP and DNS pattern matching against profiles (literals, CIDRs, RFC4592 leading- ChangesNetworkNeighbor wildcard matching feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Summary:
|
Test Results2 tests 2 ✅ 34m 20s ⏱️ Results for commit a15fb5a. ♻️ This comment has been updated with latest results. |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…ExecArgs
Storage-side recording stores execs as [resolved_path, ...argv] and
returns to consumers as (Path=resolved, Args=full-argv-with-argv[0]).
The matcher does strict positional compare with no special argv[0]
normalisation. That means profile.Args[0] MUST equal runtime argv[0]
as captured by eBPF — typically the BARE program name ("sh"), not
the resolved exepath ("/bin/sh").
The realistic-patterns table earlier in this file uses 2-element
vectors that skip argv[0] entirely, which doesn't pin the convention.
Add TestCompareExecArgs_Argv0BareName covering all the Test_32
scenarios end-to-end against the matcher, plus the two
profile-shape-mismatch cases (Args[0]=full-path vs bare argv[0], and
vice versa) so future drift on either side trips the test.
Pairs with the rewrite of node-agent's Test_32_UnexpectedProcessArguments
that changes profile.Args from [/bin/sh, -c, *] to [sh, -c, *], so
R0040 can be tested in isolation from R0001's path-resolution path.
|
Summary:
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
2 participants
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.
Summary
Brings the storage fork's
mainup to upstream parity AND keeps the v0.0.2 wildcard surface (PR #30) in one go. Merges upstream PR kubescape#317 (clean-standalone-pods cleanup edge case) on top of the wildcards branch.This is a single landing point that supersedes #30 — pick whichever you prefer for review:
What's in the branch
Wildcards (already approved on #30)
pkg/registry/file/networkmatch/— IP + DNS matchers (literal, CIDR,*, leading-/mid-/trailing-wildcards)NetworkNeighbor.IPAddresses []stringschema field (proto field 9)**, mid-position bare*, validates both deprecatedIPAddressandDNSsingular fieldsUpstream merge (clean fast-forward)
8be42b9a+ merge):fix(cleanup): enhance pod deletion logic and add tests for standalone pods—pkg/registry/file/cleanup.go+ tests, +44/-1Test plan
IPAddressesfield 9 (admission → etcd → read)go vetwarnings unchanged from main baselineCompanion PR
Node-agent companion:
k8sstormcenter/node-agent#<TBD>(merge branch carrying upstream PR #788 cache rearch + #799 projection + wildcards + exec-args matching on top).