[WIP] feat(triggers): schema-aware match conditions#7566
[WIP] feat(triggers): schema-aware match conditions#7566
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an agent-side autodiscovery endpoint to enumerate cluster resources (preferred-version GVKs) and annotate each with whether the agent ServiceAccount can list,watch it, for use in upcoming UI resourceRef selection.
Changes:
- Introduces
pkg/clusterdiscoveryto list watchable API resources and computecanWatchviaSelfSubjectRulesReview. - Exposes
GET /v1/cluster-resourcesfrom the v1 API server and wires in the discoverer fromcmd/api-server. - Adds unit tests for RBAC rule matching helpers (
canWatch,hasVerb).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/clusterdiscovery/discovery.go | New discovery + RBAC evaluation logic and response struct. |
| pkg/clusterdiscovery/discovery_test.go | Unit tests for core permission-matching helpers. |
| internal/app/api/v1/server.go | Registers the new /cluster-resources v1 route and adds optional discoverer field. |
| internal/app/api/v1/cluster_resources.go | Implements the Fiber handler for listing cluster resources. |
| cmd/api-server/main.go | Wires ClusterDiscoverer into the v1 API instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *TestkubeAPI) ListClusterResourcesHandler() fiber.Handler { | ||
| return func(c *fiber.Ctx) error { | ||
| if s.ClusterDiscoverer == nil { | ||
| return s.Error(c, http.StatusNotImplemented, fmt.Errorf("cluster discovery is not configured on this instance")) | ||
| } | ||
| resources, err := s.ClusterDiscoverer.List(c.Context()) | ||
| if err != nil { | ||
| return s.Error(c, http.StatusBadGateway, fmt.Errorf("cluster discovery: %w", err)) | ||
| } | ||
| return c.JSON(resources) | ||
| } |
There was a problem hiding this comment.
This adds a new public API endpoint, but there are no handler-level tests covering the HTTP behavior (e.g., 501 when ClusterDiscoverer is nil, 502 on discovery error, and successful JSON response). Other handlers in this package have tests, so adding a small test file for this endpoint would help prevent regressions.
| func (d *Discoverer) effectiveWatchRules(ctx context.Context) ([]authzv1.ResourceRule, error) { | ||
| review := &authzv1.SelfSubjectRulesReview{ | ||
| Spec: authzv1.SelfSubjectRulesReviewSpec{Namespace: metav1.NamespaceDefault}, | ||
| } | ||
| result, err := d.authz.AuthorizationV1().SelfSubjectRulesReviews().Create(ctx, review, metav1.CreateOptions{}) |
There was a problem hiding this comment.
SelfSubjectRulesReviewSpec.Namespace is hard-coded to metav1.NamespaceDefault, so canWatch results for namespaced resources may be incorrect whenever the agent ServiceAccount’s permissions differ between namespaces (e.g., when Testkube runs outside default). Consider plumbing the agent’s configured namespace into Discoverer (or the List call) and using that namespace for the SelfSubjectRulesReview, so the UI reflects the actual watchability in the namespace where triggers run.
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge with low risk, but the Incomplete flag omission can silently mislead the UI about RBAC permissions on clusters with certain authorizer configurations. One P1 finding: SelfSubjectRulesReview.Status.Incomplete is never checked, which means on clusters using webhook or mixed authorizers the canWatch field can be systematically wrong. The endpoint is informational (UI dropdown) so no data loss occurs, but users would get incorrect guidance about RBAC gaps. The fix is a small targeted guard. pkg/clusterdiscovery/discovery.go — effectiveWatchRules function needs Incomplete check and namespace parameter. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI
participant AgentAPI as Agent API /v1/cluster-resources
participant Discoverer as clusterdiscovery.Discoverer
participant K8sDiscovery as K8s Discovery API
participant K8sAuthz as K8s AuthZ API
UI->>AgentAPI: GET /cluster-resources
AgentAPI->>Discoverer: List(ctx)
Discoverer->>K8sDiscovery: ServerPreferredResources()
K8sDiscovery-->>Discoverer: []APIResourceList (partial ok)
Discoverer->>K8sAuthz: Create SelfSubjectRulesReview ns=default
K8sAuthz-->>Discoverer: ResourceRules (Status.Incomplete unchecked)
Discoverer->>Discoverer: filter subresources and non-watchable kinds
Discoverer->>Discoverer: canWatch per GVK
Discoverer-->>AgentAPI: []Resource with CanWatch
AgentAPI-->>UI: 200 JSON array
Reviews (1): Last reviewed commit: "feat(agent): add GET /v1/cluster-resourc..." | Re-trigger Greptile |
| func (d *Discoverer) effectiveWatchRules(ctx context.Context) ([]authzv1.ResourceRule, error) { | ||
| review := &authzv1.SelfSubjectRulesReview{ | ||
| Spec: authzv1.SelfSubjectRulesReviewSpec{Namespace: metav1.NamespaceDefault}, | ||
| } | ||
| result, err := d.authz.AuthorizationV1().SelfSubjectRulesReviews().Create(ctx, review, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("self subject rules review: %w", err) | ||
| } | ||
| return result.Status.ResourceRules, nil |
There was a problem hiding this comment.
Status.Incomplete silently ignored — canWatch can produce false negatives
SelfSubjectRulesReview sets Status.Incomplete = true when the authorizer cannot enumerate all rules (e.g. webhook authorizers, RBAC combined with Node authorizer edge cases). The code never inspects this field, so partial rule sets are treated as complete. In that scenario every resource whose RBAC rule was omitted will appear as canWatch: false in the UI, even though the agent actually has the permission. Users would then be told to add RBAC that they already have.
result, err := d.authz.AuthorizationV1().SelfSubjectRulesReviews().Create(ctx, review, metav1.CreateOptions{})
if err != nil {
return nil, fmt.Errorf("self subject rules review: %w", err)
}
if result.Status.Incomplete {
// Surface the incompleteness; callers can decide whether to trust the partial result.
return nil, fmt.Errorf("self subject rules review is incomplete (authorizer cannot enumerate rules): %s", result.Status.EvaluationError)
}
return result.Status.ResourceRules, nilAlternatively, expose Incomplete on the response so the UI can show a disclaimer rather than blocking the whole endpoint.
| review := &authzv1.SelfSubjectRulesReview{ | ||
| Spec: authzv1.SelfSubjectRulesReviewSpec{Namespace: metav1.NamespaceDefault}, | ||
| } |
There was a problem hiding this comment.
Hardcoded
default namespace for RBAC check may produce wrong canWatch results
SelfSubjectRulesReview is namespace-scoped: it returns what the caller can do in that namespace. Using metav1.NamespaceDefault ("default") is correct for ClusterRoleBindings (which apply everywhere), but it misses any RoleBindings the agent has in its own namespace (e.g. testkube). For namespace-scoped resources, permissions from a testkube-namespace RoleBinding would not appear in the result, causing those resources to show canWatch: false even if the agent has them.
Consider passing the agent's own namespace (available as the namespace config field) instead.
| review := &authzv1.SelfSubjectRulesReview{ | |
| Spec: authzv1.SelfSubjectRulesReviewSpec{Namespace: metav1.NamespaceDefault}, | |
| } | |
| review := &authzv1.SelfSubjectRulesReview{ | |
| Spec: authzv1.SelfSubjectRulesReviewSpec{Namespace: d.namespace}, | |
| } |
0ee289a to
4ba2117
Compare
4ba2117 to
17e9743
Compare
17e9743 to
25eb15f
Compare
Summary
TestTrigger.match[].path.MatchPathPattern) and rejects bracket suffixes ([*],[N]) loudly because the expression engine inpkg/expressionscan't tokenize them yet — array-path support is a follow-up.Companion PR
Required: testkube-cloud-api PR — receives + caches + serves the snapshot, validates
match[]against the schema, and ships the UI. cp-api'sgo.modis pinned to this branch's HEAD; the two should land together.Test plan
go test ./pkg/triggers/... ./internal/inventory/... ./pkg/clusterdiscovery/....status.phase, save, observe the trigger fire on a real Rollout transition.spec.replicas(integer) rejects non-numeric value.spec.containers[*].imageshows the BE rejection on saveKnown gaps (deferred)
[*]/[N]yet (validator catches at save time; runtime no-op pinned byTestMatchOnCRDArrayPathsCurrentlyUnsupported)🤖 Generated with Claude Code