ROX-36158: track deployment managing resource - #22102
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds a ChangesManaging resource tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant StaticResource
participant ResourceConverter
participant OwnerReferenceSelector
participant Deployment
StaticResource->>ResourceConverter: owner references
ResourceConverter->>OwnerReferenceSelector: select managing owner
OwnerReferenceSelector->>Deployment: ManagingResource metadata
ResourceConverter->>Deployment: converted deployment
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
proto/storage/deployment.proto (1)
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the field comment with the actual behavior.
The comment states the field is populated when the owner chain includes a CRD controller "(or in addition to)" native types.
NewDeploymentFromStaticResourcereturnsnil, nilwhen any tracked native owner reference exists. In that case no deployment record is produced at all, so the "in addition to" case never reaches this field.📝 Proposed comment change
// Non-native (CRD) resource that manages this deployment. Populated when - // a deployment's OwnerReference chain includes a CRD controller rather than - // (or in addition to) native Kubernetes resource types. + // the top-level deployment's controller OwnerReference points to a CRD + // rather than a native Kubernetes resource type. Resources owned by a + // tracked native resource are not reported as deployments at all. ManagingResource managing_resource = 36;🤖 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 `@proto/storage/deployment.proto` around lines 66 - 70, Update the comment for ManagingResource.managing_resource to state that it is populated only when the deployment’s OwnerReference chain includes a CRD controller and no tracked native owner reference causes the deployment to be discarded; remove the misleading “or in addition to” wording.pkg/protoconv/resources/resources_test.go (1)
362-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
protoassert.Equalto compare protobuf messages.
tc.expectedandresultare*storage.ManagingResource. Theassert.Equalfunction falls back toreflect.DeepEqual, which inspects unexported fields (state protoimpl.MessageState,sizeCache,unknownFields) in the generated struct. These internal fields are not part of the message contract and can differ after the protobuf runtime populates internal state.protoassert.Equaluses the generatedEqualVTmethod instead, which compares only contract-defined fields and handles nil cases correctly.Add the import
"github.com/stackrox/rox/pkg/protoassert"and replaceassert.Equalwithprotoassert.Equal:Proposed change
for name, tc := range cases { t.Run(name, func(t *testing.T) { result := managingResourceFromOwnerRefs(tc.refs) - assert.Equal(t, tc.expected, result) + protoassert.Equal(t, tc.expected, result) }) }🤖 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/protoconv/resources/resources_test.go` around lines 362 - 367, Update the protobuf comparison in the managingResourceFromOwnerRefs test loop to use protoassert.Equal instead of assert.Equal, and add the github.com/stackrox/rox/pkg/protoassert import. Keep the existing test inputs and expected-result assertions unchanged.pkg/protoconv/resources/resources.go (1)
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
strings.LastIndexwithschema.ParseGroupVersionfor stricter validation.
k8s.io/apimachineryis already a direct dependency.schema.ParseGroupVersionimplements the same mapping for valid inputs and rejects malformed values with more than one/instead of accepting them silently. The current implementation returns the substring before the last/as the group for any input with slashes;ParseGroupVersionstrictly requires exactly one/for grouped APIs and zero/for core APIs.On parse error, return the empty group and the input string as version to match the current fallback for core APIs. Remove the
stringsimport since the proposed change eliminates its only use.♻️ Proposed refactor
-func groupAndVersionFromAPIVersion(apiVersion string) (group, version string) { - if i := strings.LastIndex(apiVersion, "/"); i >= 0 { - return apiVersion[:i], apiVersion[i+1:] - } - return "", apiVersion -} +func groupAndVersionFromAPIVersion(apiVersion string) (group, version string) { + gv, err := schema.ParseGroupVersion(apiVersion) + if err != nil { + return "", apiVersion + } + return gv.Group, gv.Version +}Import
"k8s.io/apimachinery/pkg/runtime/schema"and remove"strings"from the imports.🤖 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/protoconv/resources/resources.go` around lines 107 - 112, Update groupAndVersionFromAPIVersion to use schema.ParseGroupVersion instead of strings.LastIndex so only valid core or grouped API versions are accepted. On parse success, return the parsed Group and Version; on parse error, preserve the current fallback by returning an empty group and the original apiVersion as the version. Remove the now-unused strings import and add the schema import, keeping the behavior anchored to groupAndVersionFromAPIVersion.
🤖 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 `@pkg/protoconv/resources/resources.go`:
- Around line 83-101: Update managingResourceFromOwnerRefs to skip owner
references whose APIVersion is native by applying kubernetes.IsNativeAPI
alongside the existing IsTrackedOwnerReference and controller checks, so only
non-native controller owners produce a ManagingResource. Extend
TestManagingResourceFromOwnerRefs with a native non-deployment owner case and
verify it is ignored.
---
Nitpick comments:
In `@pkg/protoconv/resources/resources_test.go`:
- Around line 362-367: Update the protobuf comparison in the
managingResourceFromOwnerRefs test loop to use protoassert.Equal instead of
assert.Equal, and add the github.com/stackrox/rox/pkg/protoassert import. Keep
the existing test inputs and expected-result assertions unchanged.
In `@pkg/protoconv/resources/resources.go`:
- Around line 107-112: Update groupAndVersionFromAPIVersion to use
schema.ParseGroupVersion instead of strings.LastIndex so only valid core or
grouped API versions are accepted. On parse success, return the parsed Group and
Version; on parse error, preserve the current fallback by returning an empty
group and the original apiVersion as the version. Remove the now-unused strings
import and add the schema import, keeping the behavior anchored to
groupAndVersionFromAPIVersion.
In `@proto/storage/deployment.proto`:
- Around line 66-70: Update the comment for ManagingResource.managing_resource
to state that it is populated only when the deployment’s OwnerReference chain
includes a CRD controller and no tracked native owner reference causes the
deployment to be discarded; remove the misleading “or in addition to” wording.
🪄 Autofix
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: ea36a4f5-6311-4b34-90fe-7705f271a72e
⛔ Files ignored due to path filters (6)
generated/api/v1/deployment_service.swagger.jsonis excluded by!**/generated/**generated/api/v1/detection_service.swagger.jsonis excluded by!**/generated/**generated/api/v1/vuln_mgmt_service.swagger.jsonis excluded by!**/generated/**generated/storage/deployment.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/storage/deployment_vtproto.pb.gois excluded by!**/*.pb.go,!**/generated/**proto/storage/proto.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pkg/protoconv/resources/resources.gopkg/protoconv/resources/resources_test.goproto/storage/deployment.protosensor/kubernetes/listener/resources/convert_test.go
| func managingResourceFromOwnerRefs(refs []metav1.OwnerReference) *storage.ManagingResource { | ||
| for _, ref := range refs { | ||
| if IsTrackedOwnerReference(ref) { | ||
| continue | ||
| } | ||
| if ref.Controller == nil || !*ref.Controller { | ||
| continue | ||
| } | ||
| group, version := groupAndVersionFromAPIVersion(ref.APIVersion) | ||
| return &storage.ManagingResource{ | ||
| Kind: ref.Kind, | ||
| ApiGroup: group, | ||
| ApiVersion: version, | ||
| Name: ref.Name, | ||
| Uid: string(ref.UID), | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a native-API check. The current filter admits native owners.
The function name, the doc comment, and the ManagingResource proto comment all state that this captures a non-native (CRD) owner. The loop never checks the API group. It only skips references that are tracked, and IsTrackedOwnerReference (Line 58) requires IsDeploymentResource(reference.Kind) && kubernetes.IsNativeAPI(reference.APIVersion).
A native controller owner whose kind is not a deployment resource therefore passes both continue guards. Such an owner is stored as a ManagingResource with an empty or *.k8s.io ApiGroup. Consumers that read this field to identify a custom resource then receive a native Kubernetes object.
Reuse the existing kubernetes.IsNativeAPI helper so one definition of "native" applies in both places.
🐛 Proposed fix
func managingResourceFromOwnerRefs(refs []metav1.OwnerReference) *storage.ManagingResource {
for _, ref := range refs {
- if IsTrackedOwnerReference(ref) {
+ // Only non-native (CRD) owners are reported as managing resources.
+ if kubernetes.IsNativeAPI(ref.APIVersion) {
continue
}
if ref.Controller == nil || !*ref.Controller {
continue
}Add a case to TestManagingResourceFromOwnerRefs for a native, non-deployment owner kind to lock the behavior in.
📝 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.
| func managingResourceFromOwnerRefs(refs []metav1.OwnerReference) *storage.ManagingResource { | |
| for _, ref := range refs { | |
| if IsTrackedOwnerReference(ref) { | |
| continue | |
| } | |
| if ref.Controller == nil || !*ref.Controller { | |
| continue | |
| } | |
| group, version := groupAndVersionFromAPIVersion(ref.APIVersion) | |
| return &storage.ManagingResource{ | |
| Kind: ref.Kind, | |
| ApiGroup: group, | |
| ApiVersion: version, | |
| Name: ref.Name, | |
| Uid: string(ref.UID), | |
| } | |
| } | |
| return nil | |
| } | |
| func managingResourceFromOwnerRefs(refs []metav1.OwnerReference) *storage.ManagingResource { | |
| for _, ref := range refs { | |
| // Only non-native (CRD) owners are reported as managing resources. | |
| if kubernetes.IsNativeAPI(ref.APIVersion) { | |
| continue | |
| } | |
| if ref.Controller == nil || !*ref.Controller { | |
| continue | |
| } | |
| group, version := groupAndVersionFromAPIVersion(ref.APIVersion) | |
| return &storage.ManagingResource{ | |
| Kind: ref.Kind, | |
| ApiGroup: group, | |
| ApiVersion: version, | |
| Name: ref.Name, | |
| Uid: string(ref.UID), | |
| } | |
| } | |
| return 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 `@pkg/protoconv/resources/resources.go` around lines 83 - 101, Update
managingResourceFromOwnerRefs to skip owner references whose APIVersion is
native by applying kubernetes.IsNativeAPI alongside the existing
IsTrackedOwnerReference and controller checks, so only non-native controller
owners produce a ManagingResource. Extend TestManagingResourceFromOwnerRefs with
a native non-deployment owner case and verify it is ignored.
🚀 Build Images ReadyImages are ready for commit 5d19c19. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-675-g5d19c19e05 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #22102 +/- ##
==========================================
- Coverage 51.27% 51.24% -0.04%
==========================================
Files 2869 2869
Lines 179349 179368 +19
==========================================
- Hits 91965 91911 -54
- Misses 79325 79379 +54
- Partials 8059 8078 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!