feat: honor install-time defaultFrameworks for posture scans - #398
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe change adds configurable default frameworks, normalizes configured names, and propagates them to startup, scheduled, direct, and security-exception scans. Explicit targets remain unchanged. Native frameworks remain the fallback when no defaults are configured. ChangesFramework selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OperatorConfig
participant ScanHandlers
participant ScanRequestBuilder
participant Kubescape
OperatorConfig->>ScanHandlers: DefaultFrameworks()
ScanHandlers->>ScanRequestBuilder: Pass configured frameworks
ScanRequestBuilder->>ScanRequestBuilder: Remove blank targets and apply fallback
ScanRequestBuilder->>Kubescape: Submit effective TargetNames
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 Warning |
Signed-off-by: manu <kiratcodes99@gmail.com>
d92d28a to
ef620a5
Compare
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed at ef620a5. Verified locally: go build ./... clean, go vet ./... clean, go test ./config/... ./mainhandler/... ./watcher/... all pass, and armoapi-go v0.0.720 does carry InstallationData.DefaultFrameworks (embedded into armometadata.ClusterConfig, so the clusterData JSON unmarshals into it). The direction is right and the legacy fallbacks are preserved as advertised.
But there are three blockers I'd want resolved before merge — the first one means the feature does not actually take effect on one of the request shapes this code already explicitly handles.
🔴 Blocker 1 — targetNames: [""] still scans all, ignoring defaultFrameworks
Inline on mainhandler/kubescapehandlerhelper.go. Reproduced with a throwaway test on this branch:
input : targetType=Framework, targetNames=[""], defaultFrameworks=["cis-eks-t1.2.0","nsa"]
output: targetNames=[all] <-- defaultFrameworks silently dropped
That [""] shape is not hypothetical — the repo has carried a test for it since before this PR (kubescapehandlerhelper_test.go, the targetNames: []string{""} block), which is exactly why the "" → "all" normalization loop exists. This PR edits that test but passes nil defaults, so the gap stays invisible. Details in the inline comment.
🔴 Blocker 2 — continuous scanning is silently narrowed, unmentioned and untested
continuousscanning/handlers.go (makeScanArgs) builds a PostScanRequest with only ScanObject/HostScanner set — no TargetType, no TargetNames. It dispatches TypeRunKubescape, so it lands in getKubescapeV1ScanRequest and hits the changed default path. Verified:
input : PostScanRequest{} (CS shape), defaultFrameworks=["cis-eks-t1.2.0"]
output: targetNames=[cis-eks-t1.2.0], targetType=Framework (was: [all])
So every per-resource continuous-scan result narrows from all to the Helm list the moment anyone sets defaultFrameworks. That may well be the desired product behavior, but it isn't in the PR description, it has no test, and CS results feed the compliance view — please make it an explicit, tested decision rather than a side effect. continuousscanning/handlers.go isn't in the diff so I couldn't anchor this inline.
🔴 Blocker 3 — nativeDefaultFrameworks duplicated across two packages
{"allcontrols", "nsa", "mitre"} is now declared twice, in mainhandler and in watcher, plus two byte-for-byte identical helpers under two different names (frameworksForFullClusterScan / frameworksOrNativeDefaults). The watcher copy is held together by a comment that says "matches mainhandler" — that's an invariant the compiler cannot enforce, and the two lists are meant to stay identical forever. Inline comments on both sites.
Should fix (non-blocking)
gofmtregression.gofmt -lis clean forconfig/config_test.goonmainand dirty on this branch: thetestsstruct fields are over-aligned and there's a trailing blank line at EOF. CI won't catch it (the sharedgo-basic-tests.yamlgolangci-lint step iscontinue-on-error: trueandgofmtisn't in the default linter set), so it needs a manualgofmt -w.- No empty-entry filtering in
DefaultFrameworks()— inline comment; Helm list rendering produces[""]more easily than you'd like. - No observability. Previously a bad framework name couldn't break anything, because the fallback was
all. Now a typo inpostureScan.defaultFrameworksyields a scan of a framework that doesn't exist, with nothing in the logs saying which list was used. One info-level log of the effective framework list (at startup, and where the default is applied) makes this diagnosable. targetTypeclobbering.targetType: Control+targetNames: []comes out astargetType=Framework, targetNames=<defaultFrameworks>(verified). The clobber is pre-existing — it used to produce[all]— but now it substitutes a narrower list into a request that explicitly asked for a different target kind. Worth gating the default onTargetType == "" || TargetType == KindFrameworkwhile you're in here.- Test gaps. Nothing covers
GetStartupActions,frameworksForFullClusterScan, or the cron path (getKubescapeRequest) with a non-emptydefaultFrameworks— the two functions that replaced the hardcoded native trio have no direct test. IConfigis exported, so addingDefaultFrameworks()breaks any out-of-tree implementer. Fine for this repo, just flagging it as an API change.
Nits
- The How to Test section of the PR description is empty.
docs/features/exists (seecel-admission-rules.md, added with its feature PR). A shortdocs/features/default-frameworks.mdcovering precedence — explicit request >defaultFrameworks> legacyall/native trio — would match that precedent and give the helm-charts side something to point at.
| // Explicit targetNames from the API / scanV1 payload take precedence over install-time defaults. | ||
| if len(postScanRequest.TargetNames) == 0 || postScanRequest.TargetType == "" { | ||
| setDefaultsKubescapeScanRequest(postScanRequest) | ||
| setDefaultsKubescapeScanRequest(postScanRequest, defaultFrameworks) |
There was a problem hiding this comment.
🔴 Blocker: defaultFrameworks is bypassed for targetNames: [""].
The guard on line 73 only calls setDefaultsKubescapeScanRequest when TargetNames is empty. A request carrying a single empty string — targetNames: [""] — is non-empty, so the defaults never run; execution falls through to the normalization loop on lines 77-81, which rewrites "" to the hardcoded "all".
Reproduced on this branch:
args := map[string]interface{}{
utils.KubescapeScanV1: map[string]interface{}{
"targetType": utilsapisv1.KindFramework,
"targetNames": []string{""},
},
}
req, _ := getKubescapeV1ScanRequest(args, []string{"cis-eks-t1.2.0", "nsa"})
// req.TargetNames == ["all"] <-- expected ["cis-eks-t1.2.0", "nsa"]This is a shape the code already knows about: the targetNames: []string{""} case has its own pre-existing test in kubescapehandlerhelper_test.go, and that normalization loop exists solely to service it. This PR updates that test's call site but passes nil for defaultFrameworks, so the miss isn't caught. Per the PR description the goal is that "posture scans that omit explicit targets use the Helm-configured framework list" — [""] omits an explicit target, so it should resolve to defaultFrameworks, not all.
Suggested fix — normalize before deciding, so blank entries are treated as absent:
// drop blank entries so [""] is treated as "no explicit target"
names := postScanRequest.TargetNames[:0]
for _, n := range postScanRequest.TargetNames {
if strings.TrimSpace(n) != "" {
names = append(names, n)
}
}
postScanRequest.TargetNames = names
if len(postScanRequest.TargetNames) == 0 || postScanRequest.TargetType == "" {
setDefaultsKubescapeScanRequest(postScanRequest, defaultFrameworks)
}and then extend the existing targetNames: [""] test with a non-nil defaultFrameworks so the behavior is pinned.
|
|
||
| // nativeDefaultFrameworks is used for startup / exception rescans when | ||
| // defaultFrameworks is not configured in clusterData. | ||
| var nativeDefaultFrameworks = []string{"allcontrols", "nsa", "mitre"} |
There was a problem hiding this comment.
🔴 Blocker: this list is duplicated in watcher/securityexceptionwatcher.go:382, and frameworksForFullClusterScan below is byte-for-byte identical to frameworksOrNativeDefaults there — same logic, two names, two packages. The watcher-side copy carries a comment saying it "matches mainhandler", which is an invariant the compiler can't enforce; change the trio here and the exception rescan silently diverges from the startup scan.
Please hoist both into one shared home — utils or config — e.g.:
// utils
var NativeDefaultFrameworks = []string{"allcontrols", "nsa", "mitre"}
// FrameworksOrDefault returns a copy of frameworks, or of fallback when empty.
func FrameworksOrDefault(frameworks, fallback []string) []string {
if len(frameworks) > 0 {
return slices.Clone(frameworks)
}
return slices.Clone(fallback)
}That also lets the "all" branch in setDefaultsKubescapeScanRequest use the same helper, so all three fallback sites read identically. slices.Clone from stdlib slices is a bit clearer than append([]string(nil), ...) here.
| } | ||
|
|
||
| // nativeDefaultFrameworks matches mainhandler when clusterData has no defaultFrameworks. | ||
| var nativeDefaultFrameworks = []string{"allcontrols", "nsa", "mitre"} |
There was a problem hiding this comment.
🔴 Blocker (other half of the duplication): this is a second copy of mainhandler/kubescapehandlerhelper.go:235, and frameworksOrNativeDefaults below duplicates frameworksForFullClusterScan exactly. The comment "matches mainhandler" documents a cross-package invariant that nothing checks — if someone edits the trio in one place, startup scans and exception rescans drift apart with no test failure.
See my comment on the mainhandler copy for the suggested shared helper. Once it lands, this becomes utils.FrameworksOrDefault(defaultFrameworks, utils.NativeDefaultFrameworks) at the single call site on line 401 and both the var and the function here can go away.
| return c.clusterConfig.ClusterName | ||
| } | ||
|
|
||
| func (c *OperatorConfig) DefaultFrameworks() []string { |
There was a problem hiding this comment.
The defensive copy is the right call 👍 — worth also dropping blank entries here, since this is the one chokepoint every consumer goes through.
Helm list rendering makes defaultFrameworks: [""] easy to produce by accident (e.g. {{ .Values.postureScan.defaultFrameworks }} over an unset-but-present value). Today that gets past the len() == 0 checks in setDefaultsKubescapeScanRequest and frameworksForFullClusterScan, so the operator treats a blank list as "the user configured frameworks" and posts targetNames: [""] — which then degrades to "all" via the normalization loop, i.e. the opposite of a narrowed scan, and with no log line explaining it.
func (c *OperatorConfig) DefaultFrameworks() []string {
out := make([]string, 0, len(c.clusterConfig.DefaultFrameworks))
for _, f := range c.clusterConfig.DefaultFrameworks {
if f = strings.TrimSpace(f); f != "" {
out = append(out, f)
}
}
if len(out) == 0 {
return nil
}
return out
}Also worth an info log of the resolved list on startup — with the hardcoded fallbacks gone, a typo'd framework name is now the difference between a real scan and an empty one, and right now nothing in the logs says which list was used.
| func TestDefaultFrameworks(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| frameworks []string |
There was a problem hiding this comment.
gofmt nit, but it is a regression: gofmt -l config/config_test.go is silent on main and reports this file on the branch. Two things — these struct fields are over-aligned (name/frameworks/want should align to the longest name, frameworks, i.e. one space after want), and there's a trailing blank line at EOF after the closing brace.
- name string
- frameworks []string
- want []string
+ name string
+ frameworks []string
+ want []string
gofmt -w config/config_test.go fixes both. Flagging it because CI won't: the shared go-basic-tests.yaml golangci-lint step is continue-on-error: true, and gofmt isn't in golangci-lint's default linter set anyway.
The test itself is good — asserting the returned slice is a copy is exactly the right thing to pin here.
Treat blank targetNames as absent, share native framework defaults via utils, and make continuous-scan inheritance of defaultFrameworks explicit and tested. Signed-off-by: manu <kiratcodes99@gmail.com>
|
Thanks for the review — addressed the three blockers plus the should-fixes. Ready for another look. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed at c57d75f. All three blockers are resolved, and I re-verified each one by probing the actual behavior on this branch rather than reading the diff. Nice work — the shared helper came out cleaner than what I sketched.
Blockers — verified fixed
1. targetNames: [""] — the blank-filter now runs before the default is applied, so the empty-string shape resolves correctly, and the legacy path is unchanged when no defaults are configured:
targetNames=[""] + defaults=[cis-eks-t1.2.0 nsa] -> [cis-eks-t1.2.0 nsa] ✅ (was [all])
targetNames=[""] + defaults=nil -> [all] ✅ unchanged
2. Continuous scanning — now an explicit, tested, documented decision rather than a side effect. Probed the real makeScanArgs shape: PostScanRequest{} + defaults → [cis-eks-t1.2.0]/Framework. The ## Continuous scanning section in the new doc says the quiet part out loud, which was the ask.
3. Duplication — utils.NativeDefaultFrameworks + utils.FrameworksOrDefault is now the single source, both packages call it, and the two divergent copies are gone. The slices.Clone aliasing test is a good touch.
Should-fix items — all addressed
gofmtclean onconfig/config_test.go, and you also fixed the pre-existing import-order drift inmain.go. The 3 filesgofmt -lstill reports (admission/rules/cel/evaluator.go,admission/ruleswatcher/watcher_test.go,utils/utils.go) are all dirty onmaintoo and untouched here — not yours.- Blank-entry filtering in
DefaultFrameworks(), with"blanks only"and"strips blanks"cases.TrimSpacehandles" ", which is the realistic Helm failure mode. - Startup log now carries both the raw and effective lists — that's exactly the diagnosability that was missing.
targetType: Controlgate, tested on both the direct and cron paths.- Tests added for
GetStartupActionsandgetKubescapeRequestwith non-empty defaults. docs/features/default-frameworks.md— the precedence list is the thing I'd want to read first. 👍
Verification
At c57d75f: go build ./... clean, go vet ./... clean, and go test green on config, mainhandler, mainhandler/remediators, watcher, utils, continuousscanning.
One follow-up, from my own suggestion (non-blocking)
The Control gate does what I asked for, but it has a consequence I under-thought when I proposed it — see the inline comment. Short version: targetType: Control + no names now forwards to Kubescape with an empty target list, where on main it became a full ["all"] scan. Silent no-op instead of a silent over-scan. A two-line addition to validateKubescapeScanRequest turns it into a loud error. Your call whether that belongs here or in a follow-up.
Nits (genuinely optional)
appendSecurityFrameworkstill appendssecurityto whatever the framework list ends up being, sodefaultFrameworks: ["nsa"]actually scans[nsa security](verified). That's pre-existing and almost certainly intended, but someone reading the new precedence doc will reasonably expect their configured list to be the exact list. One line under Precedence noting thatsecurityis always appended forFrameworkscans would close that gap.- Behavior change worth knowing you made: mixed input
["nsa", ""]used to expand to[nsa all](blank →all, so effectively a full scan) and is now just[nsa]. That reads as a bug fix to me, just noting it's a change onmainbeyond the[""]case.
Nothing here blocks merge from my side. I'm leaving this as a comment rather than an approval so a maintainer still owns the gate.
| return | ||
| } | ||
| // Do not clobber non-framework target kinds (e.g. Control with empty names). | ||
| if postScanRequest.TargetType != "" && postScanRequest.TargetType != utilsapisv1.KindFramework { |
There was a problem hiding this comment.
This is the gate I asked for and it does the right thing for the clobbering case — but following it through, it leaves a shape that now falls off the end silently. Flagging it because it's a consequence of my own suggestion, not something you got wrong.
Verified on this branch:
targetType=Control, targetNames=[] -> names=[], type=Control
targetType=Control, targetNames=[""] -> names=[], type=Control
and the cron path doesn't catch it either — getKubescapeRequest returns err=nil for that request, because validateKubescapeScanRequest only rejects the mirror case (len(TargetNames) > 0 && TargetType == "").
So a Control request with no names is now POSTed to Kubescape with an empty target list. On main that same request became ["all"] + Framework — a full scan. Both behaviors are wrong; the difference is that the old one was a silent over-scan and the new one is a silent no-op, which is harder to notice from the outside because nothing errors and nothing logs.
Since the gate already identifies the state precisely, validation can just reject it and stay symmetric with the check that's already there:
func validateKubescapeScanRequest(postScanRequest *utilsmetav1.PostScanRequest) error {
if len(postScanRequest.TargetNames) > 0 && postScanRequest.TargetType == "" {
return fmt.Errorf("received targetNames but not target types")
}
if len(postScanRequest.TargetNames) == 0 && postScanRequest.TargetType != "" &&
postScanRequest.TargetType != utilsapisv1.KindFramework {
return fmt.Errorf("received targetType %q but no targetNames", postScanRequest.TargetType)
}
return nil
}Note that only covers the cron path, since kubescapeScan doesn't call validateKubescapeScanRequest — a warning log next to the early return here would cover the rest. Fine as a follow-up if you'd rather keep this PR focused; the existing test you added pins the current behavior either way.
2 participants
Overview
This PR adds support for install-time
defaultFrameworksfromclusterData, so posture scans that omit explicit targets use the Helm-configured framework list instead of hardcoded /"all"defaults.Fixes kubescape/helm-charts#566
Signed Commits
Additional Information
InstallationDatain armoapi-go (DefaultFrameworks []string).defaultFrameworksintoks-cloud-config/clusterData."all"for empty scanV1, native trio for startup/exception rescans).How to Test
Summary by CodeRabbit
New Features
Documentation