Extracting common action framework functinalities in operator-actions…#3575
Open
valdar wants to merge 1 commit into
Open
Extracting common action framework functinalities in operator-actions…#3575valdar wants to merge 1 commit into
valdar wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3575 +/- ##
==========================================
- Coverage 55.86% 53.31% -2.56%
==========================================
Files 217 197 -20
Lines 16340 13762 -2578
==========================================
- Hits 9129 7337 -1792
+ Misses 6363 5780 -583
+ Partials 848 645 -203 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
https://github.com/opendatahub-io/odh-platform-utilities/pull/15/changes Addresses some of these discrepancies.
the pr above will lead to a couple of changes in the odh operator
|
3 tasks
valdar
force-pushed
the
operatorUtils
branch
3 times, most recently
from
191d374 to
e19efa6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/controller/modules/modules_controller_actions_inject_env.go (1)
93-104:⚠️ Potential issue | 🟠 MajorMajor supply chain risk: unvalidated image injection via
ControllerImageenv override
internal/controller/modules/modules_controller_actions_inject_env.gooverwrites the module Deployment image withcontainer["image"] = os.Getenv(mi.ControllerImage)without parsing/validating the image reference (no@sha256:-only enforcement, no registry allowlist). If the platform/operator process environment is tampered with, an attacker can inject arbitrary container images into managed Deployments (CWE-829).CI
.github/scripts/validate-related-images.shchecks thatRELATED_IMAGE_*identifiers exist, but does not validate the corresponding env values are digest-pinned/allowlisted.Remediate by validating
imgbefore assignment (digest-only + allowlist) and failing/ignoring on mismatch, e.g.:img := os.Getenv(mi.ControllerImage) if !regexp.MustCompile(`^[^@]+@sha256:[0-9a-f]{64}$`).MatchString(img) || !allowedRegistry(img) { return fmt.Errorf("invalid ControllerImage override: %q", img) } container["image"] = img🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/modules/modules_controller_actions_inject_env.go` around lines 93 - 104, The code currently assigns an unchecked env image (mi.ControllerImage -> img) into container["image"], creating a supply-chain risk; update the injection logic inside the block that handles mi.ControllerImage to validate the env value before assigning: read img := os.Getenv(mi.ControllerImage), verify it matches a digest-only pattern (e.g. host/repo@sha256:<64 hex>) and that its registry is allowlisted via a helper like allowedRegistry(img) (or implement that helper), and if validation fails either log and return an error or skip injection instead of assigning; only after passing both checks set container["image"] = img and increment injected and log the override. Ensure references remain to mi.ControllerImage, container["image"], injected and the new allowedRegistry/validation helper so reviewers can locate the changes.Dockerfiles/Dockerfile (1)
72-72:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPin the runtime base image (CWE-829).
:latesttag allows the base image to change between builds without warning, breaking reproducibility and introducing supply chain risk. An attacker who compromises the registry or the :latest tag injects malicious code into production deployments.Replace
:latestwith a specific digest or semantic version tag.🔒 Proposed fix to pin the image
-FROM --platform=$TARGETPLATFORM registry.access.redhat.com/ubi9/ubi-minimal:latest +FROM --platform=$TARGETPLATFORM registry.access.redhat.com/ubi9/ubi-minimal:9.5-1738127074🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfiles/Dockerfile` at line 72, The FROM instruction uses an unpinned image "registry.access.redhat.com/ubi9/ubi-minimal:latest"; replace the :latest tag with a specific semantic version tag or immutable digest to ensure reproducible builds and reduce supply-chain risk—update the FROM line to reference the chosen pinned identifier for "registry.access.redhat.com/ubi9/ubi-minimal" (e.g., a stable version tag or `@sha256`:<digest>) and commit that change so future builds use the exact same base image.Source: Coding guidelines
internal/controller/modules/modules_controller_actions_inject_env_test.go (1)
318-391:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncomplete migration breaks compilation (CWE-710).
Lines 327, 349, and 374 still use direct field assignment
ModuleEnvInjection: &odhtype.ModuleEnvInjection{...}while the struct field was removed. Earlier tests (lines 152, 194, 222, 278, 307) were migrated toodhtype.SetModuleEnvInjection(rr, ...)but these three were missed.🔧 Proposed fix
func TestInjectControllerImage(t *testing.T) { g := NewWithT(t) t.Setenv("RELATED_IMAGE_MODULE_CONTROLLER", "registry.example.com/module-ctrl@sha256:abc123") dep := makeDeployment("module-operator") rr := &odhtype.ReconciliationRequest{ Resources: []unstructured.Unstructured{dep}, - ModuleEnvInjection: &odhtype.ModuleEnvInjection{ - PerModuleImages: []odhtype.ModuleImages{{ - DeploymentName: "module-operator", - ControllerImage: "RELATED_IMAGE_MODULE_CONTROLLER", - }}, - }, } + odhtype.SetModuleEnvInjection(rr, &odhtype.ModuleEnvInjection{ + PerModuleImages: []odhtype.ModuleImages{{ + DeploymentName: "module-operator", + ControllerImage: "RELATED_IMAGE_MODULE_CONTROLLER", + }}, + }) err := injectModuleEnv(context.Background(), rr) g.Expect(err).ShouldNot(HaveOccurred()) img := getContainerImage(&rr.Resources[0], "manager") g.Expect(img).Should(Equal("registry.example.com/module-ctrl@sha256:abc123")) } func TestInjectControllerImageNotSet(t *testing.T) { g := NewWithT(t) dep := makeDeployment("module-operator") rr := &odhtype.ReconciliationRequest{ Resources: []unstructured.Unstructured{dep}, - ModuleEnvInjection: &odhtype.ModuleEnvInjection{ - PerModuleImages: []odhtype.ModuleImages{{ - DeploymentName: "module-operator", - ControllerImage: "RELATED_IMAGE_NOT_SET", - }}, - }, } + odhtype.SetModuleEnvInjection(rr, &odhtype.ModuleEnvInjection{ + PerModuleImages: []odhtype.ModuleImages{{ + DeploymentName: "module-operator", + ControllerImage: "RELATED_IMAGE_NOT_SET", + }}, + }) err := injectModuleEnv(context.Background(), rr) g.Expect(err).ShouldNot(HaveOccurred()) img := getContainerImage(&rr.Resources[0], "manager") g.Expect(img).Should(Equal("registry.example.com/module:latest")) } func TestInjectControllerImageWithRelatedImages(t *testing.T) { g := NewWithT(t) t.Setenv("RELATED_IMAGE_MODULE_CONTROLLER", "registry.example.com/module-ctrl@sha256:abc123") t.Setenv("RELATED_IMAGE_SIDECAR", "registry.example.com/sidecar@sha256:def456") dep := makeDeployment("module-operator") rr := &odhtype.ReconciliationRequest{ Resources: []unstructured.Unstructured{dep}, - ModuleEnvInjection: &odhtype.ModuleEnvInjection{ - PerModuleImages: []odhtype.ModuleImages{{ - DeploymentName: "module-operator", - ControllerImage: "RELATED_IMAGE_MODULE_CONTROLLER", - Images: []string{"RELATED_IMAGE_SIDECAR"}, - }}, - }, } + odhtype.SetModuleEnvInjection(rr, &odhtype.ModuleEnvInjection{ + PerModuleImages: []odhtype.ModuleImages{{ + DeploymentName: "module-operator", + ControllerImage: "RELATED_IMAGE_MODULE_CONTROLLER", + Images: []string{"RELATED_IMAGE_SIDECAR"}, + }}, + }) err := injectModuleEnv(context.Background(), rr) g.Expect(err).ShouldNot(HaveOccurred())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/modules/modules_controller_actions_inject_env_test.go` around lines 318 - 391, Tests TestInjectControllerImage, TestInjectControllerImageNotSet, and TestInjectControllerImageWithRelatedImages still assign the removed struct field directly using ModuleEnvInjection: &odhtype.ModuleEnvInjection{...}; update each test to call odhtype.SetModuleEnvInjection(rr, &odhtype.ModuleEnvInjection{ PerModuleImages: []odhtype.ModuleImages{...} }) instead, keeping the same PerModuleImages/DeploymentName/ControllerImage/Images values so injectModuleEnv receives the injection data the same way as the earlier migrated tests.Source: Pipeline failures
🟠 Major comments (19)
operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go-18-23 (1)
18-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError handling is inverted and can silently ignore malformed object structure (CWE-703).
NestedFieldNoCopyerrors should be returned unconditionally; gating onokdrops real failures.Suggested fix
sc, ok, err := unstructured.NestedFieldNoCopy(source.Object, containersPath...) - if err != nil && ok { + if err != nil { return err } tc, ok, err := unstructured.NestedFieldNoCopy(target.Object, containersPath...) - if err != nil && ok { + if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go` around lines 18 - 23, The current error handling around unstructured.NestedFieldNoCopy in the merge deployment logic is inverted: do not gate returning err on ok; instead, if err != nil return err immediately, then check ok to handle the missing field case. Update both calls that use unstructured.NestedFieldNoCopy (the one assigning tc and the earlier call) to first if err != nil { return err } and only afterwards use if !ok { /* handle absent field or continue */ }, referencing variables/identifiers containersPath, target.Object, tc, ok, and err so the fix is applied to each occurrence.operator-actions-framework/controller/predicates/hash/hash.go-14-19 (1)
14-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNil
UpdateEventobjects can panic the predicate path (CWE-476).
DeepCopyObject()is called without checkinge.ObjectOld/e.ObjectNew; nil input can crash the controller worker.Suggested fix
UpdateFunc: func(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + return true + } oldUnstructured, err := resources.ToUnstructured(e.ObjectOld.DeepCopyObject()) if err != nil { return true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/predicates/hash/hash.go` around lines 14 - 19, The predicate calls DeepCopyObject() on e.ObjectOld and e.ObjectNew without nil checks, which can panic; before calling resources.ToUnstructured(e.ObjectOld.DeepCopyObject()) and resources.ToUnstructured(e.ObjectNew.DeepCopyObject()) check if e.ObjectOld or e.ObjectNew is nil and handle that case (e.g., return true as done on other errors) to avoid a nil dereference. Ensure you reference and guard the exact symbols e.ObjectOld, e.ObjectNew, DeepCopyObject, and the subsequent resources.ToUnstructured calls so the predicate path never invokes DeepCopyObject on a nil object.cmd/health-check/go.mod-7-8 (1)
7-8:⚠️ Potential issue | 🟠 MajorFix unsupported controller-runtime ↔ k8s.io/ version matrix in cmd/health-check/go.mod (CWE-1104)*
cmd/health-check/go.mod pins
sigs.k8s.io/controller-runtime v0.22.4while also pinningk8s.io/apimachinery v0.35.3andk8s.io/client-go v0.35.3(and indirectk8s.io/api v0.35.3). controller-runtime v0.22.x is intended/tested withk8s.io/* v0.34.x, whilek8s.io/* v0.35.xaligns with controller-runtime v0.23.x—this unsupported mix can break builds or runtime behavior.Remediate by aligning versions: bump
controller-runtimetov0.23.3(matches the repo’s other go.mod files withk8s.io/* v0.35.3), or downgradek8s.io/*tov0.34.xto matchcontroller-runtime v0.22.x.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/health-check/go.mod` around lines 7 - 8, The go.mod pins an incompatible mix: sigs.k8s.io/controller-runtime v0.22.4 with k8s.io/* v0.35.3; update the controller-runtime requirement to v0.23.3 to match k8s.io/apimachinery and k8s.io/client-go v0.35.3 (look for the require line for sigs.k8s.io/controller-runtime and change the version to v0.23.3), then run go mod tidy / regenerate go.sum to ensure dependency graph is consistent; alternatively, if you prefer keeping controller-runtime v0.22.x, downgrade the k8s.io/* requires to a v0.34.x series instead.Source: Coding guidelines
cmd/mcp-server/go.mod-9-13 (1)
9-13:⚠️ Potential issue | 🟠 MajorBump controller-runtime to a version that matches k8s.io v0.35.x (or downgrade k8s.io to v0.34.x); the current pairing is unsupported (CWE-1104).
cmd/mcp-server/go.modpinsk8s.io/{api,apimachinery,client-go} v0.35.3withsigs.k8s.io/controller-runtime v0.22.4, butcontroller-runtime v0.22.4expectsk8s.io/*to be pinned tov0.34.1—sov0.35.3isn’t the tested/supported matrix.- Remediation: either update
sigs.k8s.io/controller-runtimeto thev0.23.xline that matchesk8s.io/* v0.35, or downgradek8s.io/*tov0.34.x(per the controller-runtimev0.22.4go.mod).- No
toolchaindirective;replacedirectives are only local../../pkg/...paths (not external/fork steering).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/mcp-server/go.mod` around lines 9 - 13, The go.mod currently pins k8s.io/{api,apimachinery,client-go} v0.35.3 while using sigs.k8s.io/controller-runtime v0.22.4, which is an unsupported matrix; update the controller-runtime to a v0.23.x release that is compatible with k8s.io v0.35 (e.g., change sigs.k8s.io/controller-runtime v0.22.4 -> v0.23.x), then run go get ./... and go mod tidy to resolve modules and verify builds/tests; alternatively, if you prefer downgrading k8s.io, change k8s.io/{api,apimachinery,client-go} to a v0.34.x line to match controller-runtime v0.22.4, run go get and go mod tidy, and confirm compilation.Source: Coding guidelines
operator-actions-framework/controller/actions/errors/errors.go-15-17 (1)
15-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
reasoninNewStopErrorWto prevent nil-deref panic (CWE-476).
NewStopErrorW(nil)createsStopError{nil}; laterError()at Line 12 panics one.reason.Error().Proposed fix
package errors import ( "fmt" ) type StopError struct { reason error } func (e StopError) Error() string { + if e.reason == nil { + return "stop error: unspecified reason" + } return e.reason.Error() } func NewStopErrorW(reason error) StopError { + if reason == nil { + reason = fmt.Errorf("stop error: unspecified reason") + } return StopError{reason} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/errors/errors.go` around lines 15 - 17, NewStopErrorW currently allows a nil reason which causes a nil-deref in StopError.Error() when it calls e.reason.Error(); update NewStopErrorW to validate the incoming reason and never store nil in StopError: if reason is nil, substitute a non-nil sentinel error (e.g., errors.New("nil stop reason") or a package-level sentinel) before returning StopError{reason}. Ensure you import/use the standard errors package and keep the change localized to NewStopErrorW so StopError and its Error() method remain unchanged.operator-actions-framework/controller/actions/status/deployments/action_deployments_available.go-125-134 (1)
125-134:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUnbounded List() risks memory exhaustion (CWE-400).
The deployment list query has no result limit or pagination. A label selector matching thousands of deployments will load all into memory. An attacker who can create many matching deployments, or a misconfigured selector, exhausts the controller's memory.
Set
client.Limit(...)or document the expected upper bound and validate it at runtime.🛡️ Proposed fix to add limit
err = rr.Client.List( ctx, deployments, client.InNamespace(ns), client.MatchingLabels(l), + client.Limit(1000), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/status/deployments/action_deployments_available.go` around lines 125 - 134, The rr.Client.List call in action_deployments_available.go (calling rr.Client.List with deployments, client.InNamespace(ns), client.MatchingLabels(l)) is unbounded and can exhaust memory; fix by adding a safety limit and paginating: use client.Limit(<safeMax>) on the List call and loop handling ListOptions.Continue (or repeat List with updated ListOptions) to fetch pages until Continue is empty, accumulating results into deployments but aborting with an error if the total exceeds your configured upper bound; alternatively validate/require a reasonable max before listing and return an error if the selector could match too many items. Ensure to reference and update the same rr.Client.List invocation and the deployments variable and include client.Limit and ListOptions.Continue handling.Source: Coding guidelines
operator-actions-framework/controller/handlers/handlers.go-13-31 (1)
13-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate label value before using as Kubernetes name.
Label values are not constrained to DNS-1123 subdomain format. Enqueueing a request with an invalid name silently breaks reconciliation. Validate
nameagainstk8s.io/apimachinery/pkg/util/validation.IsDNS1123Subdomainbefore constructing the request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/handlers/handlers.go` around lines 13 - 31, LabelToName currently uses the label value directly as a Kubernetes name; validate the label value using k8s.io/apimachinery/pkg/util/validation.IsDNS1123Subdomain before constructing the reconcile.Request. Import the validation package, call errs := validation.IsDNS1123Subdomain(name) after reading name, and if len(errs) != 0 return an empty []reconcile.Request (i.e., do not create the types.NamespacedName) so only DNS-1123-compliant names are enqueued.operator-actions-framework/controller/handlers/handlers.go-33-51 (1)
33-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate annotation value before using as Kubernetes name.
Same issue as
LabelToName: annotation values are unconstrained strings. Validate against DNS-1123 before enqueueing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/handlers/handlers.go` around lines 33 - 51, AnnotationToName currently trusts raw annotation values as Kubernetes names; validate the extracted value against DNS-1123 like LabelToName does. Update AnnotationToName to call the Kubernetes DNS-1123 validation (e.g., validation.IsDNS1123Subdomain or the same helper used by LabelToName) on the variable name returned from obj.GetAnnotations()[key], and only enqueue a reconcile.Request when validation passes; return an empty slice for missing or invalid values. Ensure you add the same import used by LabelToName for the validation helper and keep the function signature and returned reconcile.Request structure unchanged.operator-actions-framework/controller/predicates/dependent/dependent.go-88-96 (1)
88-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwallowed conversion errors mask security-critical failures (CWE-754).
resources.ToUnstructurederrors returnfalse, suppressing reconciliation. A corrupted object or scheme registration failure leaves the resource unreconciled, potentially allowing stale/vulnerable state. Log the error at minimum; consider whether to panic or returntrueto force reconciliation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/predicates/dependent/dependent.go` around lines 88 - 96, The current conversion of e.ObjectOld and e.ObjectNew via resources.ToUnstructured silently returns false on error, hiding critical failures; update the error handling around resources.ToUnstructured (the oldObj and newObj conversion block) to log the returned error (use the controller/runtime logger or process logger available) and then return true (or requeue) to force reconciliation instead of silently dropping it; ensure the log includes context (operation/predicate name and which object failed) so corrupted objects or scheme registration failures are visible for alerting and debugging.Source: Coding guidelines
operator-actions-framework/controller/conditions/conditions.go-47-53 (1)
47-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCWE-532: Insertion of Sensitive Information into Log File
err.Error()may contain tokens, passwords, or internal paths if callers pass through authentication/API errors. Condition messages are user-visible viakubectl describe. Consider sanitizing or wrapping errors before passing toWithError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/conditions/conditions.go` around lines 47 - 53, WithError currently sets api.Condition.Message directly to err.Error(), which can leak sensitive tokens/paths; change WithError (and any callers that construct api.Condition) to never store raw err.Error() in api.Condition.Message — instead create and call a sanitizer/wrapping helper (e.g., sanitizeError(err error) string) that strips/redacts secrets, trims stack/path details and returns a short user-safe message (or a generic "operation failed" plus non-sensitive error class or code); update WithError to set c.Message = sanitizeError(err) and optionally preserve the original error in logs or an internal field rather than the user-visible api.Condition.Message.Source: Coding guidelines
pkg/cluster/resources.go-371-377 (1)
371-377:⚠️ Potential issue | 🟠 MajorSupply-chain trust expansion: operator-actions-framework is the critical control plane for CRD discovery, deletion, and manifest ordering (CWE-829).
Critical operations are delegated via operator-actions-framework aliases/wrappers (
pkg/cluster/resources.go,pkg/controller/actions/deleteresource/action_delete_resources.go→ DeleteAllOf,pkg/resources/sort.go). The build usesreplace github.com/opendatahub-io/operator-actions-framework => ./operator-actions-frameworkand pins it tov0.0.0-00010101000000-000000000000, but this vendored snapshot contains no obviousSECURITY/CVE/SBOM/provenance/attestation (or dependabot/renovate) artifacts—so the upstream security posture for the exact code here is not evidenced.Require for the exact vendored revision:
- Evidence of security audit/review for the delegated operations (CRD discovery/status handling, deletion safety, ordering/post-processing).
- Documented CVE monitoring + patch process with SLA/owners.
- Immutable identification (real upstream tag/commit SHA) and provenance/SBOM attestations for the snapshot (not a mutable branch; no “0000…” placeholder pin).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cluster/resources.go` around lines 371 - 377, The vendored operator-actions-framework snapshot is pinned to a placeholder revision with no provenance or security artifacts; update the repository pin and documentation: replace the local replace directive with an immutable upstream tag or full commit SHA (not the "0000..." pseudo-version), and add provenance artifacts (SBOM, attestations) for that exact revision; also add a SECURITY or SECURITY.md describing CVE monitoring, patching SLA and owners, and link to any security/audit reports covering delegated operations referenced by GetCRD, HasCRD, IsAPIAvailable, HasCRDWithVersion, ListGVK and the DeleteAllOf / resources/sort.go code paths so reviewers can verify safe CRD discovery/deletion/ordering behavior.operator-actions-framework/Makefile-81-91 (1)
81-91:⚠️ Potential issue | 🟠 Majoroperator-actions-framework/Makefile: Add explicit integrity verification for
go-install-tooldownloads (CWE-829).
go-install-tool(lines 81-91) builds tool executables viago install $(2)@$(3)in a temp module and does not set/force Go integrity settings (e.g.,GOSUMDB/GOPROXY) nor verify the produced binary against any release-provided checksum/signature. A golangci-lint release includes checksum assets (e.g.,golangci-lint-2.12.2-checksums.txt), but the Makefile doesn’t use them.Fix: enforce
GOSUMDB/GOPROXYfor the recipe (and ensure they can’t be overridden to weaken verification) and/or download the official tool release binary for the pinned version and verify it against the corresponding release checksum before installing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/Makefile` around lines 81 - 91, The go-install-tool recipe currently runs go install $(2)@$(3) without enforcing Go module integrity or verifying the delivered binary; update the go-install-tool definition to (1) force secure Go verification by setting and exporting immutable GOSUMDB and GOPROXY values inside the recipe invocation (e.g., assign them inline for the install command so they cannot be overridden) and ensure GOBIN=$(LOCALBIN) remains used, and (2) for tools that publish release artifacts (like golangci-lint) prefer downloading the official tarball/zip for the pinned version and the accompanying checksum file, verify the archive signature/checksum before extracting/installing into $(LOCALBIN), failing the build on mismatch; reference the go-install-tool target, the use of GOBIN=$(LOCALBIN) and the temporary TMP_DIR creation to locate where to apply these changes.operator-actions-framework/controller/predicates/resources/resources.go-62-64 (1)
62-64:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMissing status field incorrectly treated as valid deployment.
When
unstructured.NestedMapreturns!foundorerr != nil, line 64 returns(0, 0, true), signaling "valid Deployment with zero replicas." The caller at lines 43-45 will then compare0 == 0and suppress the update event. This conflates a missing.statusfield (which should trigger reconciliation) with a genuinely zero-replica deployment (which should not). Return(0, 0, false)when!found || err != nil.Proposed fix
func getDeploymentStatus(obj client.Object) (int32, int32, bool) { if deploy, isTyped := obj.(*appsv1.Deployment); isTyped { return deploy.Status.Replicas, deploy.Status.ReadyReplicas, true } u, ok := obj.(*unstructured.Unstructured) if !ok || !hasGVK(u, gvkDeployment) { return 0, 0, false } status, found, err := unstructured.NestedMap(u.Object, "status") if err != nil || !found { - return 0, 0, true + return 0, 0, false } return getInt32Field(status, "replicas"), getInt32Field(status, "readyReplicas"), true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/predicates/resources/resources.go` around lines 62 - 64, The code treats a missing or errored status as a valid zero-replica deployment by returning (0, 0, true); update the error branch after calling unstructured.NestedMap(u.Object, "status") so that when err != nil || !found the function returns (0, 0, false) instead, ensuring the caller (which compares replica counts) treats a missing/errored .status as an invalid/missing status that should trigger reconciliation; adjust the return in the status, found, err handling accordingly in resources.go (the block handling status, found, err).operator-actions-framework/controller/types/types.go-200-212 (1)
200-212:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
context.TODO()in Hash can block reconciliation indefinitely.Line 201 calls
Values(context.TODO())to hash Helm chart values. IfValues()performs network I/O (e.g., fetching from an OCI registry or validating remote schemas),context.TODO()provides no timeout, and the reconciliation loop can hang. Pass the caller'sctxthroughHash()and propagate it toValues(), or usecontext.WithTimeout()with a bounded deadline.Proposed fix
Change the signature to accept a context:
-func Hash(rr *ReconciliationRequest) ([]byte, error) { +func Hash(ctx context.Context, rr *ReconciliationRequest) ([]byte, error) { hash := sha256.New() ... for i := range rr.HelmCharts { ... if rr.HelmCharts[i].Values != nil { - values, err := rr.HelmCharts[i].Values(context.TODO()) + values, err := rr.HelmCharts[i].Values(ctx) if err != nil { return nil, fmt.Errorf("failed to get helm chart values: %w", err) } ... } } ... } -func HashStr(rr *ReconciliationRequest) (string, error) { - h, err := Hash(rr) +func HashStr(ctx context.Context, rr *ReconciliationRequest) (string, error) { + h, err := Hash(ctx, rr) if err != nil { return "", err } return resources.EncodeToString(h), nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/types/types.go` around lines 200 - 212, The code calls rr.HelmCharts[i].Values(context.TODO()) inside Hash, which can block reconciliation; update the Hash function signature to accept a context.Context (e.g., Hash(ctx context.Context, ...)) and propagate that ctx into rr.HelmCharts[i].Values(ctx). If you need a bounded deadline instead, create a derived context with context.WithTimeout(ctx, timeout) inside Hash and pass that to Values, ensuring you cancel the derived context; update all callers of Hash (and related helpers) to pass the caller's ctx so no caller uses context.TODO().operator-actions-framework/controller/actions/gc/action_gc.go-212-233 (1)
212-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGC lists cluster-wide but RBAC-checks only a single namespace.
Line 218 always lists resources with
Namespace("")(cluster-wide), while line 199–204 usesnamespaceFnonly to authorize the delete verb in that namespace. IfnamespaceFnreturns"foo", the controller performs a cluster-wide list (returning resources in all namespaces) but only checks RBAC for namespace"foo". This over-fetches and violates least privilege: resources outside the target namespace are visible in the list result. Scope the list to the namespace returned bynamespaceFnif the resource is namespaced.Proposed fix
func (a *Action) listResources( ctx context.Context, dc dynamic.Interface, + ns string, res resources.Resource, opts metav1.ListOptions, ) ([]unstructured.Unstructured, error) { - items, err := dc.Resource(res.GroupVersionResource()).Namespace("").List(ctx, opts) + targetNS := "" + if res.Namespaced { + targetNS = ns + } + items, err := dc.Resource(res.GroupVersionResource()).Namespace(targetNS).List(ctx, opts) switch { case k8serr.IsForbidden(err) || k8serr.IsMethodNotSupported(err) || k8serr.IsNotFound(err): ... } }Then at line 175, pass
nsfrom line 199 tolistResources:+ ns, err := a.namespaceFn(ctx, rr) + if err != nil { + return fmt.Errorf("unable to compute namespace: %w", err) + } + for _, res := range items { ... - items, err := a.listResources(ctx, rr.Controller.GetDynamicClient(), res, lo) + items, err := a.listResources(ctx, rr.Controller.GetDynamicClient(), ns, res, lo) ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/gc/action_gc.go` around lines 212 - 233, The code lists resources cluster-wide in Action.listResources (Namespace("")) while RBAC checks use namespaceFn; change listResources to accept a namespace parameter (e.g., ns string) and when calling Namespace(...) use ns for namespaced resources and "" for cluster-scoped ones; update callers (where namespaceFn is invoked, e.g., the GC flow around lines with namespaceFn) to obtain ns and pass it into listResources; inside listResources use res.IsNamespaced() (or equivalent on resources.Resource) to decide between Namespace(ns) and Namespace("") so listings are properly scoped to the target namespace.operator-actions-framework/controller/reconciler/reconciler.go-413-413 (1)
413-413:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCondition reset loses state.
Conditions.Reset()unconditionally clears all conditions before running actions. If an action fails early orpreApplyFnreturns true, only the provisioning condition is re-set. Other conditions (e.g., dependency readiness, certificate validity) remain cleared until the next successful reconciliation, hiding important cluster state from users and breaking status contract.CWE-755 (Improper Handling of Exceptional Conditions).
Selective reset approach
- rr.Conditions.Reset() + // Only reset conditions that will be recomputed in this reconciliation. + // Preserve dependency/external conditions that actions may not touch. + rr.Conditions.ResetTransient()Or verify that
CleanupStaleConditions(line 461) preserves unset conditions when appropriate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/reconciler/reconciler.go` at line 413, rr.Conditions.Reset() currently wipes all condition state and should be changed to avoid losing non-provisioning conditions; modify the reconciliation start so only the provisioning-related condition(s) are reset (or selectively reset conditions that are known to be transient for this reconciliation) instead of calling rr.Conditions.Reset() wholesale, and ensure CleanupStaleConditions (and any call sites) preserves existing dependency/certificate readiness conditions when an action fails early or preApplyFn returns true; locate the reset call and replace it with a targeted reset/update of the provisioning condition(s) or implement a selective ResetExcept(...) helper used here.operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go-99-102 (1)
99-102:⚠️ Potential issue | 🟠 MajorPass reconciliation
ctxintoengine.Renderto prevent reconciliation hangs (CWE-400). Inoperator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go:99-102,Action.render(ctx, ...)callsa.engine.Render(...)without usingctx, so a blocking/hanging render (e.g., stalled file/network access inside the engine) won’t be cancellable via the reconciliation context, enabling uncontrolled resource consumption / DoS (CWE-400).
Fix: extend theEngineinterface to acceptcontext.Context(e.g.,Render(ctx context.Context, path string, opts ...RenderOpt)) and calla.engine.Render(ctx, rr.Manifests[i].String(), perManifestOpts...)so implementations can honor cancellation/timeouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go` around lines 99 - 102, The engine.Render call in Action.render is not context-aware; update the Engine interface to change Render to accept a context.Context (e.g., Render(ctx context.Context, path string, opts ...RenderOpt)) and update all Engine implementations accordingly, then change the call in Action.render from a.engine.Render(rr.Manifests[i].String(), perManifestOpts...) to a.engine.Render(ctx, rr.Manifests[i].String(), perManifestOpts...) so render operations can be cancelled via the reconciliation context.operator-actions-framework/resources/resources.go-144-160 (1)
144-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
HasLabelrequires value arguments to return true, breaking pure presence checks.If called as
HasLabel(obj, "my-label")with no values,slices.Contains([]string{}, val)returnsfalseregardless of whether the label exists. This contradicts the function name implying presence-only checking.Either rename to
HasLabelWithValueor handle the empty-values case:🐛 Fix for presence-only semantics
func HasLabel(obj client.Object, k string, values ...string) bool { if obj == nil { return false } target := obj.GetLabels() if target == nil { return false } val, found := target[k] if !found { return false } + // No values specified = presence-only check + if len(values) == 0 { + return true + } + return slices.Contains(values, val) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/resources/resources.go` around lines 144 - 160, The HasLabel function currently returns false when called with no value args (e.g., HasLabel(obj, "my-label")) because it always checks slices.Contains(values, val); change HasLabel in resources.go so that when obj.GetLabels() contains the key and len(values)==0 it returns true (presence-only), otherwise keep the existing behavior of checking that val is one of values; update the function comment/name only if you prefer to rename to HasLabelWithValue but the minimal fix is to add the empty-values presence branch in HasLabel.operator-actions-framework/resources/resources.go-207-223 (1)
207-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame logic issue in
HasAnnotation- empty values always returns false.Identical problem to
HasLabel. CallingHasAnnotation(obj, "key")without values returns false even when the annotation exists.🐛 Fix
func HasAnnotation(obj client.Object, k string, values ...string) bool { ... val, found := target[k] if !found { return false } + if len(values) == 0 { + return true + } + return slices.Contains(values, val) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/resources/resources.go` around lines 207 - 223, HasAnnotation currently returns false when no values are passed even if the annotation key exists; update the logic in HasAnnotation to return true when the annotation key is present and the variadic values slice is empty, otherwise check that the annotation value matches one of the provided values (i.e., if len(values) == 0 return true, else return slices.Contains(values, val)). Keep existing nil checks for obj and annotations and only change the final return logic in HasAnnotation.
🟡 Minor comments (4)
api/common/types.go-82-93 (1)
82-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
PlatformObjectalias may not satisfyWithReleasesconstraint.Per PR comments,
fwapi.PlatformObjectlacksWithReleases. Any downstream code assertingcommon.PlatformObjectimplementsWithReleaseswill fail at compile time. The PR comments acknowledge this requires follow-up changes in the operator.Given WIP/hold labels, flagging for tracking only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/common/types.go` around lines 82 - 93, PlatformObject alias currently points to fwapi.PlatformObject which does not implement WithReleases; update the type so downstream code can rely on PlatformObject satisfying WithReleases. Replace the alias "type PlatformObject = fwapi.PlatformObject" with a new interface (or type) named PlatformObject that embeds fwapi.PlatformObject and the WithReleases methods (or embed the WithReleases interface), ensuring it declares GetReleaseStatus() *[]ComponentRelease and SetReleaseStatus(status []ComponentRelease) so code that asserts PlatformObject implements WithReleases will compile; update any implementations or add adapter wrappers in the operator to satisfy the combined interface.operator-actions-framework/controller/actions/deploy/action_deploy.go-650-652 (1)
650-652:⚠️ Potential issue | 🟡 MinorHarden unstructured merge helpers to avoid reconciler panics (CWE-248/CWE-20).
operator-actions-framework/controller/actions/deploy/action_deploy.goroutes toRevertManagedDeploymentDrift,MergeDeployments,MergeObservabilityResources, andRemoveDeploymentsResources; these helpers perform some structural validation (e.g.,NestedSlice/NestedFieldNoCopyerror returns and slice/map assertions), so outright drift corruption from the merge/revert logic is unlikely.operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go:MergeDeploymentsstill uses unchecked type assertions that can panic on malformed/unexpected unstructured input (name.(string)at lines 62/78 andnr.(map[string]any)at line 84), enabling reconciliation DoS (CWE-248) / improper input validation (CWE-20). Replace with explicit.(type)checks and return errors.- Add unit tests for
MergeDeploymentsandRevertManagedDeploymentDriftcovering unexpected field types; current tests only coverMergeObservabilityResources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/deploy/action_deploy.go` around lines 650 - 652, MergeHelpers use unchecked type assertions that can panic; update MergeDeployments (and related helpers RevertManagedDeploymentDrift, MergeObservabilityResources, RemoveDeploymentsResources) to validate unstructured fields before casting: check errors from nested helpers (e.g., NestedSlice/NestedFieldNoCopy), replace direct assertions like name.(string) and nr.(map[string]any) with explicit type switches/type checks and return descriptive errors when types are unexpected, and ensure callers propagate those errors instead of panicking; add unit tests for MergeDeployments and RevertManagedDeploymentDrift that supply malformed/unexpected field types to assert errors are returned rather than causing panics.operator-actions-framework/controller/actions/render/helm/action_render_manifests.go-98-101 (1)
98-101:⚠️ Potential issue | 🟡 MinorRestrict Helm chart trust boundary before rendering.
operator-actions-framework/controller/actions/render/helm/action_render_manifests.gobuildschartsfromrr.HelmChartsand passeschart.Sourcestraight intohelm.New(...)with no path allowlist/validation (CWE-94). In this repo,rr.HelmChartsis not derived from CR/spec fields: cloudmanager charts use hardcoded subdirs joined ontochartsPath(internal/controller/cloudmanager/common/charts.go), and module charts use staticModuleConfigvalues joined ontoChartsBasePath(internal/controller/modules/base.go). So untrusted CRs don’t control template execution here.The real risk is misconfig/supply-chain:
ChartsBasePathcomes from operator flags/env (--default-charts-path/DEFAULT_CHARTS_PATH) and whoever controls that can point to arbitrary local chart templates, leading to arbitrary manifest generation during rendering (CWE-829).Remediation: validate that each
helm.Source.Chartresolves within the configuredChartsBasePath(and/or enforce an allowlist of expected chart subdirectories) before callinghelm.New.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/render/helm/action_render_manifests.go` around lines 98 - 101, The code appends chart.Source from rr.HelmCharts and passes it into helm.New without validating paths; before calling helm.New in action_render_manifests.go validate each chart.Source.Chart (or chart.Source path field) against the operator's configured ChartsBasePath (or chartsPath) by resolving the full path (use filepath.Abs + EvalSymlinks or filepath.Rel) and ensure the resolved path is contained within ChartsBasePath (or check against a small allowlist of expected subdirs) and return an error if any chart escapes that base; only after all entries are validated, build the charts slice and call helm.New to prevent untrusted/supply-chain-controlled chart directories from being rendered.operator-actions-framework/resources/resources.go-173-185 (1)
173-185:⚠️ Potential issue | 🟡 MinorFix
SetLabel/SetAnnotationcontract mismatch (returned old value vs promisedvoid)
operator-actions-framework/resources/resources.go’sSetLabelstill returns the previous value (stringold-value return), despite the PR claiming these setters “now return void”; this can cause API contract confusion (CWE-703). A quick scan found no call sites assigning the return value, but the signature still conflicts—alignSetLabelandSetAnnotation(near lines 236-248) with the intended API (either change the implementation/signature tovoidor update the PR/consumers accordingly).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/resources/resources.go` around lines 173 - 185, The SetLabel function returns a string old value but the PR intends setters to return void; update SetLabel and SetAnnotation to have no return value (remove the string return type and any "return old" statements) so their signatures match the intended API contract, and ensure obj.SetLabels/obj.SetAnnotations logic remains unchanged; also search for any callers referencing the return value and remove their usage if found (SetLabel and SetAnnotation are the functions to change).
🧹 Nitpick comments (5)
operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go (2)
81-92: 💤 Low valueConsider validating that types slice is non-empty.
NewActionaccepts an emptytypesslice, resulting in a no-op action. If this is intentional, document the behavior. Otherwise, validate and return an error or panic during construction to catch caller mistakes early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go` around lines 81 - 92, NewAction currently allows Action.types to be empty which yields a no-op; update NewAction to validate that the constructed Action.types slice is non-empty and fail fast: after applying opts, check len(action.types) == 0 and either panic with a clear message (e.g., "deleteresource: types must not be empty") or return a documented no-op variant if that behavior is intended; reference the NewAction function, the Action struct's types field, and the returned action.run to locate where to add the validation and message.
48-76: ⚡ Quick winDocument label scoping requirements for safe bulk deletion.
DeleteAllOfwithMatchingLabelswill delete all resources matching the provided labels across the target namespace (or cluster-wide for cluster-scoped resources). Ensure callers use sufficiently specific labels to prevent unintended deletion. Consider adding validation or warnings for overly broad label selectors (e.g., single common label likeapp=myapp).As per coding guidelines: "K8S CONTROLLER SECURITY: Reconciliation loop safety (prevent resource exhaustion)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go` around lines 48 - 76, The run function calls rr.Client.DeleteAllOf with MatchingLabels(r.labels) which can bulk-delete many resources; add validation in Action.run (before rr.Client.DeleteAllOf) to ensure r.labels is sufficiently specific: if r.labels is empty or only contains a single very common key/value (e.g., generic "app" or other configured blacklist), either return an error or refuse to run and log/warn so the caller must provide narrower selectors; also ensure cluster-scoped types (rr.Client.IsObjectNamespaced == false) require stricter checks. Update the checks around r.labels, r.namespaceFn and the DeleteAllOf invocation (referencing run, r.labels, MatchingLabels, DeleteAllOf, rr.Client.IsObjectNamespaced) to enforce or document label-scoping rules and fail-fast on overly broad selectors.Source: Coding guidelines
operator-actions-framework/controller/actions/render/template/action_render_templates.go (1)
142-151: ⚖️ Poor tradeoffTemplate execution exposes CR instance data.
Line 143 injects
rr.Instance(the Custom Resource) into template data. If the CR contains sensitive fields (passwords, tokens, API keys per guideline), templates can extract and output them into rendered manifests. Validate that templates are trusted and that CRs processed by this action contain no sensitive inline data, or sanitize the instance before injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/render/template/action_render_templates.go` around lines 142 - 151, The code injects the full CR instance into template data via data[ComponentKey] = rr.Instance which can leak secrets; before assigning, replace rr.Instance with a sanitized/de-serialized copy that removes or redacts sensitive keys (e.g., passwords, tokens, apiKeys) or alternatively inject only an allow-list/subset of fields, and ensure this sanitization happens upstream of any call to a.namespaceFn so AppNamespaceKey logic (a.namespaceFn, namespaceFn) remains intact; update the assignment site (where data[ComponentKey] is set) to perform the sanitization or selection of safe fields instead of directly passing rr.Instance.Source: Coding guidelines
operator-actions-framework/controller/conditions/conditions.go (1)
212-224: 💤 Low valueSwitch-with-break pattern obscures intent.
In Go,
breakin a switch case only exits the case (not the enclosing loop), so thesebreakstatements are no-ops that skip thedefaultclause. The logic is correct but reads as if break terminates the outer for-loop. Consider restructuring for clarity.♻️ Clearer filter logic
for _, c := range conditions { - switch { - case dn == 0 && c.Type == r.happy: - break - case dn != 0 && !slices.Contains(r.dependents, c.Type): - break - case c.Severity != api.ConditionSeverityError: - break - default: + if dn == 0 && c.Type == r.happy { + continue + } + if dn != 0 && !slices.Contains(r.dependents, c.Type) { + continue + } + if c.Severity != api.ConditionSeverityError { + continue + } conditions[n] = c n++ - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/conditions/conditions.go` around lines 212 - 224, The switch-with-break in the loop over conditions is misleading because break only exits the switch, not the for-loop; replace the switch in the loop with an explicit if/continue filter so intent is clear: for each c in conditions, if (dn == 0 && c.Type == r.happy) OR (dn != 0 && !slices.Contains(r.dependents, c.Type)) OR (c.Severity != api.ConditionSeverityError) then continue, otherwise keep the condition by assigning conditions[n] = c and incrementing n; update the loop in conditions.go (the loop that references dn, r.happy, r.dependents, api.ConditionSeverityError, and the assignments conditions[n] = c / n++) accordingly.operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go (1)
114-129: ⚡ Quick winEngine misconfiguration deferred until runtime.
NewActionreturns an action function even when noEngineis configured. The action will fail at the first reconciliation attempt (line 78-80) rather than at construction time.Early validation approach
func NewAction(opts ...ActionOpts) actions.Fn { action := Action{ cacher: resourcecacher.NewResourceCacher(rendererEngine), cache: true, } for _, opt := range opts { opt(&action) } + if action.engine == nil { + panic("kustomize engine is required; use WithEngine()") + } + if action.cache { action.cacher.SetKeyFn(types.Hash) } return action.run }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go` around lines 114 - 129, NewAction currently constructs and returns an action function even when no Engine is configured, deferring the failure to runtime; update NewAction to validate the renderer Engine at construction: after applying opts (and before using action.cacher or returning action.run) check the configured Engine (use action.Engine if present or the package-level rendererEngine) and if it's nil return a function that immediately returns a clear configuration error (or alternatively return an error from a NewAction variant) so callers fail fast; reference NewAction, Action, rendererEngine and action.run when making this change.
Comment on lines
+62
to
+64
| if r.namespaceFn == nil { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Silent skip of namespaced resource deletion is error-prone.
When a namespaced resource type is encountered but namespaceFn is nil, the action silently continues without deleting. Callers may incorrectly assume deletion succeeded. Return an error to fail fast when namespace resolution is required but unavailable.
🐛 Proposed fix to fail fast
if namespaced {
if r.namespaceFn == nil {
- continue
+ return fmt.Errorf("namespace function required for namespaced resource type %T", r.types[i])
}
appNamespace, nsErr := r.namespaceFn(ctx, rr)As per coding guidelines: "K8S CONTROLLER SECURITY: Proper error handling for security-critical operations."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if r.namespaceFn == nil { | |
| continue | |
| } | |
| if r.namespaceFn == nil { | |
| return fmt.Errorf("namespace function required for namespaced resource type %T", r.types[i]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go`
around lines 62 - 64, The loop silently skips deleting namespaced resources when
r.namespaceFn is nil; instead, detect this condition and return an error to fail
fast so callers know deletion could not proceed. Update the code that currently
does "if r.namespaceFn == nil { continue }" to return a descriptive error
(including the resource type/name context, e.g. using r.resourceType or the
current resource identifier) indicating namespace resolution is required but
namespaceFn is nil, so the delete action fails rather than silently continuing.
Source: Coding guidelines
Comment on lines
+61
to
+85
| //nolint:forcetypeassert,errcheck | ||
| resources[name.(string)] = r | ||
| } | ||
|
|
||
| for i := range targetContainers { | ||
| m, ok := targetContainers[i].(map[string]any) | ||
| if !ok { | ||
| return errors.New("field is not a map") | ||
| } | ||
|
|
||
| name, ok := m["name"] | ||
| if !ok { | ||
| // can't deal with unnamed containers | ||
| continue | ||
| } | ||
|
|
||
| //nolint:errcheck | ||
| nr, ok := resources[name.(string)] | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| //nolint:forcetypeassert,errcheck | ||
| if len(nr.(map[string]any)) == 0 { | ||
| delete(m, "resources") |
Contributor
There was a problem hiding this comment.
Unchecked type assertions allow panic on untrusted/unstructured data (CWE-20, CWE-248).
name.(string) and nr.(map[string]any) can panic when manifests are malformed, causing controller DoS.
Suggested hardening
- //nolint:forcetypeassert,errcheck
- resources[name.(string)] = r
+ nameStr, ok := name.(string)
+ if !ok || nameStr == "" {
+ continue
+ }
+ rm, ok := r.(map[string]any)
+ if !ok {
+ return errors.New("container.resources is not a map")
+ }
+ resources[nameStr] = rm
...
- //nolint:errcheck
- nr, ok := resources[name.(string)]
+ nameStr, ok := name.(string)
+ if !ok || nameStr == "" {
+ continue
+ }
+ nr, ok := resources[nameStr]
if !ok {
continue
}
-
- //nolint:forcetypeassert,errcheck
- if len(nr.(map[string]any)) == 0 {
+ nrMap, ok := nr.(map[string]any)
+ if !ok {
+ return errors.New("resource entry is not a map")
+ }
+ if len(nrMap) == 0 {
delete(m, "resources")
} else {
- m["resources"] = nr
+ m["resources"] = nrMap
}As per coding guidelines: “Validate all data from json.Unmarshal before storing…” and “REVIEW PRIORITIES: Security vulnerabilities.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go`
around lines 61 - 85, In action_deploy_merge_deployment.go, avoid unchecked type
assertions that can panic: replace all occurrences of name.(string) and
nr.(map[string]any) with safe type assertions and explicit error/skip handling
inside the loop in the function handling targetContainers; e.g., assert nameStr,
ok := name.(string) and if !ok continue/return a descriptive error, and assert
nrMap, ok := nr.(map[string]any) before calling len or delete on it, handling
the non-map case gracefully (skip or return error) so malformed manifests cannot
cause a panic.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator-actions-framework/Makefile`:
- Around line 75-79: The Makefile recipes use unquoted shell-expanded variables
which can break when paths contain spaces or special characters; update the
recipes (e.g., the mkdir and clean targets referencing LOCALBIN) to quote
expansions: replace mkdir -p $(LOCALBIN) with mkdir -p "$(LOCALBIN)" and rm -rf
$(LOCALBIN) cover.out with rm -rf "$(LOCALBIN)" "cover.out" (and similarly quote
any other uses of $(LOCALBIN) or path variables in the later recipes around the
82-89 region such as go install invocations) so shell word-splitting/globbing
cannot misinterpret paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7949bd3d-b4ec-4efe-944e-167550e329a8
⛔ Files ignored due to path filters (5)
cmd/health-check/go.sumis excluded by!**/*.sum,!**/*.sumcmd/mcp-server/go.sumis excluded by!**/*.sum,!**/*.sumoperator-actions-framework/go.sumis excluded by!**/*.sum,!**/*.sumpkg/clusterhealth/go.sumis excluded by!**/*.sum,!**/*.sumpkg/failureclassifier/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (146)
Dockerfiles/DockerfileDockerfiles/build-bundle.DockerfileDockerfiles/bundle.DockerfileDockerfiles/rhoai-bundle.DockerfileDockerfiles/rhoai.Dockerfileapi/common/types.goapi/common/zz_generated.deepcopy.gocmd/health-check/go.modcmd/mcp-server/go.modcmd/mcp-server/tool_platform_health_test.godocs/api-overview.mddocs/cloudmanager-api-overview.mdgo.modinternal/controller/components/dashboard/dashboard_controller_actions_test.gointernal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.gointernal/controller/components/kserve/kserve_controller.gointernal/controller/components/kueue/kueue_controller.gointernal/controller/components/modelcontroller/modelcontroller_controller.gointernal/controller/components/trainer/trainer_controller.gointernal/controller/components/trustyai/trustyai_test.gointernal/controller/datasciencecluster/datasciencecluster_controller_actions.gointernal/controller/modules/modules_controller_actions.gointernal/controller/modules/modules_controller_actions_inject_env.gointernal/controller/modules/modules_controller_actions_inject_env_test.gointernal/controller/services/auth/auth_controller_actions_test.gointernal/controller/services/monitoring/monitoring_controller_support_test.gooperator-actions-framework/.golangci.ymloperator-actions-framework/Makefileoperator-actions-framework/api/types.gooperator-actions-framework/cluster/cluster.gooperator-actions-framework/cluster/gvk/gvk.gooperator-actions-framework/controller/actions/actions.gooperator-actions-framework/controller/actions/cacher/cacher.gooperator-actions-framework/controller/actions/deleteresource/action_delete_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy.gooperator-actions-framework/controller/actions/deploy/action_deploy_cache.gooperator-actions-framework/controller/actions/deploy/action_deploy_managed.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.gooperator-actions-framework/controller/actions/deploy/action_deploy_metrics.gooperator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy_support.gooperator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.gooperator-actions-framework/controller/actions/errors/errors.gooperator-actions-framework/controller/actions/gc/action_gc.gooperator-actions-framework/controller/actions/gc/action_gc_metrics.gooperator-actions-framework/controller/actions/gc/action_gc_support.gooperator-actions-framework/controller/actions/render/helm/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.gooperator-actions-framework/controller/actions/render/render_metrics.gooperator-actions-framework/controller/actions/render/render_support.gooperator-actions-framework/controller/actions/render/template/action_render_templates.gooperator-actions-framework/controller/actions/resourcecacher/resourcecacher.gooperator-actions-framework/controller/actions/sanitycheck/sanitycheck.gooperator-actions-framework/controller/actions/status/deployments/action_deployments_available.gooperator-actions-framework/controller/conditions/conditions.gooperator-actions-framework/controller/conditions/conditions_support.gooperator-actions-framework/controller/handlers/handlers.gooperator-actions-framework/controller/predicates/dependent/dependent.gooperator-actions-framework/controller/predicates/generation/generation.gooperator-actions-framework/controller/predicates/hash/hash.gooperator-actions-framework/controller/predicates/label/label.gooperator-actions-framework/controller/predicates/partial/partial.gooperator-actions-framework/controller/predicates/predicates.gooperator-actions-framework/controller/predicates/resources/resources.gooperator-actions-framework/controller/reconciler/reconciler.gooperator-actions-framework/controller/reconciler/reconciler_actions.gooperator-actions-framework/controller/reconciler/reconciler_actions_test.gooperator-actions-framework/controller/reconciler/reconciler_metrics.gooperator-actions-framework/controller/reconciler/reconciler_support.gooperator-actions-framework/controller/types/types.gooperator-actions-framework/go.modoperator-actions-framework/metadata/metadata.gooperator-actions-framework/resources/resources.gooperator-actions-framework/resources/resources_types.gooperator-actions-framework/resources/sort.gooperator-actions-framework/rules/rules.gooperator-actions-framework/utils/template/template.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher_test.gooperator-actions-framework/utils/test/matchers/jq/jq_support.gooperator-actions-framework/utils/test/matchers/jq/jq_support_test.gooperator-actions-framework/utils/test/matchers/jq/jq_transform.gooperator-actions-framework/utils/test/matchers/jq/jq_transform_test.gooperator-actions-framework/utils/test/matchers/matchers.gopkg/cluster/resources.gopkg/clusterhealth/go.modpkg/controller/actions/actions.gopkg/controller/actions/cacher/cacher.gopkg/controller/actions/deleteresource/action_delete_resources.gopkg/controller/actions/deleteresource/action_delete_resources_test.gopkg/controller/actions/dependency/certmanager/bootstrap.gopkg/controller/actions/deploy/action_deploy.gopkg/controller/actions/deploy/action_deploy_cache.gopkg/controller/actions/deploy/action_deploy_cache_test.gopkg/controller/actions/deploy/action_deploy_managed.gopkg/controller/actions/deploy/action_deploy_merge_deployment.gopkg/controller/actions/deploy/action_deploy_metrics.gopkg/controller/actions/deploy/action_deploy_remove_deployment_resources.gopkg/controller/actions/deploy/action_deploy_support.gopkg/controller/actions/deploy/action_deploy_test.gopkg/controller/actions/dynamicownership/action_dynamic_ownership.gopkg/controller/actions/errors/errors.gopkg/controller/actions/gc/action_gc.gopkg/controller/actions/gc/action_gc_metrics.gopkg/controller/actions/gc/action_gc_support.gopkg/controller/actions/gc/action_gc_test.gopkg/controller/actions/render/helm/action_render_manifests.gopkg/controller/actions/render/kustomize/action_render_manifests_test.gopkg/controller/actions/render/render_metrics.gopkg/controller/actions/render/render_support.gopkg/controller/actions/render/template/action_render_templates.gopkg/controller/actions/render/template/action_render_templates_test.gopkg/controller/actions/resourcecacher/resourcecacher.gopkg/controller/actions/sanitycheck/sanitycheck.gopkg/controller/actions/status/deployments/action_deployments_available.gopkg/controller/actions/status/deployments/action_deployments_available_test.gopkg/controller/actions/status/imagestreams/action_imagestreams_available_test.gopkg/controller/cloudmanager/action_monitor_dependencies_test.gopkg/controller/cloudmanager/action_reconcile_integration_test.gopkg/controller/conditions/conditions.gopkg/controller/handlers/handlers.gopkg/controller/predicates/component/component.gopkg/controller/predicates/dependent/dependent.gopkg/controller/predicates/generation/generation.gopkg/controller/predicates/hash/hash.gopkg/controller/predicates/partial/partial.gopkg/controller/predicates/predicates.gopkg/controller/predicates/resources/resources.gopkg/controller/reconciler/reconciler.gopkg/controller/reconciler/reconciler_finalizer_test.gopkg/controller/reconciler/reconciler_metrics.gopkg/controller/reconciler/reconciler_support.gopkg/controller/reconciler/reconciler_test.gopkg/controller/types/types.gopkg/controller/types/types_test.gopkg/failureclassifier/go.modpkg/metadata/annotations/annotations.gopkg/resources/resources.gopkg/resources/resources_types.gopkg/resources/sort.gopkg/resources/sort_test.gopkg/rules/rules.gopkg/utils/template/template.go
💤 Files with no reviewable changes (4)
- pkg/controller/actions/deploy/action_deploy_support.go
- docs/cloudmanager-api-overview.md
- cmd/mcp-server/tool_platform_health_test.go
- api/common/zz_generated.deepcopy.go
✅ Files skipped from review due to trivial changes (3)
- operator-actions-framework/metadata/metadata.go
- internal/controller/services/monitoring/monitoring_controller_support_test.go
- Dockerfiles/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (122)
- pkg/controller/actions/deploy/action_deploy_remove_deployment_resources.go
- pkg/controller/actions/render/kustomize/action_render_manifests_test.go
- pkg/controller/actions/gc/action_gc_metrics.go
- operator-actions-framework/controller/actions/errors/errors.go
- Dockerfiles/rhoai-bundle.Dockerfile
- operator-actions-framework/controller/predicates/predicates.go
- internal/controller/components/trustyai/trustyai_test.go
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.go
- internal/controller/modules/modules_controller_actions_inject_env.go
- operator-actions-framework/controller/actions/render/render_metrics.go
- operator-actions-framework/controller/actions/render/render_support.go
- internal/controller/components/kueue/kueue_controller.go
- operator-actions-framework/controller/actions/actions.go
- operator-actions-framework/controller/actions/gc/action_gc_support.go
- operator-actions-framework/controller/predicates/label/label.go
- pkg/controller/reconciler/reconciler_finalizer_test.go
- pkg/controller/conditions/conditions.go
- cmd/mcp-server/go.mod
- pkg/controller/actions/deploy/action_deploy_managed.go
- Dockerfiles/rhoai.Dockerfile
- operator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.go
- pkg/controller/actions/dependency/certmanager/bootstrap.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform_test.go
- operator-actions-framework/utils/template/template.go
- pkg/controller/reconciler/reconciler_metrics.go
- internal/controller/components/modelcontroller/modelcontroller_controller.go
- pkg/resources/resources_types.go
- pkg/controller/actions/gc/action_gc.go
- operator-actions-framework/controller/actions/deploy/action_deploy_metrics.go
- pkg/controller/actions/deleteresource/action_delete_resources_test.go
- pkg/clusterhealth/go.mod
- pkg/controller/actions/dynamicownership/action_dynamic_ownership.go
- internal/controller/services/auth/auth_controller_actions_test.go
- pkg/controller/actions/errors/errors.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.go
- pkg/controller/actions/status/deployments/action_deployments_available_test.go
- pkg/controller/cloudmanager/action_reconcile_integration_test.go
- pkg/controller/actions/actions.go
- operator-actions-framework/resources/resources_types.go
- operator-actions-framework/controller/reconciler/reconciler_metrics.go
- internal/controller/components/kserve/kserve_controller.go
- pkg/controller/cloudmanager/action_monitor_dependencies_test.go
- internal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.go
- operator-actions-framework/controller/predicates/hash/hash.go
- operator-actions-framework/controller/predicates/generation/generation.go
- internal/controller/components/dashboard/dashboard_controller_actions_test.go
- pkg/controller/handlers/handlers.go
- pkg/controller/actions/deploy/action_deploy_merge_deployment.go
- pkg/controller/actions/deploy/action_deploy_cache_test.go
- api/common/types.go
- pkg/controller/predicates/hash/hash.go
- pkg/controller/actions/resourcecacher/resourcecacher.go
- pkg/controller/types/types_test.go
- operator-actions-framework/go.mod
- pkg/failureclassifier/go.mod
- operator-actions-framework/utils/test/matchers/jq/jq_matcher_test.go
- operator-actions-framework/controller/handlers/handlers.go
- operator-actions-framework/utils/test/matchers/jq/jq_matcher.go
- operator-actions-framework/resources/sort.go
- operator-actions-framework/controller/actions/render/template/action_render_templates.go
- pkg/cluster/resources.go
- operator-actions-framework/controller/actions/sanitycheck/sanitycheck.go
- operator-actions-framework/cluster/cluster.go
- pkg/controller/actions/render/helm/action_render_manifests.go
- operator-actions-framework/controller/reconciler/reconciler_actions.go
- pkg/controller/actions/render/render_support.go
- operator-actions-framework/utils/test/matchers/jq/jq_support_test.go
- pkg/controller/predicates/partial/partial.go
- operator-actions-framework/controller/actions/deploy/action_deploy_cache.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go
- operator-actions-framework/controller/actions/deploy/action_deploy_support.go
- pkg/controller/actions/render/template/action_render_templates_test.go
- pkg/resources/sort_test.go
- operator-actions-framework/controller/predicates/dependent/dependent.go
- pkg/controller/actions/status/imagestreams/action_imagestreams_available_test.go
- operator-actions-framework/.golangci.yml
- pkg/controller/predicates/generation/generation.go
- operator-actions-framework/controller/reconciler/reconciler_actions_test.go
- Dockerfiles/bundle.Dockerfile
- operator-actions-framework/controller/conditions/conditions_support.go
- pkg/controller/actions/deploy/action_deploy_metrics.go
- internal/controller/modules/modules_controller_actions.go
- operator-actions-framework/controller/actions/cacher/cacher.go
- operator-actions-framework/controller/actions/render/helm/action_render_manifests.go
- operator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.go
- pkg/resources/sort.go
- go.mod
- pkg/controller/predicates/component/component.go
- pkg/controller/predicates/dependent/dependent.go
- pkg/resources/resources.go
- operator-actions-framework/utils/test/matchers/jq/jq_support.go
- operator-actions-framework/controller/actions/gc/action_gc_metrics.go
- pkg/controller/actions/deploy/action_deploy_cache.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform.go
- pkg/metadata/annotations/annotations.go
- operator-actions-framework/controller/actions/status/deployments/action_deployments_available.go
- pkg/controller/actions/deploy/action_deploy.go
- operator-actions-framework/controller/actions/gc/action_gc.go
- operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go
- operator-actions-framework/controller/predicates/resources/resources.go
- pkg/controller/actions/gc/action_gc_test.go
- pkg/controller/reconciler/reconciler_support.go
- operator-actions-framework/api/types.go
- operator-actions-framework/rules/rules.go
- pkg/controller/actions/render/template/action_render_templates.go
- pkg/controller/actions/cacher/cacher.go
- operator-actions-framework/controller/actions/resourcecacher/resourcecacher.go
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go
- pkg/controller/predicates/resources/resources.go
- pkg/controller/actions/deleteresource/action_delete_resources.go
- pkg/controller/types/types.go
- operator-actions-framework/controller/conditions/conditions.go
- pkg/controller/actions/deploy/action_deploy_test.go
- operator-actions-framework/controller/types/types.go
- operator-actions-framework/resources/resources.go
- operator-actions-framework/cluster/gvk/gvk.go
- operator-actions-framework/controller/reconciler/reconciler.go
- operator-actions-framework/controller/actions/deploy/action_deploy.go
- operator-actions-framework/controller/reconciler/reconciler_support.go
- docs/api-overview.md
- pkg/controller/reconciler/reconciler_test.go
- pkg/rules/rules.go
valdar
added
run-xks-e2e
run-gateway-tests
run-cloudmanager-e2e
and removed
run-gateway-tests
run-cloudmanager-e2e
run-xks-e2e
labels
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/predicates/resources/resources.go (1)
85-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCondition type replacement not detected.
If a condition type is removed and a different type is added (same total count), this predicate returns
falsebecause the new type finds no match in the inner loop and silently continues. Reconciliation is skipped despite meaningful status changes.Example:
old=[{Type:"Ready"}],new=[{Type:"Progressing"}]→ both have length 1, butProgressingnever matchesReady, so loop completes and returnsfalse.Consider using a map-based lookup or comparing the full condition sets:
oldConditions := oldDSC.Status.Conditions newConditions := newDSC.Status.Conditions if len(oldConditions) != len(newConditions) { return true } - for _, nc := range newConditions { - for _, oc := range oldConditions { - if nc.Type == oc.Type { - if !reflect.DeepEqual(nc.Status, oc.Status) { - return true - } - } - } + oldMap := make(map[string]metav1.ConditionStatus, len(oldConditions)) + for _, oc := range oldConditions { + oldMap[string(oc.Type)] = oc.Status + } + for _, nc := range newConditions { + if oldStatus, found := oldMap[string(nc.Type)]; !found || oldStatus != nc.Status { + return true + } } return false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/predicates/resources/resources.go` around lines 85 - 93, The loop over newConditions/oldConditions misses type replacements because it only checks matching types and silently ignores unmatched types; update the predicate (the comparison around newConditions and oldConditions in resources.go) to first detect added/removed condition types and then compare statuses using a lookup: build a map[string]Condition from oldConditions keyed by Type, check if any new condition Type is missing in that map (or any old Type missing by comparing lengths or doing a reverse check), and return true for added/removed types or when reflect.DeepEqual(nc.Status, oc.Status) indicates a status change; this ensures type replacements (e.g., Ready -> Progressing) are detected and trigger reconciliation.
🧹 Nitpick comments (2)
pkg/controller/actions/deploy/action_deploy_remove_deployment_resources.go (1)
7-7: ⚡ Quick winKeep
RemoveDeploymentsResourcesas a wrapper function, not an exported variable.Line 7 changes the symbol from an exported
functo an exported function-typedvar. That widens the public surface unnecessarily and drifts from the previous API contract during the module extraction. Keep a thin forwarding function here instead.Proposed fix
package deploy import ( fwdeploy "github.com/opendatahub-io/operator-actions-framework/controller/actions/deploy" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -var RemoveDeploymentsResources = fwdeploy.RemoveDeploymentsResources +func RemoveDeploymentsResources(obj *unstructured.Unstructured) error { + return fwdeploy.RemoveDeploymentsResources(obj) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/actions/deploy/action_deploy_remove_deployment_resources.go` at line 7, The current change re-exports fwdeploy.RemoveDeploymentsResources as a package-level variable which expands the public surface; restore it as a thin forwarding function named RemoveDeploymentsResources that calls fwdeploy.RemoveDeploymentsResources so the exported symbol remains a function (not a function-typed var) and preserves the original signature and behavior; locate the current var RemoveDeploymentsResources and replace it with a function RemoveDeploymentsResources(...) matching the original parameters and return types that simply returns fwdeploy.RemoveDeploymentsResources(...) to forward the call.operator-actions-framework/controller/conditions/conditions_support.go (1)
24-31: 💤 Low valueRedundant
LastTransitionTime.IsZero()check.Lines 25-27 will never execute.
newCondition.LastTransitionTimeis already set tometav1.NewTime(time.Now())on lines 16-18 when zero, so the check at line 25 is always false.if idx == -1 { - if newCondition.LastTransitionTime.IsZero() { - newCondition.LastTransitionTime = metav1.NewTime(time.Now()) - } conditions = append(conditions, newCondition) a.SetConditions(conditions) return true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/conditions/conditions_support.go` around lines 24 - 31, The conditional checking newCondition.LastTransitionTime.IsZero() before setting LastTransitionTime inside the idx == -1 branch is redundant because newCondition.LastTransitionTime is already initialized earlier when zero; remove the inner check and directly set newCondition.LastTransitionTime = metav1.NewTime(time.Now()) (or omit resetting if already set) inside the block where idx == -1, then append to conditions and call a.SetConditions(conditions) as before (refer to newCondition, LastTransitionTime, idx, a.SetConditions).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/controller/predicates/resources/resources.go`:
- Around line 85-93: The loop over newConditions/oldConditions misses type
replacements because it only checks matching types and silently ignores
unmatched types; update the predicate (the comparison around newConditions and
oldConditions in resources.go) to first detect added/removed condition types and
then compare statuses using a lookup: build a map[string]Condition from
oldConditions keyed by Type, check if any new condition Type is missing in that
map (or any old Type missing by comparing lengths or doing a reverse check), and
return true for added/removed types or when reflect.DeepEqual(nc.Status,
oc.Status) indicates a status change; this ensures type replacements (e.g.,
Ready -> Progressing) are detected and trigger reconciliation.
---
Nitpick comments:
In `@operator-actions-framework/controller/conditions/conditions_support.go`:
- Around line 24-31: The conditional checking
newCondition.LastTransitionTime.IsZero() before setting LastTransitionTime
inside the idx == -1 branch is redundant because newCondition.LastTransitionTime
is already initialized earlier when zero; remove the inner check and directly
set newCondition.LastTransitionTime = metav1.NewTime(time.Now()) (or omit
resetting if already set) inside the block where idx == -1, then append to
conditions and call a.SetConditions(conditions) as before (refer to
newCondition, LastTransitionTime, idx, a.SetConditions).
In `@pkg/controller/actions/deploy/action_deploy_remove_deployment_resources.go`:
- Line 7: The current change re-exports fwdeploy.RemoveDeploymentsResources as a
package-level variable which expands the public surface; restore it as a thin
forwarding function named RemoveDeploymentsResources that calls
fwdeploy.RemoveDeploymentsResources so the exported symbol remains a function
(not a function-typed var) and preserves the original signature and behavior;
locate the current var RemoveDeploymentsResources and replace it with a function
RemoveDeploymentsResources(...) matching the original parameters and return
types that simply returns fwdeploy.RemoveDeploymentsResources(...) to forward
the call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5510f2d8-c2a9-48b8-b91b-3b09b6007a9b
⛔ Files ignored due to path filters (5)
cmd/health-check/go.sumis excluded by!**/*.sum,!**/*.sumcmd/mcp-server/go.sumis excluded by!**/*.sum,!**/*.sumoperator-actions-framework/go.sumis excluded by!**/*.sum,!**/*.sumpkg/clusterhealth/go.sumis excluded by!**/*.sum,!**/*.sumpkg/failureclassifier/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (147)
.gitignoreDockerfiles/DockerfileDockerfiles/build-bundle.DockerfileDockerfiles/bundle.DockerfileDockerfiles/rhoai-bundle.DockerfileDockerfiles/rhoai.Dockerfileapi/common/types.goapi/common/zz_generated.deepcopy.gocmd/health-check/go.modcmd/mcp-server/go.modcmd/mcp-server/tool_platform_health_test.godocs/api-overview.mddocs/cloudmanager-api-overview.mdgo.modinternal/controller/components/dashboard/dashboard_controller_actions_test.gointernal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.gointernal/controller/components/kserve/kserve_controller.gointernal/controller/components/kueue/kueue_controller.gointernal/controller/components/modelcontroller/modelcontroller_controller.gointernal/controller/components/trainer/trainer_controller.gointernal/controller/components/trustyai/trustyai_test.gointernal/controller/datasciencecluster/datasciencecluster_controller_actions.gointernal/controller/modules/modules_controller_actions.gointernal/controller/modules/modules_controller_actions_inject_env.gointernal/controller/modules/modules_controller_actions_inject_env_test.gointernal/controller/services/auth/auth_controller_actions_test.gointernal/controller/services/monitoring/monitoring_controller_support_test.gooperator-actions-framework/.golangci.ymloperator-actions-framework/Makefileoperator-actions-framework/api/types.gooperator-actions-framework/cluster/cluster.gooperator-actions-framework/cluster/gvk/gvk.gooperator-actions-framework/controller/actions/actions.gooperator-actions-framework/controller/actions/cacher/cacher.gooperator-actions-framework/controller/actions/deleteresource/action_delete_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy.gooperator-actions-framework/controller/actions/deploy/action_deploy_cache.gooperator-actions-framework/controller/actions/deploy/action_deploy_managed.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.gooperator-actions-framework/controller/actions/deploy/action_deploy_metrics.gooperator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy_support.gooperator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.gooperator-actions-framework/controller/actions/errors/errors.gooperator-actions-framework/controller/actions/gc/action_gc.gooperator-actions-framework/controller/actions/gc/action_gc_metrics.gooperator-actions-framework/controller/actions/gc/action_gc_support.gooperator-actions-framework/controller/actions/render/helm/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.gooperator-actions-framework/controller/actions/render/render_metrics.gooperator-actions-framework/controller/actions/render/render_support.gooperator-actions-framework/controller/actions/render/template/action_render_templates.gooperator-actions-framework/controller/actions/resourcecacher/resourcecacher.gooperator-actions-framework/controller/actions/sanitycheck/sanitycheck.gooperator-actions-framework/controller/actions/status/deployments/action_deployments_available.gooperator-actions-framework/controller/conditions/conditions.gooperator-actions-framework/controller/conditions/conditions_support.gooperator-actions-framework/controller/handlers/handlers.gooperator-actions-framework/controller/predicates/dependent/dependent.gooperator-actions-framework/controller/predicates/generation/generation.gooperator-actions-framework/controller/predicates/hash/hash.gooperator-actions-framework/controller/predicates/label/label.gooperator-actions-framework/controller/predicates/partial/partial.gooperator-actions-framework/controller/predicates/predicates.gooperator-actions-framework/controller/predicates/resources/resources.gooperator-actions-framework/controller/reconciler/reconciler.gooperator-actions-framework/controller/reconciler/reconciler_actions.gooperator-actions-framework/controller/reconciler/reconciler_actions_test.gooperator-actions-framework/controller/reconciler/reconciler_metrics.gooperator-actions-framework/controller/reconciler/reconciler_support.gooperator-actions-framework/controller/types/types.gooperator-actions-framework/go.modoperator-actions-framework/metadata/metadata.gooperator-actions-framework/resources/resources.gooperator-actions-framework/resources/resources_types.gooperator-actions-framework/resources/sort.gooperator-actions-framework/rules/rules.gooperator-actions-framework/utils/template/template.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher_test.gooperator-actions-framework/utils/test/matchers/jq/jq_support.gooperator-actions-framework/utils/test/matchers/jq/jq_support_test.gooperator-actions-framework/utils/test/matchers/jq/jq_transform.gooperator-actions-framework/utils/test/matchers/jq/jq_transform_test.gooperator-actions-framework/utils/test/matchers/matchers.gopkg/cluster/resources.gopkg/clusterhealth/go.modpkg/controller/actions/actions.gopkg/controller/actions/cacher/cacher.gopkg/controller/actions/deleteresource/action_delete_resources.gopkg/controller/actions/deleteresource/action_delete_resources_test.gopkg/controller/actions/dependency/certmanager/bootstrap.gopkg/controller/actions/deploy/action_deploy.gopkg/controller/actions/deploy/action_deploy_cache.gopkg/controller/actions/deploy/action_deploy_cache_test.gopkg/controller/actions/deploy/action_deploy_managed.gopkg/controller/actions/deploy/action_deploy_merge_deployment.gopkg/controller/actions/deploy/action_deploy_metrics.gopkg/controller/actions/deploy/action_deploy_remove_deployment_resources.gopkg/controller/actions/deploy/action_deploy_support.gopkg/controller/actions/deploy/action_deploy_test.gopkg/controller/actions/dynamicownership/action_dynamic_ownership.gopkg/controller/actions/errors/errors.gopkg/controller/actions/gc/action_gc.gopkg/controller/actions/gc/action_gc_metrics.gopkg/controller/actions/gc/action_gc_support.gopkg/controller/actions/gc/action_gc_test.gopkg/controller/actions/render/helm/action_render_manifests.gopkg/controller/actions/render/kustomize/action_render_manifests_test.gopkg/controller/actions/render/render_metrics.gopkg/controller/actions/render/render_support.gopkg/controller/actions/render/template/action_render_templates.gopkg/controller/actions/render/template/action_render_templates_test.gopkg/controller/actions/resourcecacher/resourcecacher.gopkg/controller/actions/sanitycheck/sanitycheck.gopkg/controller/actions/status/deployments/action_deployments_available.gopkg/controller/actions/status/deployments/action_deployments_available_test.gopkg/controller/actions/status/imagestreams/action_imagestreams_available_test.gopkg/controller/cloudmanager/action_monitor_dependencies_test.gopkg/controller/cloudmanager/action_reconcile_integration_test.gopkg/controller/conditions/conditions.gopkg/controller/handlers/handlers.gopkg/controller/predicates/component/component.gopkg/controller/predicates/dependent/dependent.gopkg/controller/predicates/generation/generation.gopkg/controller/predicates/hash/hash.gopkg/controller/predicates/partial/partial.gopkg/controller/predicates/predicates.gopkg/controller/predicates/resources/resources.gopkg/controller/reconciler/reconciler.gopkg/controller/reconciler/reconciler_finalizer_test.gopkg/controller/reconciler/reconciler_metrics.gopkg/controller/reconciler/reconciler_support.gopkg/controller/reconciler/reconciler_test.gopkg/controller/types/types.gopkg/controller/types/types_test.gopkg/failureclassifier/go.modpkg/metadata/annotations/annotations.gopkg/resources/resources.gopkg/resources/resources_types.gopkg/resources/sort.gopkg/resources/sort_test.gopkg/rules/rules.gopkg/utils/template/template.go
💤 Files with no reviewable changes (8)
- pkg/controller/actions/deploy/action_deploy_support.go
- pkg/resources/sort.go
- pkg/resources/resources_types.go
- pkg/resources/sort_test.go
- docs/cloudmanager-api-overview.md
- pkg/resources/resources.go
- api/common/zz_generated.deepcopy.go
- cmd/mcp-server/tool_platform_health_test.go
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- operator-actions-framework/utils/test/matchers/matchers.go
- cmd/health-check/go.mod
- internal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.go
- pkg/controller/actions/render/kustomize/action_render_manifests_test.go
- operator-actions-framework/.golangci.yml
🚧 Files skipped from review as they are similar to previous changes (116)
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.go
- Dockerfiles/rhoai.Dockerfile
- internal/controller/components/trainer/trainer_controller.go
- internal/controller/modules/modules_controller_actions_inject_env.go
- operator-actions-framework/controller/actions/deploy/action_deploy_metrics.go
- operator-actions-framework/controller/actions/deploy/action_deploy_support.go
- pkg/controller/actions/deleteresource/action_delete_resources_test.go
- Dockerfiles/Dockerfile
- internal/controller/components/trustyai/trustyai_test.go
- operator-actions-framework/controller/actions/render/render_support.go
- pkg/clusterhealth/go.mod
- operator-actions-framework/go.mod
- pkg/controller/predicates/predicates.go
- pkg/controller/reconciler/reconciler_metrics.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform_test.go
- Dockerfiles/build-bundle.Dockerfile
- pkg/controller/predicates/dependent/dependent.go
- operator-actions-framework/controller/predicates/hash/hash.go
- Dockerfiles/bundle.Dockerfile
- pkg/controller/actions/sanitycheck/sanitycheck.go
- internal/controller/components/dashboard/dashboard_controller_actions_test.go
- pkg/controller/actions/render/helm/action_render_manifests.go
- pkg/controller/actions/errors/errors.go
- pkg/controller/actions/resourcecacher/resourcecacher.go
- pkg/controller/actions/render/render_support.go
- internal/controller/services/auth/auth_controller_actions_test.go
- operator-actions-framework/metadata/metadata.go
- operator-actions-framework/controller/predicates/label/label.go
- pkg/controller/actions/gc/action_gc_support.go
- pkg/controller/actions/deploy/action_deploy_merge_deployment.go
- pkg/controller/predicates/partial/partial.go
- internal/controller/datasciencecluster/datasciencecluster_controller_actions.go
- operator-actions-framework/controller/predicates/partial/partial.go
- pkg/controller/actions/render/template/action_render_templates_test.go
- pkg/controller/actions/gc/action_gc_metrics.go
- pkg/controller/types/types_test.go
- operator-actions-framework/utils/test/matchers/jq/jq_support_test.go
- pkg/controller/predicates/hash/hash.go
- operator-actions-framework/controller/actions/render/render_metrics.go
- pkg/controller/cloudmanager/action_monitor_dependencies_test.go
- cmd/mcp-server/go.mod
- internal/controller/components/modelcontroller/modelcontroller_controller.go
- Dockerfiles/rhoai-bundle.Dockerfile
- internal/controller/components/kueue/kueue_controller.go
- internal/controller/components/kserve/kserve_controller.go
- operator-actions-framework/controller/actions/gc/action_gc_metrics.go
- operator-actions-framework/controller/actions/actions.go
- pkg/failureclassifier/go.mod
- operator-actions-framework/controller/actions/gc/action_gc_support.go
- pkg/controller/actions/dependency/certmanager/bootstrap.go
- pkg/controller/actions/deploy/action_deploy_metrics.go
- pkg/metadata/annotations/annotations.go
- operator-actions-framework/controller/reconciler/reconciler_actions.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.go
- operator-actions-framework/cluster/cluster.go
- operator-actions-framework/resources/resources_types.go
- operator-actions-framework/controller/actions/resourcecacher/resourcecacher.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go
- operator-actions-framework/controller/actions/errors/errors.go
- pkg/controller/predicates/component/component.go
- pkg/controller/actions/status/deployments/action_deployments_available_test.go
- operator-actions-framework/utils/test/matchers/jq/jq_matcher_test.go
- operator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.go
- operator-actions-framework/rules/rules.go
- operator-actions-framework/utils/test/matchers/jq/jq_support.go
- pkg/controller/actions/deploy/action_deploy_cache_test.go
- operator-actions-framework/utils/template/template.go
- pkg/controller/actions/deploy/action_deploy_managed.go
- operator-actions-framework/controller/reconciler/reconciler_metrics.go
- go.mod
- operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go
- pkg/controller/predicates/generation/generation.go
- pkg/controller/actions/gc/action_gc_test.go
- operator-actions-framework/utils/test/matchers/jq/jq_matcher.go
- pkg/controller/reconciler/reconciler_support.go
- internal/controller/modules/modules_controller_actions.go
- pkg/controller/actions/dynamicownership/action_dynamic_ownership.go
- operator-actions-framework/controller/actions/render/helm/action_render_manifests.go
- pkg/controller/actions/gc/action_gc.go
- operator-actions-framework/controller/predicates/dependent/dependent.go
- pkg/cluster/resources.go
- operator-actions-framework/controller/handlers/handlers.go
- operator-actions-framework/controller/types/types.go
- operator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.go
- pkg/controller/actions/render/render_metrics.go
- pkg/controller/actions/status/deployments/action_deployments_available.go
- operator-actions-framework/api/types.go
- pkg/controller/actions/deploy/action_deploy.go
- operator-actions-framework/controller/actions/deploy/action_deploy_managed.go
- pkg/controller/actions/deploy/action_deploy_cache.go
- operator-actions-framework/controller/reconciler/reconciler_actions_test.go
- pkg/controller/actions/deleteresource/action_delete_resources.go
- operator-actions-framework/resources/resources.go
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go
- operator-actions-framework/controller/predicates/generation/generation.go
- operator-actions-framework/controller/actions/gc/action_gc.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform.go
- operator-actions-framework/controller/actions/deploy/action_deploy_cache.go
- pkg/controller/cloudmanager/action_reconcile_integration_test.go
- operator-actions-framework/controller/actions/cacher/cacher.go
- pkg/controller/actions/deploy/action_deploy_test.go
- operator-actions-framework/controller/actions/render/template/action_render_templates.go
- operator-actions-framework/controller/reconciler/reconciler_support.go
- pkg/controller/actions/status/imagestreams/action_imagestreams_available_test.go
- pkg/controller/actions/render/template/action_render_templates.go
- operator-actions-framework/controller/actions/sanitycheck/sanitycheck.go
- operator-actions-framework/controller/actions/status/deployments/action_deployments_available.go
- operator-actions-framework/resources/sort.go
- pkg/controller/conditions/conditions.go
- operator-actions-framework/controller/predicates/resources/resources.go
- operator-actions-framework/controller/reconciler/reconciler.go
- operator-actions-framework/cluster/gvk/gvk.go
- operator-actions-framework/controller/conditions/conditions.go
- docs/api-overview.md
- operator-actions-framework/controller/actions/deploy/action_deploy.go
- pkg/controller/handlers/handlers.go
Member
Author
|
/test opendatahub-operator-rhoai-e2e |
1 similar comment
Member
Author
|
/test opendatahub-operator-rhoai-e2e |
Member
Author
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
operator-actions-framework/Makefile (1)
75-75: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winQuote all shell-expanded paths/arguments in recipes (CWE-78, CWE-88).
Line 75, Line 79, and Line 82-89 still use unquoted expansions; a path with spaces or glob chars can alter command arguments and target unintended files.
Patch
$(LOCALBIN): - mkdir -p $(LOCALBIN) + mkdir -p "$(LOCALBIN)" .PHONY: clean clean: ## Remove build artifacts. - rm -rf $(LOCALBIN) cover.out + rm -rf "$(LOCALBIN)" "cover.out" define go-install-tool -@[ -f $(1) ] || { \ +@[ -f "$(1)" ] || { \ set -e ;\ TMP_DIR=$$(mktemp -d) ;\ -cd $$TMP_DIR ;\ +cd "$$TMP_DIR" ;\ go mod init tmp ;\ echo "Downloading $(2)@$(3)" ;\ -GOBIN=$(LOCALBIN) go install $(2)@$(3) ;\ -rm -rf $$TMP_DIR ;\ +GOBIN="$(LOCALBIN)" go install "$(2)@$(3)" ;\ +rm -rf "$$TMP_DIR" ;\ } endefAs per path instructions, "Quote shell variables in targets".
Also applies to: 79-79, 82-82, 85-85, 88-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/Makefile` at line 75, Quote all shell variable expansions throughout the Makefile recipes to prevent unintended command argument interpretation. In the mkdir command using $(LOCALBIN) on line 75, wrap the variable reference in quotes as "$(LOCALBIN)". Apply the same quoting pattern to all other unquoted shell expansions on lines 79, 82, 85, and 88-89 to ensure paths with spaces or special characters are treated as single arguments rather than being expanded or interpreted as multiple arguments.Source: Path instructions
🧹 Nitpick comments (1)
operator-actions-framework/controller/actions/deploy/action_deploy.go (1)
355-369: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead commented guard; CRD part-of label can be set to empty string.
The commented-out
// if a.partOfLabelDefault != "" {at Line 357 leaves the intended guard disabled.deployCRDunconditionally callsSetLabel(&obj, a.partOfLabelKey, a.partOfLabelDefault)even whenpartOfLabelDefault == "", writing an empty label value. Compare withdeploy(Line 435) which only sets when the resolved owner is non-empty. Either restore the guard or drop the stale comment.Proposed fix
if a.partOfLabelKey == a.defaultPartOfLabelKey { - // if a.partOfLabelDefault != "" { - resources.SetLabel(&obj, a.partOfLabelKey, a.partOfLabelDefault) + if a.partOfLabelDefault != "" { + resources.SetLabel(&obj, a.partOfLabelKey, a.partOfLabelDefault) + } } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-actions-framework/controller/actions/deploy/action_deploy.go` around lines 355 - 369, Restore the disabled guard for setting the partOfLabelDefault in the CRD deploy path. Uncomment the conditional check `if a.partOfLabelDefault != ""` that precedes the resources.SetLabel call with a.partOfLabelDefault, and add the corresponding closing brace after the SetLabel statement. This ensures the label is only set when partOfLabelDefault has a non-empty value, matching the pattern used in the alternative branch where resources.SetLabel is only called when fo is non-empty, preventing empty label values from being written.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator-actions-framework/controller/actions/deploy/action_deploy.go`:
- Around line 540-562: The function returns the wrong object after successfully
creating a new resource when old is nil. In the create branch where old == nil,
the function performs a successful create operation but then returns old (which
is still nil) instead of the newly created object obj. Fix this by modifying the
final return statement to conditionally return the appropriate object: return
obj when old is nil (creation succeeded), or return old after the patch
succeeds. Alternatively, if both paths result in the same final state, return
obj unconditionally after both the create and patch operations complete
successfully.
---
Duplicate comments:
In `@operator-actions-framework/Makefile`:
- Line 75: Quote all shell variable expansions throughout the Makefile recipes
to prevent unintended command argument interpretation. In the mkdir command
using $(LOCALBIN) on line 75, wrap the variable reference in quotes as
"$(LOCALBIN)". Apply the same quoting pattern to all other unquoted shell
expansions on lines 79, 82, 85, and 88-89 to ensure paths with spaces or special
characters are treated as single arguments rather than being expanded or
interpreted as multiple arguments.
---
Nitpick comments:
In `@operator-actions-framework/controller/actions/deploy/action_deploy.go`:
- Around line 355-369: Restore the disabled guard for setting the
partOfLabelDefault in the CRD deploy path. Uncomment the conditional check `if
a.partOfLabelDefault != ""` that precedes the resources.SetLabel call with
a.partOfLabelDefault, and add the corresponding closing brace after the SetLabel
statement. This ensures the label is only set when partOfLabelDefault has a
non-empty value, matching the pattern used in the alternative branch where
resources.SetLabel is only called when fo is non-empty, preventing empty label
values from being written.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 742bc9a9-4e90-44d2-8426-ffc11bc93831
⛔ Files ignored due to path filters (5)
cmd/health-check/go.sumis excluded by!**/*.sum,!**/*.sumcmd/mcp-server/go.sumis excluded by!**/*.sum,!**/*.sumoperator-actions-framework/go.sumis excluded by!**/*.sum,!**/*.sumpkg/clusterhealth/go.sumis excluded by!**/*.sum,!**/*.sumpkg/failureclassifier/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (148)
.gitignoreDockerfiles/DockerfileDockerfiles/build-bundle.DockerfileDockerfiles/bundle.DockerfileDockerfiles/rhoai-bundle.DockerfileDockerfiles/rhoai.Dockerfileapi/common/types.goapi/common/zz_generated.deepcopy.gocmd/health-check/go.modcmd/mcp-server/go.modcmd/mcp-server/tool_platform_health_test.godocs/api-overview.mddocs/cloudmanager-api-overview.mdgo.modinternal/controller/components/dashboard/dashboard_controller_actions_test.gointernal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.gointernal/controller/components/kserve/kserve_controller.gointernal/controller/components/kueue/kueue_controller.gointernal/controller/components/modelcontroller/modelcontroller_controller.gointernal/controller/components/modelsasservice/modelsasservice_support_test.gointernal/controller/components/trainer/trainer_controller.gointernal/controller/components/trustyai/trustyai_test.gointernal/controller/datasciencecluster/datasciencecluster_controller_actions.gointernal/controller/modules/modules_controller_actions.gointernal/controller/modules/modules_controller_actions_inject_env.gointernal/controller/modules/modules_controller_actions_inject_env_test.gointernal/controller/services/auth/auth_controller_actions_test.gointernal/controller/services/monitoring/monitoring_controller_support_test.gooperator-actions-framework/.golangci.ymloperator-actions-framework/Makefileoperator-actions-framework/api/types.gooperator-actions-framework/cluster/cluster.gooperator-actions-framework/cluster/gvk/gvk.gooperator-actions-framework/controller/actions/actions.gooperator-actions-framework/controller/actions/cacher/cacher.gooperator-actions-framework/controller/actions/deleteresource/action_delete_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy.gooperator-actions-framework/controller/actions/deploy/action_deploy_cache.gooperator-actions-framework/controller/actions/deploy/action_deploy_managed.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.gooperator-actions-framework/controller/actions/deploy/action_deploy_metrics.gooperator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy_support.gooperator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.gooperator-actions-framework/controller/actions/errors/errors.gooperator-actions-framework/controller/actions/gc/action_gc.gooperator-actions-framework/controller/actions/gc/action_gc_metrics.gooperator-actions-framework/controller/actions/gc/action_gc_support.gooperator-actions-framework/controller/actions/render/helm/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.gooperator-actions-framework/controller/actions/render/render_metrics.gooperator-actions-framework/controller/actions/render/render_support.gooperator-actions-framework/controller/actions/render/template/action_render_templates.gooperator-actions-framework/controller/actions/resourcecacher/resourcecacher.gooperator-actions-framework/controller/actions/sanitycheck/sanitycheck.gooperator-actions-framework/controller/actions/status/deployments/action_deployments_available.gooperator-actions-framework/controller/conditions/conditions.gooperator-actions-framework/controller/conditions/conditions_support.gooperator-actions-framework/controller/handlers/handlers.gooperator-actions-framework/controller/predicates/dependent/dependent.gooperator-actions-framework/controller/predicates/generation/generation.gooperator-actions-framework/controller/predicates/hash/hash.gooperator-actions-framework/controller/predicates/label/label.gooperator-actions-framework/controller/predicates/partial/partial.gooperator-actions-framework/controller/predicates/predicates.gooperator-actions-framework/controller/predicates/resources/resources.gooperator-actions-framework/controller/reconciler/reconciler.gooperator-actions-framework/controller/reconciler/reconciler_actions.gooperator-actions-framework/controller/reconciler/reconciler_actions_test.gooperator-actions-framework/controller/reconciler/reconciler_metrics.gooperator-actions-framework/controller/reconciler/reconciler_support.gooperator-actions-framework/controller/types/types.gooperator-actions-framework/go.modoperator-actions-framework/metadata/metadata.gooperator-actions-framework/resources/resources.gooperator-actions-framework/resources/resources_types.gooperator-actions-framework/resources/sort.gooperator-actions-framework/rules/rules.gooperator-actions-framework/utils/template/template.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher_test.gooperator-actions-framework/utils/test/matchers/jq/jq_support.gooperator-actions-framework/utils/test/matchers/jq/jq_support_test.gooperator-actions-framework/utils/test/matchers/jq/jq_transform.gooperator-actions-framework/utils/test/matchers/jq/jq_transform_test.gooperator-actions-framework/utils/test/matchers/matchers.gopkg/cluster/resources.gopkg/clusterhealth/go.modpkg/controller/actions/actions.gopkg/controller/actions/cacher/cacher.gopkg/controller/actions/deleteresource/action_delete_resources.gopkg/controller/actions/deleteresource/action_delete_resources_test.gopkg/controller/actions/dependency/certmanager/bootstrap.gopkg/controller/actions/deploy/action_deploy.gopkg/controller/actions/deploy/action_deploy_cache.gopkg/controller/actions/deploy/action_deploy_cache_test.gopkg/controller/actions/deploy/action_deploy_managed.gopkg/controller/actions/deploy/action_deploy_merge_deployment.gopkg/controller/actions/deploy/action_deploy_metrics.gopkg/controller/actions/deploy/action_deploy_remove_deployment_resources.gopkg/controller/actions/deploy/action_deploy_support.gopkg/controller/actions/deploy/action_deploy_test.gopkg/controller/actions/dynamicownership/action_dynamic_ownership.gopkg/controller/actions/errors/errors.gopkg/controller/actions/gc/action_gc.gopkg/controller/actions/gc/action_gc_metrics.gopkg/controller/actions/gc/action_gc_support.gopkg/controller/actions/gc/action_gc_test.gopkg/controller/actions/render/helm/action_render_manifests.gopkg/controller/actions/render/kustomize/action_render_manifests_test.gopkg/controller/actions/render/render_metrics.gopkg/controller/actions/render/render_support.gopkg/controller/actions/render/template/action_render_templates.gopkg/controller/actions/render/template/action_render_templates_test.gopkg/controller/actions/resourcecacher/resourcecacher.gopkg/controller/actions/sanitycheck/sanitycheck.gopkg/controller/actions/status/deployments/action_deployments_available.gopkg/controller/actions/status/deployments/action_deployments_available_test.gopkg/controller/actions/status/imagestreams/action_imagestreams_available_test.gopkg/controller/cloudmanager/action_monitor_dependencies_test.gopkg/controller/cloudmanager/action_reconcile_integration_test.gopkg/controller/conditions/conditions.gopkg/controller/handlers/handlers.gopkg/controller/predicates/component/component.gopkg/controller/predicates/dependent/dependent.gopkg/controller/predicates/generation/generation.gopkg/controller/predicates/hash/hash.gopkg/controller/predicates/partial/partial.gopkg/controller/predicates/predicates.gopkg/controller/predicates/resources/resources.gopkg/controller/reconciler/reconciler.gopkg/controller/reconciler/reconciler_finalizer_test.gopkg/controller/reconciler/reconciler_metrics.gopkg/controller/reconciler/reconciler_support.gopkg/controller/reconciler/reconciler_test.gopkg/controller/types/types.gopkg/controller/types/types_test.gopkg/failureclassifier/go.modpkg/metadata/annotations/annotations.gopkg/resources/resources.gopkg/resources/resources_types.gopkg/resources/sort.gopkg/resources/sort_test.gopkg/rules/rules.gopkg/utils/template/template.go
💤 Files with no reviewable changes (31)
- cmd/mcp-server/tool_platform_health_test.go
- pkg/controller/reconciler/reconciler_finalizer_test.go
- pkg/controller/handlers/handlers.go
- docs/cloudmanager-api-overview.md
- pkg/controller/predicates/predicates.go
- pkg/controller/predicates/partial/partial.go
- pkg/controller/predicates/generation/generation.go
- pkg/controller/types/types_test.go
- pkg/metadata/annotations/annotations.go
- pkg/resources/sort.go
- pkg/controller/actions/deploy/action_deploy_support.go
- pkg/controller/predicates/dependent/dependent.go
- pkg/controller/cloudmanager/action_monitor_dependencies_test.go
- pkg/controller/conditions/conditions.go
- pkg/controller/reconciler/reconciler_metrics.go
- pkg/resources/resources.go
- pkg/controller/predicates/component/component.go
- pkg/resources/resources_types.go
- pkg/controller/actions/status/deployments/action_deployments_available.go
- pkg/controller/reconciler/reconciler.go
- pkg/failureclassifier/go.mod
- pkg/controller/cloudmanager/action_reconcile_integration_test.go
- pkg/controller/actions/status/deployments/action_deployments_available_test.go
- api/common/zz_generated.deepcopy.go
- pkg/controller/predicates/hash/hash.go
- pkg/controller/actions/status/imagestreams/action_imagestreams_available_test.go
- pkg/resources/sort_test.go
- pkg/controller/types/types.go
- pkg/controller/predicates/resources/resources.go
- pkg/controller/reconciler/reconciler_support.go
- pkg/controller/reconciler/reconciler_test.go
✅ Files skipped from review due to trivial changes (8)
- internal/controller/components/modelsasservice/modelsasservice_support_test.go
- operator-actions-framework/controller/actions/render/render_support.go
- .gitignore
- cmd/health-check/go.mod
- operator-actions-framework/controller/actions/actions.go
- pkg/controller/actions/render/kustomize/action_render_manifests_test.go
- Dockerfiles/bundle.Dockerfile
- operator-actions-framework/go.mod
🚧 Files skipped from review as they are similar to previous changes (103)
- internal/controller/components/modelcontroller/modelcontroller_controller.go
- operator-actions-framework/utils/test/matchers/matchers.go
- pkg/controller/actions/deleteresource/action_delete_resources_test.go
- operator-actions-framework/controller/predicates/hash/hash.go
- operator-actions-framework/controller/actions/gc/action_gc_metrics.go
- operator-actions-framework/controller/predicates/label/label.go
- internal/controller/modules/modules_controller_actions_inject_env.go
- operator-actions-framework/controller/actions/deploy/action_deploy_metrics.go
- operator-actions-framework/controller/predicates/predicates.go
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.go
- internal/controller/components/kserve/kserve_controller.go
- pkg/clusterhealth/go.mod
- internal/controller/components/trainer/trainer_controller.go
- operator-actions-framework/controller/actions/gc/action_gc_support.go
- pkg/controller/actions/actions.go
- operator-actions-framework/.golangci.yml
- internal/controller/components/kueue/kueue_controller.go
- operator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.go
- internal/controller/datasciencecluster/datasciencecluster_controller_actions.go
- operator-actions-framework/utils/template/template.go
- operator-actions-framework/controller/actions/render/render_metrics.go
- operator-actions-framework/controller/actions/errors/errors.go
- operator-actions-framework/metadata/metadata.go
- pkg/controller/actions/render/template/action_render_templates_test.go
- internal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.go
- operator-actions-framework/controller/predicates/generation/generation.go
- operator-actions-framework/controller/reconciler/reconciler_actions_test.go
- operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go
- internal/controller/components/dashboard/dashboard_controller_actions_test.go
- operator-actions-framework/cluster/cluster.go
- internal/controller/services/monitoring/monitoring_controller_support_test.go
- operator-actions-framework/resources/resources_types.go
- pkg/controller/actions/cacher/cacher.go
- pkg/controller/actions/sanitycheck/sanitycheck.go
- operator-actions-framework/controller/handlers/handlers.go
- cmd/mcp-server/go.mod
- Dockerfiles/rhoai-bundle.Dockerfile
- operator-actions-framework/utils/test/matchers/jq/jq_matcher.go
- pkg/controller/actions/render/helm/action_render_manifests.go
- operator-actions-framework/controller/actions/cacher/cacher.go
- pkg/controller/actions/gc/action_gc_support.go
- internal/controller/services/auth/auth_controller_actions_test.go
- pkg/controller/actions/render/render_metrics.go
- pkg/controller/actions/render/template/action_render_templates.go
- internal/controller/modules/modules_controller_actions.go
- Dockerfiles/rhoai.Dockerfile
- pkg/controller/actions/deleteresource/action_delete_resources.go
- pkg/controller/actions/deploy/action_deploy.go
- pkg/controller/actions/deploy/action_deploy_managed.go
- pkg/controller/actions/deploy/action_deploy_merge_deployment.go
- operator-actions-framework/controller/actions/deploy/action_deploy_support.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.go
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go
- pkg/controller/actions/deploy/action_deploy_cache.go
- operator-actions-framework/controller/actions/render/template/action_render_templates.go
- operator-actions-framework/controller/actions/render/helm/action_render_manifests.go
- pkg/controller/actions/dependency/certmanager/bootstrap.go
- operator-actions-framework/api/types.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform_test.go
- api/common/types.go
- pkg/controller/actions/gc/action_gc_metrics.go
- operator-actions-framework/controller/reconciler/reconciler_metrics.go
- operator-actions-framework/controller/actions/deploy/action_deploy_cache.go
- operator-actions-framework/resources/sort.go
- pkg/controller/actions/dynamicownership/action_dynamic_ownership.go
- operator-actions-framework/controller/conditions/conditions_support.go
- operator-actions-framework/controller/actions/resourcecacher/resourcecacher.go
- operator-actions-framework/controller/predicates/dependent/dependent.go
- pkg/cluster/resources.go
- internal/controller/components/trustyai/trustyai_test.go
- internal/controller/modules/modules_controller_actions_inject_env_test.go
- operator-actions-framework/controller/predicates/partial/partial.go
- Dockerfiles/build-bundle.Dockerfile
- pkg/controller/actions/deploy/action_deploy_metrics.go
- operator-actions-framework/utils/test/matchers/jq/jq_matcher_test.go
- pkg/controller/actions/resourcecacher/resourcecacher.go
- pkg/controller/actions/deploy/action_deploy_remove_deployment_resources.go
- Dockerfiles/Dockerfile
- pkg/controller/actions/gc/action_gc_test.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform.go
- operator-actions-framework/controller/actions/status/deployments/action_deployments_available.go
- pkg/controller/actions/deploy/action_deploy_cache_test.go
- operator-actions-framework/utils/test/matchers/jq/jq_support_test.go
- operator-actions-framework/controller/conditions/conditions.go
- operator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.go
- operator-actions-framework/utils/test/matchers/jq/jq_support.go
- go.mod
- pkg/controller/actions/gc/action_gc.go
- operator-actions-framework/resources/resources.go
- pkg/controller/actions/render/render_support.go
- operator-actions-framework/controller/reconciler/reconciler_actions.go
- operator-actions-framework/controller/predicates/resources/resources.go
- operator-actions-framework/controller/reconciler/reconciler.go
- operator-actions-framework/controller/types/types.go
- operator-actions-framework/rules/rules.go
- operator-actions-framework/controller/actions/gc/action_gc.go
- operator-actions-framework/cluster/gvk/gvk.go
- pkg/controller/actions/deploy/action_deploy_test.go
- operator-actions-framework/controller/actions/sanitycheck/sanitycheck.go
- operator-actions-framework/controller/reconciler/reconciler_support.go
- operator-actions-framework/controller/actions/deploy/action_deploy_managed.go
- docs/api-overview.md
Comment on lines
+540
to
+562
| if old == nil { | ||
| err := cli.Create(ctx, obj) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) | ||
| } | ||
| } else { | ||
| data, err := json.Marshal(obj) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = cli.Patch( | ||
| ctx, | ||
| old, | ||
| client.RawPatch(types.ApplyPatchType, data), | ||
| opts..., | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to patch object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) | ||
| } | ||
| } | ||
|
|
||
| return old, nil |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'cache.go' operator-actions-framework/controller/actions/deploy -x sed -n '1,200p'
ast-grep run --pattern 'func ($_ *Cache) Add($$$) $$$ { $$$ }' --lang go operator-actions-frameworkRepository: opendatahub-io/opendatahub-operator
Length of output: 4577
🏁 Script executed:
rg -n "cache\.Add" operator-actions-framework/controller/actions/deploy/action_deploy.go -A 2 -B 2Repository: opendatahub-io/opendatahub-operator
Length of output: 508
🏁 Script executed:
sed -n '390,410p' operator-actions-framework/controller/actions/deploy/action_deploy.goRepository: opendatahub-io/opendatahub-operator
Length of output: 719
🏁 Script executed:
sed -n '490,500p' operator-actions-framework/controller/actions/deploy/action_deploy.goRepository: opendatahub-io/opendatahub-operator
Length of output: 355
Incorrect return value breaks create operation: patch() returns nil on successful creation (CWE-391).
When old == nil, the create operation succeeds but the function returns (nil, nil) instead of (obj, nil). This causes deployedObj to be nil at lines 407 and 495, where it's passed to cache.Add(nil, origObj). The cache rejects nil with error "invalid input", causing the entire operation to fail despite the resource being successfully created. The caller receives "failed to cache object: invalid input" instead of the created object, breaking the intended behavior. The apply path (line 588) correctly returns the created object; patch must do the same.
Proposed fix
if old == nil {
err := cli.Create(ctx, obj)
if err != nil {
return nil, fmt.Errorf("failed to create object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err)
}
+ return obj, nil
} else {
data, err := json.Marshal(obj)
if err != nil {
return nil, err
}
err = cli.Patch(
ctx,
old,
client.RawPatch(types.ApplyPatchType, data),
opts...,
)
if err != nil {
return nil, fmt.Errorf("failed to patch object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err)
}
}
return old, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if old == nil { | |
| err := cli.Create(ctx, obj) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) | |
| } | |
| } else { | |
| data, err := json.Marshal(obj) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = cli.Patch( | |
| ctx, | |
| old, | |
| client.RawPatch(types.ApplyPatchType, data), | |
| opts..., | |
| ) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to patch object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) | |
| } | |
| } | |
| return old, nil | |
| if old == nil { | |
| err := cli.Create(ctx, obj) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) | |
| } | |
| return obj, nil | |
| } else { | |
| data, err := json.Marshal(obj) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = cli.Patch( | |
| ctx, | |
| old, | |
| client.RawPatch(types.ApplyPatchType, data), | |
| opts..., | |
| ) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to patch object %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) | |
| } | |
| } | |
| return old, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-actions-framework/controller/actions/deploy/action_deploy.go` around
lines 540 - 562, The function returns the wrong object after successfully
creating a new resource when old is nil. In the create branch where old == nil,
the function performs a successful create operation but then returns old (which
is still nil) instead of the newly created object obj. Fix this by modifying the
final return statement to conditionally return the appropriate object: return
obj when old is nil (creation succeeded), or return old after the patch
succeeds. Alternatively, if both paths result in the same final state, return
obj unconditionally after both the create and patch operations complete
successfully.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 668: The unit-test-operator target is excluding
operator-actions-framework from CI coverage, leaving that module ungated. Update
the Makefile rule for unit-test-operator so operator-actions-framework is still
included in the test/lint flow, and verify the relevant CI workflow continues to
run that module by keeping the skip-package list aligned with the intended
coverage.
In `@operator-actions-framework/controller/actions/deploy/action_deploy.go`:
- Around line 452-459: The create path in actionDeploy should not treat
k8serr.IsAlreadyExists as a successful apply when calling a.create after the
NotFound check. Update the switch branch around deployedObj, err = a.create(...)
so AlreadyExists returns a non-nil error (or otherwise forces a re-read) and
does not let this reconciler cache obj as newly deployed. Make the fix in the
actionDeploy method and keep the deployedObj assignment/return logic aligned
with actual ownership so stale cache state cannot suppress future
reconciliation.
In `@operator-actions-framework/Makefile`:
- Line 20: Quote the golangci-lint executable path in the Makefile recipes that
invoke $(GOLANGCI_LINT), especially the fmt target and the related targets
around the other occurrences, so shell word-splitting or globbing from
PROJECT_DIR cannot alter the command path. Update the affected recipe lines to
treat $(GOLANGCI_LINT) as a single shell argument wherever it is executed.
In `@operator-actions-framework/rules/rules.go`:
- Around line 42-46: RetrieveSelfSubjectRules in rules.go currently logs
EvaluationError but still returns ResourceRules even when the
SelfSubjectRulesReview is incomplete. Update the function to fail closed by
checking rulesReview.Status.Incomplete and returning an error instead of the
rules; include rulesReview.Status.EvaluationError in that error context, and
keep the existing ResourceRules return only for complete results.
- Around line 79-83: The RBAC check in RequiredVerbs/IsResourceMatchingRule is
incorrectly treating rules with ResourceNames as full resource authorization.
Update the matching logic so ResourceNames-scoped rules are not accepted for
whole-resource verbs like list/delete, and instead either reject them in this
path or handle them only when a specific object name is being checked. Use the
existing RequiredVerbs loop and IsResourceMatchingRule helper to locate and
adjust the authorization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1ab9aa73-1f0f-4d59-8b12-346c685396a5
⛔ Files ignored due to path filters (5)
cmd/health-check/go.sumis excluded by!**/*.sum,!**/*.sumcmd/mcp-server/go.sumis excluded by!**/*.sum,!**/*.sumoperator-actions-framework/go.sumis excluded by!**/*.sum,!**/*.sumpkg/clusterhealth/go.sumis excluded by!**/*.sum,!**/*.sumpkg/failureclassifier/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (148)
.gitignoreDockerfiles/DockerfileDockerfiles/build-bundle.DockerfileDockerfiles/bundle.DockerfileDockerfiles/rhoai-bundle.DockerfileDockerfiles/rhoai.DockerfileMakefileapi/common/types.goapi/common/zz_generated.deepcopy.gocmd/health-check/go.modcmd/mcp-server/go.modcmd/mcp-server/tool_platform_health_test.godocs/api-overview.mddocs/cloudmanager-api-overview.mdgo.modinternal/controller/components/dashboard/dashboard_controller_actions_test.gointernal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.gointernal/controller/components/kserve/kserve_controller.gointernal/controller/components/kueue/kueue_controller.gointernal/controller/components/modelcontroller/modelcontroller_controller.gointernal/controller/components/trainer/trainer_controller.gointernal/controller/components/trustyai/trustyai_test.gointernal/controller/datasciencecluster/datasciencecluster_controller_actions.gointernal/controller/modules/modules_controller_actions.gointernal/controller/modules/modules_controller_actions_inject_env.gointernal/controller/modules/modules_controller_actions_inject_env_test.gointernal/controller/services/auth/auth_controller_actions_test.gointernal/controller/services/monitoring/monitoring_controller_support_test.gooperator-actions-framework/.golangci.ymloperator-actions-framework/Makefileoperator-actions-framework/api/types.gooperator-actions-framework/cluster/cluster.gooperator-actions-framework/cluster/gvk/gvk.gooperator-actions-framework/controller/actions/actions.gooperator-actions-framework/controller/actions/cacher/cacher.gooperator-actions-framework/controller/actions/deleteresource/action_delete_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy.gooperator-actions-framework/controller/actions/deploy/action_deploy_cache.gooperator-actions-framework/controller/actions/deploy/action_deploy_managed.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring.gooperator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.gooperator-actions-framework/controller/actions/deploy/action_deploy_metrics.gooperator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.gooperator-actions-framework/controller/actions/deploy/action_deploy_support.gooperator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.gooperator-actions-framework/controller/actions/errors/errors.gooperator-actions-framework/controller/actions/gc/action_gc.gooperator-actions-framework/controller/actions/gc/action_gc_metrics.gooperator-actions-framework/controller/actions/gc/action_gc_support.gooperator-actions-framework/controller/actions/render/helm/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests.gooperator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.gooperator-actions-framework/controller/actions/render/render_metrics.gooperator-actions-framework/controller/actions/render/render_support.gooperator-actions-framework/controller/actions/render/template/action_render_templates.gooperator-actions-framework/controller/actions/resourcecacher/resourcecacher.gooperator-actions-framework/controller/actions/sanitycheck/sanitycheck.gooperator-actions-framework/controller/actions/status/deployments/action_deployments_available.gooperator-actions-framework/controller/conditions/conditions.gooperator-actions-framework/controller/conditions/conditions_support.gooperator-actions-framework/controller/handlers/handlers.gooperator-actions-framework/controller/predicates/dependent/dependent.gooperator-actions-framework/controller/predicates/generation/generation.gooperator-actions-framework/controller/predicates/hash/hash.gooperator-actions-framework/controller/predicates/label/label.gooperator-actions-framework/controller/predicates/partial/partial.gooperator-actions-framework/controller/predicates/predicates.gooperator-actions-framework/controller/predicates/resources/resources.gooperator-actions-framework/controller/reconciler/reconciler.gooperator-actions-framework/controller/reconciler/reconciler_actions.gooperator-actions-framework/controller/reconciler/reconciler_actions_test.gooperator-actions-framework/controller/reconciler/reconciler_metrics.gooperator-actions-framework/controller/reconciler/reconciler_support.gooperator-actions-framework/controller/types/types.gooperator-actions-framework/go.modoperator-actions-framework/metadata/metadata.gooperator-actions-framework/resources/resources.gooperator-actions-framework/resources/resources_types.gooperator-actions-framework/resources/sort.gooperator-actions-framework/rules/rules.gooperator-actions-framework/utils/template/template.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher.gooperator-actions-framework/utils/test/matchers/jq/jq_matcher_test.gooperator-actions-framework/utils/test/matchers/jq/jq_support.gooperator-actions-framework/utils/test/matchers/jq/jq_support_test.gooperator-actions-framework/utils/test/matchers/jq/jq_transform.gooperator-actions-framework/utils/test/matchers/jq/jq_transform_test.gooperator-actions-framework/utils/test/matchers/matchers.gopkg/cluster/resources.gopkg/clusterhealth/go.modpkg/controller/actions/actions.gopkg/controller/actions/cacher/cacher.gopkg/controller/actions/deleteresource/action_delete_resources.gopkg/controller/actions/deleteresource/action_delete_resources_test.gopkg/controller/actions/dependency/certmanager/bootstrap.gopkg/controller/actions/deploy/action_deploy.gopkg/controller/actions/deploy/action_deploy_cache.gopkg/controller/actions/deploy/action_deploy_cache_test.gopkg/controller/actions/deploy/action_deploy_managed.gopkg/controller/actions/deploy/action_deploy_merge_deployment.gopkg/controller/actions/deploy/action_deploy_metrics.gopkg/controller/actions/deploy/action_deploy_remove_deployment_resources.gopkg/controller/actions/deploy/action_deploy_support.gopkg/controller/actions/deploy/action_deploy_test.gopkg/controller/actions/dynamicownership/action_dynamic_ownership.gopkg/controller/actions/errors/errors.gopkg/controller/actions/gc/action_gc.gopkg/controller/actions/gc/action_gc_metrics.gopkg/controller/actions/gc/action_gc_support.gopkg/controller/actions/gc/action_gc_test.gopkg/controller/actions/render/helm/action_render_manifests.gopkg/controller/actions/render/kustomize/action_render_manifests_test.gopkg/controller/actions/render/render_metrics.gopkg/controller/actions/render/render_support.gopkg/controller/actions/render/template/action_render_templates.gopkg/controller/actions/render/template/action_render_templates_test.gopkg/controller/actions/resourcecacher/resourcecacher.gopkg/controller/actions/sanitycheck/sanitycheck.gopkg/controller/actions/status/deployments/action_deployments_available.gopkg/controller/actions/status/deployments/action_deployments_available_test.gopkg/controller/actions/status/imagestreams/action_imagestreams_available_test.gopkg/controller/cloudmanager/action_monitor_dependencies_test.gopkg/controller/cloudmanager/action_reconcile_integration_test.gopkg/controller/conditions/conditions.gopkg/controller/handlers/handlers.gopkg/controller/predicates/component/component.gopkg/controller/predicates/dependent/dependent.gopkg/controller/predicates/generation/generation.gopkg/controller/predicates/hash/hash.gopkg/controller/predicates/partial/partial.gopkg/controller/predicates/predicates.gopkg/controller/predicates/resources/resources.gopkg/controller/reconciler/reconciler.gopkg/controller/reconciler/reconciler_finalizer_test.gopkg/controller/reconciler/reconciler_metrics.gopkg/controller/reconciler/reconciler_support.gopkg/controller/reconciler/reconciler_test.gopkg/controller/types/types.gopkg/controller/types/types_test.gopkg/failureclassifier/go.modpkg/metadata/annotations/annotations.gopkg/resources/resources.gopkg/resources/resources_types.gopkg/resources/sort.gopkg/resources/sort_test.gopkg/rules/rules.gopkg/utils/template/template.go
💤 Files with no reviewable changes (4)
- docs/cloudmanager-api-overview.md
- cmd/mcp-server/tool_platform_health_test.go
- pkg/controller/actions/deploy/action_deploy_support.go
- api/common/zz_generated.deepcopy.go
✅ Files skipped from review due to trivial changes (6)
- operator-actions-framework/controller/predicates/predicates.go
- operator-actions-framework/metadata/metadata.go
- operator-actions-framework/controller/reconciler/reconciler_metrics.go
- .gitignore
- operator-actions-framework/utils/template/template.go
- internal/controller/services/auth/auth_controller_actions_test.go
🚧 Files skipped from review as they are similar to previous changes (131)
- pkg/controller/actions/deleteresource/action_delete_resources_test.go
- internal/controller/datasciencecluster/datasciencecluster_controller_actions.go
- operator-actions-framework/utils/test/matchers/matchers.go
- operator-actions-framework/controller/predicates/hash/hash.go
- pkg/controller/reconciler/reconciler_metrics.go
- pkg/controller/types/types_test.go
- pkg/controller/actions/render/render_metrics.go
- operator-actions-framework/controller/actions/gc/action_gc_metrics.go
- pkg/controller/actions/deploy/action_deploy_remove_deployment_resources.go
- operator-actions-framework/controller/actions/render/render_metrics.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform_test.go
- pkg/failureclassifier/go.mod
- pkg/controller/actions/deploy/action_deploy_merge_deployment.go
- operator-actions-framework/controller/actions/render/render_support.go
- operator-actions-framework/controller/actions/deploy/action_deploy_metrics.go
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_monitoring_test.go
- internal/controller/components/kueue/kueue_controller.go
- pkg/controller/handlers/handlers.go
- internal/controller/components/datasciencepipelines/datasciencepipelines_controller_actions_test.go
- operator-actions-framework/controller/actions/deploy/action_deploy_support.go
- internal/controller/components/modelcontroller/modelcontroller_controller.go
- pkg/controller/predicates/component/component.go
- pkg/controller/predicates/generation/generation.go
- pkg/resources/sort.go
- pkg/controller/predicates/dependent/dependent.go
- operator-actions-framework/cluster/cluster.go
- pkg/controller/conditions/conditions.go
- pkg/resources/sort_test.go
- pkg/controller/actions/sanitycheck/sanitycheck.go
- pkg/controller/actions/actions.go
- operator-actions-framework/api/types.go
- pkg/controller/actions/errors/errors.go
- operator-actions-framework/utils/test/matchers/jq/jq_support_test.go
- pkg/controller/actions/render/render_support.go
- pkg/controller/reconciler/reconciler_finalizer_test.go
- internal/controller/components/dashboard/dashboard_controller_actions_test.go
- operator-actions-framework/controller/predicates/label/label.go
- pkg/controller/actions/render/helm/action_render_manifests.go
- internal/controller/components/trustyai/trustyai_test.go
- Dockerfiles/bundle.Dockerfile
- pkg/controller/actions/dependency/certmanager/bootstrap.go
- operator-actions-framework/.golangci.yml
- operator-actions-framework/controller/actions/actions.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests_test.go
- pkg/controller/predicates/partial/partial.go
- pkg/controller/actions/deploy/action_deploy_cache.go
- internal/controller/services/monitoring/monitoring_controller_support_test.go
- Dockerfiles/rhoai-bundle.Dockerfile
- pkg/controller/actions/deploy/action_deploy_managed.go
- pkg/controller/actions/gc/action_gc_test.go
- operator-actions-framework/controller/actions/errors/errors.go
- operator-actions-framework/utils/test/matchers/jq/jq_matcher_test.go
- operator-actions-framework/controller/actions/deleteresource/action_delete_resources.go
- pkg/controller/reconciler/reconciler.go
- internal/controller/components/trainer/trainer_controller.go
- pkg/controller/actions/status/imagestreams/action_imagestreams_available_test.go
- pkg/controller/actions/deleteresource/action_delete_resources.go
- pkg/controller/actions/gc/action_gc.go
- pkg/controller/predicates/predicates.go
- pkg/resources/resources_types.go
- pkg/metadata/annotations/annotations.go
- pkg/controller/actions/render/kustomize/action_render_manifests_test.go
- pkg/controller/actions/deploy/action_deploy_metrics.go
- cmd/mcp-server/go.mod
- pkg/controller/actions/cacher/cacher.go
- operator-actions-framework/controller/predicates/dependent/dependent.go
- pkg/controller/cloudmanager/action_monitor_dependencies_test.go
- operator-actions-framework/controller/actions/render/kustomize/action_render_manifests.go
- pkg/controller/actions/render/template/action_render_templates.go
- operator-actions-framework/controller/predicates/partial/partial.go
- operator-actions-framework/controller/handlers/handlers.go
- cmd/health-check/go.mod
- operator-actions-framework/controller/actions/status/deployments/action_deployments_available.go
- operator-actions-framework/go.mod
- operator-actions-framework/controller/actions/render/helm/action_render_manifests.go
- operator-actions-framework/controller/conditions/conditions_support.go
- internal/controller/components/kserve/kserve_controller.go
- pkg/controller/actions/gc/action_gc_metrics.go
- Dockerfiles/build-bundle.Dockerfile
- operator-actions-framework/resources/resources_types.go
- operator-actions-framework/controller/actions/render/template/action_render_templates.go
- operator-actions-framework/controller/predicates/generation/generation.go
- operator-actions-framework/utils/test/matchers/jq/jq_support.go
- operator-actions-framework/utils/test/matchers/jq/jq_matcher.go
- operator-actions-framework/controller/actions/gc/action_gc_support.go
- internal/controller/modules/modules_controller_actions_inject_env.go
- pkg/controller/actions/resourcecacher/resourcecacher.go
- pkg/controller/actions/status/deployments/action_deployments_available_test.go
- pkg/controller/actions/deploy/action_deploy_cache_test.go
- pkg/controller/actions/gc/action_gc_support.go
- pkg/clusterhealth/go.mod
- pkg/controller/actions/deploy/action_deploy.go
- operator-actions-framework/controller/reconciler/reconciler_actions.go
- pkg/controller/cloudmanager/action_reconcile_integration_test.go
- Dockerfiles/Dockerfile
- operator-actions-framework/controller/actions/resourcecacher/resourcecacher.go
- go.mod
- pkg/controller/actions/render/template/action_render_templates_test.go
- operator-actions-framework/controller/actions/deploy/action_deploy_merge_deployment.go
- pkg/controller/actions/dynamicownership/action_dynamic_ownership.go
- pkg/controller/predicates/hash/hash.go
- operator-actions-framework/resources/sort.go
- internal/controller/modules/modules_controller_actions.go
- operator-actions-framework/controller/actions/sanitycheck/sanitycheck.go
- operator-actions-framework/controller/actions/dynamicownership/action_dynamic_ownership.go
- internal/controller/modules/modules_controller_actions_inject_env_test.go
- operator-actions-framework/controller/actions/cacher/cacher.go
- api/common/types.go
- Dockerfiles/rhoai.Dockerfile
- pkg/controller/reconciler/reconciler_support.go
- pkg/cluster/resources.go
- operator-actions-framework/controller/reconciler/reconciler_actions_test.go
- operator-actions-framework/controller/actions/deploy/action_deploy_cache.go
- pkg/controller/actions/status/deployments/action_deployments_available.go
- pkg/controller/types/types.go
- operator-actions-framework/utils/test/matchers/jq/jq_transform.go
- pkg/controller/actions/deploy/action_deploy_test.go
- operator-actions-framework/controller/types/types.go
- operator-actions-framework/controller/actions/deploy/action_deploy_remove_deployment_resources.go
- operator-actions-framework/controller/predicates/resources/resources.go
- pkg/resources/resources.go
- operator-actions-framework/controller/actions/deploy/action_deploy_managed.go
- docs/api-overview.md
- pkg/controller/predicates/resources/resources.go
- operator-actions-framework/controller/reconciler/reconciler_support.go
- operator-actions-framework/cluster/gvk/gvk.go
- operator-actions-framework/controller/conditions/conditions.go
- operator-actions-framework/controller/actions/gc/action_gc.go
- operator-actions-framework/resources/resources.go
- pkg/controller/reconciler/reconciler_test.go
- operator-actions-framework/controller/reconciler/reconciler.go
| --coverprofile=cover.out \ | ||
| --succinct \ | ||
| --skip-package=pkg/clusterhealth,cmd/health-check \ | ||
| --skip-package=pkg/clusterhealth,cmd/health-check,operator-actions-framework \ |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Make targets referencing tests/lint/framework =="
rg -n --glob 'Makefile' --glob '**/Makefile' 'unit-test-operator|operator-actions-framework|ginkgo|test|lint'
echo
echo "== CI workflows invoking those targets =="
fd -H -t f '.+\.ya?ml$' .github/workflows | while read -r f; do
rg -n 'unit-test-operator|operator-actions-framework|make .*test|make .*lint|ginkgo' "$f" || true
doneRepository: opendatahub-io/opendatahub-operator
Length of output: 327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files of interest =="
git ls-files | rg '(^|/)(Makefile|\.github/workflows/.*\.ya?ml)$'
echo
echo "== unit-test-operator target and surrounding context =="
awk 'NR>=640 && NR<=690 {print NR ":" $0}' Makefile
echo
echo "== Search for operator-actions-framework tests/lint references =="
rg -n 'operator-actions-framework|unit-test-operator|ginkgo|go test|golangci|lint' .github/workflows Makefile operator-actions-framework/Makefile || trueRepository: opendatahub-io/opendatahub-operator
Length of output: 11391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files of interest =="
git ls-files | rg '(^|/)(Makefile|\.github/workflows/.*\.ya?ml)$'
echo
echo "== unit-test-operator target and surrounding context =="
awk 'NR>=640 && NR<=690 {print NR ":" $0}' Makefile
echo
echo "== Search for operator-actions-framework tests/lint references =="
rg -n 'operator-actions-framework|unit-test-operator|ginkgo|go test|golangci|lint' .github/workflows Makefile operator-actions-framework/Makefile || trueRepository: opendatahub-io/opendatahub-operator
Length of output: 11391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant workflow files =="
fd -H -t f -e yml -e yaml .github/workflows
echo
echo "== Search exact target names across workflows and Makefiles =="
rg -n --hidden --glob '.github/workflows/**' --glob 'Makefile' --glob '**/Makefile' \
'unit-test-operator|operator-actions-framework|ginkgo|go test|golangci|lint|test:' .
echo
echo "== operator-actions-framework Makefile snippet =="
if [ -f operator-actions-framework/Makefile ]; then
awk 'NR>=1 && NR<=220 {print NR ":" $0}' operator-actions-framework/Makefile
fiRepository: opendatahub-io/opendatahub-operator
Length of output: 577
Keep operator-actions-framework in CI coverage
Makefile:668 drops operator-actions-framework from unit-test-operator, and no workflow runs that module’s tests or lint. That leaves the module outside the gate and opens a CWE-693 bypass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` at line 668, The unit-test-operator target is excluding
operator-actions-framework from CI coverage, leaving that module ungated. Update
the Makefile rule for unit-test-operator so operator-actions-framework is still
included in the test/lint flow, and verify the relevant CI workflow continues to
run that module by keeping the skip-package list aligned with the intended
coverage.
Source: Path instructions
Comment on lines
+452
to
+459
| switch { | ||
| case resources.GetAnnotation(&obj, a.managedByAnnotation) == "false": | ||
| resources.RemoveAnnotation(&obj, a.managedByAnnotation) | ||
|
|
||
| deployedObj, err = a.create(ctx, rr.Client, &obj) | ||
| if err != nil && !k8serr.IsAlreadyExists(err) { | ||
| return false, err | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not treat AlreadyExists as a successful create.
Line 456 follows a prior NotFound lookup; if another actor creates the object before this Create, Line 457 ignores AlreadyExists and the code can still cache deployedObj as if this reconciler applied it. That is a TOCTOU reconciliation bug, CWE-367, and can suppress future applies with stale cache state.
Remediation
deployedObj, err = a.create(ctx, rr.Client, &obj)
- if err != nil && !k8serr.IsAlreadyExists(err) {
- return false, err
+ if err != nil {
+ return false, fmt.Errorf("failed to create resource %s/%s: %w", obj.GetNamespace(), obj.GetName(), err)
}As per path instructions, "Reconciliation loop safety" and "Bug-prone patterns and error handling gaps".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch { | |
| case resources.GetAnnotation(&obj, a.managedByAnnotation) == "false": | |
| resources.RemoveAnnotation(&obj, a.managedByAnnotation) | |
| deployedObj, err = a.create(ctx, rr.Client, &obj) | |
| if err != nil && !k8serr.IsAlreadyExists(err) { | |
| return false, err | |
| } | |
| switch { | |
| case resources.GetAnnotation(&obj, a.managedByAnnotation) == "false": | |
| resources.RemoveAnnotation(&obj, a.managedByAnnotation) | |
| deployedObj, err = a.create(ctx, rr.Client, &obj) | |
| if err != nil { | |
| return false, fmt.Errorf("failed to create resource %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-actions-framework/controller/actions/deploy/action_deploy.go` around
lines 452 - 459, The create path in actionDeploy should not treat
k8serr.IsAlreadyExists as a successful apply when calling a.create after the
NotFound check. Update the switch branch around deployedObj, err = a.create(...)
so AlreadyExists returns a non-nil error (or otherwise forces a re-read) and
does not let this reconciler cache obj as newly deployed. Make the fix in the
actionDeploy method and keep the deployedObj assignment/return logic aligned
with actual ownership so stale cache state cannot suppress future
reconciliation.
Source: Path instructions
| .PHONY: fmt | ||
| fmt: golangci-lint ## Format code and imports. | ||
| go fmt ./... | ||
| $(GOLANGCI_LINT) fmt |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Quote the golangci-lint executable path.
$(GOLANGCI_LINT) expands to $(LOCALBIN)/golangci-lint; if PROJECT_DIR contains whitespace or glob characters, these recipes execute the wrong command path. This is shell word-splitting/globbing risk, CWE-78/CWE-88.
Remediation
- $(GOLANGCI_LINT) fmt
+ "$(GOLANGCI_LINT)" fmt
@@
- $(GOLANGCI_LINT) run --timeout=5m
+ "$(GOLANGCI_LINT)" run --timeout=5m
@@
- $(GOLANGCI_LINT) run --fix --timeout=5m
+ "$(GOLANGCI_LINT)" run --fix --timeout=5mAs per path instructions, "Quote shell variables in targets".
Also applies to: 56-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-actions-framework/Makefile` at line 20, Quote the golangci-lint
executable path in the Makefile recipes that invoke $(GOLANGCI_LINT), especially
the fmt target and the related targets around the other occurrences, so shell
word-splitting or globbing from PROJECT_DIR cannot alter the command path.
Update the affected recipe lines to treat $(GOLANGCI_LINT) as a single shell
argument wherever it is executed.
Source: Path instructions
Comment on lines
+42
to
+46
| if rulesReview.Status.EvaluationError != "" { | ||
| logf.FromContext(ctx).Info("error occurred during rule evaluation: " + rulesReview.Status.EvaluationError) | ||
| } | ||
|
|
||
| return rulesReview.Status.ResourceRules, nil |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the helper currently returns partial SelfSubjectRulesReview results
# and inspect callers that rely on the result for resource authorization.
rg -n 'Status\.Incomplete|EvaluationError|RetrieveSelfSubjectRules|ListAuthorizedResources|ComputeAuthorizedResources' operator-actions-frameworkRepository: opendatahub-io/opendatahub-operator
Length of output: 1049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and the immediate call sites.
sed -n '1,240p' operator-actions-framework/rules/rules.go
printf '\n--- callers ---\n'
sed -n '180,230p' operator-actions-framework/controller/actions/gc/action_gc.goRepository: opendatahub-io/opendatahub-operator
Length of output: 5884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and the immediate call sites.
sed -n '1,240p' operator-actions-framework/rules/rules.go
printf '\n--- callers ---\n'
sed -n '180,230p' operator-actions-framework/controller/actions/gc/action_gc.goRepository: opendatahub-io/opendatahub-operator
Length of output: 5884
Fail closed on incomplete SelfSubjectRulesReview results. RetrieveSelfSubjectRules logs EvaluationError but still returns status.resourceRules; if the review is incomplete, downstream code treats a partial RBAC set as authoritative and can make incorrect authorization decisions. Return an error when status.incomplete is true (and include evaluationError in that error). CWE-863.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-actions-framework/rules/rules.go` around lines 42 - 46,
RetrieveSelfSubjectRules in rules.go currently logs EvaluationError but still
returns ResourceRules even when the SelfSubjectRulesReview is incomplete. Update
the function to fail closed by checking rulesReview.Status.Incomplete and
returning an error instead of the rules; include
rulesReview.Status.EvaluationError in that error context, and keep the existing
ResourceRules return only for complete results.
Comment on lines
+79
to
+83
| for _, requiredVerb := range requiredVerbs { | ||
| if !slices.ContainsFunc(permissionRules, func(rule authorizationv1.ResourceRule) bool { | ||
| return (slices.Contains(rule.Verbs, requiredVerb) || slices.Contains(rule.Verbs, VerbAny)) && | ||
| IsResourceMatchingRule(group, apiRes, rule) | ||
| }) { |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no existing ResourceNames handling exists and inspect authorization call sites.
rg -n 'ResourceNames|HasPermissions|ComputeAuthorizedResources|ListAuthorizedResources' operator-actions-frameworkRepository: opendatahub-io/opendatahub-operator
Length of output: 798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant authorization logic and its call sites.
file="operator-actions-framework/rules/rules.go"
wc -l "$file"
sed -n '1,220p' "$file"
echo "---- call site ----"
sed -n '180,240p' operator-actions-framework/controller/actions/gc/action_gc.goRepository: opendatahub-io/opendatahub-operator
Length of output: 6131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant functions structurally to inspect conditions around ResourceNames.
ast-grep outline operator-actions-framework/rules/rules.go --view expandedRepository: opendatahub-io/opendatahub-operator
Length of output: 458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant authorization logic and its call sites.
file="operator-actions-framework/rules/rules.go"
wc -l "$file"
sed -n '1,220p' "$file"
echo "---- call site ----"
sed -n '180,240p' operator-actions-framework/controller/actions/gc/action_gc.goRepository: opendatahub-io/opendatahub-operator
Length of output: 6131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant functions structurally to inspect conditions around ResourceNames.
ast-grep outline operator-actions-framework/rules/rules.go --view expandedRepository: opendatahub-io/opendatahub-operator
Length of output: 458
Do not widen resourceNames-scoped RBAC to whole-resource access. operator-actions-framework/rules/rules.go:69-83 treats a rule with ResourceNames as authorization for the entire resource type, which is incorrect under Kubernetes RBAC and can drive controllers into unauthorized delete/list flows. CWE-863. Reject ResourceNames-scoped rules here or evaluate them separately at the object-name level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-actions-framework/rules/rules.go` around lines 79 - 83, The RBAC
check in RequiredVerbs/IsResourceMatchingRule is incorrectly treating rules with
ResourceNames as full resource authorization. Update the matching logic so
ResourceNames-scoped rules are not accepted for whole-resource verbs like
list/delete, and instead either reject them in this path or handle them only
when a specific object name is being checked. Use the existing RequiredVerbs
loop and IsResourceMatchingRule helper to locate and adjust the authorization
behavior.
3 tasks
valdar
force-pushed
the
operatorUtils
branch
2 times, most recently
from
648ce7d to
9a7720c
Compare
Member
Author
|
/retest-required |
1 similar comment
Member
Author
|
/retest-required |
valdar
force-pushed
the
operatorUtils
branch
3 times, most recently
from
c75c77b to
5aa7751
Compare
…-framework/ go module
|
@valdar: 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. |
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
2 participants
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.
…-framework/ go module
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification
Code was just moved to a separate module, no feature added. Existing e2e tests should be enough.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor