move owner fetcher from member-operator #496
Conversation
WalkthroughAdds a new OwnerFetcher that resolves Kubernetes ownerReference chains using discovery and dynamic clients, introduces ObjectWithGVR and recursive GetOwners logic, includes comprehensive unit tests for various owner kinds and failure cases, and updates Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Fetcher as OwnerFetcher
participant Discovery as DiscoveryClient
participant Dynamic as DynamicClient
Caller->>Fetcher: GetOwners(ctx, object)
Fetcher->>Fetcher: inspect ownerReferences
alt controller owner present
Fetcher->>Fetcher: choose controller owner
else
Fetcher->>Fetcher: choose first non-controller owner
end
Fetcher->>Discovery: ServerPreferredResources()
Discovery-->>Fetcher: APIResourceLists
Fetcher->>Fetcher: resolve GVR (gvrForKind / findGVRForKind)
Fetcher->>Dynamic: Get(ctx, GVR, namespace, name)
Dynamic-->>Fetcher: owner object or error
Fetcher->>Fetcher: append ObjectWithGVR and recurse
Note over Fetcher: repeat until no owner refs
Fetcher-->>Caller: return owner chain or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #496 +/- ##
==========================================
+ Coverage 78.59% 78.84% +0.24%
==========================================
Files 52 53 +1
Lines 2626 2207 -419
==========================================
- Hits 2064 1740 -324
+ Misses 500 404 -96
- Partials 62 63 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
pkg/owners/fetcher.go (4)
43-51: Discovery cache needs basic concurrency safety.If used from concurrent callers, lazy init of resourceLists can race. Protect with sync.Once or a mutex.
+import "sync" @@ type OwnerFetcher struct { resourceLists []*metav1.APIResourceList // All available API in the cluster discoveryClient discovery.ServerResourcesInterface dynamicClient dynamic.Interface + once sync.Once + resErr error } @@ - if o.resourceLists == nil { - resourceLists, err := o.discoveryClient.ServerPreferredResources() - if err != nil { - return nil, err - } - o.resourceLists = resourceLists - } + o.once.Do(func() { + o.resourceLists, o.resErr = o.discoveryClient.ServerPreferredResources() + }) + if o.resErr != nil { + return nil, o.resErr + }
80-84: Handle cluster-scoped owners (don’t always set Namespace).Some owners can be cluster-scoped. Use the APIResource’s Namespaced flag to decide whether to scope the GET.
- ownerObject, err := o.dynamicClient.Resource(*gvr).Namespace(obj.GetNamespace()).Get(ctx, ownerReference.Name, metav1.GetOptions{}) + ownerRes := o.dynamicClient.Resource(*gvr) + // Determine scoping from discovery data + namespaced := isNamespaced(ownerReference.Kind, ownerReference.APIVersion, o.resourceLists) + if namespaced { + ownerObject, err = ownerRes.Namespace(obj.GetNamespace()).Get(ctx, ownerReference.Name, metav1.GetOptions{}) + } else { + ownerObject, err = ownerRes.Get(ctx, ownerReference.Name, metav1.GetOptions{}) + }Add helper:
+func isNamespaced(kind, apiVersion string, resourceLists []*metav1.APIResourceList) bool { + for _, rl := range resourceLists { + if rl.GroupVersion == apiVersion { + for _, r := range rl.APIResources { + if r.Kind == kind { + return r.Namespaced + } + } + } + } + return true +}
96-133: Optional: return Namespaced flag with GVR to avoid double scans.Minor perf/clarity: have findGVRForKind also return whether the resource is namespaced, so you don’t re-scan in GetOwners.
-func gvrForKind(kind, apiVersion string, resourceLists []*metav1.APIResourceList) (*schema.GroupVersionResource, error) { - gvr, err := findGVRForKind(kind, apiVersion, resourceLists) +func gvrForKind(kind, apiVersion string, resourceLists []*metav1.APIResourceList) (*schema.GroupVersionResource, bool, error) { + gvr, namespaced, err := findGVRForKind(kind, apiVersion, resourceLists) if gvr == nil && err == nil { return nil, fmt.Errorf("no resource found for kind %s in %s", kind, apiVersion) } - return gvr, err + return gvr, namespaced, err } @@ -func findGVRForKind(kind, apiVersion string, resourceLists []*metav1.APIResourceList) (*schema.GroupVersionResource, error) { +func findGVRForKind(kind, apiVersion string, resourceLists []*metav1.APIResourceList) (*schema.GroupVersionResource, bool, error) { gv, err := schema.ParseGroupVersion(apiVersion) if err != nil { - return nil, fmt.Errorf("failed to parse APIVersion %s: %w", apiVersion, err) + return nil, false, fmt.Errorf("failed to parse APIVersion %s: %w", apiVersion, err) } for _, resourceList := range resourceLists { if resourceList.GroupVersion == apiVersion { for _, apiResource := range resourceList.APIResources { if apiResource.Kind == kind { return &schema.GroupVersionResource{ Group: gv.Group, Version: gv.Version, Resource: apiResource.Name, - }, nil + }, apiResource.Namespaced, nil } } } } - return nil, nil + return nil, false, nil }And update the call site accordingly.
43-51: Consider tolerating partial discovery errors.ServerPreferredResources can return partial results with ErrGroupDiscoveryFailed. You may want to proceed with discovered lists and only fail when the needed kind is missing.
If you want this, gate on errors.As(err, *discovery.ErrGroupDiscoveryFailed) and keep the partial lists; return errors only when gvrForKind can’t resolve the specific kind.
pkg/owners/fetcher_test.go (2)
216-222: Nit: duplicate subtest name.Second subtest should be “not found error” to reflect behavior.
- t.Run("general error", func(t *testing.T) { + t.Run("not found error", func(t *testing.T) { assertCanNotGetObject(t, inaccessibleResource, inaccessibleObject, true) })
245-256: Typo in subtest name.“retrival” → “retrieval”.
- t.Run("no APIs retrival when once done", func(t *testing.T) { + t.Run("no APIs retrieval when once done", func(t *testing.T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(2 hunks)pkg/owners/fetcher.go(1 hunks)pkg/owners/fetcher_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/owners/fetcher_test.go (2)
pkg/owners/fetcher.go (1)
NewOwnerFetcher(25-30)pkg/apis/apis.go (1)
AddToScheme(14-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Verify Dependencies
- GitHub Check: GolangCI Lint
- GitHub Check: Test with Coverage
🔇 Additional comments (2)
go.mod (2)
120-120: OK to add k8s metrics as indirect.Matches the k8s 0.32.x line you already use. No action needed.
29-31: Member-operator dependency correctly scoped to tests.Verified: the member-operator import appears only in
pkg/owners/fetcher_test.go(test context). Runtime code does not reference it, confirming the dependency remains test-only and does not widen the production surface.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/owners/fetcher.go (1)
89-93: The variable check is correct.The condition on line 90 correctly checks
ownerOwners(the recursive result), notowners(the input's owner references). The logic properly builds the ownership chain from top-level to immediate owner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/owners/fetcher.go(1 hunks)pkg/owners/fetcher_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T10:18:07.177Z
Learnt from: MatousJobanek
PR: codeready-toolchain/toolchain-common#496
File: pkg/owners/fetcher_test.go:29-32
Timestamp: 2025-10-24T10:18:07.177Z
Learning: In the codeready-toolchain repositories, the import pattern `controllerruntime "sigs.k8s.io/controller-runtime"` should be kept for consistency across the codebase, even if specific functions like SetControllerReference are used from sub-packages like controllerutil.
Applied to files:
pkg/owners/fetcher_test.go
🧬 Code graph analysis (1)
pkg/owners/fetcher_test.go (2)
pkg/owners/fetcher.go (1)
NewOwnerFetcher(25-30)pkg/apis/apis.go (1)
AddToScheme(14-23)
🔇 Additional comments (5)
pkg/owners/fetcher.go (3)
14-30: LGTM! Clean structure and constructor.The OwnerFetcher design with lazy resource list initialization and clear separation of discovery/dynamic client responsibilities is well-structured.
32-37: LGTM! Appropriate return type.The ObjectWithGVR struct cleanly pairs the object with its GVR, providing necessary type information to callers.
96-133: LGTM! Clean GVR resolution logic.The helper functions correctly parse GroupVersion and search resource lists. The separation between findGVRForKind (returns nil when not found) and gvrForKind (converts to error) provides good flexibility.
pkg/owners/fetcher_test.go (2)
227-273: LGTM! Comprehensive resource list testing.The test properly validates lazy initialization, caching behavior, and error propagation for API resource list retrieval.
275-356: LGTM! Well-structured test helpers.The helper functions and JSON fixtures are clean and reusable. The VM resource generation from JSON and the fake discovery client wrapper provide good test isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/owners/fetcher_test.go (2)
79-84: Consider adding a comment to clarify the noise owner pattern.The use of DaemonSets as noise owners is a good testing pattern to verify that the fetcher correctly filters out non-relevant owner types. A brief comment explaining this intent would improve test readability.
+ // Add a noise owner (DaemonSet) that should be filtered out by the fetcher noiseOwner := &appsv1.DaemonSet{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("noise-owner-%d", i), Namespace: owner.GetNamespace()}} err := controllerutil.SetOwnerReference(noiseOwner, initObjects[len(initObjects)-1].(client.Object), scheme.Scheme) require.NoError(t, err)
133-133: Consider using ErrorContains for more flexible error assertions.Using
EqualErrorwith an exact error message string makes the test brittle. If the error message format changes in the implementation, this test will fail even though the error condition is still correctly handled.- require.EqualError(t, err, "no resource found for kind VirtualMachine in kubevirt.io/v1") + require.ErrorContains(t, err, "no resource found for kind VirtualMachine") + require.ErrorContains(t, err, "kubevirt.io/v1")This approach is more maintainable while still verifying the essential error information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/owners/fetcher_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T10:18:07.177Z
Learnt from: MatousJobanek
PR: codeready-toolchain/toolchain-common#496
File: pkg/owners/fetcher_test.go:29-32
Timestamp: 2025-10-24T10:18:07.177Z
Learning: In the codeready-toolchain repositories, the import pattern `controllerruntime "sigs.k8s.io/controller-runtime"` should be kept for consistency across the codebase, even if specific functions like SetControllerReference are used from sub-packages like controllerutil.
Applied to files:
pkg/owners/fetcher_test.go
🧬 Code graph analysis (1)
pkg/owners/fetcher_test.go (2)
pkg/owners/fetcher.go (1)
NewOwnerFetcher(25-30)pkg/apis/apis.go (1)
AddToScheme(14-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Verify Dependencies
🔇 Additional comments (2)
pkg/owners/fetcher_test.go (2)
88-88: No issues found.The project's
go.modspecifiesgo 1.23.0, which satisfies the Go 1.22+ requirement forslices.Concatused at line 88.
313-313: The function signature reveals thatUnsafeGuessKindToResourcereturns twoschema.GroupVersionResourcevalues, not one. The code at line 313 correctly uses both return values:resource, _ := meta.UnsafeGuessKindToResource(gvk)This is syntactically and semantically correct—the blank identifier
_intentionally discards the second return value. The original concern was based on an incorrect assumption about the function signature.Likely an incorrect or invalid review comment.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/owners/fetcher_test.go (1)
313-315: Consider the implications of UnsafeGuessKindToResource.The function
meta.UnsafeGuessKindToResourceis marked as "Unsafe" in the Kubernetes API and its documentation discourages use outside its package. While this works fine for test code with types following standard naming conventions, the API signal is concerning.For test purposes this is acceptable, but if you encounter issues with resource name mismatches, consider using an explicit GVK-to-resource mapping instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(1 hunks)pkg/owners/fetcher_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T10:18:07.200Z
Learnt from: MatousJobanek
Repo: codeready-toolchain/toolchain-common PR: 496
File: pkg/owners/fetcher_test.go:29-32
Timestamp: 2025-10-24T10:18:07.200Z
Learning: In the codeready-toolchain repositories, the import pattern `controllerruntime "sigs.k8s.io/controller-runtime"` should be kept for consistency across the codebase, even if specific functions like SetControllerReference are used from sub-packages like controllerutil.
Applied to files:
pkg/owners/fetcher_test.go
🧬 Code graph analysis (1)
pkg/owners/fetcher_test.go (2)
pkg/owners/fetcher.go (1)
NewOwnerFetcher(25-30)pkg/apis/apis.go (1)
AddToScheme(14-23)
🔇 Additional comments (3)
pkg/owners/fetcher_test.go (3)
33-104: Excellent test coverage and structure.The test suite comprehensively covers multiple owner chain scenarios (0, 1, 2, and 4 owners) and cleverly uses noise objects and noise owners to verify that filtering works correctly. The alternating controller/non-controller owner references ensure both types are properly handled.
106-224: Thorough error handling and edge case coverage.The failure tests comprehensively cover API unavailability, resource access errors, and not-found scenarios across all owner types. The test correctly validates that intermediate owners are returned when subsequent owner fetches fail (line 197), and properly distinguishes between not-found and general errors using
apierrors.IsNotFound.
226-272: Good coverage of API discovery and caching behavior.The tests properly verify that API resource lists are cached after the first fetch (lines 244-255) and that discovery errors are correctly propagated.
MatousJobanek
merged commit 4066619
into
codeready-toolchain:master
6 participants
moves owner fetcher from member-operator so it can be used at other places too
simplifies the tests a bit (compared to what was in member-operator)
related PR: codeready-toolchain/member-operator#706
Assisted-by: Cursor
Summary by CodeRabbit
New Features
Chores
Tests