Skip to content

fix(presets): use continue instead of return for stale CID in preset …#2770

Open
VirajMandavkar wants to merge 1 commit into
kubearmor:mainfrom
VirajMandavkar:patch-4
Open

fix(presets): use continue instead of return for stale CID in preset …#2770
VirajMandavkar wants to merge 1 commit into
kubearmor:mainfrom
VirajMandavkar:patch-4

Conversation

@VirajMandavkar

Copy link
Copy Markdown
Contributor

Purpose of PR?:
While digging into #2761 I found another bug in the preset enforcer's container loop (KubeArmor/presets/protectproc/preset.go, around line 288), inside UpdateSecurityPolicies.

There's already a check here for a legit race condition — CRI can unregister a container before K8s sends the update through, so the code checks if the CID is even in ContainerMap before applying rules:

if !ok {
    // It maybe possible that CRI has unregistered the containers but K8s construct still has not sent this update while the policy was being applied,
    // so the need to check if the container is present in the map before we apply policy.
    p.Logger.Warnf("container not registered in map: %s", cid)
    return
}
​

That check is fine, the problem is what happens after it fails. return here doesn't just skip the current container - it exits the whole function. Since this is inside a loop over endPoint.Containers, if even one CID in the middle of the list isn't registered yet, every container that comes after it in that iteration gets skipped entirely. No preset rules applied, no error surfaced anywhere, KubeArmor just carries on like everything's protected.

Fixes # (haven't filed a separate issue for this one, flagging directly — found while working on #2761)

Does this PR introduce a breaking change?
No

If the changes in this PR are manually verified, list down the scenarios covered:

  1. Built locally with the fix(core): resolve newendpoints typo to properly track all containers… #2761 fix already in.
  2. Spun up a pod with 3 containers (alpha, beta, gamma).
  3. Applied a ProtectProc preset on it.
  4. Killed alpha, saw the "container not registered in map" warning fire like expected.
  5. Did the same for beta and gamma one at a time to make sure the warning fires consistently, not just for one specific container.
  6. Read through the code carefully to confirm return is inside the loop — so this isn't something that only shows up under specific timing, it's just how the loop is written. Any miss anywhere in the list kills processing for the rest.

Additional information for reviewer?:

Root cause

for _, cid := range endPoint.Containers {
    ckv, ok := p.ContainerMap[cid]
    if !ok {
        // It maybe possible that CRI has unregistered the containers but K8s construct still has not sent this update while the policy was being applied,
        // so the need to check if the container is present in the map before we apply policy.
        p.Logger.Warnf("container not registered in map: %s", cid)
        return   // exits the whole function, not just this iteration
    }
    // apply preset rules for ckv...
}
​

The race condition check itself was clearly intentional and it's a real scenario worth guarding against. But bailing out of the entire function because one container in the list isn't ready yet doesn't make sense — the other containers in the pod are unrelated and shouldn't be affected. Swapping return for continue keeps the original intent (skip this one container) without taking down enforcement for everyone else in the same call.

The fix

- return
+ continue
​

On proof/evidence — being upfront here since I couldn't fully nail this down live: I was able to consistently trigger the warning log by killing containers and forcing stale CIDs, so I know this branch does get hit in real clusters. What I couldn't do is capture a clean log showing one specific later container getting skipped because of it — timestamps were too close together (sub-millisecond) to say for certain in a live run. That said, I don't think that ambiguity matters much here, because this isn't really a "maybe it happens" bug. return inside a Go for-loop will always skip everything after it once hit — that's just what the keyword does, it's not conditional on timing or environment. Happy to add a unit test that isolates the loop logic directly (fake ContainerMap missing a middle CID, assert the containers after it still get processed) if that's useful for review — can push that as a follow-up commit.

How to reproduce

  1. Deploy a pod with 3+ containers, apply a ProtectProc (or similar) preset policy.
  2. Restart/kill one of the non-last containers (crictl stop works fine).
  3. You'll see container not registered in map fire for that CID.
  4. Check the enforcement state for containers listed after it in endPoint.Containers — right now they won't get preset rules applied for that call.
  5. Swap returncontinue, repeat the steps — the rest of the containers should get processed regardless of the earlier miss.

Checklist:

  • Bug fix. Fixes #(logic bug, no issue filed yet)
  • New feature
  • Breaking change
  • This change requires a documentation update
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Commit has unit tests (offered above, pending reviewer input)
  • Commit has integration tests

…loop

Preset enforcer's UpdateSecurityPolicies exits the entire function via
`return` when a container CID isn't yet registered in ContainerMap,
instead of skipping just that container. Since this check runs inside
the container iteration loop, any container appearing after a stale
CID is silently left without preset rule enforcement.

The stale-CID guard itself is valid (CRI may unregister a container
before K8s sends the update), but the exit behavior was too broad.
Changed `return` to `continue` so remaining containers are still
processed.

Signed-off-by: VirajMandavkar <mandavkarviraj03@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant