allow network-slices to be a cel selector for internal/external traffic - #902
allow network-slices to be a cel selector for internal/external traffic#902entlein wants to merge 1 commit into
Conversation
…c allowlisting Signed-off-by: entlein <einentlein@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the diff and checked it out locally (go build ./..., go test ./pkg/objectcache/containerprofilecache/... ./pkg/rulemanager/cel/... — all pass; gofmt -l clean on the changed files).
No blockers found in the code itself. A few notes:
- Verified
namespaceSelectorMatches's "nil selector ⇒ same namespace as the profiled workload" rule against the actual generator (pkg/containerprofilemanager/v1/container_data.gocreateNetworkNeighbor/getNamespaceMatchLabels): the generator only omitsNamespaceSelectorwhendestinationNamespace == sourceNamespace, and always setsPodSelectoralongside anyNamespaceSelectorfor Pod/Service kinds — soextractPeers'sPodSelector != nilgate won't silently drop any entry the current generator actually produces. Good match between the CEL-side semantics and the write-side encoding. - Confirmed
L3Endpoint.Namespace/PodLabelsreally are populated cluster-wide by IG'sKubeIPResolveroperator (not node-local), which backs the PR's cross-node design claim. was_selector_in_{ingress,egress}correctly reuses the existingConvertProfileNotAvailableErrToBoolerror path and cost-estimator pattern used by the othercp.*helpers; thenoCachebypass is reasonable since the map arg has no stable cache key.- Minor/non-blocking:
extractPeersdrops any neighbor entry with only aNamespaceSelectorand noPodSelector. The current learned-profile generator never emits that shape, but if a hand-authored/editedContainerProfileever did (valid perNetworkPolicyPeersemantics — namespaceSelector alone = "all pods in matching namespaces"), it would silently never match. Not worth blocking on given today's only producer, but maybe worth a one-line comment or a follow-up if user-edited profiles become a supported path.
Holding off on approval since this PR is still a draft and the description itself flags it as not merge-ready yet — the author's own TODO calls for re-testing after the rebase specifically for same-node vs. inter-node (cross-node) attribution, int/ext, which isn't covered by the new unit tests here (those exercise the CEL/projection logic against fixed inputs, not the live IG resolver path across nodes). The companion rule-library wiring (R0012 in default-rules.yaml) is also intentionally not part of this PR. Happy to take another pass and approve once it's marked ready for review and the rebase re-test is done.
Good point and here's my two cents: On the contrary:
From a threat-model PoV: I see is little advantage to match only on Reason both of these Rule PRs are in draft is so we have time to test it on IRL usecases for exactly those rationales and weigh UX vs number of false-positives when user does it wrong vs how its easiest to get it right. If anyone (at all out there), has IRL examples that lean to either or, please do comment |
The use-case I wrote this for is to allow-list a client esp for cluster interal traffic using the well-known network-policy like selectors.
I also suggest to symmterizice the rules, to have them for internal and external traffic and both egress and ingress (seperate as the threat-model is different).
Example can be found here k8sstormcenter/bob#187
Watch R0012 on valkey-primary's node
Deploy the client without matching the labes — R0012 fires:
Allowlist that identity — R0012 stops:
Why I think we should have this
Cause excluding traffic in the profiles, based on IP is impossible (changing IPs) . This works for external static IPs or ranges.
Thus, if we wish to alert if internal connections are happening that we didnt intend, we need to match something
predictable and static.The NWP syntax is well-established
TODO
I need to test this again after the large rebase, cause previous bugs where that the inter-cluster (across several nodes) traffic wasnt getting properly attributed.
If they got lost, this needs tests to check for
Needs a followup PR in rulelibrary