feat(sandbox)!: introduce reusable sandbox templates - #2781
Conversation
Signed-off-by: Gordon Sim <gsim@redhat.com>
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: Project-valid implementation of linked, validated issue #2680.
Head SHA: 90410d1126c65b9c386d40c19311d8124336f3ec
Base SHA: 88cf35efe426024eb19fdc403f594a1602f44d49
Merge base SHA: 88cf35efe426024eb19fdc403f594a1602f44d49
Patch ID: 1509788c1e04fbf97b0964a4804cb5ec9eafa93e
Gator payload: 4
Review mode: initial
Previous reviewed SHA: none
Review budget exhausted: no
Maintainer decision required: no
Blocking findings:
GATOR-90410d11-01: A CLI --all-workspaces option must clear the workspace field before calling an API that defines all_workspaces and workspace as mutually exclusive.GATOR-90410d11-02: Supported SDK create methods must reject provisioning fields that are no longer valid create inputs rather than silently discard them and create a differently configured sandbox.GATOR-90410d11-03: Bundled agent command guidance must not recommend a workflow that a breaking CLI change now rejects, and must cover the replacement workflow identified by the skill-maintenance map.
Carried findings:
- None
Non-blocking suggestions:
- None
Docs: Relevant Fern docs under docs/ were updated; existing navigation does not need a new entry. Companion agent command guidance still needs the correction in GATOR-90410d11-03.
Next state: gator:in-review
| .list_sandbox_templates(ListSandboxTemplatesRequest { | ||
| limit, | ||
| offset, | ||
| workspace: workspace.to_string(), |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-90410d11-01
Invariant: A CLI --all-workspaces option must clear the workspace field before calling an API that defines all_workspaces and workspace as mutually exclusive.
Prerequisite: A platform administrator invokes openshell sandbox template list --all-workspaces; the global workspace flag retains its default value of default even when the operator does not specify it.
Entry point → sink: openshell sandbox template list --all-workspaces. → The CLI sends ListSandboxTemplatesRequest to ListSandboxTemplates, whose server handler rejects requests containing both all_workspaces=true and a non-empty workspace.
Base → head: The base tree had no template-list command, while its existing sandbox, service, and provider all-workspace list paths explicitly serialized an empty workspace when all_workspaces was true. → The new template-list path always serializes workspace.to_string(); because the CLI workspace defaults to default, every normal --all-workspaces invocation reaches the server with both mutually exclusive fields set.
Impact: Cross-workspace template listing is deterministically unusable from the new CLI and returns INVALID_ARGUMENT instead of listing templates.
Reproducer: Call sandbox_template_list(server, 100, 0, "json", "default", true, tls) against the real handler or a validating test server. Observe a request with workspace="default" and all_workspaces=true, followed by all_workspaces and workspace are mutually exclusive.
PR ownership: This PR introduces the template list command, its request serialization, and the mutually-exclusive server validation; the mismatch exists only on the newly added path.
Requested change: Serialize workspace: String::new() when all_workspaces is true, matching the existing list implementations, and add an integration assertion covering the true case.
There was a problem hiding this comment.
Thanks @drew, I have fixed this and merged the fix into the cli focused commit.
| req.Annotations = converter.CopyStringMap(opts[0].Annotations) | ||
| } | ||
| if protoSpec != nil { | ||
| req.WorkloadSource = &pb.CreateSandboxRequest_Workload{Workload: protoSpec.GetWorkload()} |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-90410d11-02
Invariant: Supported SDK create methods must reject provisioning fields that are no longer valid create inputs rather than silently discard them and create a differently configured sandbox.
Prerequisite: An application migrates to the new Go or Python SDK types and supplies non-empty SandboxSpec.driver_config, including security-sensitive settings such as runtime isolation, user namespaces, or mounts.
Entry point → sink: Go Sandboxes().Create(..., *SandboxSpec, ...) and Python SandboxClient.create(spec=SandboxSpec(...)). → The SDK request builders construct CreateSandboxRequest and invoke CreateSandbox; the gateway provisions the sandbox from the emitted workload without the caller's driver configuration.
Base → head: The base Go client sent the converted SandboxSpec wholesale and the Python client copied the supplied spec wholesale, so driver config placed in the old inline template reached CreateSandbox. → The new public Go SandboxSpec still exposes DriverConfig and its converter accepts it, but the client copies only workload, policy, and providers. Python performs the same selective copy. Both calls can succeed while silently omitting the requested driver configuration.
Impact: Applications can report successful sandbox creation while the runtime lacks the requested driver behavior, potentially omitting intended isolation controls or exposing different mounts. This is a fail-open configuration error for security-related settings (CWE-693).
Reproducer: In the Go mock-client test, call Create with SandboxSpec{Workload: ..., DriverConfig: map[string]any{"kubernetes": ...}} and capture the RPC; it succeeds with no driver config represented. Repeat with Python using SandboxSpec(driver_config=...); _create_sandbox_request returns a workload request and silently drops the field.
PR ownership: This PR removes driver config from direct create and rewrites both SDK request adapters, but leaves the resolved SandboxSpec type as their accepted input without validating the newly output-only field.
Requested change: Before issuing the RPC, return a clear invalid-argument error whenever create receives non-empty DriverConfig, directing callers to a named template. Alternatively, introduce a create-specific input that cannot express resolved-only fields. Add Go and Python tests proving the value is rejected rather than ignored.
Sibling sites: python/openshell/sandbox.py:1122
There was a problem hiding this comment.
Thanks @drew, I have improved all the SDKs and addressed these comments within those changes.
| driver_config, | ||
| ..SandboxTemplate::default() | ||
| }) | ||
| if driver_config_json.is_some() { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-90410d11-03
Invariant: Bundled agent command guidance must not recommend a workflow that a breaking CLI change now rejects, and must cover the replacement workflow identified by the skill-maintenance map.
Prerequisite: A user or agent follows the repository's openshell-cli skill to apply driver-specific sandbox configuration.
Entry point → sink: The openshell-cli skill's Sandbox Lifecycle workflow and its companion CLI reference. → sandbox_create rejects the skill-recommended direct --driver-config-json command before sending CreateSandbox.
Base → head: Direct openshell sandbox create --driver-config-json ... was supported and matched the bundled skill and CLI reference. → Direct create now always rejects that flag and requires named templates, but the skill still lists --driver-config-json as a direct-create flag and contains no template-management or --template workflow.
Impact: Agents following the maintained project skill emit commands that now fail and receive no skill-level guidance for creating, inspecting, using, or deleting the replacement template resource.
Reproducer: Follow .agents/skills/openshell-cli/SKILL.md Workflow 3 or the create flag table in cli-reference.md and invoke openshell sandbox create --driver-config-json '{"kubernetes":{}}'; observe the deterministic error that driver config is only supported through named templates.
PR ownership: The PR intentionally makes this CLI workflow breaking. The sync-agent-infra maintenance map routes CLI command and workflow changes to openshell-cli, but the changed-file set omits that skill.
Requested change: Update the skill and companion CLI reference to document template create/get/list/delete, sandbox create --template, template conflicts, and the removal of direct-create driver config.
Sibling sites: .agents/skills/openshell-cli/SKILL.md:205, .agents/skills/openshell-cli/cli-reference.md:218
There was a problem hiding this comment.
Thanks @drew, I have fixed this and merged the fix into the cli focused commit.
Signed-off-by: Gordon Sim <gsim@redhat.com>
90410d1 to
8b55925
Compare
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: Project-valid implementation of linked, validated issue #2680.
Head SHA: 8b559258e1e0100bc0ecae441874cca4c3cfd118
Base SHA: dc374e88784a7e4a7366abd1fdb56c969efe6769
Merge base SHA: 88cf35efe426024eb19fdc403f594a1602f44d49
Patch ID: c9156e4e480bf533c7a8a1b351c96e6610cb0c14
Gator payload: 4
Review mode: follow_up
Previous reviewed SHA: 90410d1126c65b9c386d40c19311d8124336f3ec
Review budget exhausted: no
Maintainer decision required: no
Thanks @grs. I checked your notes that the CLI all-workspaces path and bundled CLI guidance were fixed in the CLI-focused commit. The list request now clears workspace and has an integration assertion, and the skill/reference now document template CRUD, --template, conflicts, and the removal of direct-create driver config. I also checked the remaining SDK obligation: the Go and Python create APIs now take create-specific workload/policy/provider inputs, so they can no longer silently accept and discard resolved driver configuration.
Resolved prior findings:
GATOR-90410d11-01: resolved by the corrected CLI request and integration coverage.GATOR-90410d11-02: resolved by the create-specific Go and Python API inputs.GATOR-90410d11-03: resolved by the updated bundled skill and CLI reference.
Blocking findings:
GATOR-8b559258-01: migrate the Python E2E harness to the breaking create API so required security-sensitive suites remain executable.GATOR-8b559258-02: make the Go fake resolve workspace-scoped templates with production-equivalent NotFound, workload, driver-config, and provenance behavior.
Carried findings:
- None.
Docs: Fern docs and companion agent guidance are updated; existing navigation does not need a new entry.
Next state: gator:in-review
| sandbox: str | SandboxRef | None = None, | ||
| delete_on_exit: bool = True, | ||
| spec: openshell_pb2.SandboxSpec | None = None, | ||
| workload: openshell_pb2.SandboxWorkloadConfig | None = None, |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-8b559258-01
Invariant: Breaking changes to the Python sandbox creation surface must migrate the supported E2E harness so security-sensitive sandbox tests remain executable.
Prerequisite: An operator runs the Python E2E suite through its shared sandbox fixture or the direct provider-test client path.
Entry point → sink: e2e/python/conftest.py or test_sandbox_providers.py → Python argument binding for Sandbox.__init__ or SandboxClient.create.
Base → head: The prior API accepted spec, matching the shared E2E callers. This delta replaces it with workload, policy, and providers, while those callers still pass spec, so they fail before issuing an RPC.
Impact: Policy, Landlock, inference-routing, and provider E2E coverage aborts before reaching the gateway and cannot detect regressions in this breaking API.
Reproducer: Invoke the existing fixture as sandbox(spec=datamodel_pb2.SandboxSpec(policy=...)); it deterministically raises TypeError: unexpected keyword argument 'spec'. The direct call at e2e/python/test_sandbox_providers.py:423 fails likewise.
PR ownership: This delta changes the Python create signatures but leaves repository callers of the removed parameter unchanged.
Requested change: Update the shared fixture to unpack its SandboxSpec into workload, policy, and providers, and migrate the direct SandboxClient caller so existing tests exercise the new API.
Sibling sites: e2e/python/conftest.py:94, e2e/python/test_sandbox_providers.py:423.
There was a problem hiding this comment.
Thanks again! I have fixed that and verified all python e2e tests pass.
| if spec == nil { | ||
| spec = &types.SandboxSpec{} | ||
| // CreateFromTemplate creates a new sandbox from a named template with Provisioning phase. | ||
| func (c *fakeSandboxClient) CreateFromTemplate(_ context.Context, workspace, name, templateName string, policy *types.SandboxPolicy, providers []string, labels map[string]string, opts ...types.CreateOptions) (*types.Sandbox, error) { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-8b559258-02
Invariant: The Go fake client's CreateFromTemplate path must model workspace-scoped resolution, including NotFound behavior and the resolved workload, driver configuration, and provenance returned by the gateway.
Prerequisite: An SDK consumer tests template-based creation with fake.NewClient.
Entry point → sink: fake.Client.Sandboxes().CreateFromTemplate → the fake sandbox store persists a sandbox without consulting the template store.
Base → head: The previous tree had no fake template-create path. The new method accepts any non-empty name and returns a sandbox with nil workload and driver config, even when the template is absent or contains those values.
Impact: Supported fake-client tests can pass when production returns NotFound and cannot verify application of security-sensitive driver configuration or workload settings.
Reproducer: Call CreateFromTemplate for missing and observe success; then create gpu-kata with an image and driver config and observe the returned sandbox has nil Spec.Workload and Spec.DriverConfig.
PR ownership: This delta introduces both the template store and fake CreateFromTemplate, but wires the sandbox and template clients to independent stores without resolution semantics.
Requested change: Pass the template store into the fake sandbox client; look up the template in the requested workspace, return NotFound when absent, deep-copy its workload and driver config, populate provenance with its resource version, and add tests for missing and resolved templates.
Sibling site: sdk/go/openshell/v1/fake/fake.go:82.
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
8b55925 to
97e5fa4
Compare
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: Project-valid implementation of linked, validated issue #2680.
Head SHA: 97e5fa442a59e19765f6549b9659719890192b99
Base SHA: dc374e88784a7e4a7366abd1fdb56c969efe6769
Merge base SHA: 88cf35efe426024eb19fdc403f594a1602f44d49
Patch ID: 2c1133ee75c560e59cac0e31c7d56eaf841f9cd5
Gator payload: 4
Review mode: follow_up
Previous reviewed SHA: 8b559258e1e0100bc0ecae441874cca4c3cfd118
Review budget exhausted: no
Maintainer decision required: no
I checked the latest author delta against the two outstanding obligations. The Python E2E fixture and direct provider-test client now adapt the old spec shape to the new create inputs, so the security-sensitive suites can reach the gateway. The Go fake now resolves templates through the workspace-scoped template store, returns NotFound for missing templates, deep-copies workload and driver configuration, and records template resource-version provenance with coverage for those behaviors.
Resolved prior findings:
GATOR-8b559258-01: resolved by the Python E2E harness migration.GATOR-8b559258-02: resolved by production-equivalent fake template resolution and tests.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None.
Docs: Fern docs and companion agent guidance are updated; existing navigation does not need a new entry.
Next state: gator:in-review pending required test dispatch confirmation.
drew
added
test:e2e
|
Label |
|
Label |
|
/ok to test 97e5fa4 |
drew
added
test:e2e
|
Label |
|
Label |
drew
added
gator:blocked
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
While I don't disagree that we're mixing different concepts when creating a sandbox, I don't know whether exposing templates as a first-class concept is what we want here (or at least not at this stage).
We also haven't answered the question as to whether OpenShell should be the entity that manage the lifecycle of templates. Would storing these out of band (assuming something like #1520) not be a better solution to start with?
Let's discuss this further in #2680.
drew
added
gator:in-review
I very much like the idea behind #1520. To me though it is orthogonal to whether you have a first class template type. The value of a template is that it removes one set of concerns from those creating sandboxes. The workspace admin can setup up tested templates which can then be used by name without needing to know the details. The templates can be changed as needed, and use cases that reference them pick up those changes for any new sandboxes. A specific benefit is warm pooling. Knowing up front what sorts of workloads are likely to be created means that warm pools can be created if desired. This is of course not the only way to do so, but I think it works well as a secondary motivation. Being able to create both templates and sandboxes through some form of As an example, lets say a particular use case has the need for two categories of sandbox, lets say 'codex small' and 'codex large' for example. By having templates to reflect these two categories, the definitions of the sandboxes remain simpler, and more focused on the providers. If it is decided that the memory needs need to be increased for one or other, you can do that in one place rather than in all the sandbox definitions. |
drew
added
gator:in-review
3 participants
Summary
Introduce workspace-scoped reusable sandbox templates and move driver-specific sandbox configuration behind those named templates. Direct sandbox creates now use a portable workload shape, while templates can own shared image, environment, resources, driver config, and startup service objectives.
Note: this is a breaking change!
Related Issue
Fixes #2680
Changes
CreateSandboxRequestto accept either an inlineSandboxWorkloadConfigor aworkload_template_name, with policy and providers promoted to top-level request fields.openshell sandbox template create|get|list|deleteCLI commands.openshell sandbox create --template <name>and reject conflicting inline workload flags such as--from,--cpu,--memory,--gpu,--env, and--driver-config-json.sandbox create --driver-config-jsonto template creation.Testing
mise run pre-commitpassesChecklist