Introduce agent sandboxing in to agent manager#1281
Open
vish-mv wants to merge 43 commits into
Open
Conversation
… taints in log collection
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds gVisor, Kata Containers, and Agent Sandbox support across deployment APIs, release-binding reconciliation, console environment management, Kubernetes setup scripts, Helm resources, observability configuration, Make targets, and operational documentation. ChangesBackend API and Console Isolation Tiers
Sandbox Runtime Infrastructure and Documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/models/environment.go (1)
43-53: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a
validatetag toIsolationTierfor consistency with sibling fields.Every other field on
CreateEnvironmentRequest(Name,DisplayName,DNSPrefix,DataplaneRef) carries avalidatetag, butIsolationTierhas none. Combined with the missing OpenAPIenum(see api_v1_openapi.yaml comment), an invalid tier string can pass through both layers silently.🛠️ Proposed fix
- IsolationTier string `json:"isolationTier,omitempty"` + IsolationTier string `json:"isolationTier,omitempty" validate:"omitempty,oneof=runc gvisor kata"`🤖 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/models/environment.go` around lines 43 - 53, Add a validate tag to the IsolationTier field in CreateEnvironmentRequest, enforcing the supported isolation-tier values and allowing omission when the field is empty. Use the project’s existing isolation-tier validation convention or enum symbols rather than accepting arbitrary strings.
🧹 Nitpick comments (6)
console/workspaces/libs/types/src/api/deployments.ts (1)
134-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing a single isolation-tier union type instead of a loose
string.
environmentSchema.tsdefines its ownisolationTiers/IsolationTierliteral union locally. TypingEnvironment.isolationTieras plainstringhere means the two can silently drift (e.g. a new tier added in one place but not the other) with no compiler check.♻️ Hoist the tier union into this shared package
+export const isolationTiers = ["runc", "gvisor", "kata"] as const; +export type IsolationTier = (typeof isolationTiers)[number]; + export interface Environment { ... - isolationTier?: string; + isolationTier?: IsolationTier; }Then import
isolationTiers/IsolationTierfrom this package inenvironmentSchema.tsinstead of redefining them.🤖 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 `@console/workspaces/libs/types/src/api/deployments.ts` at line 134, Replace the loose string type on Environment.isolationTier with a shared exported IsolationTier union, and expose its isolationTiers values from this API types package. Update environmentSchema.ts to import and reuse these shared symbols instead of defining a local union, ensuring both schemas remain synchronized.agent-manager-service/tests/promote_agent_test.go (1)
88-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSignature updates are correct; consider asserting
componentTypeConfigsforwarding.All three subtests correctly update
PromoteComponentFuncto the new signature, but none assert on the newcomponentTypeConfigsargument (onlyenvOverrides/fileOverridesare captured/checked). Since this parameter is new plumbing through the promote flow, a regression here (e.g., always passingnilor the wrong map) wouldn't be caught by these tests.✅ Example assertion to add
require.Equal(t, http.StatusAccepted, rr.Code) require.Len(t, ocClient.PromoteComponentCalls(), 1) call := ocClient.PromoteComponentCalls()[0] require.Equal(t, "development", call.SourceEnvironment) require.Equal(t, "production", call.TargetEnvironment) + // e.g. assert expected component type configs were forwarded + // require.Equal(t, expectedConfigs, call.ComponentTypeConfigs)Also applies to: 120-124, 155-158
🤖 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/tests/promote_agent_test.go` around lines 88 - 90, Extend the PromoteComponentFunc stubs in all three promote-agent subtests to capture the componentTypeConfigs argument and assert that each invocation forwards the expected map. Keep the existing envOverrides and fileOverrides assertions, and ensure the checks distinguish the expected component-type configuration from nil or an incorrect map.deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml (1)
118-120: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider constraining
runtimeClassNameto known values.No enum restricts this field to the actual RuntimeClasses (
gvisor,kata-qemu) plus empty. A typo would only surface as a pod scheduling failure rather than a schema validation error.♻️ Proposed enum constraint
runtimeClassName: type: string default: "" + enum: + - "" + - gvisor + - kata-qemuAlso applies to: 143-143
🤖 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-amp-platform-resources-extension/templates/component-types/agent-api.yaml` around lines 118 - 120, Constrain the runtimeClassName schema fields in the agent API component definitions to the supported values: empty, gvisor, and kata-qemu. Add the same enum restriction to both occurrences of runtimeClassName, while preserving the existing default empty string.deployments/scripts/setup-sandbox.sh (1)
28-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate chart reference/version constants across install paths.
The same
agent-sandboxchart ref and version variables are independently hardcoded here and ininstall-helpers.sh, risking version drift between the quick-start and standalone setup flows.♻️ Suggested consolidation
Move
AGENT_SANDBOX_CHART_REF,AGENT_SANDBOX_MODULE_VERSION, andAGENT_SANDBOX_UPSTREAM_VERSION(with their defaults) intodeployments/setup/env.shas the single source of truth, and haveinstall-helpers.shreference them instead of redefining.🤖 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/scripts/setup-sandbox.sh` around lines 28 - 38, Consolidate AGENT_SANDBOX_CHART_REF, AGENT_SANDBOX_MODULE_VERSION, and AGENT_SANDBOX_UPSTREAM_VERSION, including defaults, in deployments/setup/env.sh. Update setup-sandbox.sh and install-helpers.sh to source and reuse these shared variables, removing their independent definitions and chart reference so both installation paths use the same values.deployments/setup/setup-gvisor-node.sh (1)
106-117: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSame missing checksum verification as install-gvisor.sh.
For consistency with gVisor's documented install flow (which always verifies the
.sha512sidecar files), consider adding the samesha512sum -cstep here beforedocker cp'ing the binaries into the node container.🤖 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/setup/setup-gvisor-node.sh` around lines 106 - 117, Add checksum verification to the gVisor download flow before the docker cp commands, fetching each binary’s .sha512 sidecar from BASE and running sha512sum -c against the downloaded files. Keep installation proceeding only after both checks pass, then retain the existing chmod and container-copy behavior.deployments/scripts/add-environment.sh (1)
210-221: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
ISOLATION_TIERagainst the allowed set, likeIS_PRODUCTION/IDP_SKIP_TLS_VERIFY.Unlike those two flags,
ISOLATION_TIERhas no allow-list check and is embedded into the JSON body unescaped — a stray"breaks the request, and typos only surface as an opaque API error.🛡️ Proposed fix
ISOLATION_TIER="${ISOLATION_TIER:-}" +case "$ISOLATION_TIER" in + ""|gvisor|kata) ;; + *) + echo "❌ ISOLATION_TIER must be 'gvisor' or 'kata' (got '${ISOLATION_TIER}')" + exit 1 + ;; +esac ISOLATION_TIER_FIELD="" if [ -n "${ISOLATION_TIER}" ]; then ISOLATION_TIER_FIELD="\"isolationTier\": \"${ISOLATION_TIER}\"," echo " Isolation tier: ${ISOLATION_TIER}" fi🤖 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/scripts/add-environment.sh` around lines 210 - 221, Validate ISOLATION_TIER before constructing ISOLATION_TIER_FIELD, accepting only the supported values "gvisor" and "kata" (or the empty default); reject any other value with the same clear validation/error handling used for IS_PRODUCTION and IDP_SKIP_TLS_VERIFY. Keep the field omitted when empty and prevent unvalidated input from being embedded in the JSON payload.
🤖 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 `@agent-manager-service/clients/openchoreosvc/client/deployments.go`:
- Around line 364-368: Update the write paths in buildComponentTypeEnvConfigs
handling so an empty target-tier configuration removes any existing
runtimeClassName from ComponentTypeEnvironmentConfigs. Ensure this cleanup
occurs for in-place merges, empty desired values, and cases where the component
configuration map would otherwise be skipped, while preserving normal non-empty
configuration merging.
- Around line 383-416: The pre-retry current-value check in
EnsureReleaseBindingRuntimeClass is stale and allows concurrent callers to both
update and bump restartedAt. Move the desired-runtime-class comparison into the
retryReleaseBindingUpdate mutation closure, using the freshly fetched rb state
to skip mutation when runtimeClassName already matches; preserve initialization
and restartedAt updates only when correction is still required.
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 12474-12477: Add an enum constraint containing the supported
isolationTier values, "gvisor" and "kata", to the isolationTier property in both
environment schemas. Keep the existing type, description, example, and default
behavior unchanged while preventing arbitrary values from being accepted.
In `@agent-manager-service/services/agent_manager.go`:
- Around line 4139-4186: Update reconcileIsolationRuntimeClass and its caller to
run only for API agents, using the existing component/agent-type lookup or an
isAPIAgent value passed from GetAgentDeployments; skip reconciliation entirely
for non-API agents so their bindings are not modified. Also avoid performing
ListEnvironments and per-deployment EnsureReleaseBindingRuntimeClass calls on
every status read by adding the appropriate environment-list caching or
reconciliation rate limit, while preserving best-effort behavior.
In `@agent-manager-service/services/environment_service.go`:
- Around line 73-79: Validate req.IsolationTier before constructing or
submitting the environment through the OpenChoreo flow, accepting only "",
"gvisor", and "kata"; reject unsupported values instead of persisting them.
Implement this in the environment creation path surrounding the request mapping,
or add equivalent schema validation that prevents invalid isolation tiers from
being stored.
In
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml`:
- Around line 199-206: Update the gateway namespace egress rule in the agent
telemetry NetworkPolicy to include a ports restriction covering only the actual
gateway telemetry port(s) and required protocol(s). Keep the existing
namespaceSelector label match unchanged and do not allow unrestricted traffic to
other ports.
In `@deployments/scripts/add-environment.sh`:
- Around line 249-261: Update the kubectl annotate command in the isolation-tier
handling block to target the OpenChoreo control-plane namespace from
OPEN_CHOREO_DEFAULT_NAMESPACE instead of ORG_NAME. Use that same namespace in
the manual fallback command, while preserving the existing annotation and
overwrite behavior.
In `@deployments/setup/install-gvisor.sh`:
- Around line 74-82: Update the gVisor download block around BASE to use unique
temporary paths from mktemp rather than fixed /tmp/runsc and
/tmp/containerd-shim-runsc-v1 paths. Download the matching .sha512 files, verify
each binary with sha512sum -c before making it executable or moving it into
/usr/local/bin, and clean up temporary files after installation while preserving
the existing architecture-specific URLs.
---
Outside diff comments:
In `@agent-manager-service/models/environment.go`:
- Around line 43-53: Add a validate tag to the IsolationTier field in
CreateEnvironmentRequest, enforcing the supported isolation-tier values and
allowing omission when the field is empty. Use the project’s existing
isolation-tier validation convention or enum symbols rather than accepting
arbitrary strings.
---
Nitpick comments:
In `@agent-manager-service/tests/promote_agent_test.go`:
- Around line 88-90: Extend the PromoteComponentFunc stubs in all three
promote-agent subtests to capture the componentTypeConfigs argument and assert
that each invocation forwards the expected map. Keep the existing envOverrides
and fileOverrides assertions, and ensure the checks distinguish the expected
component-type configuration from nil or an incorrect map.
In `@console/workspaces/libs/types/src/api/deployments.ts`:
- Line 134: Replace the loose string type on Environment.isolationTier with a
shared exported IsolationTier union, and expose its isolationTiers values from
this API types package. Update environmentSchema.ts to import and reuse these
shared symbols instead of defining a local union, ensuring both schemas remain
synchronized.
In
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml`:
- Around line 118-120: Constrain the runtimeClassName schema fields in the agent
API component definitions to the supported values: empty, gvisor, and kata-qemu.
Add the same enum restriction to both occurrences of runtimeClassName, while
preserving the existing default empty string.
In `@deployments/scripts/add-environment.sh`:
- Around line 210-221: Validate ISOLATION_TIER before constructing
ISOLATION_TIER_FIELD, accepting only the supported values "gvisor" and "kata"
(or the empty default); reject any other value with the same clear
validation/error handling used for IS_PRODUCTION and IDP_SKIP_TLS_VERIFY. Keep
the field omitted when empty and prevent unvalidated input from being embedded
in the JSON payload.
In `@deployments/scripts/setup-sandbox.sh`:
- Around line 28-38: Consolidate AGENT_SANDBOX_CHART_REF,
AGENT_SANDBOX_MODULE_VERSION, and AGENT_SANDBOX_UPSTREAM_VERSION, including
defaults, in deployments/setup/env.sh. Update setup-sandbox.sh and
install-helpers.sh to source and reuse these shared variables, removing their
independent definitions and chart reference so both installation paths use the
same values.
In `@deployments/setup/setup-gvisor-node.sh`:
- Around line 106-117: Add checksum verification to the gVisor download flow
before the docker cp commands, fetching each binary’s .sha512 sidecar from BASE
and running sha512sum -c against the downloaded files. Keep installation
proceeding only after both checks pass, then retain the existing chmod and
container-copy behavior.
🪄 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: 024596c0-35c5-4b8b-a08f-f6bfc2d8b162
⛔ Files ignored due to path filters (1)
cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (61)
Makefileagent-manager-service/clients/clientmocks/openchoreo_client_fake.goagent-manager-service/clients/openchoreosvc/client/client.goagent-manager-service/clients/openchoreosvc/client/constants.goagent-manager-service/clients/openchoreosvc/client/deployments.goagent-manager-service/clients/openchoreosvc/client/infrastructure.goagent-manager-service/clients/openchoreosvc/client/types.goagent-manager-service/controllers/environment_controller.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/build_deployment.goagent-manager-service/models/environment.goagent-manager-service/models/infra_resources.goagent-manager-service/services/agent_manager.goagent-manager-service/services/environment_service.goagent-manager-service/spec/model_create_environment_request.goagent-manager-service/spec/model_deployment_details_response.goagent-manager-service/spec/model_gateway_environment_response.goagent-manager-service/tests/apitestutils/mock_clients.goagent-manager-service/tests/promote_agent_test.gocli/pkg/cmd/agent/status.goconsole/workspaces/libs/shared-component/src/components/EnvironmentCard/EnvironmentCard.tsxconsole/workspaces/libs/shared-component/src/components/IsolationTierIndicator/IsolationTierIndicator.tsxconsole/workspaces/libs/shared-component/src/components/IsolationTierIndicator/index.tsconsole/workspaces/libs/shared-component/src/components/index.tsconsole/workspaces/libs/types/src/api/deployments.tsconsole/workspaces/pages/deploy/src/subComponent/DeployCard.tsxconsole/workspaces/pages/deployment-pipelines/src/form/environmentSchema.tsconsole/workspaces/pages/deployment-pipelines/src/subComponents/CreateEnvironmentDrawer.tsxconsole/workspaces/pages/deployment-pipelines/src/subComponents/DeploymentPipelineTable.tsxconsole/workspaces/pages/deployment-pipelines/src/subComponents/EnvironmentTable.tsxconsole/workspaces/pages/deployment-pipelines/src/subComponents/PipelineChainEditor.tsxdeployments/helm-charts/wso2-amp-platform-resources-extension/templates/cluster-workflow-templates/ballerina-buildpack-build.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/cluster-workflow-templates/dockefile-build.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/cluster-workflow-templates/gcp-buildpack-build.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/bal-otel-instrumentation-trait.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/instrumentation-trait-env-injection.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/python-otel-instrumentation-trait.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/values.yamldeployments/k8s/gvisor-runtimeclass.yamldeployments/k8s/kata-runtimeclass.yamldeployments/quick-start/install-helpers.shdeployments/quick-start/install.shdeployments/scripts/add-environment.shdeployments/scripts/setup-sandbox.shdeployments/setup/env.shdeployments/setup/install-gvisor.shdeployments/setup/install-kata.shdeployments/setup/setup-gvisor-node.shdeployments/setup/setup-kata-node.shdeployments/setup/setup-openchoreo.shdeployments/values/observability-metrics-prometheus.yamldocumentation/docs/administration/environment-management.mdxdocumentation/docs/administration/isolation-tiers/gvisor.mdxdocumentation/docs/administration/isolation-tiers/kata.mdxdocumentation/docs/getting-started/_partials/_amp-installation.mdxdocumentation/docs/getting-started/on-a-vm.mdxdocumentation/docs/getting-started/on-k3d.mdxdocumentation/docs/getting-started/on-your-environment.mdxdocumentation/docs/getting-started/quick-start.mdxdocumentation/sidebars.ts
…am kgateway routing
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
deployments/scripts/add-environment.sh (1)
227-232: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
ISOLATION_TIERagainst allowed values.The value is interpolated directly into the JSON body (line 230) without escaping or validation, unlike
DISPLAY_NAMEwhich is escaped at line 201. An invalid value (e.g. typo or special characters) would either produce invalid JSON or a confusing API error. Adding a simple allowlist check improves error clarity and prevents malformed JSON.♻️ Proposed validation for ISOLATION_TIER
ISOLATION_TIER="${ISOLATION_TIER:-}" +case "$ISOLATION_TIER" in + ""|runc|gvisor|kata) ;; + *) + echo "❌ ISOLATION_TIER must be 'runc', 'gvisor', or 'kata' (got '${ISOLATION_TIER}')" + exit 1 + ;; +esac ISOLATION_TIER_FIELD="" if [ -n "${ISOLATION_TIER}" ]; then🤖 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/scripts/add-environment.sh` around lines 227 - 232, Validate ISOLATION_TIER against the supported isolation-tier values before constructing ISOLATION_TIER_FIELD, rejecting empty or unrecognized values with a clear error and nonzero exit. Only interpolate the validated value into the JSON payload, preserving the existing optional-field behavior when ISOLATION_TIER is unset.agent-manager-service/services/environment_service.go (1)
466-469: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the
ResolveNamespaceerror for consistency.Other error returns in this function wrap with contextual messages (e.g.,
"list environments for org %s: %w"), but theResolveNamespaceerror is returned bare. Wrapping it preserves the call-site context for debugging.♻️ Proposed fix
orgNamespace, err := ResolveNamespace(ctx, s.ocClient) if err != nil { - return nil, err + return nil, fmt.Errorf("resolve org namespace for org %s: %w", ouID, err) }🤖 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/services/environment_service.go` around lines 466 - 469, Update the ResolveNamespace error path in the surrounding function to wrap the returned error with contextual information, using the function’s existing error-wrapping style and preserving the original error via %w.
🤖 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.
Nitpick comments:
In `@agent-manager-service/services/environment_service.go`:
- Around line 466-469: Update the ResolveNamespace error path in the surrounding
function to wrap the returned error with contextual information, using the
function’s existing error-wrapping style and preserving the original error via
%w.
In `@deployments/scripts/add-environment.sh`:
- Around line 227-232: Validate ISOLATION_TIER against the supported
isolation-tier values before constructing ISOLATION_TIER_FIELD, rejecting empty
or unrecognized values with a clear error and nonzero exit. Only interpolate the
validated value into the JSON payload, preserving the existing optional-field
behavior when ISOLATION_TIER is unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9b56ef4-afcb-4d9e-8bdd-54d8bb98837c
⛔ Files ignored due to path filters (1)
cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (23)
Makefileagent-manager-service/clients/openchoreosvc/client/deployments.goagent-manager-service/clients/openchoreosvc/client/types.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/build_deployment.goagent-manager-service/services/agent_manager.goagent-manager-service/services/environment_service.goagent-manager-service/spec/model_create_environment_request.goagent-manager-service/spec/model_gateway_environment_response.goagent-manager-service/tests/apitestutils/mock_clients.godeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/instrumentation-trait-env-injection.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/values.yamldeployments/quick-start/install-helpers.shdeployments/quick-start/install.shdeployments/scripts/add-environment.shdeployments/setup/install-gvisor.shdeployments/setup/setup-openchoreo.shdocumentation/docs/getting-started/_partials/_amp-installation.mdxdocumentation/docs/getting-started/on-a-vm.mdxdocumentation/docs/getting-started/on-k3d.mdxdocumentation/docs/getting-started/on-your-environment.mdxdocumentation/docs/getting-started/quick-start.mdx
💤 Files with no reviewable changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (19)
- deployments/quick-start/install-helpers.sh
- deployments/helm-charts/wso2-amp-platform-resources-extension/values.yaml
- agent-manager-service/models/build_deployment.go
- agent-manager-service/spec/model_create_environment_request.go
- documentation/docs/getting-started/on-a-vm.mdx
- agent-manager-service/tests/apitestutils/mock_clients.go
- deployments/quick-start/install.sh
- documentation/docs/getting-started/on-k3d.mdx
- agent-manager-service/clients/openchoreosvc/client/types.go
- deployments/setup/install-gvisor.sh
- documentation/docs/getting-started/_partials/_amp-installation.mdx
- deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/instrumentation-trait-env-injection.yaml
- deployments/setup/setup-openchoreo.sh
- agent-manager-service/spec/model_gateway_environment_response.go
- documentation/docs/getting-started/on-your-environment.mdx
- deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml
- agent-manager-service/services/agent_manager.go
- agent-manager-service/clients/openchoreosvc/client/deployments.go
- agent-manager-service/docs/api_v1_openapi.yaml
hanzjk
reviewed
Jul 14, 2026
|
|
||
| const ( | ||
| ComponentTypeInternalAgentAPI ComponentType = "deployment/agent-api" | ||
| ComponentTypeInternalAgentAPI ComponentType = "proxy/agent-api" |
Contributor
There was a problem hiding this comment.
Why did we have to chnage the component type ?
Contributor
Author
There was a problem hiding this comment.
This PR moves platform-hosted agents from Deployments to OpenChoreo Sandboxes. After this change, the default deployment is a Sandbox managed by the Sandbox Controller instead of the Deployment Controller. Since agent-api now uses workloadType: proxy rather than deployment, the component changes from deployment/agent-api to proxy/agent-api. Because we need to render the SandboxTemplate instead of Deployment. ( This change has been discussed with OpenChoreo team and their suggestion was this)
hanzjk
reviewed
Jul 14, 2026
| // runtimeClassName the agent-api ComponentType should request. Empty means "omit" (default runc). | ||
| func runtimeClassForIsolationTier(tier string) string { | ||
| switch tier { | ||
| case "gvisor": |
Contributor
There was a problem hiding this comment.
Shall we define constants for these types ?
hanzjk
reviewed
Jul 14, 2026
| # subPath file mounts are not supported under Kata Containers (virtiofs | ||
| # exposes them as directories), so the mount must stay subPath-free to | ||
| # work across all isolation tiers (runc/gVisor/Kata). | ||
| default: "/amp-instrumentation" |
Contributor
There was a problem hiding this comment.
Did we specifically change this to /amp-instrumentation
Contributor
Author
There was a problem hiding this comment.
Yes, this is intentional. Previously we mounted only am-instrumentation.toml into /workspace using a single-file subPath mount, but Kata doesn't support this with virtiofs. Instead, we now mount the config volume to /amp-instrumentation and reference the file using its absolute path in BAL_CONFIG_FILES. This works the same on runc, gVisor, and Kata while keeping /workspace unchanged. Tested live on a GCP VM and a harvester cluster across all three isolation tiers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@agent-manager-service/services/agent_manager.go`:
- Around line 1778-1790: Re-run ValidateResourceRequestsWithinLimits against the
freshly fetched resource configuration immediately before each
UpdateEnvResourceConfigs attempt, including retries after a newer
resourceVersion. Ensure concurrent requests-only and limits-only updates cannot
persist an invalid combined binding, either by validating the binding being
written or serializing updates per component/environment.
🪄 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: 2b5af93e-ea07-4261-8059-184a483d34c4
📒 Files selected for processing (6)
agent-manager-service/clients/openchoreosvc/client/components.goagent-manager-service/services/agent_manager.goagent-manager-service/utils/utils.goagent-manager-service/utils/validation_test.goconsole/workspaces/libs/shared-component/src/components/IsolationTierIndicator/IsolationTierIndicator.tsxconsole/workspaces/pages/deployment-pipelines/src/subComponents/CreateEnvironmentDrawer.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- console/workspaces/libs/shared-component/src/components/IsolationTierIndicator/IsolationTierIndicator.tsx
- console/workspaces/pages/deployment-pipelines/src/subComponents/CreateEnvironmentDrawer.tsx
…en and update outdated dependencies
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/clients/openchoreosvc/client/components.go (1)
680-697: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMerging overrides may prevent removing existing resource overrides.
By changing this to a merge operation to allow partial updates, it might now be impossible for a user to remove an existing override to restore the component default.
If the fields in
req.Resources(e.g.,CPU string) useomitempty, explicitly setting a field to""to clear it will cause it to be dropped duringstructToMap. Consequently,mergeResourceOverrideswill not see the key in the incoming map and will inadvertently preserve the old value fromexistingResources.Ensure the struct uses pointer types for these fields (so explicit
""can be differentiated from an omitted field during marshaling), or provide an explicit mechanism to unset overrides.🤖 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/clients/openchoreosvc/client/components.go` around lines 680 - 697, Update the resource override request fields consumed by structToMap and mergeResourceOverrides to preserve whether each field was omitted or explicitly cleared. Prefer pointer types for fields such as CPU and memory so an explicit empty string is serialized and removes the corresponding existing override, while nil continues to preserve omitted fields; ensure the merge behavior and validation handle the resulting unset values.
🧹 Nitpick comments (1)
agent-manager-service/tests/promote_agent_test.go (1)
89-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid bypassing the repository layer in tests.
As per coding guidelines, each layer must depend on the interface of the layer below, injected through constructors. Directly calling
db.DB(context.Background())relies on a global database accessor and bypasses the repository layer.Consider injecting the required repository or database connection into the test setup to decouple it from global state and comply with the layer dependency guidelines.
🤖 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/tests/promote_agent_test.go` around lines 89 - 103, Update seedReadyAgentIdentityRow to create the AgentThunderClient through the existing repository interface instead of calling the global db.DB accessor directly. Inject or reuse the repository in the test setup via its constructor, then persist the binding through that repository while preserving the current test data.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@agent-manager-service/clients/openchoreosvc/client/components.go`:
- Around line 680-697: Update the resource override request fields consumed by
structToMap and mergeResourceOverrides to preserve whether each field was
omitted or explicitly cleared. Prefer pointer types for fields such as CPU and
memory so an explicit empty string is serialized and removes the corresponding
existing override, while nil continues to preserve omitted fields; ensure the
merge behavior and validation handle the resulting unset values.
---
Nitpick comments:
In `@agent-manager-service/tests/promote_agent_test.go`:
- Around line 89-103: Update seedReadyAgentIdentityRow to create the
AgentThunderClient through the existing repository interface instead of calling
the global db.DB accessor directly. Inject or reuse the repository in the test
setup via its constructor, then persist the binding through that repository
while preserving the current test data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce407677-c177-43fc-b47c-7fe6ca237b33
⛔ Files ignored due to path filters (1)
cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (12)
agent-manager-service/clients/openchoreosvc/client/components.goagent-manager-service/clients/openchoreosvc/client/constants.goagent-manager-service/clients/openchoreosvc/client/types.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/services/agent_manager.goagent-manager-service/tests/promote_agent_test.goagent-manager-service/utils/utils.goconsole/workspaces/libs/shared-component/src/components/EnvironmentCard/EnvironmentCard.tsxconsole/workspaces/libs/shared-component/src/components/index.tsdeployments/setup/setup-openchoreo.shdocumentation/docs/getting-started/on-a-vm.mdxdocumentation/docs/getting-started/on-your-environment.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- console/workspaces/libs/shared-component/src/components/index.ts
- console/workspaces/libs/shared-component/src/components/EnvironmentCard/EnvironmentCard.tsx
- documentation/docs/getting-started/on-a-vm.mdx
- agent-manager-service/clients/openchoreosvc/client/types.go
- deployments/setup/setup-openchoreo.sh
- agent-manager-service/docs/api_v1_openapi.yaml
- agent-manager-service/utils/utils.go
- agent-manager-service/services/agent_manager.go
- documentation/docs/getting-started/on-your-environment.mdx
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
2 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
This PR introduces per-environment isolation tiers so operators can choose how strongly agent workloads are sandboxed, per environment:
Resolves: - #1082
Goals
Approach
Control Plane
Data Plane
Node Setup
Console
User stories
Release note
Agent Manager now supports per-environment isolation tiers for agent workloads.
In addition to the default sandboxed runc runtime (Sandboxing T1), environments can now be created with:
Agents automatically run under the environment's configured runtime during deployment and promotion, without requiring any application changes.
Documentation
New Pages
Updated Pages
Training
Certification
Marketing
Automation tests
Security checks
Summary by CodeRabbit
lastDeployedwhen unavailable to prevent confusing timestamps.