fix(resources): resolve built-in workloads addressed by their API group (#797)

nadaverell · web-flow · commit f47420a48b60 · 2026-05-27T20:15:25.000+03:00
diff --git a/internal/k8s/builtin_group_test.go b/internal/k8s/builtin_group_test.go
@@ -0,0 +1,95 @@
+package k8s
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+// TestTypedKindOwnsGroup pins the typed-vs-dynamic routing contract used by the
+// resource GET/LIST handlers (REST + AI). The bug this guards: the SPA threads
+// the real apiGroup for built-in workloads (deployments?group=apps), and the
+// handlers' "explicit group ⇒ dynamic cache" dispatch sent those to the dynamic
+// cache — which has no informer for built-ins — yielding a 400 "unknown
+// resource kind: deployments (group: apps)" and an empty resource drawer.
+//
+// A built-in kind addressed by its OWN group must resolve via the typed cache
+// (TypedKindOwnsGroup == true); a CRD whose plural shadows a core kind must NOT
+// (so it still routes to the dynamic cache).
+func TestTypedKindOwnsGroup(t *testing.T) {
+	cases := []struct {
+		kind  string
+		group string
+		want  bool
+	}{
+		// Built-in workloads addressed by their real group — the regression.
+		{"deployments", "apps", true},
+		{"deployment", "apps", true},
+		{"statefulsets", "apps", true},
+		{"daemonsets", "apps", true},
+		{"replicasets", "apps", true},
+		{"jobs", "batch", true},
+		{"cronjobs", "batch", true},
+		{"horizontalpodautoscalers", "autoscaling", true},
+		{"hpa", "autoscaling", true},
+		{"ingresses", "networking.k8s.io", true},
+		{"networkpolicies", "networking.k8s.io", true},
+		{"netpols", "networking.k8s.io", true},
+		{"poddisruptionbudgets", "policy", true},
+		{"storageclasses", "storage.k8s.io", true},
+		{"clusterroles", "rbac.authorization.k8s.io", true},
+
+		// Core kinds: own group is "" — typed, with or without an explicit "".
+		{"pods", "", true},
+		{"services", "", true},
+		{"sa", "", true},
+		{"secrets", "", true},
+
+		// CRD whose plural shadows a core/built-in kind — must stay dynamic.
+		{"services", "serving.knative.dev", false},
+		{"deployments", "argoproj.io", false},
+
+		// A built-in addressed with the WRONG group — not owned, routes dynamic
+		// (and discovery will reject it).
+		{"deployments", "batch", false},
+
+		// Genuine CRD kinds — never typed.
+		{"widgets", "example.com", false},
+		{"applications", "argoproj.io", false},
+	}
+	for _, tc := range cases {
+		if got := TypedKindOwnsGroup(tc.kind, tc.group); got != tc.want {
+			t.Errorf("TypedKindOwnsGroup(%q, %q) = %v, want %v", tc.kind, tc.group, got, tc.want)
+		}
+	}
+}
+
+// TestBuiltinGVR pins the static GVR fallback used when API discovery can't
+// resolve a built-in (partial discovery) — the safety net that keeps GitOps
+// drift's live last-applied GET from silently returning nil for built-in
+// managed resources. The GVR must carry the canonical plural + GA version even
+// when addressed by a singular/abbreviation form, and must reject CRD-group
+// collisions.
+func TestBuiltinGVR(t *testing.T) {
+	cases := []struct {
+		kind, group string
+		want        schema.GroupVersionResource
+		ok          bool
+	}{
+		{"deployment", "apps", schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, true},
+		{"Deployment", "apps", schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, true},
+		{"pdb", "policy", schema.GroupVersionResource{Group: "policy", Version: "v1", Resource: "poddisruptionbudgets"}, true},
+		{"hpa", "autoscaling", schema.GroupVersionResource{Group: "autoscaling", Version: "v2", Resource: "horizontalpodautoscalers"}, true},
+		{"pods", "", schema.GroupVersionResource{Version: "v1", Resource: "pods"}, true},
+		{"sa", "", schema.GroupVersionResource{Version: "v1", Resource: "serviceaccounts"}, true},
+		{"netpols", "networking.k8s.io", schema.GroupVersionResource{Group: "networking.k8s.io", Version: "v1", Resource: "networkpolicies"}, true},
+		{"services", "serving.knative.dev", schema.GroupVersionResource{}, false}, // CRD collision
+		{"widgets", "example.com", schema.GroupVersionResource{}, false},          // genuine CRD
+	}
+	for _, tc := range cases {
+		got, ok := BuiltinGVR(tc.kind, tc.group)
+		if ok != tc.ok || got != tc.want {
+			t.Errorf("BuiltinGVR(%q, %q) = (%v, %v), want (%v, %v)", tc.kind, tc.group, got, ok, tc.want, tc.ok)
+		}
+	}
+}
diff --git a/internal/k8s/cache.go b/internal/k8s/cache.go
@@ -722,6 +722,10 @@ func (c *ResourceCache) ListDynamicWithGroup(ctx context.Context, kind string, n
 		gvr, ok = discovery.GetGVR(kind)
 	}
 
+	if !ok {
+		gvr, ok = builtinGVRFallback(kind, group)
+	}
+
 	if !ok {
 		if group != "" {
 			return nil, fmt.Errorf("%w: %s (group: %s)", ErrUnknownDynamicKind, kind, group)
@@ -737,6 +741,17 @@ func (c *ResourceCache) ListDynamicWithGroup(ctx context.Context, kind string, n
 	return dynamicCache.List(gvr, namespace)
 }
 
+// builtinGVRFallback resolves a built-in kind's GVR from the static table when
+// API discovery couldn't (partial discovery under restricted RBAC, or a
+// transient refresh miss). It only resolves built-ins addressed by their own
+// group, so a CRD whose plural shadows a built-in still falls through as
+// unknown. Live/dynamic fetch paths (notably the GitOps drift last-applied GET,
+// which can't use the typed cache) rely on this so they don't silently fail
+// when discovery is incomplete.
+func builtinGVRFallback(kind, group string) (schema.GroupVersionResource, bool) {
+	return BuiltinGVR(kind, group)
+}
+
 // ErrUnknownDynamicKind is returned by ListDynamic / GetDynamicWithGroup when
 // the requested kind has no registered GVR in API discovery. Wrapped with
 // fmt.Errorf("%w: ..."), so callers should match with errors.Is. The HTTP
@@ -775,6 +790,10 @@ func (c *ResourceCache) getDynamicWithGroup(ctx context.Context, kind string, na
 		gvr, ok = discovery.GetGVR(kind)
 	}
 
+	if !ok {
+		gvr, ok = builtinGVRFallback(kind, group)
+	}
+
 	if !ok {
 		if group != "" {
 			return nil, fmt.Errorf("%w: %s (group: %s)", ErrUnknownDynamicKind, kind, group)
diff --git a/internal/k8s/fetch.go b/internal/k8s/fetch.go
@@ -2,6 +2,7 @@ package k8s
 
 import (
 	"fmt"
+	"strings"
 
 	appsv1 "k8s.io/api/apps/v1"
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
@@ -13,12 +14,102 @@ import (
 	storagev1 "k8s.io/api/storage/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 // ErrUnknownKind is returned by FetchResourceList/FetchResource when the kind
 // is not a built-in typed resource. The caller should fall through to the dynamic cache.
 var ErrUnknownKind = fmt.Errorf("unknown typed kind")
 
+// builtinGVRs maps every lowercase kind form the typed informer cache serves
+// (plural, singular, and abbreviations — the union of what
+// FetchResource/FetchResourceList and the REST switch in internal/server
+// accept) to its canonical GroupVersionResource. Versions are the GA
+// group/versions every supported cluster serves.
+//
+// One table, two jobs:
+//   - TypedKindOwnsGroup decides typed-vs-dynamic routing for the resource
+//     GET/LIST handlers (a built-in addressed by its own group must use the
+//     typed cache, not the dynamic/CRD cache).
+//   - BuiltinGVR is a static fallback for live/dynamic fetches when API
+//     discovery can't resolve a built-in's GVR (partial discovery under
+//     restricted RBAC, or a transient refresh miss). The drift/insights live
+//     GET (GetDynamicWithGroupPreserveLastApplied) can't use the typed cache —
+//     it needs last-applied, which the typed cache strips — so without this it
+//     would silently return nil and drop drift for built-in managed resources.
+//
+// Keep in sync with the typed switches in this file and internal/server.
+var builtinGVRs = func() map[string]schema.GroupVersionResource {
+	defs := []struct {
+		forms    []string
+		group    string
+		version  string
+		resource string
+	}{
+		{[]string{"pod", "pods"}, "", "v1", "pods"},
+		{[]string{"service", "services"}, "", "v1", "services"},
+		{[]string{"configmap", "configmaps"}, "", "v1", "configmaps"},
+		{[]string{"secret", "secrets"}, "", "v1", "secrets"},
+		{[]string{"event", "events"}, "", "v1", "events"},
+		{[]string{"persistentvolumeclaim", "persistentvolumeclaims", "pvc", "pvcs"}, "", "v1", "persistentvolumeclaims"},
+		{[]string{"node", "nodes"}, "", "v1", "nodes"},
+		{[]string{"namespace", "namespaces"}, "", "v1", "namespaces"},
+		{[]string{"persistentvolume", "persistentvolumes", "pv", "pvs"}, "", "v1", "persistentvolumes"},
+		{[]string{"serviceaccount", "serviceaccounts", "sa"}, "", "v1", "serviceaccounts"},
+		{[]string{"limitrange", "limitranges"}, "", "v1", "limitranges"},
+		{[]string{"resourcequota", "resourcequotas"}, "", "v1", "resourcequotas"},
+		{[]string{"deployment", "deployments"}, "apps", "v1", "deployments"},
+		{[]string{"daemonset", "daemonsets"}, "apps", "v1", "daemonsets"},
+		{[]string{"statefulset", "statefulsets"}, "apps", "v1", "statefulsets"},
+		{[]string{"replicaset", "replicasets"}, "apps", "v1", "replicasets"},
+		{[]string{"job", "jobs"}, "batch", "v1", "jobs"},
+		{[]string{"cronjob", "cronjobs"}, "batch", "v1", "cronjobs"},
+		{[]string{"hpa", "hpas", "horizontalpodautoscaler", "horizontalpodautoscalers"}, "autoscaling", "v2", "horizontalpodautoscalers"},
+		{[]string{"ingress", "ingresses"}, "networking.k8s.io", "v1", "ingresses"},
+		{[]string{"networkpolicy", "networkpolicies", "netpol", "netpols"}, "networking.k8s.io", "v1", "networkpolicies"},
+		{[]string{"ingressclass", "ingressclasses"}, "networking.k8s.io", "v1", "ingressclasses"},
+		{[]string{"poddisruptionbudget", "poddisruptionbudgets", "pdb", "pdbs"}, "policy", "v1", "poddisruptionbudgets"},
+		{[]string{"storageclass", "storageclasses", "sc"}, "storage.k8s.io", "v1", "storageclasses"},
+		{[]string{"role", "roles"}, "rbac.authorization.k8s.io", "v1", "roles"},
+		{[]string{"clusterrole", "clusterroles"}, "rbac.authorization.k8s.io", "v1", "clusterroles"},
+		{[]string{"rolebinding", "rolebindings"}, "rbac.authorization.k8s.io", "v1", "rolebindings"},
+		{[]string{"clusterrolebinding", "clusterrolebindings"}, "rbac.authorization.k8s.io", "v1", "clusterrolebindings"},
+	}
+	m := make(map[string]schema.GroupVersionResource)
+	for _, d := range defs {
+		gvr := schema.GroupVersionResource{Group: d.group, Version: d.version, Resource: d.resource}
+		for _, f := range d.forms {
+			m[f] = gvr
+		}
+	}
+	return m
+}()
+
+// TypedKindOwnsGroup reports whether (kind, group) names a built-in kind
+// addressed by its own API group — i.e. it must resolve via the typed cache,
+// not the dynamic/CRD cache. `deployments`+`apps` is a typed lookup;
+// `services`+`serving.knative.dev` is a CRD (dynamic) lookup; `services` with
+// an empty group is the core typed Service. Handlers use this to gate the "explicit group ⇒
+// dynamic cache" dispatch so built-in workloads addressed with their real group
+// don't fall through to the dynamic cache (which has no informer for them).
+func TypedKindOwnsGroup(kind, group string) bool {
+	gvr, ok := builtinGVRs[strings.ToLower(kind)]
+	return ok && gvr.Group == group
+}
+
+// BuiltinGVR returns the canonical GroupVersionResource for a built-in kind in
+// the given group, for use as a static fallback when API discovery can't
+// resolve it. group must match the kind's canonical group ("" for core kinds);
+// a mismatch (e.g. a CRD whose plural shadows a built-in) returns ok=false so
+// the caller keeps treating it as unknown rather than mis-resolving.
+func BuiltinGVR(kind, group string) (schema.GroupVersionResource, bool) {
+	gvr, ok := builtinGVRs[strings.ToLower(kind)]
+	if !ok || gvr.Group != group {
+		return schema.GroupVersionResource{}, false
+	}
+	return gvr, true
+}
+
 // ToRuntimeObjects converts a typed slice to []runtime.Object using generics.
 func ToRuntimeObjects[T runtime.Object](items []T) []runtime.Object {
 	out := make([]runtime.Object, len(items))
diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go
@@ -673,14 +673,15 @@ func handleListResources(ctx context.Context, req *mcp.CallToolRequest, input li
 		listScope = nil
 	}
 
-	// When a group is specified, route straight to the dynamic cache so
-	// CRDs whose plural collides with a core kind (e.g. Knative
-	// serving.knative.dev/Service vs corev1 ""/Service) reach the right
-	// resource. FetchResourceList is group-blind — it would silently
-	// return the core typed list, dropping the caller's group filter on
-	// the floor. Mirrors the group-aware short-circuit in REST
-	// handleAIListResources and handleGetResource (PR #721).
-	if group != "" {
+	// When a group is specified, route to the dynamic cache so CRDs whose
+	// plural collides with a core kind (e.g. Knative serving.knative.dev/Service
+	// vs corev1 ""/Service) reach the right resource. FetchResourceList is
+	// group-blind — it would silently return the core typed list, dropping the
+	// caller's group filter. But a built-in addressed by its own group
+	// (deployments?group=apps) is a typed lookup — the dynamic cache has no
+	// informer for built-ins — so only true CRDs / plural collisions go dynamic.
+	// Mirrors the group-aware dispatch in the REST handlers.
+	if group != "" && !k8s.TypedKindOwnsGroup(kind, group) {
 		return listDynamicResources(ctx, cache, kind, group, listScope, clusterScoped, input.Context)
 	}
 
@@ -828,7 +829,7 @@ func handleGetResource(ctx context.Context, req *mcp.CallToolRequest, input getR
 	// Service would silently leak the wrong object.
 	var resourceData any
 	var rawObj runtime.Object
-	if group != "" {
+	if group != "" && !k8s.TypedKindOwnsGroup(kind, group) {
 		u, dynErr := cache.GetDynamicWithGroup(ctx, kind, namespace, name, group)
 		if dynErr != nil {
 			return nil, nil, fmt.Errorf("resource not found: %w", dynErr)
diff --git a/internal/server/ai_handlers.go b/internal/server/ai_handlers.go
@@ -143,14 +143,15 @@ func (s *Server) handleAIListResources(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// When a group is specified, route straight to the dynamic cache so
-	// CRDs whose plural collides with a core kind (e.g. Knative
-	// serving.knative.dev/Service vs corev1 ""/Service, KEDA's HPA-like
-	// kinds) reach the right resource. FetchResourceList is group-blind
-	// — it would silently return the core typed list, dropping the
-	// query's group filter on the floor. Mirrors the same group-aware
-	// short-circuit in handleGetResource (PR #721).
-	if group != "" {
+	// When a group is specified, route to the dynamic cache so CRDs whose
+	// plural collides with a core kind (e.g. Knative serving.knative.dev/Service
+	// vs corev1 ""/Service, KEDA's HPA-like kinds) reach the right resource.
+	// FetchResourceList is group-blind — it would silently return the core typed
+	// list, dropping the query's group filter. EXCEPT a built-in kind addressed
+	// by its own group (deployments?group=apps) is a typed lookup: the dynamic
+	// cache has no informer for built-ins, so it would 400. Mirrors the
+	// group-aware dispatch in handleGetResource.
+	if group != "" && !k8s.TypedKindOwnsGroup(kind, group) {
 		s.aiListDynamic(w, r, cache, kind, namespaces, group, level, skipContext)
 		return
 	}
@@ -338,16 +339,16 @@ func (s *Server) handleAIGetResource(w http.ResponseWriter, r *http.Request) {
 // fetchAIResource resolves the resource from the typed cache or dynamic cache.
 // The bool reports whether the returned object is an unstructured (CRD) value.
 //
-// When a group is provided, the typed cache is skipped entirely and the
-// dynamic cache is consulted with the group qualifier. This prevents kind
-// collisions where a CRD plural shadows a core kind (e.g., Knative
-// serving.knative.dev/Service vs core/v1 Service): without this branch,
-// FetchResource("services", ...) would return the core Service from the
-// typed informer and the requested group would never be consulted, leaking
-// the wrong object via the AI surface. Mirrors handleGetResource's
-// group-first dispatch in server.go.
+// When a group is provided, the typed cache is skipped and the dynamic cache is
+// consulted with the group qualifier. This prevents kind collisions where a CRD
+// plural shadows a core kind (e.g., Knative serving.knative.dev/Service vs
+// core/v1 Service): without this branch, FetchResource("services", ...) would
+// return the core Service and the requested group would never be consulted,
+// leaking the wrong object via the AI surface. The exception is a built-in kind
+// addressed by its OWN group (deployments?group=apps) — that's a typed lookup;
+// the dynamic cache has no informer for built-ins. Mirrors handleGetResource.
 func (s *Server) fetchAIResource(ctx context.Context, cache *k8s.ResourceCache, kind, namespace, name, group string) (runtime.Object, bool, error) {
-	if group != "" {
+	if group != "" && !k8s.TypedKindOwnsGroup(kind, group) {
 		u, err := cache.GetDynamicWithGroup(ctx, kind, namespace, name, group)
 		if err != nil {
 			return nil, false, err
diff --git a/internal/server/server.go b/internal/server/server.go
@@ -1188,11 +1188,14 @@ func (s *Server) handleListResources(w http.ResponseWriter, r *http.Request) {
 		forbiddenMsg(resourceKind)
 	}
 
-	// When a group is specified, skip the typed cache and use the dynamic cache
-	// directly. This handles CRDs whose plural name collides with core resources
-	// (e.g., KNative "services" vs core "services"). Cluster-scoped gating for
-	// these is already done at the top of this handler via k8s.ClassifyKindScope.
-	if group != "" {
+	// A non-empty group routes to the dynamic/CRD cache so CRDs whose plural
+	// collides with a core kind (e.g. KNative "services" vs core "services")
+	// reach the right resource. Built-in workloads addressed by their real group
+	// (e.g. deployments?group=apps) live in the typed cache, so they must fall
+	// through to the typed switch below — TypedKindOwnsGroup keeps them off the
+	// dynamic path (which has no informer for built-ins). Cluster-scoped gating
+	// is already done at the top of this handler via k8s.ClassifyKindScope.
+	if group != "" && !k8s.TypedKindOwnsGroup(kind, group) {
 		if len(namespaces) > 0 {
 			var merged []any
 			for _, ns := range namespaces {
@@ -1648,11 +1651,16 @@ func (s *Server) handleGetResource(w http.ResponseWriter, r *http.Request) {
 		forbiddenGet(resourceKind)
 	}
 
-	// When a group is specified, skip the typed cache and use the dynamic cache
-	// directly. This handles CRDs whose plural name collides with core resources
-	// (e.g., KNative "services" vs core "services"). Cluster-scoped gating for
-	// these is already done at the top of this handler via k8s.ClassifyKindScope.
-	if group != "" {
+	// A non-empty group routes to the dynamic/CRD cache so CRDs whose plural
+	// collides with a core kind (e.g. KNative serving.knative.dev/services vs
+	// core "services") reach the right resource. But the SPA also threads the
+	// real apiGroup for BUILT-IN workloads (e.g. apps/Deployment), and those
+	// live in the typed cache, not the dynamic one — so a built-in addressed by
+	// its own group must still take the typed path below. Without this guard,
+	// deployments?group=apps fell through to the dynamic cache and 400'd with
+	// "unknown resource kind: deployments (group: apps)". Cluster-scoped gating
+	// is already done at the top of this handler via k8s.ClassifyKindScope.
+	if group != "" && !k8s.TypedKindOwnsGroup(kind, group) {
 		resource, err = cache.GetDynamicWithGroup(r.Context(), kind, namespace, name, group)
 		if err != nil {
 			if strings.Contains(err.Error(), "unknown resource kind") {
