Scope namespaced program pod selection to the owning namespace - #541
Open
frobware wants to merge 2 commits into
Open
Scope namespaced program pod selection to the owning namespace#541frobware wants to merge 2 commits into
frobware wants to merge 2 commits into
Conversation
Reproduce the cross-namespace pod selection reported in bpfman#540. A namespaced BpfApplication in one namespace carries a TC program whose network-namespace selector matches an identically-labelled pod in a second namespace on the same node. The test asserts that the application's BpfApplicationState never holds more than the single link for the pod in its own namespace. The two target pods are given distinct names deliberately. GetOneContainerPerPod deduplicates by pod name, so identically-named pods across namespaces would collapse to one link and hide the leak. Against an unfixed agent this fails on the extra link into the foreign namespace; the fix in the following commit makes it pass. Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
The namespaced program reconcilers (TC, TCX, XDP, uprobe, uretprobe) resolved their pod and network-namespace selectors against an empty namespace. In client-go an empty namespace lists pods across every namespace, filtered only by the label and node-name selectors, so a namespaced BpfApplication could attach eBPF programs into pods owned by other namespaces on the same node, defeating the isolation the namespaced CRD exists to provide. The namespace of a namespaced program is always its owning application's namespace. Hold the owning BpfApplication on NsProgramReconcilerCommon and derive the namespace from it rather than copying it into a separate field that the reconciler constructions left unset. This removes the field that could silently be empty and folds the duplicated getNamespace() methods into one. Fixes: bpfman#540 Signed-off-by: Andrew McDermott <amcdermo@redhat.com>
Contributor
Author
|
cc @alebedev87 |
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
1 participant
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.
A namespaced
BpfApplicationresolves its pod and network-namespace selectors against an empty namespace, which client-go treats as all namespaces, so the selectors match pods in every namespace on the node rather than only the application's own. A tenant confined to one namespace can therefore attach eBPF programs into other namespaces' pods on the same node. Issue #540 has the full analysis, the regression history (it dates to the load/attach split in 7c11035), and a reproducer with captured output.The namespace of a namespaced program is always its owning application's namespace, so this derives it from the owning
BpfApplicationheld onNsProgramReconcilerCommonrather than copying it into a field that the reconciler constructions left unset. That removes the empty-able field and folds the three duplicatedgetNamespace()methods into one. All five namespaced program types (TC, TCX, XDP, uprobe, uretprobe) share this path, so all are covered.The change is split into two discrete commits so the failure is reproducible before the fix. The first commit adds only the integration test, which reproduces the leak with two identically-labelled pods in different namespaces and one namespaced application. Build the agent at that commit and run the test, and it fails on the extra link into the foreign namespace; build again at the second commit, which applies the fix and the unit-level regression tests, and the same test passes. Checking the test commit out on its own is the intended way to watch it go from red to green.
Fixes #540
Test plan
go test ./controllers/bpfman-agent/covers the unit tests, including the new derivation and container-lookup scoping tests.make test-integration-local TEST=TestNamespacedAppDoesNotSelectPodsInOtherNamespacespasses against an agent carrying this fix and fails against one built frommain(the application's state holds two TC links, the second resolving to a pod in the foreign namespace), verified on a local single-node kind cluster.