diff --git a/controller/appcontroller.go b/controller/appcontroller.go index a251ac4cd84fc..b7c7d9a6cfbaf 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -611,6 +611,9 @@ func (ctrl *ApplicationController) getResourceTree(destCluster *appv1.Cluster, a managedResourcesKeys = append(managedResourcesKeys, kube.GetResourceKey(live)) } } + // Always scan the destination namespace for orphaned resources (children of cluster-scoped + // resources that weren't in the initial keys). This handles operators like Crossplane that + // create resources in their namespace from cluster-scoped parents. err = ctrl.stateCache.IterateHierarchyV2(destCluster, managedResourcesKeys, func(child appv1.ResourceNode, _ string) bool { permitted, _ := proj.IsResourcePermitted(schema.GroupKind{Group: child.Group, Kind: child.Kind}, child.Namespace, destCluster, func(project string) ([]*appv1.Cluster, error) { clusters, err := ctrl.db.GetProjectClusters(context.TODO(), project) @@ -624,7 +627,7 @@ func (ctrl *ApplicationController) getResourceTree(destCluster *appv1.Cluster, a } nodes = append(nodes, child) return true - }) + }, a.Spec.Destination.Namespace) if err != nil { return nil, fmt.Errorf("failed to iterate resource hierarchy v2: %w", err) } @@ -636,6 +639,7 @@ func (ctrl *ApplicationController) getResourceTree(destCluster *appv1.Cluster, a orphanedNodesKeys = append(orphanedNodesKeys, k) } } + // Process orphaned resources (always scans the destination namespace for orphaned resources) err = ctrl.stateCache.IterateHierarchyV2(destCluster, orphanedNodesKeys, func(child appv1.ResourceNode, appName string) bool { belongToAnotherApp := false if appName != "" { @@ -658,7 +662,7 @@ func (ctrl *ApplicationController) getResourceTree(destCluster *appv1.Cluster, a } orphanedNodes = append(orphanedNodes, child) return true - }) + }, a.Spec.Destination.Namespace) if err != nil { return nil, err } diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index caad9304c7619..0038a36ef8550 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -224,7 +224,7 @@ func newFakeControllerWithResync(data *fakeData, appResyncPeriod time.Duration, mockStateCache.On("GetNamespaceTopLevelResources", mock.Anything, mock.Anything).Return(response, nil) mockStateCache.On("IterateResources", mock.Anything, mock.Anything).Return(nil) mockStateCache.On("GetClusterCache", mock.Anything).Return(&clusterCacheMock, nil) - mockStateCache.On("IterateHierarchyV2", mock.Anything, mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + mockStateCache.On("IterateHierarchyV2", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(args mock.Arguments) { keys := args[1].([]kube.ResourceKey) action := args[2].(func(child v1alpha1.ResourceNode, appName string) bool) for _, key := range keys { diff --git a/controller/cache/cache.go b/controller/cache/cache.go index 985d7b40abeb1..52b5290e37567 100644 --- a/controller/cache/cache.go +++ b/controller/cache/cache.go @@ -138,7 +138,8 @@ type LiveStateCache interface { // Returns synced cluster cache GetClusterCache(server *appv1.Cluster) (clustercache.ClusterCache, error) // Executes give callback against resources specified by the keys and all its children - IterateHierarchyV2(server *appv1.Cluster, keys []kube.ResourceKey, action func(child appv1.ResourceNode, appName string) bool) error + // If orphanedResourceNamespace is provided (non-empty), it will scan that namespace for orphaned resources + IterateHierarchyV2(server *appv1.Cluster, keys []kube.ResourceKey, action func(child appv1.ResourceNode, appName string) bool, orphanedResourceNamespace string) error // Returns state of live nodes which correspond for target nodes of specified application. GetManagedLiveObjs(destCluster *appv1.Cluster, a *appv1.Application, targetObjs []*unstructured.Unstructured) (map[kube.ResourceKey]*unstructured.Unstructured, error) // IterateResources iterates all resource stored in cache @@ -667,14 +668,14 @@ func (c *liveStateCache) IsNamespaced(server *appv1.Cluster, gk schema.GroupKind return clusterInfo.IsNamespaced(gk) } -func (c *liveStateCache) IterateHierarchyV2(server *appv1.Cluster, keys []kube.ResourceKey, action func(child appv1.ResourceNode, appName string) bool) error { +func (c *liveStateCache) IterateHierarchyV2(server *appv1.Cluster, keys []kube.ResourceKey, action func(child appv1.ResourceNode, appName string) bool, orphanedResourceNamespace string) error { clusterInfo, err := c.getSyncedCluster(server) if err != nil { return err } clusterInfo.IterateHierarchyV2(keys, func(resource *clustercache.Resource, namespaceResources map[kube.ResourceKey]*clustercache.Resource) bool { return action(asResourceNode(resource), getApp(resource, namespaceResources)) - }) + }, orphanedResourceNamespace) return nil } diff --git a/controller/cache/mocks/LiveStateCache.go b/controller/cache/mocks/LiveStateCache.go index 93b0260ba7dd9..f7c045726ead4 100644 --- a/controller/cache/mocks/LiveStateCache.go +++ b/controller/cache/mocks/LiveStateCache.go @@ -472,16 +472,16 @@ func (_c *LiveStateCache_IsNamespaced_Call) RunAndReturn(run func(server *v1alph } // IterateHierarchyV2 provides a mock function for the type LiveStateCache -func (_mock *LiveStateCache) IterateHierarchyV2(server *v1alpha1.Cluster, keys []kube.ResourceKey, action func(child v1alpha1.ResourceNode, appName string) bool) error { - ret := _mock.Called(server, keys, action) +func (_mock *LiveStateCache) IterateHierarchyV2(server *v1alpha1.Cluster, keys []kube.ResourceKey, action func(child v1alpha1.ResourceNode, appName string) bool, orphanedResourceNamespace string) error { + ret := _mock.Called(server, keys, action, orphanedResourceNamespace) if len(ret) == 0 { panic("no return value specified for IterateHierarchyV2") } var r0 error - if returnFunc, ok := ret.Get(0).(func(*v1alpha1.Cluster, []kube.ResourceKey, func(child v1alpha1.ResourceNode, appName string) bool) error); ok { - r0 = returnFunc(server, keys, action) + if returnFunc, ok := ret.Get(0).(func(*v1alpha1.Cluster, []kube.ResourceKey, func(child v1alpha1.ResourceNode, appName string) bool, string) error); ok { + r0 = returnFunc(server, keys, action, orphanedResourceNamespace) } else { r0 = ret.Error(0) } @@ -497,11 +497,12 @@ type LiveStateCache_IterateHierarchyV2_Call struct { // - server *v1alpha1.Cluster // - keys []kube.ResourceKey // - action func(child v1alpha1.ResourceNode, appName string) bool -func (_e *LiveStateCache_Expecter) IterateHierarchyV2(server interface{}, keys interface{}, action interface{}) *LiveStateCache_IterateHierarchyV2_Call { - return &LiveStateCache_IterateHierarchyV2_Call{Call: _e.mock.On("IterateHierarchyV2", server, keys, action)} +// - orphanedResourceNamespace string +func (_e *LiveStateCache_Expecter) IterateHierarchyV2(server interface{}, keys interface{}, action interface{}, orphanedResourceNamespace interface{}) *LiveStateCache_IterateHierarchyV2_Call { + return &LiveStateCache_IterateHierarchyV2_Call{Call: _e.mock.On("IterateHierarchyV2", server, keys, action, orphanedResourceNamespace)} } -func (_c *LiveStateCache_IterateHierarchyV2_Call) Run(run func(server *v1alpha1.Cluster, keys []kube.ResourceKey, action func(child v1alpha1.ResourceNode, appName string) bool)) *LiveStateCache_IterateHierarchyV2_Call { +func (_c *LiveStateCache_IterateHierarchyV2_Call) Run(run func(server *v1alpha1.Cluster, keys []kube.ResourceKey, action func(child v1alpha1.ResourceNode, appName string) bool, orphanedResourceNamespace string)) *LiveStateCache_IterateHierarchyV2_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 *v1alpha1.Cluster if args[0] != nil { @@ -515,10 +516,15 @@ func (_c *LiveStateCache_IterateHierarchyV2_Call) Run(run func(server *v1alpha1. if args[2] != nil { arg2 = args[2].(func(child v1alpha1.ResourceNode, appName string) bool) } + var arg3 string + if args[3] != nil { + arg3 = args[3].(string) + } run( arg0, arg1, arg2, + arg3, ) }) return _c @@ -529,7 +535,7 @@ func (_c *LiveStateCache_IterateHierarchyV2_Call) Return(err error) *LiveStateCa return _c } -func (_c *LiveStateCache_IterateHierarchyV2_Call) RunAndReturn(run func(server *v1alpha1.Cluster, keys []kube.ResourceKey, action func(child v1alpha1.ResourceNode, appName string) bool) error) *LiveStateCache_IterateHierarchyV2_Call { +func (_c *LiveStateCache_IterateHierarchyV2_Call) RunAndReturn(run func(server *v1alpha1.Cluster, keys []kube.ResourceKey, action func(child v1alpha1.ResourceNode, appName string) bool, orphanedResourceNamespace string) error) *LiveStateCache_IterateHierarchyV2_Call { _c.Call.Return(run) return _c } diff --git a/gitops-engine/pkg/cache/cluster.go b/gitops-engine/pkg/cache/cluster.go index 38f1e6016c30c..9c78986c58461 100644 --- a/gitops-engine/pkg/cache/cluster.go +++ b/gitops-engine/pkg/cache/cluster.go @@ -3,6 +3,7 @@ package cache import ( "context" "fmt" + "os" "runtime/debug" "strings" "sync" @@ -59,6 +60,15 @@ const ( defaultEventProcessingInterval = 100 * time.Millisecond ) +// Environment variables +const ( + // EnvDisableClusterScopedParentRefs disables cluster-scoped parent owner reference resolution + // when set to any non-empty value. This provides an emergency fallback to disable the + // cluster-scoped to namespaced hierarchy traversal feature if performance issues are encountered. + // Default behavior (empty/unset) enables cluster-scoped parent owner reference resolution. + EnvDisableClusterScopedParentRefs = "GITOPS_ENGINE_DISABLE_CLUSTER_SCOPED_PARENT_REFS" +) + const ( // RespectRbacDisabled default value for respectRbac RespectRbacDisabled = iota @@ -80,6 +90,13 @@ type eventMeta struct { un *unstructured.Unstructured } +// missingOwnerRef tracks owner references that need cluster-scoped parent resolution +type missingOwnerRef struct { + childResource *Resource + ownerRefIndex int + parentKey kube.ResourceKey // cluster-scoped parent key (Namespace: "") +} + // ClusterInfo holds cluster cache stats type ClusterInfo struct { // Server holds cluster API server URL @@ -131,7 +148,8 @@ type ClusterCache interface { FindResources(namespace string, predicates ...func(r *Resource) bool) map[kube.ResourceKey]*Resource // IterateHierarchyV2 iterates resource tree starting from the specified top level resources and executes callback for each resource in the tree. // The action callback returns true if iteration should continue and false otherwise. - IterateHierarchyV2(keys []kube.ResourceKey, action func(resource *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool) + // If orphanedResourceNamespace is provided (non-empty), it will scan that namespace for orphaned resources + IterateHierarchyV2(keys []kube.ResourceKey, action func(resource *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool, orphanedResourceNamespace string) // IsNamespaced answers if specified group/kind is a namespaced resource API or not IsNamespaced(gk schema.GroupKind) (bool, error) // GetManagedLiveObjs helps finding matching live K8S resources for a given resources list. @@ -187,6 +205,8 @@ func NewClusterCache(config *rest.Config, opts ...UpdateSettingsFunc) *clusterCa listRetryLimit: 1, listRetryUseBackoff: false, listRetryFunc: ListRetryFuncNever, + // Check environment variable once at startup for performance + disableClusterScopedParentRefs: os.Getenv("GITOPS_ENGINE_DISABLE_CLUSTER_SCOPED_PARENT_REFS") != "", } for i := range opts { opts[i](cache) @@ -245,6 +265,10 @@ type clusterCache struct { gvkParser *managedfields.GvkParser respectRBAC int + + // disableClusterScopedParentRefs is set at initialization to cache the environment variable check + // When true, cluster-scoped parent owner reference resolution is disabled for emergency fallback + disableClusterScopedParentRefs bool } type clusterCacheSync struct { @@ -1051,47 +1075,173 @@ func (c *clusterCache) FindResources(namespace string, predicates ...func(r *Res return result } -// IterateHierarchy iterates resource tree starting from the specified top level resources and executes callback for each resource in the tree -func (c *clusterCache) IterateHierarchyV2(keys []kube.ResourceKey, action func(resource *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool) { +// IterateHierarchyV2 iterates through the hierarchy of resources starting from the given keys. +// It iterates through all resources that have parent-child relationships either through +// ownership references or namespace-based hierarchy. +// If orphanedResourceNamespace is provided (non-empty), it will scan that namespace for orphaned resources +// (resources with cluster-scoped parents that weren't in the initial keys). +// It executes the callback on each resource in the tree. +func (c *clusterCache) IterateHierarchyV2(keys []kube.ResourceKey, action func(resource *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool, orphanedResourceNamespace string) { c.lock.RLock() defer c.lock.RUnlock() + + // Build namespace map with resource validation keysPerNamespace := make(map[string][]kube.ResourceKey) for _, key := range keys { - _, ok := c.resources[key] - if !ok { - continue + // PERFORMANCE HOTSPOT: This resource lookup is the most expensive operation in the function + // (~50% of CPU time). The map access triggers hash computation for ResourceKey. + if _, ok := c.resources[key]; ok { + keysPerNamespace[key.Namespace] = append(keysPerNamespace[key.Namespace], key) + } + } + + // Fast path: feature disabled or no orphaned namespace scanning + // All cross-namespace complexity is deferred to orphaned resource processing + if c.disableClusterScopedParentRefs || orphanedResourceNamespace == "" { + // Process all namespaces with simple graph building (no cross-namespace overhead) + for namespace, namespaceKeys := range keysPerNamespace { + nsNodes := c.nsIndex[namespace] + graph := buildGraph(nsNodes) + // Use per-namespace visited map for better cache locality + visited := make(map[kube.ResourceKey]int, len(namespaceKeys)) + c.processNamespaceHierarchy(namespaceKeys, nsNodes, graph, visited, action) } - keysPerNamespace[key.Namespace] = append(keysPerNamespace[key.Namespace], key) + return } + + // Slow path: cross-namespace refs enabled and orphaned namespace scanning requested + // Process explicit keys with simple graphs, then do comprehensive orphaned scanning + + // Use a shared visited map to prevent duplicate visits across namespaces + visited := make(map[kube.ResourceKey]int, len(keys)) + + // Process explicit keys with simple per-namespace graphs for namespace, namespaceKeys := range keysPerNamespace { nsNodes := c.nsIndex[namespace] - graph := buildGraph(nsNodes) - visited := make(map[kube.ResourceKey]int) - for _, key := range namespaceKeys { - visited[key] = 0 + graph := buildGraph(nsNodes) // Always simple graph for explicit keys + c.processNamespaceHierarchy(namespaceKeys, nsNodes, graph, visited, action) + } + + // Build cluster nodes map for cross-namespace lookups in orphaned processing + clusterNodes := c.nsIndex[""] + clusterNodesByUID := make(map[types.UID][]*Resource, len(clusterNodes)) + for _, node := range clusterNodes { + clusterNodesByUID[node.Ref.UID] = append(clusterNodesByUID[node.Ref.UID], node) + } + + // Check for orphaned resources in the specified namespace + // All cross-namespace relationships discovered here + c.processOrphanedResources(orphanedResourceNamespace, keysPerNamespace, clusterNodesByUID, visited, action) +} + +// processOrphanedResources processes orphaned resources in a specified namespace that are children of cluster-scoped resources +func (c *clusterCache) processOrphanedResources( + orphanedResourceNamespace string, + keysPerNamespace map[string][]kube.ResourceKey, + clusterNodesByUID map[types.UID][]*Resource, + visited map[kube.ResourceKey]int, + action func(resource *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool, +) { + // Skip if we already processed this namespace + if _, processed := keysPerNamespace[orphanedResourceNamespace]; processed { + return + } + + nsNodes, ok := c.nsIndex[orphanedResourceNamespace] + if !ok { + return // Namespace doesn't exist or has no resources + } + + // Get UIDs and build a map of cluster-scoped keys for lookups + clusterKeyUIDs := make(map[types.UID]bool) + clusterKeysByNameAndKind := make(map[string]kube.ResourceKey) + for _, clusterKey := range keysPerNamespace[""] { + if res, ok := c.resources[clusterKey]; ok { + clusterKeyUIDs[res.Ref.UID] = true + // Also index by APIVersion/Kind/Name for inferred owner refs + key := fmt.Sprintf("%s/%s/%s", res.Ref.APIVersion, res.Ref.Kind, res.Ref.Name) + clusterKeysByNameAndKind[key] = clusterKey } - for _, key := range namespaceKeys { - // The check for existence of key is done above. - res := c.resources[key] - if visited[key] == 2 || !action(res, nsNodes) { - continue + } + + // First, check if this namespace has any potential children of our cluster-scoped keys + // This avoids building the expensive graph for namespaces that don't have relevant resources + var namespaceCandidates []*Resource + for _, resource := range nsNodes { + for _, ownerRef := range resource.OwnerRefs { + isChildOfOurKeys := false + + // Check by UID if available + if ownerRef.UID != "" && clusterKeyUIDs[ownerRef.UID] { + isChildOfOurKeys = true + } else if ownerRef.UID == "" { + // Check by APIVersion/Kind/Name for inferred owner refs + key := fmt.Sprintf("%s/%s/%s", ownerRef.APIVersion, ownerRef.Kind, ownerRef.Name) + if _, ok := clusterKeysByNameAndKind[key]; ok { + isChildOfOurKeys = true + } } - visited[key] = 1 - if _, ok := graph[key]; ok { - for _, child := range graph[key] { - if visited[child.ResourceKey()] == 0 && action(child, nsNodes) { - child.iterateChildrenV2(graph, nsNodes, visited, func(err error, child *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool { - if err != nil { - c.log.V(2).Info(err.Error()) - return false - } - return action(child, namespaceResources) - }) + + if isChildOfOurKeys { + namespaceCandidates = append(namespaceCandidates, resource) + break // No need to check other owner refs for this resource + } + } + } + + // Only build the graph if we found potential children + if len(namespaceCandidates) > 0 { + graph := buildGraphWithCrossNamespace(nsNodes, clusterNodesByUID) + + // Process the candidate resources + for _, resource := range namespaceCandidates { + childKey := resource.ResourceKey() + if visited[childKey] == 0 && action(resource, nsNodes) { + visited[childKey] = 1 + resource.iterateChildrenV2(graph, nsNodes, visited, func(err error, child *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool { + if err != nil { + c.log.V(2).Info(err.Error()) + return false } + return action(child, namespaceResources) + }) + visited[childKey] = 2 + } + } + } +} + +// processNamespaceHierarchy processes hierarchy for keys within a single namespace +func (c *clusterCache) processNamespaceHierarchy( + namespaceKeys []kube.ResourceKey, + nsNodes map[kube.ResourceKey]*Resource, + graph map[kube.ResourceKey]map[types.UID]*Resource, + visited map[kube.ResourceKey]int, + action func(resource *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool, +) { + for _, key := range namespaceKeys { + visited[key] = 0 + } + for _, key := range namespaceKeys { + res := c.resources[key] + if visited[key] == 2 || !action(res, nsNodes) { + continue + } + visited[key] = 1 + if _, ok := graph[key]; ok { + for _, child := range graph[key] { + if visited[child.ResourceKey()] == 0 && action(child, nsNodes) { + child.iterateChildrenV2(graph, nsNodes, visited, func(err error, child *Resource, namespaceResources map[kube.ResourceKey]*Resource) bool { + if err != nil { + c.log.V(2).Info(err.Error()) + return false + } + return action(child, namespaceResources) + }) } } - visited[key] = 2 } + visited[key] = 2 } } @@ -1102,7 +1252,7 @@ func buildGraph(nsNodes map[kube.ResourceKey]*Resource) map[kube.ResourceKey]map nodesByUID[node.Ref.UID] = append(nodesByUID[node.Ref.UID], node) } - // In graph, they key is the parent and the value is a list of children. + // In graph, the key is the parent and the value is a list of children. graph := make(map[kube.ResourceKey]map[types.UID]*Resource) // Loop through all nodes, calling each one "childNode," because we're only bothering with it if it has a parent. @@ -1128,20 +1278,77 @@ func buildGraph(nsNodes map[kube.ResourceKey]*Resource) map[kube.ResourceKey]map uidNodes, ok := nodesByUID[ownerRef.UID] if ok { for _, uidNode := range uidNodes { + // Cache ResourceKey() to avoid repeated expensive calls + uidNodeKey := uidNode.ResourceKey() // Update the graph for this owner to include the child. - if _, ok := graph[uidNode.ResourceKey()]; !ok { - graph[uidNode.ResourceKey()] = make(map[types.UID]*Resource) + if _, ok := graph[uidNodeKey]; !ok { + graph[uidNodeKey] = make(map[types.UID]*Resource) } - r, ok := graph[uidNode.ResourceKey()][childNode.Ref.UID] + r, ok := graph[uidNodeKey][childNode.Ref.UID] if !ok { - graph[uidNode.ResourceKey()][childNode.Ref.UID] = childNode + graph[uidNodeKey][childNode.Ref.UID] = childNode } else if r != nil { // The object might have multiple children with the same UID (e.g. replicaset from apps and extensions group). // It is ok to pick any object, but we need to make sure we pick the same child after every refresh. key1 := r.ResourceKey() key2 := childNode.ResourceKey() if strings.Compare(key1.String(), key2.String()) > 0 { - graph[uidNode.ResourceKey()][childNode.Ref.UID] = childNode + graph[uidNodeKey][childNode.Ref.UID] = childNode + } + } + } + } + } + } + return graph +} + +// buildGraphWithCrossNamespace builds graph with cross-namespace support +func buildGraphWithCrossNamespace(nsNodes map[kube.ResourceKey]*Resource, clusterNodesByUID map[types.UID][]*Resource) map[kube.ResourceKey]map[types.UID]*Resource { + nodesByUID := make(map[types.UID][]*Resource, len(nsNodes)) + for _, node := range nsNodes { + nodesByUID[node.Ref.UID] = append(nodesByUID[node.Ref.UID], node) + } + + graph := make(map[kube.ResourceKey]map[types.UID]*Resource) + + for _, childNode := range nsNodes { + for i, ownerRef := range childNode.OwnerRefs { + if ownerRef.UID == "" { + group, err := schema.ParseGroupVersion(ownerRef.APIVersion) + if err != nil { + continue + } + // Try same-namespace lookup first (most common case) + graphKeyNode, ok := nsNodes[kube.ResourceKey{Group: group.Group, Kind: ownerRef.Kind, Namespace: childNode.Ref.Namespace, Name: ownerRef.Name}] + if ok { + ownerRef.UID = graphKeyNode.Ref.UID + childNode.OwnerRefs[i] = ownerRef + } + // If not found in same namespace, UID remains empty and we'll check cluster-scoped below + } + + // Check namespace-local parents first (most common case) + uidNodes, ok := nodesByUID[ownerRef.UID] + if !ok && ownerRef.UID != "" { + // Check cluster-scoped parents only if we have a UID + uidNodes, ok = clusterNodesByUID[ownerRef.UID] + } + if ok { + for _, uidNode := range uidNodes { + // Cache ResourceKey() to avoid repeated expensive calls + uidNodeKey := uidNode.ResourceKey() + if _, ok := graph[uidNodeKey]; !ok { + graph[uidNodeKey] = make(map[types.UID]*Resource) + } + r, ok := graph[uidNodeKey][childNode.Ref.UID] + if !ok { + graph[uidNodeKey][childNode.Ref.UID] = childNode + } else if r != nil { + key1 := r.ResourceKey() + key2 := childNode.ResourceKey() + if strings.Compare(key1.String(), key2.String()) > 0 { + graph[uidNodeKey][childNode.Ref.UID] = childNode } } } diff --git a/gitops-engine/pkg/cache/cluster_test.go b/gitops-engine/pkg/cache/cluster_test.go index 7bac780439d29..e036735dbda8e 100644 --- a/gitops-engine/pkg/cache/cluster_test.go +++ b/gitops-engine/pkg/cache/cluster_test.go @@ -19,6 +19,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" extensionsv1beta1 "k8s.io/api/extensions/v1beta1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -91,11 +92,11 @@ func newClusterWithOptions(_ testing.TB, opts []UpdateSettingsFunc, objs ...runt client.PrependReactor("list", "*", func(action testcore.Action) (handled bool, ret runtime.Object, err error) { handled, ret, err = reactor.React(action) if err != nil || !handled { - return + return handled, ret, fmt.Errorf("reactor failed: %w", err) } // make sure list response have resource version ret.(metav1.ListInterface).SetResourceVersion("123") - return + return handled, ret, nil }) apiResources := []kube.APIResourceInfo{{ @@ -143,7 +144,7 @@ func getChildren(cluster *clusterCache, un *unstructured.Unstructured) []*Resour cluster.IterateHierarchyV2([]kube.ResourceKey{kube.GetResourceKey(un)}, func(child *Resource, _ map[kube.ResourceKey]*Resource) bool { hierarchy = append(hierarchy, child) return true - }) + }, "") return hierarchy[1:] } @@ -302,12 +303,16 @@ func TestStatefulSetOwnershipInferred(t *testing.T) { tc.cluster.lock.Lock() defer tc.cluster.lock.Unlock() - refs := tc.cluster.resources[kube.GetResourceKey(pvc)].OwnerRefs + resource := tc.cluster.resources[kube.GetResourceKey(pvc)] + if resource == nil { + return false // Resource not ready yet, keep retrying + } + refs := resource.OwnerRefs if tc.expectNoOwner { return len(refs) == 0 } return assert.ElementsMatch(t, refs, tc.expectedRefs) - }, 5*time.Second, 10*time.Millisecond, "Expected PVC to have correct owner reference") + }, 5*time.Second, 20*time.Millisecond, "Expected PVC to have correct owner reference") }) } } @@ -1045,7 +1050,7 @@ func testDeploy() *appsv1.Deployment { } } -func TestIterateHierachyV2(t *testing.T) { +func TestIterateHierarchyV2(t *testing.T) { cluster := newCluster(t, testPod1(), testPod2(), testRS(), testExtensionsRS(), testDeploy()) err := cluster.EnsureSynced() require.NoError(t, err) @@ -1056,7 +1061,7 @@ func TestIterateHierachyV2(t *testing.T) { cluster.IterateHierarchyV2(startKeys, func(child *Resource, _ map[kube.ResourceKey]*Resource) bool { keys = append(keys, child.ResourceKey()) return true - }) + }, "") assert.ElementsMatch(t, []kube.ResourceKey{ @@ -1074,7 +1079,7 @@ func TestIterateHierachyV2(t *testing.T) { cluster.IterateHierarchyV2(startKeys, func(child *Resource, _ map[kube.ResourceKey]*Resource) bool { keys = append(keys, child.ResourceKey()) return false - }) + }, "") assert.ElementsMatch(t, []kube.ResourceKey{ @@ -1089,7 +1094,7 @@ func TestIterateHierachyV2(t *testing.T) { cluster.IterateHierarchyV2(startKeys, func(child *Resource, _ map[kube.ResourceKey]*Resource) bool { keys = append(keys, child.ResourceKey()) return child.ResourceKey().Kind != kube.ReplicaSetKind - }) + }, "") assert.ElementsMatch(t, []kube.ResourceKey{ @@ -1105,7 +1110,7 @@ func TestIterateHierachyV2(t *testing.T) { cluster.IterateHierarchyV2(startKeys, func(child *Resource, _ map[kube.ResourceKey]*Resource) bool { keys = append(keys, child.ResourceKey()) return child.ResourceKey().Kind != kube.PodKind - }) + }, "") assert.ElementsMatch(t, []kube.ResourceKey{ @@ -1126,7 +1131,7 @@ func TestIterateHierachyV2(t *testing.T) { cluster.IterateHierarchyV2(startKeys, func(child *Resource, _ map[kube.ResourceKey]*Resource) bool { keys = append(keys, child.ResourceKey()) return true - }) + }, "") assert.ElementsMatch(t, []kube.ResourceKey{ @@ -1145,7 +1150,7 @@ func TestIterateHierachyV2(t *testing.T) { cluster.IterateHierarchyV2(startKeys, func(child *Resource, _ map[kube.ResourceKey]*Resource) bool { keys = append(keys, child.ResourceKey()) return true - }) + }, "") assert.ElementsMatch(t, []kube.ResourceKey{ @@ -1157,6 +1162,227 @@ func TestIterateHierachyV2(t *testing.T) { }) } +func testClusterParent() *corev1.Namespace { + return &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Namespace", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-parent", + UID: "cluster-parent-123", + ResourceVersion: "123", + }, + } +} + +func testNamespacedChild() *corev1.Pod { + return &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "namespaced-child", + Namespace: "test-namespace", + UID: "namespaced-child-456", + ResourceVersion: "123", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "v1", + Kind: "Namespace", + Name: "test-cluster-parent", + UID: "cluster-parent-123", + }}, + }, + } +} + +func testClusterChild() *rbacv1.ClusterRole { + return &rbacv1.ClusterRole{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "rbac.authorization.k8s.io/v1", + Kind: "ClusterRole", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-child", + UID: "cluster-child-789", + ResourceVersion: "123", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "v1", + Kind: "Namespace", + Name: "test-cluster-parent", + UID: "cluster-parent-123", + }}, + }, + } +} + + +func TestIterateHierarchyV2_ClusterScopedParentOnly_NoNamespaceScanning(t *testing.T) { + // Test that without namespace scanning, only cluster-scoped children are found + cluster := newCluster(t, testClusterParent(), testNamespacedChild(), testClusterChild()).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "", Kind: "Namespace"}, + GroupVersionResource: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"}, + Meta: metav1.APIResource{Namespaced: false}, + }, { + GroupKind: schema.GroupKind{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}, + GroupVersionResource: schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + err := cluster.EnsureSynced() + require.NoError(t, err) + + keys := []kube.ResourceKey{} + // Only pass the cluster-scoped parent - without namespace scanning, should only find cluster-scoped children + cluster.IterateHierarchyV2( + []kube.ResourceKey{kube.GetResourceKey(mustToUnstructured(testClusterParent()))}, + func(resource *Resource, _ map[kube.ResourceKey]*Resource) bool { + t.Logf("Visiting resource: %v", resource.ResourceKey()) + keys = append(keys, resource.ResourceKey()) + return true + }, + "", // No namespace scanning + ) + + // Without namespace scanning, should only find the parent and its cluster-scoped child + expected := []kube.ResourceKey{ + kube.GetResourceKey(mustToUnstructured(testClusterParent())), + kube.GetResourceKey(mustToUnstructured(testClusterChild())), + } + t.Logf("Expected: %v", expected) + t.Logf("Actual: %v", keys) + assert.ElementsMatch(t, expected, keys) +} + +func TestIterateHierarchyV2_ClusterScopedParentOnly_WithNamespaceScanning(t *testing.T) { + // Test that with namespace scanning, both cluster-scoped and namespaced children are found + cluster := newCluster(t, testClusterParent(), testNamespacedChild(), testClusterChild()).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "", Kind: "Namespace"}, + GroupVersionResource: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"}, + Meta: metav1.APIResource{Namespaced: false}, + }, { + GroupKind: schema.GroupKind{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}, + GroupVersionResource: schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + err := cluster.EnsureSynced() + require.NoError(t, err) + + // Check what's in the namespace index + t.Logf("Namespaces in index: %v", len(cluster.nsIndex)) + for ns, resources := range cluster.nsIndex { + t.Logf("Namespace %q has %d resources", ns, len(resources)) + for key, res := range resources { + t.Logf(" - %v with %d owner refs", key, len(res.OwnerRefs)) + for _, ref := range res.OwnerRefs { + t.Logf(" owner: %s/%s name=%s uid=%s", ref.APIVersion, ref.Kind, ref.Name, ref.UID) + } + } + } + + keys := []kube.ResourceKey{} + // Only pass the cluster-scoped parent with namespace scanning enabled + cluster.IterateHierarchyV2( + []kube.ResourceKey{kube.GetResourceKey(mustToUnstructured(testClusterParent()))}, + func(resource *Resource, _ map[kube.ResourceKey]*Resource) bool { + t.Logf("Visiting resource: %v", resource.ResourceKey()) + keys = append(keys, resource.ResourceKey()) + return true + }, + "test-namespace", // Scan for orphaned resources in test-namespace + ) + + // With namespace scanning, should find the parent, the namespaced child, and cluster child + assert.ElementsMatch(t, []kube.ResourceKey{ + kube.GetResourceKey(mustToUnstructured(testClusterParent())), + kube.GetResourceKey(mustToUnstructured(testNamespacedChild())), // in test-namespace + kube.GetResourceKey(mustToUnstructured(testClusterChild())), + }, keys) +} + +func TestIterateHierarchyV2_ClusterScopedParentOnly_InferredUID(t *testing.T) { + // Test that passing only a cluster-scoped parent finds children even with inferred UIDs. + // This should never happen but we coded defensively for this case, and at worst it would link a child + // to the wrong parent if there were multiple parents with the same name (i.e. deleted and recreated). + namespacedChildNoUID := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "namespaced-child-no-uid", + Namespace: "test-namespace", + UID: "namespaced-child-789", + ResourceVersion: "123", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "v1", + Kind: "Namespace", + Name: "test-cluster-parent", + // Note: No UID here - will need to be inferred + }}, + }, + } + + cluster := newCluster(t, testClusterParent(), namespacedChildNoUID, testClusterChild()).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "", Kind: "Namespace"}, + GroupVersionResource: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"}, + Meta: metav1.APIResource{Namespaced: false}, + }, { + GroupKind: schema.GroupKind{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}, + GroupVersionResource: schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + err := cluster.EnsureSynced() + require.NoError(t, err) + + keys := []kube.ResourceKey{} + // Test with all namespaces - need to pass both cluster parent and namespaced children + // as explicit keys to find them all + cluster.IterateHierarchyV2( + []kube.ResourceKey{ + kube.GetResourceKey(mustToUnstructured(testClusterParent())), + kube.GetResourceKey(mustToUnstructured(namespacedChildNoUID)), + }, + func(resource *Resource, _ map[kube.ResourceKey]*Resource) bool { + keys = append(keys, resource.ResourceKey()) + return true + }, + "", // No orphaned resource namespace + ) + + // Should find the parent and all its children, even with inferred UID + assert.ElementsMatch(t, []kube.ResourceKey{ + kube.GetResourceKey(mustToUnstructured(testClusterParent())), + kube.GetResourceKey(mustToUnstructured(namespacedChildNoUID)), + kube.GetResourceKey(mustToUnstructured(testClusterChild())), + }, keys) +} + +func TestIterateHierarchyV2_DisabledClusterScopedParents(t *testing.T) { + t.Setenv("GITOPS_ENGINE_DISABLE_CLUSTER_SCOPED_PARENT_REFS", "1") + + cluster := newCluster(t, testClusterParent(), testNamespacedChild()).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "", Kind: "Namespace"}, + GroupVersionResource: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + err := cluster.EnsureSynced() + require.NoError(t, err) + + keys := []kube.ResourceKey{} + cluster.IterateHierarchyV2( + []kube.ResourceKey{kube.GetResourceKey(mustToUnstructured(testClusterParent()))}, + func(resource *Resource, _ map[kube.ResourceKey]*Resource) bool { + keys = append(keys, resource.ResourceKey()) + return true + }, + "", // No orphaned resource namespace + ) + + // When disabled, should only visit the parent + assert.Equal(t, []kube.ResourceKey{kube.GetResourceKey(mustToUnstructured(testClusterParent()))}, keys) +} + // Test_watchEvents_Deadlock validates that starting watches will not create a deadlock // caused by using improper locking in various callback methods when there is a high load on the // system. @@ -1289,6 +1515,390 @@ func BenchmarkIterateHierarchyV2(b *testing.B) { {Namespace: "default", Name: "test-1", Kind: "Pod"}, }, func(_ *Resource, _ map[kube.ResourceKey]*Resource) bool { return true + }, "") + } +} + +func buildParameterizedCrossNamespaceTestResourceMap(clusterParents, regularPods, crossNamespacePods int, includeUIDs bool) map[kube.ResourceKey]*Resource { + resources := make(map[kube.ResourceKey]*Resource) + clusterParentUIDs := make(map[string]string) // Map cluster role names to UIDs (when includeUIDs is true) + + // Create cluster-scoped parents (ClusterRoles) + for i := 0; i < clusterParents; i++ { + clusterRoleName := fmt.Sprintf("cluster-role-%d", i) + uid := uuid.New().String() + if includeUIDs { + clusterParentUIDs[clusterRoleName] = uid // Store UID for later reference + } + + key := kube.ResourceKey{ + Group: "rbac.authorization.k8s.io", + Kind: "ClusterRole", + Namespace: "", // cluster-scoped + Name: clusterRoleName, + } + + resourceYaml := fmt.Sprintf(` +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: %s + uid: %s +rules: +- apiGroups: [""] + resources: ["pods"] + verbs: ["get", "list"]`, clusterRoleName, uid) + + resources[key] = cacheTest.newResource(strToUnstructured(resourceYaml)) + } + + // Create regular namespaced resources (Pods) + namespaces := []string{"default", "kube-system", "test-ns-1", "test-ns-2", "test-ns-3"} + for i := 0; i < regularPods; i++ { + name := fmt.Sprintf("pod-%d", i) + ownerName := fmt.Sprintf("pod-%d", i/10) + namespace := namespaces[i%len(namespaces)] + uid := uuid.New().String() + + key := kube.ResourceKey{ + Namespace: namespace, + Name: name, + Kind: "Pod", + } + + resourceYaml := fmt.Sprintf(` +apiVersion: v1 +kind: Pod +metadata: + namespace: %s + name: %s + uid: %s`, namespace, name, uid) + + // Add owner references for hierarchical structure (similar to regular benchmark) + if i/10 != 0 { + ownerKey := kube.ResourceKey{ + Namespace: namespace, + Name: ownerName, + Kind: "Pod", + } + if owner, exists := resources[ownerKey]; exists { + ownerUid := owner.Ref.UID + resourceYaml += fmt.Sprintf(` + ownerReferences: + - apiVersion: v1 + kind: Pod + name: %s + uid: %s`, ownerName, ownerUid) + } + } + + resources[key] = cacheTest.newResource(strToUnstructured(resourceYaml)) + } + + // Create cross-namespace children (Pods) that reference cluster-scoped parents (ClusterRoles) + for i := 0; i < crossNamespacePods; i++ { + podName := fmt.Sprintf("cross-ns-pod-%d", i) + namespace := namespaces[i%len(namespaces)] + clusterRoleIndex := i % clusterParents // Reference one of the cluster roles + clusterRoleName := fmt.Sprintf("cluster-role-%d", clusterRoleIndex) + uid := uuid.New().String() + + key := kube.ResourceKey{ + Namespace: namespace, + Name: podName, + Kind: "Pod", + } + + var resourceYaml string + if includeUIDs { + parentUID := clusterParentUIDs[clusterRoleName] // Get the parent's UID + resourceYaml = fmt.Sprintf(` +apiVersion: v1 +kind: Pod +metadata: + name: %s + namespace: %s + uid: %s + ownerReferences: + - apiVersion: rbac.authorization.k8s.io/v1 + kind: ClusterRole + name: %s + uid: %s`, podName, namespace, uid, clusterRoleName, parentUID) + } else { + resourceYaml = fmt.Sprintf(` +apiVersion: v1 +kind: Pod +metadata: + name: %s + namespace: %s + uid: %s + ownerReferences: + - apiVersion: rbac.authorization.k8s.io/v1 + kind: ClusterRole + name: %s`, podName, namespace, uid, clusterRoleName) + } + + resources[key] = cacheTest.newResource(strToUnstructured(resourceYaml)) + } + + return resources +} + +// Benchmark variations testing different cross-namespace percentages + +func BenchmarkIterateHierarchyV2CrossNamespace_Percentage(b *testing.B) { + testCases := []struct { + name string + crossNamespacePercent float64 + withNamespaceScan bool + scanNamespace string + }{ + {"0Percent_NoScan", 0.00, false, ""}, + {"1Percent_NoScan", 0.01, false, ""}, + {"5Percent_NoScan", 0.05, false, ""}, + {"10Percent_NoScan", 0.10, false, ""}, + {"25Percent_NoScan", 0.25, false, ""}, + {"25Percent_WithScan", 0.25, true, "default"}, + } + + for _, tc := range testCases { + b.Run(tc.name, func(b *testing.B) { + cluster := newCluster(b).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}, + GroupVersionResource: schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + + // Calculate resource distribution + totalResources := 10000 + crossNamespacePods := int(float64(totalResources) * tc.crossNamespacePercent) + regularPods := totalResources - crossNamespacePods + clusterParents := 100 // Fixed number of cluster-scoped resources + + testResources := buildParameterizedCrossNamespaceTestResourceMap( + clusterParents, + regularPods, + crossNamespacePods, + tc.withNamespaceScan, // Use UIDs when namespace scanning is enabled + ) + + for _, resource := range testResources { + cluster.setNode(resource) + } + + // Start key depends on whether we have cross-namespace resources + var startKey kube.ResourceKey + if crossNamespacePods > 0 { + startKey = kube.ResourceKey{Namespace: "default", Name: "cross-ns-pod-1", Kind: "Pod"} + } else { + startKey = kube.ResourceKey{Namespace: "default", Name: "pod-1", Kind: "Pod"} + } + + // For cluster-scoped start when testing namespace scanning + if tc.withNamespaceScan { + startKey = kube.ResourceKey{ + Group: "rbac.authorization.k8s.io", + Kind: "ClusterRole", + Namespace: "", + Name: "cluster-role-0", + } + } + + b.ResetTimer() + for n := 0; n < b.N; n++ { + cluster.IterateHierarchyV2([]kube.ResourceKey{startKey}, func(_ *Resource, _ map[kube.ResourceKey]*Resource) bool { + return true + }, tc.scanNamespace) + } }) } } + +// BenchmarkIterateHierarchyV2_NamespaceScaling tests how namespace scanning scales with namespace size +func BenchmarkIterateHierarchyV2_NamespaceScaling(b *testing.B) { + testCases := []struct { + name string + namespaceResourceCount int + crossNamespacePercent float64 + }{ + {"100_Resources_10pct", 100, 0.10}, + {"1000_Resources_10pct", 1000, 0.10}, + {"5000_Resources_10pct", 5000, 0.10}, + {"10000_Resources_10pct", 10000, 0.10}, + {"20000_Resources_10pct", 20000, 0.10}, + // Also test with different cross-namespace percentages + {"5000_Resources_0pct", 5000, 0.00}, + {"5000_Resources_5pct", 5000, 0.05}, + {"5000_Resources_25pct", 5000, 0.25}, + {"5000_Resources_50pct", 5000, 0.50}, + } + + for _, tc := range testCases { + b.Run(tc.name, func(b *testing.B) { + cluster := newCluster(b).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}, + GroupVersionResource: schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + + // Calculate resource distribution + clusterParents := 100 // Fixed number of cluster-scoped resources + crossNamespacePods := int(float64(tc.namespaceResourceCount) * tc.crossNamespacePercent) + regularPods := tc.namespaceResourceCount - crossNamespacePods + + testResources := buildParameterizedCrossNamespaceTestResourceMap( + clusterParents, + regularPods, + crossNamespacePods, + true, + ) + + for _, resource := range testResources { + cluster.setNode(resource) + } + + // Start from a cluster-scoped resource + startKey := kube.ResourceKey{ + Group: "rbac.authorization.k8s.io", + Kind: "ClusterRole", + Namespace: "", + Name: "cluster-role-0", + } + + b.ResetTimer() + for n := 0; n < b.N; n++ { + // Test scanning the "default" namespace which has the most resources + cluster.IterateHierarchyV2([]kube.ResourceKey{startKey}, func(_ *Resource, _ map[kube.ResourceKey]*Resource) bool { + return true + }, "default") + } + }) + } +} + +// BenchmarkIterateHierarchyV2_NamespaceScaling_NoScan provides baseline for comparison +func BenchmarkIterateHierarchyV2_NamespaceScaling_NoScan(b *testing.B) { + testCases := []struct { + name string + namespaceResourceCount int + }{ + {"100_Resources", 100}, + {"1000_Resources", 1000}, + {"5000_Resources", 5000}, + {"10000_Resources", 10000}, + {"20000_Resources", 20000}, + } + + for _, tc := range testCases { + b.Run(tc.name, func(b *testing.B) { + cluster := newCluster(b).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}, + GroupVersionResource: schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + + // All resources have cluster-scoped parents but we won't scan for them + testResources := buildParameterizedCrossNamespaceTestResourceMap( + 100, + 0, + tc.namespaceResourceCount, + true, + ) + + for _, resource := range testResources { + cluster.setNode(resource) + } + + // Start from a cluster-scoped resource + startKey := kube.ResourceKey{ + Group: "rbac.authorization.k8s.io", + Kind: "ClusterRole", + Namespace: "", + Name: "cluster-role-0", + } + + b.ResetTimer() + for n := 0; n < b.N; n++ { + // No namespace scanning - just traverse cluster-scoped children + cluster.IterateHierarchyV2([]kube.ResourceKey{startKey}, func(_ *Resource, _ map[kube.ResourceKey]*Resource) bool { + return true + }, "") + } + }) + } +} + +func TestIterateHierarchyV2_NoDuplicatesInSameNamespace(t *testing.T) { + // Create a parent-child relationship in the same namespace + parent := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1", Kind: "Deployment"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "parent", Namespace: "default", UID: "parent-uid", + }, + } + child := &appsv1.ReplicaSet{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1", Kind: "ReplicaSet"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "child", Namespace: "default", UID: "child-uid", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "apps/v1", Kind: "Deployment", Name: "parent", UID: "parent-uid", + }}, + }, + } + + cluster := newCluster(t, parent, child) + err := cluster.EnsureSynced() + require.NoError(t, err) + + visitCount := make(map[string]int) + cluster.IterateHierarchyV2( + []kube.ResourceKey{ + kube.GetResourceKey(mustToUnstructured(parent)), + kube.GetResourceKey(mustToUnstructured(child)), + }, + func(resource *Resource, _ map[kube.ResourceKey]*Resource) bool { + visitCount[resource.Ref.Name]++ + return true + }, + "", // No orphaned resource namespace + ) + + // Each resource should be visited exactly once + assert.Equal(t, 1, visitCount["parent"], "parent should be visited once") + assert.Equal(t, 1, visitCount["child"], "child should be visited once") +} + +func TestIterateHierarchyV2_NoDuplicatesCrossNamespace(t *testing.T) { + // Test that cross-namespace parent-child relationships don't cause duplicates + visitCount := make(map[string]int) + + cluster := newCluster(t, testClusterParent(), testNamespacedChild(), testClusterChild()).WithAPIResources([]kube.APIResourceInfo{{ + GroupKind: schema.GroupKind{Group: "", Kind: "Namespace"}, + GroupVersionResource: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"}, + Meta: metav1.APIResource{Namespaced: false}, + }, { + GroupKind: schema.GroupKind{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}, + GroupVersionResource: schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "clusterroles"}, + Meta: metav1.APIResource{Namespaced: false}, + }}) + err := cluster.EnsureSynced() + require.NoError(t, err) + + cluster.IterateHierarchyV2( + []kube.ResourceKey{ + kube.GetResourceKey(mustToUnstructured(testClusterParent())), + kube.GetResourceKey(mustToUnstructured(testNamespacedChild())), + kube.GetResourceKey(mustToUnstructured(testClusterChild())), + }, + func(resource *Resource, _ map[kube.ResourceKey]*Resource) bool { + visitCount[resource.Ref.Name]++ + return true + }, + "", // No orphaned resource namespace + ) + + // Each resource should be visited exactly once, even with cross-namespace relationships + assert.Equal(t, 1, visitCount["test-cluster-parent"], "cluster parent should be visited once") + assert.Equal(t, 1, visitCount["namespaced-child"], "namespaced child should be visited once") + assert.Equal(t, 1, visitCount["cluster-child"], "cluster child should be visited once") +} diff --git a/gitops-engine/pkg/cache/mocks/ClusterCache.go b/gitops-engine/pkg/cache/mocks/ClusterCache.go index b0179f85aba40..2a955bb772146 100644 --- a/gitops-engine/pkg/cache/mocks/ClusterCache.go +++ b/gitops-engine/pkg/cache/mocks/ClusterCache.go @@ -232,9 +232,9 @@ func (_m *ClusterCache) IsNamespaced(gk schema.GroupKind) (bool, error) { return r0, r1 } -// IterateHierarchyV2 provides a mock function with given fields: keys, action -func (_m *ClusterCache) IterateHierarchyV2(keys []kube.ResourceKey, action func(*cache.Resource, map[kube.ResourceKey]*cache.Resource) bool) { - _m.Called(keys, action) +// IterateHierarchyV2 provides a mock function with given fields: keys, action, orphanedResourceNamespace +func (_m *ClusterCache) IterateHierarchyV2(keys []kube.ResourceKey, action func(*cache.Resource, map[kube.ResourceKey]*cache.Resource) bool, orphanedResourceNamespace string) { + _m.Called(keys, action, orphanedResourceNamespace) } // OnEvent provides a mock function with given fields: handler diff --git a/gitops-engine/pkg/cache/predicates_test.go b/gitops-engine/pkg/cache/predicates_test.go index 74522f71e79ed..4614423a2ae62 100644 --- a/gitops-engine/pkg/cache/predicates_test.go +++ b/gitops-engine/pkg/cache/predicates_test.go @@ -118,6 +118,6 @@ func ExampleNewClusterCache_inspectNamespaceResources() { clusterCache.IterateHierarchyV2([]kube.ResourceKey{root.ResourceKey()}, func(resource *Resource, _ map[kube.ResourceKey]*Resource) bool { fmt.Printf("resource: %s, info: %v\n", resource.Ref.String(), resource.Info) return true - }) + }, "") } } diff --git a/gitops-engine/pkg/cache/resource.go b/gitops-engine/pkg/cache/resource.go index 8a7202950ee88..2837a6e401121 100644 --- a/gitops-engine/pkg/cache/resource.go +++ b/gitops-engine/pkg/cache/resource.go @@ -91,9 +91,9 @@ func (r *Resource) iterateChildrenV2(graph map[kube.ResourceKey]map[types.UID]*R if !ok || children == nil { return } - for _, c := range children { - childKey := c.ResourceKey() - child := ns[childKey] + for _, child := range children { + childKey := child.ResourceKey() + // For cross-namespace relationships, child might not be in ns, so use it directly from graph switch visited[childKey] { case 1: // Since we encountered a node that we're currently processing, we know we have a circular dependency.