Front the Agent Manager API with the API Platform Gateway (phase 1)#1307
Closed
RAVEENSR wants to merge 2 commits into
Closed
Front the Agent Manager API with the API Platform Gateway (phase 1)#1307RAVEENSR wants to merge 2 commits into
RAVEENSR wants to merge 2 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds AST-based gateway route manifest generation with CI drift checks, then introduces optional API Platform Gateway Helm resources, scope-aware RestApi operations, configurable gateway settings, and ChangesGateway integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPRoute
participant GatewayRuntime
participant RestApi
participant AgentManager
Client->>HTTPRoute: request /api/v1
HTTPRoute->>GatewayRuntime: forward to port 22893
GatewayRuntime->>RestApi: apply route and JWT policy
RestApi->>AgentManager: proxy authorized request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
agent-manager-service/scripts/gen_gateway_route_manifest/generate.go (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resourceServerPrefixis hardcoded; consider parsing it from the rbac source.The generator already parses
rbac/permissions.goviaparsePermissionsbut skips theResourceServerconstant, hardcoding"amp"instead. Ifrbac.ResourceServerever changes, the generator will silently emit incorrect scope strings. Extracting the prefix from the same AST walk is straightforward and eliminates the drift risk.♻️ Proposed refactor: parse ResourceServer alongside permissions
const resourceServerPrefix = "amp" +// or make it a var populated during parsePermissions // parsePermissions reads an rbac permissions source file and returns a map from // the Go constant name (e.g. "MonitorScoreRead") to its full scope string // (e.g. "amp:monitor:score-read"), matching rbac.Permission.Scope(). -func parsePermissions(file *ast.File) (map[string]string, error) { +func parsePermissions(file *ast.File) (string, map[string]string, error) { + prefix := "amp" // fallback perms := map[string]string{} for _, decl := range file.Decls { gen, ok := decl.(*ast.GenDecl) if !ok { continue } for _, spec := range gen.Specs { vs, ok := spec.(*ast.ValueSpec) if !ok { continue } + // Extract ResourceServer constant + if len(vs.Names) == 1 && vs.Names[0].Name == "ResourceServer" && len(vs.Values) == 1 { + if lit, ok := vs.Values[0].(*ast.BasicLit); ok { + if p, err := strconv.Unquote(lit.Value); err == nil { + prefix = p + } + } + continue + } typeName, ok := vs.Type.(*ast.Ident) if !ok || typeName.Name != "Permission" || len(vs.Names) != 1 || len(vs.Values) != 1 { continue } lit, ok := vs.Values[0].(*ast.BasicLit) if !ok { continue } value, err := strconv.Unquote(lit.Value) if err != nil { - return nil, fmt.Errorf("permission %s: %w", vs.Names[0].Name, err) + return "", nil, fmt.Errorf("permission %s: %w", vs.Names[0].Name, err) } - perms[vs.Names[0].Name] = resourceServerPrefix + ":" + value + perms[vs.Names[0].Name] = prefix + ":" + value } } - return perms, nil + return prefix, perms, nil }Then update callers (
main.go,generate_test.go,integration_test.go) to use the returned prefix instead of the hardcoded constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent-manager-service/scripts/gen_gateway_route_manifest/generate.go` around lines 44 - 45, Update parsePermissions to extract the rbac.ResourceServer constant from the same AST traversal and return it alongside the parsed permissions. Remove the hardcoded resourceServerPrefix usage, and update callers in main.go, generate_test.go, and integration_test.go to consume the parsed prefix when generating scope strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@deployments/helm-charts/wso2-agent-manager/templates/api-gateway/config.yaml`:
- Around line 100-106: Update the ThunderKeyManager JWKS remote configuration in
the Helm template so skipTlsVerify is controlled by a chart value, defaulting to
false when unset; preserve operator override support for deployments that
explicitly require TLS verification to be skipped.
---
Nitpick comments:
In `@agent-manager-service/scripts/gen_gateway_route_manifest/generate.go`:
- Around line 44-45: Update parsePermissions to extract the rbac.ResourceServer
constant from the same AST traversal and return it alongside the parsed
permissions. Remove the hardcoded resourceServerPrefix usage, and update callers
in main.go, generate_test.go, and integration_test.go to consume the parsed
prefix when generating scope strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa1cd000-57ce-4926-bd09-a9fef07e0831
📒 Files selected for processing (14)
.github/workflows/agent-manager-service-pr-checks.yamlagent-manager-service/AGENTS.mdagent-manager-service/Makefileagent-manager-service/scripts/gen_gateway_route_manifest/generate.goagent-manager-service/scripts/gen_gateway_route_manifest/generate_test.goagent-manager-service/scripts/gen_gateway_route_manifest/integration_test.goagent-manager-service/scripts/gen_gateway_route_manifest/main.godeployments/helm-charts/wso2-agent-manager/files/amp-api-route-scopes.yamldeployments/helm-charts/wso2-agent-manager/templates/_helpers.tpldeployments/helm-charts/wso2-agent-manager/templates/api-gateway/apigateway.yamldeployments/helm-charts/wso2-agent-manager/templates/api-gateway/config.yamldeployments/helm-charts/wso2-agent-manager/templates/api-gateway/restapi.yamldeployments/helm-charts/wso2-agent-manager/templates/oc-ingress.yamldeployments/helm-charts/wso2-agent-manager/values.yaml
Comment on lines
+100
to
+106
| - name: ThunderKeyManager | ||
| issuer: {{ .Values.agentManagerService.config.keyManager.issuer | quote }} | ||
| jwks: | ||
| remote: | ||
| uri: {{ .Values.agentManagerService.config.keyManager.jwksUrl | quote }} | ||
| skipTlsVerify: true | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
skipTlsVerify: true is hardcoded for the default JWKS remote.
The default JWKS URL uses plain HTTP so this is currently harmless, but if an operator configures an HTTPS JWKS endpoint, TLS verification will be silently skipped. Consider making this configurable via values or defaulting to false so HTTPS JWKS endpoints are verified by default.
🔒 Suggested fix: make skipTlsVerify configurable
- name: ThunderKeyManager
issuer: {{ .Values.agentManagerService.config.keyManager.issuer | quote }}
jwks:
remote:
uri: {{ .Values.agentManagerService.config.keyManager.jwksUrl | quote }}
- skipTlsVerify: true
+ skipTlsVerify: {{ .Values.apiGateway.jwksSkipTlsVerify | default false }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: ThunderKeyManager | |
| issuer: {{ .Values.agentManagerService.config.keyManager.issuer | quote }} | |
| jwks: | |
| remote: | |
| uri: {{ .Values.agentManagerService.config.keyManager.jwksUrl | quote }} | |
| skipTlsVerify: true | |
| {{- end }} | |
| - name: ThunderKeyManager | |
| issuer: {{ .Values.agentManagerService.config.keyManager.issuer | quote }} | |
| jwks: | |
| remote: | |
| uri: {{ .Values.agentManagerService.config.keyManager.jwksUrl | quote }} | |
| skipTlsVerify: {{ .Values.apiGateway.jwksSkipTlsVerify | default false }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deployments/helm-charts/wso2-agent-manager/templates/api-gateway/config.yaml`
around lines 100 - 106, Update the ThunderKeyManager JWKS remote configuration
in the Helm template so skipTlsVerify is controlled by a chart value, defaulting
to false when unset; preserve operator override support for deployments that
explicitly require TLS verification to be skipped.
The Agent Manager API is being fronted by the API Platform Gateway so that JWT validation and scope enforcement happen at the gateway, not only inside the service. The gateway needs a per-operation list of required scopes, and that mapping already lives in the Go route registrars (api/*.go declare an rbac permission per route). Maintaining a second hand-written copy for the gateway would drift. Add an AST-based generator (scripts/gen_gateway_route_manifest) that reads the RouteRegistrar calls and rbac permission constants and emits a deterministic manifest (amp-api-route-scopes.yaml) the Helm chart will render into the RestApi CR's jwt-auth policies. It folds string literals, local variables, concatenation, and the route() helper, and hard-fails on any pattern or permission it cannot resolve statically so drift becomes a build error rather than a silent gap. Wire make gen-gateway-scopes / gen-gateway-scopes-check and run the drift check in the existing codegen-format CI job. Routes classified as dynamic, root-OU, or validation-only are emitted as jwt-only; amp-api remains the authority for those. Related to wso2#1131
Render a standalone API Platform Gateway in front of the Agent Manager API so JWT validation and per-operation scope enforcement can happen at the gateway (issue wso2#1131). This is phase 1: everything is gated behind apiGateway.enabled and defaults off, so existing installs are unchanged. New templates under templates/api-gateway/: * APIGateway CR — standalone (no controlPlane, no registration token); the cluster-wide gateway-operator reconciles it into the runtime. * configRef ConfigMap — the gateway stack values, cloned from the per-env extension with the controlPlane block dropped and the jwt-auth keymanager derived from the same Thunder issuer/JWKS amp-api validates against, so both hops accept identical tokens. The instance route timeout is raised (timeouts are instance-global in gateway 1.1.0 and 60s is too tight for slow build-log/metric calls). * RestApi CR — context /api/v1, operations generated from the checked-in route-scope manifest. Scoped routes carry requiredScopes only when enforcement is on; OPTIONS /* and GET /config carry no policy so CORS preflight and pre-login discovery pass through. The jwt-auth policy sets forwardedTokenHeader: Authorization so the original bearer reaches amp-api under Authorization (the policy default, x-forwarded-authorization, would strip it) — amp-api keeps validating the token and enforcing org/tenant isolation as defense in depth. oc-ingress repoints /api/v1 to the gateway runtime (with a ReferenceGrant entry); /mcp, /auth/*, /.well-known/* and /healthz stay direct on amp-api. Scope enforcement follows agentManagerService.config.rbacEnabled unless apiGateway.enforceScopes overrides it, so the gateway never enforces scopes the service is configured to skip. Related to wso2#1131
RAVEENSR
force-pushed
the
amp-api-gateway-scope-manifest
branch
from
fddea0e to
5c159ba
Compare
Member
Author
|
We decided not to introduce the API Platform API Gateway between the controlplane amp-api and the edg cp kagateway. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Today all traffic to
api.amp.localhostis routed by the control-plane kgateway straight toamp-api, and JWT validation plus scope checks happen only inside the service. There is no gateway-level enforcement point in front of the platform API, even though the platform already ships the WSO2 API Platform Gateway (deployed per-environment as the data-plane "AI Gateway").This PR is phase 1 of putting the Agent Manager API behind that gateway so token validation and per-operation scope enforcement can happen at the edge. Everything is gated behind a feature flag that defaults off, so this is a no-op for existing installs.
Related to #1131
Goals
/api/v1surface, without hand-maintaining a scope list that can drift from the service's route registrars.amp-api's own JWT + RBAC middleware as defense in depth — the gateway is an additional layer, never the sole authority.Approach
Route → scope manifest generator. A new AST-based tool (
agent-manager-service/scripts/gen_gateway_route_manifest) reads the HTTP route registrars inapi/*.goand therbacpermission constants, and emits a deterministic manifest (deployments/helm-charts/wso2-agent-manager/files/amp-api-route-scopes.yaml). It folds string literals, local variables, concatenation and theroute()helper, resolvesrbac.<Perm>selectors, and hard-fails on anything it cannot resolve statically so drift becomes a build error.make gen-gateway-scopesregenerates it;make gen-gateway-scopes-checkruns in the existingcodegen-formatCI job. Routes that are dynamic, root-OU, or validation-only are classifiedjwt-only(authenticated but not scope-enforced at the gateway).Chart templates (flag-gated, default off). Under
templates/api-gateway/:APIGatewayCR (no control-plane registration/token) reconciled by the cluster-wide gateway-operator.ConfigMapfor the gateway stack, cloned from the per-env extension with the control-plane block dropped and thejwt-authkeymanager derived from the same Thunder issuer/JWKSamp-apivalidates against, so both hops accept identical tokens. The instance-global upstream route timeout is raised (gateway 1.1.0 has no per-route knob and its 60s default is too tight for slow build-log/metric calls).RestApiCR whose operations are generated from the manifest. Scoped routes carryrequiredScopesonly when enforcement is on;OPTIONS /*andGET /configcarry no policy so CORS preflight and pre-login discovery pass through.oc-ingressrepoints/api/v1to the gateway runtime (with aReferenceGrantentry);/mcp,/auth/*,/.well-known/*and/healthzstay direct onamp-api. Scope enforcement followsagentManagerService.config.rbacEnabledunlessapiGateway.enforceScopesoverrides it, so the gateway never enforces scopes the service is configured to skip.User stories
As a platform operator, I can enforce OAuth scopes for the Agent Manager API at the gateway edge, keeping in-service checks as a second layer, and turn the whole thing on or off with a single flag.
Release note
Added an opt-in API Platform Gateway in front of the Agent Manager API for gateway-level JWT validation and per-operation scope enforcement (disabled by default).
Documentation
N/A for this phase — the feature is disabled by default and not yet documented in the install guides. Documentation and enablement follow once the standalone gateway is validated on a real cluster.
Training
N/A — no training-content impact.
Certification
N/A — no certification-exam impact; this is an internal deployment/enforcement change with no new end-user concepts.
Marketing
N/A — no marketing content for an internal, default-off infrastructure change.
Automation tests
Security checks
Samples
N/A
Related PRs
N/A — first PR in the phased rollout.
Migrations (if applicable)
N/A — feature flag defaults off; enabling and disabling are both handled by Helm with no data migration or state change.
Test environment
go testfor the generator)helm template/helm lint) for chart renderingLearning
Design was grounded in the WSO2 API Platform Gateway 1.1.0 sources (gateway-controller, gateway-runtime, gateway-controllers
jwt-authpolicy) and the published gateway Helm chart, confirming standaloneAPIGatewaysupport,{param}path-template matching, therequiredScopessemantics, and the instance-global timeout model.https://claude.ai/code/session_013NPK1n7XSXtedczmv2pXx7
Summary by CodeRabbit
New Features
Bug Fixes