From d09f7cbe00130c0bb8de8a9e55990445e2b66542 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Fri, 18 Aug 2023 14:39:13 +0200 Subject: [PATCH] cri: Fix sandbox_mode "shim" This is a partial revert of "cri/sbserver: Use platform instead of GOOS for userns detection". While what that commit did is 100% the right thing to do, when the sandbox_mode is "shim" all controller.XXX() calls are RPCs and the controller.Create() call initializes the controller. Therefore, things like "getSandboxController()" don't work in the case of "shim" sandbox_mode until after the controller.Create(). Due to this asymmetry and the lack of tests for shim mode, we didn't catch it before. This patch just reverts that commit so that the Create() and getSandboxController() calls remain where they were, and just relies on the config Linux section as a hack to detect if the pod sandbox will use user namespaces or not. Signed-off-by: Rodrigo Campos --- pkg/cri/sbserver/sandbox_run.go | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/pkg/cri/sbserver/sandbox_run.go b/pkg/cri/sbserver/sandbox_run.go index c6baa7709..f9bba6503 100644 --- a/pkg/cri/sbserver/sandbox_run.go +++ b/pkg/cri/sbserver/sandbox_run.go @@ -143,18 +143,16 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox } }() - controller, err := c.getSandboxController(sandbox.Config, sandbox.RuntimeHandler) - if err != nil { - return nil, fmt.Errorf("failed to get sandbox controller: %w", err) - } - platform, err := controller.Platform(ctx, sandbox.ID) - if err != nil { - return nil, fmt.Errorf("failed to query sandbox platform: %w", err) - } - + // XXX: What we really want here is to call controller.Platform() and then check + // platform.OS, but that is only populated after controller.Create() and that needs to be + // done later (uses sandbox.NSPath that we will set just _after_ this). + // So, lets check for the Linux section on the config, if that is populated, we assume the + // platform is linux. + // This is a hack, we should improve the controller interface to return the platform + // earlier. But should work fine for this specific use. userNsEnabled := false - if platform.OS == "linux" { - usernsOpts := config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetUsernsOptions() + if linux := config.GetLinux(); linux != nil { + usernsOpts := linux.GetSecurityContext().GetNamespaceOptions().GetUsernsOptions() if usernsOpts != nil && usernsOpts.GetMode() == runtime.NamespaceMode_POD { userNsEnabled = true } @@ -241,6 +239,11 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox return nil, fmt.Errorf("unable to save sandbox %q to store: %w", id, err) } + controller, err := c.getSandboxController(config, r.GetRuntimeHandler()) + if err != nil { + return nil, fmt.Errorf("failed to get sandbox controller: %w", err) + } + // Save sandbox metadata to store if sandboxInfo, err = c.client.SandboxStore().Update(ctx, sandboxInfo, "extensions"); err != nil { return nil, fmt.Errorf("unable to update extensions for sandbox %q: %w", id, err)