CMP-4605: Guard kubevirt NAD rules against NADs without a spec - #15020
CMP-4605: Guard kubevirt NAD rules against NADs without a spec#15020Vincent056 wants to merge 2 commits into
Conversation
kubevirt-localnet-vlan-required and kubevirt-bridge-mac-spoof-filtering guarded only has(n.spec.config). A NetworkAttachmentDefinition with no spec at all is legitimate (e.g. the istio-cni NAD), but it makes has(n.spec.config) raise 'no such key: spec' at evaluation time, and the scanner maps missing-key errors to FAIL - so both rules false-FAIL on any cluster carrying such a NAD. Prepend !has(n.spec) to both expressions and add a spec-less NAD regression fixture: it fails 5/6 and 6/7 against the unfixed expressions and all cases pass with the fix (celctl, the operator's scanner engine). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A NetworkAttachmentDefinition with spec.config set to an empty string passes both has() guards but makes parseJSON crash with 'unexpected end of JSON input', which the scanner surfaces as ERROR. Seen on a large CNV engineering cluster during profile validation. Treat an empty config as compliant, like an absent one, and add regression fixtures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
@Vincent056 Looks like this should actually handle an empty spec.config string?
It does handle the empty spec.config.
But I wonder if allowing a NAD without spec is healthy.
|
Good question — I went back and forth on this too. I think skipping is correct for these two rules:
Config-less NADs are also a legitimate, spec-defined pattern rather than cruft: the NPWG NetworkAttachmentDefinition de-facto standard, §3.4.1 "Determining CNI Plugins for a NetworkAttachmentDefinition Object", makes them delegate to an on-disk CNI config file matching the NAD name (lookup rules 2–3), and real operators create them — the cluster that hit the original crash has a Maistra/service-mesh NAD with The kernel of truth in the concern: for delegation-style NADs the effective config lives on disk, where a platform scan can't see it. Failing these two rules wouldn't fix that blind spot, it would just mislabel it; if we want coverage there it'd be a separate (likely manual) control — the CIS VM-extension benchmark doesn't currently ask for it. Happy to file that as a follow-up if you think it's worth tracking. (Noted while testing: a NAD with malformed non-empty JSON config makes these rules report ERROR, since CEL's parseJSON has no safe variant. The object is genuinely broken at that point, but an SDK |
|
/retest-required |
|
/test e2e-aws-openshift-platform-compliance |
|
@Vincent056: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Description
kubevirt-localnet-vlan-requiredandkubevirt-bridge-mac-spoof-filteringguard onlyhas(n.spec.config). A NetworkAttachmentDefinition with nospecat all is legitimate (e.g. theistio-cniNAD), but it makeshas(n.spec.config)raiseno such key: specat evaluation time — and the scanner maps missing-key errors to FAIL, so both rules false-FAIL on any cluster carrying such a NAD. Observed during CIS OCP-Virt profile validation on a 4.22 + Virtualization cluster.Fix
Prepend
!has(n.spec) ||to both expressions, and add a spec-less NAD regression fixture to bothcel/tests/cases.yaml.Testing
Via
celctl(the Compliance Operator's scanner engine): the new fixture fails against the unfixed expressions (5/6 and 6/7 cases) and 6/6 / 7/7 cases pass with the fix.🤖 Generated with Claude Code