sandbox: forward Create fields, fix event topics#13260
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes two issues in the sandbox gRPC controller service: it now forwards all Create request fields to the underlying controller and publishes events with valid, leading-/ topics. It also adds unit tests for the controller service’s RPC methods.
Changes:
- Forward
netns_path,rootfs, andannotations(in addition tooptions) fromControllerCreateRequestintosandbox.Controller.Create. - Fix event publish topics to include the required leading
/forCreate,Start, andWait. - Add unit tests covering the happy path for most RPC methods plus
Createfield-forwarding and invalid/unknown sandboxer handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugins/services/sandbox/controller_service.go | Fix Create option forwarding and correct event topic strings to satisfy exchange topic validation. |
| plugins/services/sandbox/controller_service_test.go | Add unit tests for controller service behavior, including Create forwarding and sandboxer error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
plugins/services/sandbox/controller_service.go:145
- When
req.Sandboxis provided,sb(andsb.ID) comes fromsandbox.FromProto(req.Sandbox), but the event and response IDs are taken fromreq.GetSandboxID(). If a client sets onlysandbox.sandbox_id(or the two IDs diverge), the controller may create one ID while the response/event report another (possibly empty). Consider deriving a single effective ID (validate equality when both are set) and use that consistently forctrl.Create, event publishing, and the response.
var sb sandbox.Sandbox
if req.Sandbox != nil {
sb = sandbox.FromProto(req.Sandbox)
} else {
sb = sandbox.Sandbox{ID: req.GetSandboxID()}
}
err = ctrl.Create(ctx, sb,
sandbox.WithOptions(req.GetOptions()),
sandbox.WithNetNSPath(req.GetNetnsPath()),
sandbox.WithRootFS(mount.FromProto(req.GetRootfs())),
sandbox.WithAnnotations(req.GetAnnotations()),
)
if err != nil {
return &api.ControllerCreateResponse{}, errgrpc.ToGRPC(err)
}
if err := s.publisher.Publish(ctx, "/sandboxes/create", &eventtypes.SandboxCreate{
SandboxID: req.GetSandboxID(),
}); err != nil {
return &api.ControllerCreateResponse{}, errgrpc.ToGRPC(err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resp, err := svc.Create(nsCtx(), &api.ControllerCreateRequest{ | ||
| SandboxID: "sb-1", | ||
| Sandboxer: "test", | ||
| NetnsPath: "/var/run/netns/test", | ||
| Rootfs: []*apitypes.Mount{ | ||
| {Type: "overlay", Source: "src", Options: []string{"ro"}}, | ||
| }, | ||
| Annotations: map[string]string{"io.containerd.netns": "/var/run/netns/test"}, | ||
| }) |
There was a problem hiding this comment.
ControllerCreateRequest.Options is omitted in this test, but controllerService.Create always applies sandbox.WithOptions(req.GetOptions()). When Options is nil, WithOptions will fail to marshal and Create will return an error, so this "happy path" test will fail (or pass/fail for the wrong reason depending on future changes). Set Options (e.g., an empty anypb.Any{}) in the request, or update controllerService.Create to skip WithOptions when req.GetOptions() is nil.
There was a problem hiding this comment.
This is inaccurate; test currently passes with this as nil. The current behavior accurately reflects how the methods are used, i.e. the Options field is optional on the request.
| _, err := svc.Create(nsCtx(), &api.ControllerCreateRequest{ | ||
| SandboxID: "sb-2", | ||
| Sandboxer: "test", | ||
| Sandbox: &apitypes.Sandbox{ | ||
| SandboxID: "sb-2", | ||
| Runtime: &apitypes.Sandbox_Runtime{Name: "test-runtime"}, | ||
| Labels: map[string]string{"key": "val"}, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
This Create request also omits Options, but controllerService.Create unconditionally calls sandbox.WithOptions(req.GetOptions()). With a nil Options, WithOptions will error during marshaling and the test won’t actually cover the intended "Sandbox proto" behavior. Provide a non-nil Options in the request (or change the service to treat nil options as "not provided").
There was a problem hiding this comment.
Same as above comment.
|
|
||
| _, err := svc.Create(nsCtx(), &api.ControllerCreateRequest{ | ||
| SandboxID: "sb-3", | ||
| Sandboxer: "test", |
There was a problem hiding this comment.
This test aims to verify controller errors are propagated, but since the request doesn’t set Options, the call can fail earlier inside sandbox.WithOptions(req.GetOptions()) (marshaling a nil) and never reach ctrl.createErr. Set a non-nil Options in the request so the failure you observe is definitely coming from the controller.
| Sandboxer: "test", | |
| Sandboxer: "test", | |
| Options: &anypb.Any{}, |
There was a problem hiding this comment.
Same as above comment.
The gRPC sandbox controller service only forwarded the `options` field
when calling the local controller. The `netns_path`, `rootfs`, and
`annotations` fields were silently dropped, causing clients using the
gRPC proxy path to receive incomplete sandbox configurations.
Event topics were missing the leading `/` prefix ("sandboxes/create"
instead of "/sandboxes/create"), causing the event exchange to reject
the publish and return an error to the caller.
Add unit tests for the controller service that exercise all RPC
methods.
Signed-off-by: William Myers <[email protected]>
estesp
added
cherry-pick/2.0.x
github-project-automation
Bot
moved this from Needs Reviewers
to Review In Progress
in Pull Request Review
github-project-automation
Bot
moved this from Review In Progress
to Done
in Pull Request Review
|
/cherry-pick release/2.0 release/2.1 release/2.2 |
|
@estesp: #13260 failed to apply on top of branch "release/2.0": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release/2.1 |
|
@estesp: #13260 failed to apply on top of branch "release/2.1": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@estesp: new pull request created: #13266 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
estesp
added
cherry-picked/2.2.x
estesp
added
cherry-picked/2.0.x
sandbox: forward Create fields, fix event topics
What
Fixes two bugs in the gRPC sandbox controller service and adds unit tests for the service.
Bug 1: Create silently dropped fields
The
controllerService.Createmethod only forwarded theoptionsfield when calling the local controller. Thenetns_path,rootfs, andannotationsfields from the gRPC request were silently discarded, causing clients using the gRPC proxy path to receive incomplete sandbox configurations.All four
CreateOptionsfields are now forwarded:Bug 2: Event topics missing leading
/Event topics were published as
"sandboxes/create"instead of"/sandboxes/create". The event exchange validates that topics start with/, so everyPublishcall returned an error to the caller. Fixed for three event-publishing methods:Create,Start, andWait.Testing
Added
controller_service_test.gowith unit tests exercising the happy path for all implemented RPC methods, field forwarding forCreate, and error handling for invalid/unknown sandboxer names.