Open
Conversation
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughIntroduces an API Platform Gateway flow: adds a new Helm chart and bootstrap Job, updates configs and scripts to install and manage the API Platform Gateway, replaces observability gateway artifacts, and adds a Makefile target plus orchestration scripts for gateway setup and port-forwarding. Changes
Sequence DiagramsequenceDiagram
participant User
participant Setup as setup-gateway.sh
participant AgentMgr as Agent Manager
participant Thunder as Thunder IDP
participant APIPlatform as API Platform (APIGateway)
participant K8s as Kubernetes
User->>Setup: run setup-gateway.sh
Setup->>AgentMgr: poll /healthz (60s timeout)
AgentMgr-->>Setup: healthy
Setup->>Thunder: request JWT (client creds)
Thunder-->>Setup: JWT
Setup->>APIPlatform: check gateway exists
APIPlatform-->>Setup: not found / needs registration
Setup->>APIPlatform: register gateway & request token
APIPlatform-->>Setup: registration token
Setup->>K8s: create Secret (token) in openchoreo-data-plane
K8s-->>Setup: Secret created
Setup->>K8s: install Helm chart & wait for APIGateway Programmed (180s)
K8s-->>Setup: Programmed / ready
Setup->>User: success (port-forward + apply OTEL manifest)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deployments/scripts/setup-openchoreo.sh (1)
365-390:⚠️ Potential issue | 🟠 MajorScope RBAC verbs to least privilege instead of wildcard
*.
verbs: ["*"]forrestapis,apigateways, andbackendsgrants unnecessary cluster-wide mutation power tocluster-agent-dataplane. Please narrow this to required verbs.🔒 Suggested RBAC tightening
rules: - apiGroups: ["gateway.api-platform.wso2.com"] resources: ["restapis", "apigateways"] - verbs: ["*"] + verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - apiGroups: ["gateway.kgateway.dev"] resources: ["backends"] - verbs: ["*"] + verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/scripts/setup-openchoreo.sh` around lines 365 - 390, Replace the wildcard verbs in the ClusterRole named wso2-api-platform-gateway-module: update the rules for resources "restapis" and "apigateways" (apiGroup "gateway.api-platform.wso2.com") and "backends" (apiGroup "gateway.kgateway.dev") to use least-privilege verb lists instead of ["*"]; for example, restrict read-only operations to ["get","list","watch"] and scope mutation verbs to only those actually required (e.g., ["create","update","patch","delete"] as needed) so that the ServiceAccount cluster-agent-dataplane in namespace openchoreo-data-plane only receives the minimal necessary permissions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@deployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/_helpers.tpl`:
- Around line 56-61: The template define
"wso2-amp-gateway-extension.apiGatewayName" can emit mixed/upper-case components
(.Values.agentManager.orgName and .Values.gateway.environment) which may violate
DNS-1123; update the printf pipeline to apply the lower filter to those
components (and ensure the final result is lowercased) before
truncation/trimSuffix so the generated name always uses lowercase characters
while preserving the existing fallback on .Values.gateway.name.
- Around line 75-102: The templates "wso2-amp-gateway-extension.idpClientIdEnv"
and "wso2-amp-gateway-extension.idpClientSecretEnv" currently reference
.Values.agentManager.idp.existingSecretClientIdKey and
.Values.agentManager.idp.existingSecretClientSecretKey without validation;
update the secretKeyRef.key usages to wrap those values with the Helm required
function (e.g. required "message"
.Values.agentManager.idp.existingSecretClientIdKey) when
.Values.agentManager.idp.existingSecret is true so Helm fails with a clear error
if the key is unset, and provide a descriptive error string for each required
call to indicate which key is missing.
In
`@deployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/gateway-config.yaml`:
- Line 34: The template currently hardcodes insecure_skip_verify: true in
gateway-config.yaml which disables TLS verification; change the template to not
hardcode this value and instead read a values key (e.g.,
apiGateway.controlPlane.tls.insecureSkipVerify) so operators can set it; update
gateway-config.yaml to use the chart value for insecure_skip_verify and add
insecureSkipVerify: false as the default in values.yaml under
apiGateway.controlPlane.tls to preserve secure defaults.
In `@deployments/helm-charts/wso2-amp-api-platform-gateway-extension/values.yaml`:
- Around line 26-34: Remove the plaintext default by clearing the hardcoded
idp.clientSecret value and rely on idp.existingSecret (and
idp.existingSecretClientIdKey / idp.existingSecretClientSecretKey) for
production; update the values under the idp section so clientSecret is empty and
add a comment indicating that existingSecret should be used instead of shipping
a default secret (referencing idp.clientSecret, idp.existingSecret,
idp.existingSecretClientIdKey, idp.existingSecretClientSecretKey).
In `@deployments/scripts/port-forward.sh`:
- Around line 58-64: The summary block later still advertises the old
"Observability Gateway" link; update that banner to reference "API Platform
Gateway" and its new ports (HTTP 22893 and HTTPS 22894) so the printed URLs
match the port-forward commands that target
svc/api-platform-default-default-gateway-gateway-runtime; find the
summary/summary-printing section that mentions "Observability Gateway" and
replace the label and port numbers/URLs accordingly (ensure both HTTP and HTTPS
entries match 22893 and 22894).
In `@deployments/scripts/setup-gateway.sh`:
- Around line 45-49: The script currently prints a success message immediately
after launching kubectl port-forward into the background (using
PORT_FORWARD_PID) without verifying it started correctly; modify the block that
runs kubectl port-forward to capture its PID (PORT_FORWARD_PID), wait a short
period, then check that the process is still running and that kubectl did not
exit with an error (e.g., test the PID with kill -0 or check the process exists
and/or inspect the command's exit status), and if the check fails, print an
error and exit non‑zero; ensure the success echo ("✅ Port-forward established")
only runs when the liveness check of PORT_FORWARD_PID confirms the port-forward
is alive.
- Around line 53-55: The kubectl readiness check is waiting for the wrong
RestApi resource name; update the kubectl wait invocation that currently targets
"restapi/traces-api-secure" to wait on
"restapi/amp-otel-collector-tracing-rest-api" (the resource created by the otel
manifest) and adjust the corresponding success message to match that resource
name so the script recognizes the actual programmed API.
In `@deployments/values/otel-collector-rest-api.yaml`:
- Around line 4-7: The deployment manifest renamed the RestApi to
amp-otel-collector-tracing-rest-api but the bootstrap script still waits on
restapi/traces-api-secure; update deployments/scripts/setup-gateway.sh so its
wait/ready check targets the new RestApi name
(amp-otel-collector-tracing-rest-api) instead of restapi/traces-api-secure,
ensuring any kubectl/oc wait or resource polling logic references the new
resource identifier and namespace openchoreo-data-plane.
In `@Makefile`:
- Around line 50-60: The post-setup printed "Next steps" order is reversed
causing users to run make port-forward before the gateway exists; update the
Makefile echo block so the recommended sequence shows "1. make setup-gateway"
first and "2. make port-forward" second (refer to the echo lines that print "📊
Next steps — install the API Platform Gateway:" and the two following echo lines
listing "make port-forward" and "make setup-gateway") so users run setup-gateway
before port-forward.
---
Outside diff comments:
In `@deployments/scripts/setup-openchoreo.sh`:
- Around line 365-390: Replace the wildcard verbs in the ClusterRole named
wso2-api-platform-gateway-module: update the rules for resources "restapis" and
"apigateways" (apiGroup "gateway.api-platform.wso2.com") and "backends"
(apiGroup "gateway.kgateway.dev") to use least-privilege verb lists instead of
["*"]; for example, restrict read-only operations to ["get","list","watch"] and
scope mutation verbs to only those actually required (e.g.,
["create","update","patch","delete"] as needed) so that the ServiceAccount
cluster-agent-dataplane in namespace openchoreo-data-plane only receives the
minimal necessary permissions.
🪄 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: 8fd95062-fe0b-4522-a27f-138b51508a98
📒 Files selected for processing (18)
Makefileagent-manager-service/config/config_loader.godeployments/helm-charts/wso2-agent-manager/values.yamldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/Chart.yamldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/_helpers.tpldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/gateway-bootstrap-job.yamldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/gateway-config.yamldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/gateway-cr.yamldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/values-dev.yamldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/values.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/api-management-trait.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/values.yamldeployments/quick-start/install.shdeployments/scripts/port-forward.shdeployments/scripts/setup-gateway.shdeployments/scripts/setup-openchoreo.shdeployments/values/obs-gateway.yamldeployments/values/otel-collector-rest-api.yaml
💤 Files with no reviewable changes (1)
- deployments/values/obs-gateway.yaml
Comment on lines
+56
to
+61
| {{- define "wso2-amp-gateway-extension.apiGatewayName" -}} | ||
| {{- if .Values.gateway.name }} | ||
| {{- .Values.gateway.name }} | ||
| {{- else }} | ||
| {{- printf "api-platform-%s-%s" .Values.agentManager.orgName .Values.gateway.environment | trunc 63 | trimSuffix "-" }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --context 3 'orgName|environment|apiGatewayName|gateway\.name|pattern' deployments/helm-charts/wso2-amp-api-platform-gateway-extension
fd -t f 'values.schema.json' deployments/helm-charts/wso2-amp-api-platform-gateway-extensionRepository: wso2/agent-manager
Length of output: 14368
Add lower filter to sanitize APIGateway name components.
agentManager.orgName and gateway.environment lack validation constraints in the values schema, so uppercase or mixed-case inputs will produce invalid Kubernetes resource names. Apply the lowercase filter to ensure the generated name always conforms to DNS-1123 requirements.
Fix
-{{- printf "api-platform-%s-%s" .Values.agentManager.orgName .Values.gateway.environment | trunc 63 | trimSuffix "-" }}
+{{- printf "api-platform-%s-%s" .Values.agentManager.orgName .Values.gateway.environment | lower | trunc 63 | trimSuffix "-" }}📝 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
| {{- define "wso2-amp-gateway-extension.apiGatewayName" -}} | |
| {{- if .Values.gateway.name }} | |
| {{- .Values.gateway.name }} | |
| {{- else }} | |
| {{- printf "api-platform-%s-%s" .Values.agentManager.orgName .Values.gateway.environment | trunc 63 | trimSuffix "-" }} | |
| {{- end }} | |
| {{- define "wso2-amp-gateway-extension.apiGatewayName" -}} | |
| {{- if .Values.gateway.name }} | |
| {{- .Values.gateway.name }} | |
| {{- else }} | |
| {{- printf "api-platform-%s-%s" .Values.agentManager.orgName .Values.gateway.environment | lower | trunc 63 | trimSuffix "-" }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@deployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/_helpers.tpl`
around lines 56 - 61, The template define
"wso2-amp-gateway-extension.apiGatewayName" can emit mixed/upper-case components
(.Values.agentManager.orgName and .Values.gateway.environment) which may violate
DNS-1123; update the printf pipeline to apply the lower filter to those
components (and ensure the final result is lowercased) before
truncation/trimSuffix so the generated name always uses lowercase characters
while preserving the existing fallback on .Values.gateway.name.
Comment on lines
+75
to
+102
| {{- define "wso2-amp-gateway-extension.idpClientIdEnv" -}} | ||
| {{- if .Values.agentManager.idp.existingSecret }} | ||
| - name: IDP_CLIENT_ID | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.agentManager.idp.existingSecret }} | ||
| key: {{ .Values.agentManager.idp.existingSecretClientIdKey }} | ||
| {{- else }} | ||
| - name: IDP_CLIENT_ID | ||
| value: {{ .Values.agentManager.idp.clientId | quote }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
| Resolve the IDP client secret from secret or direct value | ||
| */}} | ||
| {{- define "wso2-amp-gateway-extension.idpClientSecretEnv" -}} | ||
| {{- if .Values.agentManager.idp.existingSecret }} | ||
| - name: IDP_CLIENT_SECRET | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.agentManager.idp.existingSecret }} | ||
| key: {{ .Values.agentManager.idp.existingSecretClientSecretKey }} | ||
| {{- else }} | ||
| - name: IDP_CLIENT_SECRET | ||
| value: {{ .Values.agentManager.idp.clientSecret | quote }} | ||
| {{- end }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --context 3 'existingSecretClientIdKey|existingSecretClientSecretKey|existingSecret|idp:' deployments/helm-charts/wso2-amp-api-platform-gateway-extension
fd -t f 'values.schema.json' deployments/helm-charts/wso2-amp-api-platform-gateway-extensionRepository: wso2/agent-manager
Length of output: 5016
🏁 Script executed:
# Search for any schema or validation files in the chart
find deployments/helm-charts/wso2-amp-api-platform-gateway-extension -type f \( -name "*.json" -o -name "Chart.yaml" -o -name "*.md" \) | head -20Repository: wso2/agent-manager
Length of output: 137
🏁 Script executed:
# Look for any validation or documentation related to the IDP configuration
rg -n "existingSecret|idp|schema|required" deployments/helm-charts/wso2-amp-api-platform-gateway-extension/Chart.yaml deployments/helm-charts/wso2-amp-api-platform-gateway-extension/README.md 2>/dev/null || echo "No matches or files not found"Repository: wso2/agent-manager
Length of output: 92
🏁 Script executed:
# Check the full values.yaml structure to understand dependency relationships
cat -n deployments/helm-charts/wso2-amp-api-platform-gateway-extension/values.yaml | head -50Repository: wso2/agent-manager
Length of output: 2780
🏁 Script executed:
# See if there are any other templates that use these values and how they handle validation
rg -n "existingSecretClientIdKey|existingSecretClientSecretKey" deployments/helm-charts/wso2-amp-api-platform-gateway-extension/templatesRepository: wso2/agent-manager
Length of output: 382
Use required to validate secret key names when existingSecret is enabled.
When agentManager.idp.existingSecret is set, the templates reference existingSecretClientIdKey and existingSecretClientSecretKey without validation. If either is empty or unset, Helm renders a blank secretKeyRef.key, breaking the pod spec at admission time.
While values.yaml provides defaults (client-id and client-secret), these only work if the referenced secret uses those exact key names. For secrets with different key names, explicit validation prevents broken deployments. Apply the required function to enforce that these keys are explicitly set whenever existingSecret is used:
Suggested fix
- key: {{ .Values.agentManager.idp.existingSecretClientIdKey }}
+ key: {{ required "agentManager.idp.existingSecretClientIdKey is required when agentManager.idp.existingSecret is set" .Values.agentManager.idp.existingSecretClientIdKey }}
...
- key: {{ .Values.agentManager.idp.existingSecretClientSecretKey }}
+ key: {{ required "agentManager.idp.existingSecretClientSecretKey is required when agentManager.idp.existingSecret is set" .Values.agentManager.idp.existingSecretClientSecretKey }}📝 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
| {{- define "wso2-amp-gateway-extension.idpClientIdEnv" -}} | |
| {{- if .Values.agentManager.idp.existingSecret }} | |
| - name: IDP_CLIENT_ID | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.agentManager.idp.existingSecret }} | |
| key: {{ .Values.agentManager.idp.existingSecretClientIdKey }} | |
| {{- else }} | |
| - name: IDP_CLIENT_ID | |
| value: {{ .Values.agentManager.idp.clientId | quote }} | |
| {{- end }} | |
| {{- end }} | |
| {{/* | |
| Resolve the IDP client secret from secret or direct value | |
| */}} | |
| {{- define "wso2-amp-gateway-extension.idpClientSecretEnv" -}} | |
| {{- if .Values.agentManager.idp.existingSecret }} | |
| - name: IDP_CLIENT_SECRET | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.agentManager.idp.existingSecret }} | |
| key: {{ .Values.agentManager.idp.existingSecretClientSecretKey }} | |
| {{- else }} | |
| - name: IDP_CLIENT_SECRET | |
| value: {{ .Values.agentManager.idp.clientSecret | quote }} | |
| {{- end }} | |
| {{- end }} | |
| {{- define "wso2-amp-gateway-extension.idpClientIdEnv" -}} | |
| {{- if .Values.agentManager.idp.existingSecret }} | |
| - name: IDP_CLIENT_ID | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.agentManager.idp.existingSecret }} | |
| key: {{ required "agentManager.idp.existingSecretClientIdKey is required when agentManager.idp.existingSecret is set" .Values.agentManager.idp.existingSecretClientIdKey }} | |
| {{- else }} | |
| - name: IDP_CLIENT_ID | |
| value: {{ .Values.agentManager.idp.clientId | quote }} | |
| {{- end }} | |
| {{- end }} | |
| {{/* | |
| Resolve the IDP client secret from secret or direct value | |
| */}} | |
| {{- define "wso2-amp-gateway-extension.idpClientSecretEnv" -}} | |
| {{- if .Values.agentManager.idp.existingSecret }} | |
| - name: IDP_CLIENT_SECRET | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.agentManager.idp.existingSecret }} | |
| key: {{ required "agentManager.idp.existingSecretClientSecretKey is required when agentManager.idp.existingSecret is set" .Values.agentManager.idp.existingSecretClientSecretKey }} | |
| {{- else }} | |
| - name: IDP_CLIENT_SECRET | |
| value: {{ .Values.agentManager.idp.clientSecret | quote }} | |
| {{- end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@deployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/_helpers.tpl`
around lines 75 - 102, The templates "wso2-amp-gateway-extension.idpClientIdEnv"
and "wso2-amp-gateway-extension.idpClientSecretEnv" currently reference
.Values.agentManager.idp.existingSecretClientIdKey and
.Values.agentManager.idp.existingSecretClientSecretKey without validation;
update the secretKeyRef.key usages to wrap those values with the Helm required
function (e.g. required "message"
.Values.agentManager.idp.existingSecretClientIdKey) when
.Values.agentManager.idp.existingSecret is true so Helm fails with a clear error
if the key is unset, and provide a descriptive error string for each required
call to indicate which key is missing.
| path: ./data/gateway.db | ||
| controlplane: | ||
| insecure_skip_verify: false | ||
| insecure_skip_verify: true |
Contributor
There was a problem hiding this comment.
Do not hardcode insecure_skip_verify: true for control-plane TLS.
This disables certificate verification globally and weakens gateway-to-control-plane trust guarantees.
🔒 Suggested fix
- insecure_skip_verify: true
+ insecure_skip_verify: {{ .Values.apiGateway.controlPlane.tls.insecureSkipVerify | default false }}# values.yaml (add under apiGateway.controlPlane.tls)
insecureSkipVerify: 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
| insecure_skip_verify: true | |
| insecure_skip_verify: {{ .Values.apiGateway.controlPlane.tls.insecureSkipVerify | default false }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@deployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/gateway-config.yaml`
at line 34, The template currently hardcodes insecure_skip_verify: true in
gateway-config.yaml which disables TLS verification; change the template to not
hardcode this value and instead read a values key (e.g.,
apiGateway.controlPlane.tls.insecureSkipVerify) so operators can set it; update
gateway-config.yaml to use the chart value for insecure_skip_verify and add
insecureSkipVerify: false as the default in values.yaml under
apiGateway.controlPlane.tls to preserve secure defaults.
Comment on lines
+26
to
+34
| idp: | ||
| tokenUrl: "http://amp-thunder-extension-service.amp-thunder.svc.cluster.local:8090/oauth2/token" | ||
| clientId: "amp-api-client" | ||
| clientSecret: "amp-api-client-secret" # Prefer existingSecret; only set this for local/dev use | ||
| # Reference an existing secret for IDP credentials (recommended for production). | ||
| # When set, clientId and clientSecret above are ignored. | ||
| existingSecret: "" | ||
| existingSecretClientIdKey: "client-id" | ||
| existingSecretClientSecretKey: "client-secret" |
Contributor
There was a problem hiding this comment.
Avoid shipping a plaintext default clientSecret in chart values.
Keeping a concrete secret value in defaults increases secret leakage risk and weakens baseline security posture.
🔒 Suggested fix
idp:
tokenUrl: "http://amp-thunder-extension-service.amp-thunder.svc.cluster.local:8090/oauth2/token"
clientId: "amp-api-client"
- clientSecret: "amp-api-client-secret" # Prefer existingSecret; only set this for local/dev use
+ clientSecret: ""
# Reference an existing secret for IDP credentials (recommended for production).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployments/helm-charts/wso2-amp-api-platform-gateway-extension/values.yaml`
around lines 26 - 34, Remove the plaintext default by clearing the hardcoded
idp.clientSecret value and rely on idp.existingSecret (and
idp.existingSecretClientIdKey / idp.existingSecretClientSecretKey) for
production; update the values under the idp section so clientSecret is empty and
add a comment indicating that existingSecret should be used instead of shipping
a default secret (referencing idp.clientSecret, idp.existingSecret,
idp.existingSecretClientIdKey, idp.existingSecretClientSecretKey).
Comment on lines
+45
to
+49
| echo "🔌 Setting up port-forward for gateway... (port 22893)" | ||
| kubectl port-forward -n openchoreo-data-plane apigateway/api-platform-default-default 22893:22893 > /dev/null 2>&1 & | ||
| PORT_FORWARD_PID=$! | ||
| sleep 2 | ||
| echo "✅ Port-forward established (PID: $PORT_FORWARD_PID)" |
Contributor
There was a problem hiding this comment.
Port-forward is reported as successful without verifying the process is alive.
If kubectl port-forward exits immediately, the script still prints success, which masks setup failures.
🛠️ Suggested fix
kubectl port-forward -n openchoreo-data-plane apigateway/api-platform-default-default 22893:22893 > /dev/null 2>&1 &
PORT_FORWARD_PID=$!
sleep 2
-echo "✅ Port-forward established (PID: $PORT_FORWARD_PID)"
+if kill -0 "$PORT_FORWARD_PID" 2>/dev/null; then
+ echo "✅ Port-forward established (PID: $PORT_FORWARD_PID)"
+else
+ echo "❌ Port-forward failed to start"
+ exit 1
+fi📝 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
| echo "🔌 Setting up port-forward for gateway... (port 22893)" | |
| kubectl port-forward -n openchoreo-data-plane apigateway/api-platform-default-default 22893:22893 > /dev/null 2>&1 & | |
| PORT_FORWARD_PID=$! | |
| sleep 2 | |
| echo "✅ Port-forward established (PID: $PORT_FORWARD_PID)" | |
| echo "🔌 Setting up port-forward for gateway... (port 22893)" | |
| kubectl port-forward -n openchoreo-data-plane apigateway/api-platform-default-default 22893:22893 > /dev/null 2>&1 & | |
| PORT_FORWARD_PID=$! | |
| sleep 2 | |
| if kill -0 "$PORT_FORWARD_PID" 2>/dev/null; then | |
| echo "✅ Port-forward established (PID: $PORT_FORWARD_PID)" | |
| else | |
| echo "❌ Port-forward failed to start" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployments/scripts/setup-gateway.sh` around lines 45 - 49, The script
currently prints a success message immediately after launching kubectl
port-forward into the background (using PORT_FORWARD_PID) without verifying it
started correctly; modify the block that runs kubectl port-forward to capture
its PID (PORT_FORWARD_PID), wait a short period, then check that the process is
still running and that kubectl did not exit with an error (e.g., test the PID
with kill -0 or check the process exists and/or inspect the command's exit
status), and if the check fails, print an error and exit non‑zero; ensure the
success echo ("✅ Port-forward established") only runs when the liveness check of
PORT_FORWARD_PID confirms the port-forward is alive.
Comment on lines
+53
to
+55
| echo "⏳ Waiting for RestApi to be programmed..." | ||
| if kubectl wait --for=condition=Programmed restapi/traces-api-secure -n openchoreo-data-plane --timeout=120s; then | ||
| echo "✅ RestApi is programmed" |
Contributor
There was a problem hiding this comment.
RestApi readiness check waits on the wrong resource name.
The applied manifest (deployments/values/otel-collector-rest-api.yaml) creates restapi/amp-otel-collector-tracing-rest-api, so waiting on restapi/traces-api-secure will timeout even when the API is healthy.
🛠️ Suggested fix
-if kubectl wait --for=condition=Programmed restapi/traces-api-secure -n openchoreo-data-plane --timeout=120s; then
+if kubectl wait --for=condition=Programmed restapi/amp-otel-collector-tracing-rest-api -n openchoreo-data-plane --timeout=120s; then📝 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
| echo "⏳ Waiting for RestApi to be programmed..." | |
| if kubectl wait --for=condition=Programmed restapi/traces-api-secure -n openchoreo-data-plane --timeout=120s; then | |
| echo "✅ RestApi is programmed" | |
| echo "⏳ Waiting for RestApi to be programmed..." | |
| if kubectl wait --for=condition=Programmed restapi/amp-otel-collector-tracing-rest-api -n openchoreo-data-plane --timeout=120s; then | |
| echo "✅ RestApi is programmed" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployments/scripts/setup-gateway.sh` around lines 53 - 55, The kubectl
readiness check is waiting for the wrong RestApi resource name; update the
kubectl wait invocation that currently targets "restapi/traces-api-secure" to
wait on "restapi/amp-otel-collector-tracing-rest-api" (the resource created by
the otel manifest) and adjust the corresponding success message to match that
resource name so the script recognizes the actual programmed API.
Comment on lines
+4
to
+7
| name: amp-otel-collector-tracing-rest-api | ||
| namespace: openchoreo-data-plane | ||
| spec: | ||
| displayName: traces-api-secure | ||
| displayName: amp-otel-collector-tracing-rest-api |
Contributor
There was a problem hiding this comment.
Keep the wait target in sync with the renamed RestApi.
deployments/scripts/setup-gateway.sh still waits on restapi/traces-api-secure; renaming the manifest here means the bootstrap script will watch the wrong resource and report a false timeout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployments/values/otel-collector-rest-api.yaml` around lines 4 - 7, The
deployment manifest renamed the RestApi to amp-otel-collector-tracing-rest-api
but the bootstrap script still waits on restapi/traces-api-secure; update
deployments/scripts/setup-gateway.sh so its wait/ready check targets the new
RestApi name (amp-otel-collector-tracing-rest-api) instead of
restapi/traces-api-secure, ensuring any kubectl/oc wait or resource polling
logic references the new resource identifier and namespace
openchoreo-data-plane.
Comment on lines
+50
to
+60
| @echo "✅ Setup finished!" | ||
| @echo "" | ||
| @echo "🌐 Access your services:" | ||
| @echo " Console: http://localhost:3000" | ||
| @echo " API: http://localhost:8080" | ||
| @echo " Traces Observer Service: http://localhost:9098" | ||
| @echo " Database: localhost:5432" | ||
| @echo "" | ||
| @echo "📊 To access OpenChoreo services, run:" | ||
| @echo " make port-forward" | ||
| @echo "📊 Next steps — install the API Platform Gateway:" | ||
| @echo " 1. make port-forward (in a separate terminal)" | ||
| @echo " 2. make setup-gateway" |
Contributor
There was a problem hiding this comment.
Swap the post-setup order.
make port-forward will fail until the gateway service exists, so the instructions currently tell users to run the commands in the wrong order.
Suggested fix
- `@echo` "📊 Next steps — install the API Platform Gateway:"
- `@echo` " 1. make port-forward (in a separate terminal)"
- `@echo` " 2. make setup-gateway"
+ `@echo` "📊 Next steps — install the API Platform Gateway:"
+ `@echo` " 1. make setup-gateway"
+ `@echo` " 2. make port-forward (in a separate terminal)"📝 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
| @echo "✅ Setup finished!" | |
| @echo "" | |
| @echo "🌐 Access your services:" | |
| @echo " Console: http://localhost:3000" | |
| @echo " API: http://localhost:8080" | |
| @echo " Traces Observer Service: http://localhost:9098" | |
| @echo " Database: localhost:5432" | |
| @echo "" | |
| @echo "📊 To access OpenChoreo services, run:" | |
| @echo " make port-forward" | |
| @echo "📊 Next steps — install the API Platform Gateway:" | |
| @echo " 1. make port-forward (in a separate terminal)" | |
| @echo " 2. make setup-gateway" | |
| `@echo` "✅ Setup finished!" | |
| `@echo` "" | |
| `@echo` "🌐 Access your services:" | |
| `@echo` " Console: http://localhost:3000" | |
| `@echo` " API: http://localhost:8080" | |
| `@echo` " Traces Observer Service: http://localhost:9098" | |
| `@echo` " Database: localhost:5432" | |
| `@echo` "" | |
| `@echo` "📊 Next steps — install the API Platform Gateway:" | |
| `@echo` " 1. make setup-gateway" | |
| `@echo` " 2. make port-forward (in a separate terminal)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 50 - 60, The post-setup printed "Next steps" order is
reversed causing users to run make port-forward before the gateway exists;
update the Makefile echo block so the recommended sequence shows "1. make
setup-gateway" first and "2. make port-forward" second (refer to the echo lines
that print "📊 Next steps — install the API Platform Gateway:" and the two
following echo lines listing "make port-forward" and "make setup-gateway") so
users run setup-gateway before port-forward.
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
Signed-off-by: Anoshan Jayahanthan <101160077+AnoshanJ@users.noreply.github.com>
AnoshanJ
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deployments/quick-start/install-helpers.sh (1)
371-371:⚠️ Potential issue | 🟡 MinorMissing
log_infofallback function.The script defines fallback implementations for
log_error(line 68) andlog_warning(line 74) but not forlog_info. If the sourcing script doesn't providelog_info, this line will fail with "command not found".Proposed fix — add fallback for log_info
if ! declare -f log_warning >/dev/null 2>&1; then log_warning() { echo "WARNING: $1" >&2 } fi + +if ! declare -f log_info >/dev/null 2>&1; then + log_info() { + echo "INFO: $1" + } +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/quick-start/install-helpers.sh` at line 371, The script calls log_info but lacks a fallback; add a default no-op or stdout wrapper function named log_info analogous to the existing log_error and log_warning fallbacks so the script won't fail when the caller doesn't provide log_info; locate the existing fallback definitions for log_error and log_warning and implement the same pattern for log_info (same function name: log_info) to ensure safe logging calls.
🧹 Nitpick comments (2)
console/workspaces/pages/gateways/src/subComponents/AIGatewaysTable.tsx (1)
342-349: Normalize gateway type once and reuse the derived flag.The inline
toUpperCase()check is duplicated in this component (and also inViewGateway). DeriveisAIGatewayonce to reduce repetition and prevent divergence.Proposed local cleanup
+ const isAIGateway = gateway.gatewayType.toUpperCase() === "AI"; return ( <ListingTable.Row key={gateway.uuid} @@ <ListingTable.Cell align="center"> <Chip - label={gateway.gatewayType?.toUpperCase() === "AI" ? "AI" : "Regular"} + label={isAIGateway ? "AI" : "Regular"} size="small" variant="outlined" - color={gateway.gatewayType?.toUpperCase() === "AI" ? "info" : "default"} + color={isAIGateway ? "info" : "default"} /> </ListingTable.Cell>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/gateways/src/subComponents/AIGatewaysTable.tsx` around lines 342 - 349, Derive a single normalized flag (e.g., const isAIGateway = (gateway.gatewayType || "").toUpperCase() === "AI") near the top of the component and use that flag for both the label and color props instead of repeating gateway.gatewayType?.toUpperCase() inline; update the ListingTable.Cell/Chip usage here and mirror the same refactor in ViewGateway to reuse isAIGateway so behavior stays consistent and duplication is removed.console/workspaces/pages/gateways/src/AddAIGateway.Organization.tsx (1)
123-126: Drop local mutationonErrorto keep error UX centralized.This local
onErroronly logs to console and can conflict with global React Query error handling patterns used across the app.Proposed simplification
createGateway( { params: { orgName: orgId ?? "" }, body: payload, }, { onSuccess: (data) => { const viewPath = generatePath( absoluteRouteMap.children.org.children.gateways.children.view.path, { orgId: orgId ?? "", gatewayId: data.uuid } ); navigate(viewPath); }, - onError: (e: unknown) => { - // eslint-disable-next-line no-console - console.error("Failed to create gateway:", e); - }, } );Based on learnings: In TSX React components that use React Query, implement global error handling for mutations via QueryClient cache callbacks and remove per-mutation
onErrorhandlers for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/gateways/src/AddAIGateway.Organization.tsx` around lines 123 - 126, Remove the local mutation onError handler in AddAIGateway.Organization.tsx (the inline onError: (e: unknown) => console.error(...) block) so the mutation no longer logs to console; rely on the app's global React Query error handling (e.g., QueryClient/QueryCache onError callbacks or setDefaultOptions) instead, and ensure no other per-mutation error side-effects remain in the createGateway mutation or its surrounding handler code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@console/workspaces/libs/types/src/api/gateways.ts`:
- Line 21: GatewayType currently allows lowercase values ("ai"/"regular") that
don't match the backend enum; update the type definition for GatewayType to only
the backend-allowed string literals ("AI" and "REGULAR") so compile-time checks
prevent sending invalid values; locate the GatewayType alias in this file and
remove the lowercase alternatives, then run type checks to ensure callers are
updated to use "AI" or "REGULAR".
---
Outside diff comments:
In `@deployments/quick-start/install-helpers.sh`:
- Line 371: The script calls log_info but lacks a fallback; add a default no-op
or stdout wrapper function named log_info analogous to the existing log_error
and log_warning fallbacks so the script won't fail when the caller doesn't
provide log_info; locate the existing fallback definitions for log_error and
log_warning and implement the same pattern for log_info (same function name:
log_info) to ensure safe logging calls.
---
Nitpick comments:
In `@console/workspaces/pages/gateways/src/AddAIGateway.Organization.tsx`:
- Around line 123-126: Remove the local mutation onError handler in
AddAIGateway.Organization.tsx (the inline onError: (e: unknown) =>
console.error(...) block) so the mutation no longer logs to console; rely on the
app's global React Query error handling (e.g., QueryClient/QueryCache onError
callbacks or setDefaultOptions) instead, and ensure no other per-mutation error
side-effects remain in the createGateway mutation or its surrounding handler
code.
In `@console/workspaces/pages/gateways/src/subComponents/AIGatewaysTable.tsx`:
- Around line 342-349: Derive a single normalized flag (e.g., const isAIGateway
= (gateway.gatewayType || "").toUpperCase() === "AI") near the top of the
component and use that flag for both the label and color props instead of
repeating gateway.gatewayType?.toUpperCase() inline; update the
ListingTable.Cell/Chip usage here and mirror the same refactor in ViewGateway to
reuse isAIGateway so behavior stays consistent and duplication is removed.
🪄 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: 7b46ef22-523e-4dd3-a131-60f173780fde
📒 Files selected for processing (17)
.github/release-config.jsonconsole/workspaces/libs/types/src/api/gateways.tsconsole/workspaces/pages/gateways/src/AddAIGateway.Organization.tsxconsole/workspaces/pages/gateways/src/Gateways.Organization.tsxconsole/workspaces/pages/gateways/src/index.tsconsole/workspaces/pages/gateways/src/subComponents/AIGatewaysTable.tsxconsole/workspaces/pages/gateways/src/subComponents/AddAIGatewayForm.tsxconsole/workspaces/pages/gateways/src/subComponents/EditGatewayDrawer.tsxconsole/workspaces/pages/gateways/src/subComponents/ViewGateway.tsxdeployments/docker-compose.ymldeployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/gateway-cr.yamldeployments/quick-start/install-helpers.shdeployments/quick-start/install.shdeployments/scripts/port-forward.shdocumentation/docs/getting-started/_partials/_amp-installation.mdxdocumentation/docs/getting-started/on-k3d.mdxdocumentation/docs/getting-started/on-your-environment.mdx
✅ Files skipped from review due to trivial changes (7)
- console/workspaces/pages/gateways/src/Gateways.Organization.tsx
- .github/release-config.json
- console/workspaces/pages/gateways/src/subComponents/AddAIGatewayForm.tsx
- deployments/docker-compose.yml
- console/workspaces/pages/gateways/src/subComponents/EditGatewayDrawer.tsx
- deployments/helm-charts/wso2-amp-api-platform-gateway-extension/templates/gateway-cr.yaml
- console/workspaces/pages/gateways/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/quick-start/install.sh
| import type { ListQuery, OrgPathParams, PaginationMeta } from "./common"; | ||
|
|
||
| export type GatewayType = "AI" | "REGULAR"; | ||
| export type GatewayType = "AI" | "REGULAR" | "ai" | "regular"; |
Contributor
There was a problem hiding this comment.
Keep GatewayType aligned with the backend enum contract.
The API model now accepts "ai"/"regular" at type level, but the backend contract only declares "AI" and "REGULAR" as allowed values. This can let invalid request/query values compile and fail at runtime.
Proposed fix
-export type GatewayType = "AI" | "REGULAR" | "ai" | "regular";
+export type GatewayType = "AI" | "REGULAR";📝 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
| export type GatewayType = "AI" | "REGULAR" | "ai" | "regular"; | |
| export type GatewayType = "AI" | "REGULAR"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/workspaces/libs/types/src/api/gateways.ts` at line 21, GatewayType
currently allows lowercase values ("ai"/"regular") that don't match the backend
enum; update the type definition for GatewayType to only the backend-allowed
string literals ("AI" and "REGULAR") so compile-time checks prevent sending
invalid values; locate the GatewayType alias in this file and remove the
lowercase alternatives, then run type checks to ensure callers are updated to
use "AI" or "REGULAR".
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
3 participants
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
The platform used two separate gateways (obs-gateway for trace ingestion and wso2-amp-ai-gateway-extension for AI gateway registration) that are redundant — both use the same operator, CRDs, and namespace. The obs-gateway was a raw CR with no Agent Manager registration or bootstrap pattern, and its configuration was scattered across static YAML files applied via kubectl apply instead of being managed through Helm.
Goals
obs-gatewayandwso2-amp-ai-gateway-extensionwith a newwso2-amp-api-platform-gateway-extensionHelm chart that serves as the single API Platform Gateway for both ingress (agent traffic) and egress (trace ingestion)values.yamland rendered viagateway-config.yamltemplate, eliminating the need for separate ConfigMap filesapi-managementtrait template now uses Helm-templatedgateway.backendHost/gateway.backendPortvalues instead of a hardcodedobs-gatewayservice nameregulartype gateways alongsideaitypeobs-gateway.yaml,api-platform-operator-full-config.yaml,api-platform-operator-local-config.yaml, and the oldwso2-amp-ai-gateway-extensionchartaApproach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Chores