Skip to content

Commit 8137e41

Browse files
Kirtana AshokKirtana Ashok
authored andcommitted
Add ArgsEscaped support for CRI
This commit adds supports for the ArgsEscaped value for the image got from the dockerfile. It is used to evaluate and process the image entrypoint/cmd and container entrypoint/cmd options got from the podspec. Signed-off-by: Kirtana Ashok <[email protected]>
1 parent 97480af commit 8137e41

File tree

11 files changed

+363
-20
lines changed

11 files changed

+363
-20
lines changed

integration/images/image_list.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ type ImageList struct {
3636
ResourceConsumer string
3737
VolumeCopyUp string
3838
VolumeOwnership string
39+
ArgsEscaped string
3940
}
4041

4142
var (
@@ -53,6 +54,7 @@ func initImages(imageListFile string) {
5354
ResourceConsumer: "registry.k8s.io/e2e-test-images/resource-consumer:1.10",
5455
VolumeCopyUp: "ghcr.io/containerd/volume-copy-up:2.1",
5556
VolumeOwnership: "ghcr.io/containerd/volume-ownership:2.1",
57+
ArgsEscaped: "cplatpublic.azurecr.io/args-escaped-test-image-ns:latest",
5658
}
5759

5860
if imageListFile != "" {
@@ -88,6 +90,8 @@ const (
8890
VolumeCopyUp
8991
// VolumeOwnership image
9092
VolumeOwnership
93+
// Test image for ArgsEscaped windows bug
94+
ArgsEscaped
9195
)
9296

9397
func initImageMap(imageList ImageList) map[int]string {
@@ -98,6 +102,7 @@ func initImageMap(imageList ImageList) map[int]string {
98102
images[ResourceConsumer] = imageList.ResourceConsumer
99103
images[VolumeCopyUp] = imageList.VolumeCopyUp
100104
images[VolumeOwnership] = imageList.VolumeOwnership
105+
images[ArgsEscaped] = imageList.ArgsEscaped
101106
return images
102107
}
103108

integration/images/image_list.sample.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@ busybox = "docker.io/library/busybox:latest"
33
pause = "registry.k8s.io/pause:3.7"
44
VolumeCopyUp = "ghcr.io/containerd/volume-copy-up:2.1"
55
VolumeOwnership = "ghcr.io/containerd/volume-ownership:2.1"
6+
ArgsEscaped = "cplatpublic.azurecr.io/args-escaped-test-image-ns:latest"

integration/windows_hostprocess_test.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,15 @@ package integration
2121
import (
2222
"fmt"
2323
"os"
24+
"strconv"
2425
"testing"
2526
"time"
2627

28+
"github.com/Microsoft/hcsshim/osversion"
2729
"github.com/containerd/containerd/integration/images"
2830
"github.com/stretchr/testify/assert"
2931
"github.com/stretchr/testify/require"
32+
"golang.org/x/sys/windows/registry"
3033
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
3134
v1 "k8s.io/cri-api/pkg/apis/runtime/v1"
3235
)
@@ -123,3 +126,65 @@ func runHostProcess(t *testing.T, expectErr bool, image string, action hpcAction
123126

124127
action(t, cn, containerConfig)
125128
}
129+
130+
func startAndTestContainer(t *testing.T, sb string, sbConfig *runtime.PodSandboxConfig, cnConfig *runtime.ContainerConfig) {
131+
t.Log("Create the container")
132+
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
133+
require.NoError(t, err)
134+
t.Log("Start the container")
135+
require.NoError(t, runtimeService.StartContainer(cn))
136+
137+
t.Log("Stop the container")
138+
require.NoError(t, runtimeService.StopContainer(cn, 0))
139+
t.Log("Remove the container")
140+
require.NoError(t, runtimeService.RemoveContainer(cn))
141+
}
142+
143+
func TestArgsEscapedImagesOnWindows(t *testing.T) {
144+
// the ArgsEscaped test image is based on nanoserver:ltsc2022, so ensure we run on the correct OS version
145+
k, err := registry.OpenKey(registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\Windows NT\CurrentVersion`, registry.QUERY_VALUE)
146+
if err != nil {
147+
t.Skip("Error in getting OS version")
148+
}
149+
defer k.Close()
150+
151+
b, _, _ := k.GetStringValue("CurrentBuild")
152+
buildNum, _ := strconv.Atoi(b)
153+
if buildNum < osversion.V21H2Server {
154+
t.Skip()
155+
}
156+
157+
containerName := "test-container"
158+
testImage := images.Get(images.ArgsEscaped)
159+
sbConfig := &runtime.PodSandboxConfig{
160+
Metadata: &runtime.PodSandboxMetadata{
161+
Name: "sandbox",
162+
Namespace: testImage,
163+
},
164+
Windows: &runtime.WindowsPodSandboxConfig{},
165+
}
166+
sb, err := runtimeService.RunPodSandbox(sbConfig, *runtimeHandler)
167+
require.NoError(t, err)
168+
t.Cleanup(func() {
169+
assert.NoError(t, runtimeService.StopPodSandbox(sb))
170+
assert.NoError(t, runtimeService.RemovePodSandbox(sb))
171+
})
172+
173+
EnsureImageExists(t, testImage)
174+
175+
cnConfigWithCtrCmd := ContainerConfig(
176+
containerName,
177+
testImage,
178+
WithCommand("ping", "-t", "127.0.0.1"),
179+
localSystemUsername,
180+
)
181+
182+
cnConfigNoCtrCmd := ContainerConfig(
183+
containerName,
184+
testImage,
185+
localSystemUsername,
186+
)
187+
188+
startAndTestContainer(t, sb, sbConfig, cnConfigWithCtrCmd)
189+
startAndTestContainer(t, sb, sbConfig, cnConfigNoCtrCmd)
190+
}

oci/spec_opts_windows.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,16 @@ func escapeAndCombineArgs(args []string) string {
3535
return strings.Join(escaped, " ")
3636
}
3737

38+
// WithProcessCommandLine replaces the command line on the generated spec
39+
func WithProcessCommandLine(cmdLine string) SpecOpts {
40+
return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error {
41+
setProcess(s)
42+
s.Process.Args = nil
43+
s.Process.CommandLine = cmdLine
44+
return nil
45+
}
46+
}
47+
3848
// WithHostDevices adds all the hosts device nodes to the container's spec
3949
//
4050
// Not supported on windows

pkg/cri/opts/spec_nonwindows.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
//go:build !windows
2+
3+
/*
4+
Copyright The containerd Authors.
5+
6+
Licensed under the Apache License, Version 2.0 (the "License");
7+
you may not use this file except in compliance with the License.
8+
You may obtain a copy of the License at
9+
10+
http://www.apache.org/licenses/LICENSE-2.0
11+
12+
Unless required by applicable law or agreed to in writing, software
13+
distributed under the License is distributed on an "AS IS" BASIS,
14+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
See the License for the specific language governing permissions and
16+
limitations under the License.
17+
*/
18+
19+
package opts
20+
21+
import (
22+
"context"
23+
24+
"github.com/containerd/containerd/containers"
25+
"github.com/containerd/containerd/errdefs"
26+
"github.com/containerd/containerd/oci"
27+
imagespec "github.com/opencontainers/image-spec/specs-go/v1"
28+
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
29+
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
30+
)
31+
32+
func WithProcessCommandLineOrArgsForWindows(config *runtime.ContainerConfig, image *imagespec.ImageConfig) oci.SpecOpts {
33+
return func(ctx context.Context, client oci.Client, c *containers.Container, s *runtimespec.Spec) (err error) {
34+
return errdefs.ErrNotImplemented
35+
}
36+
}

pkg/cri/opts/spec_windows.go

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
//go:build windows
2+
3+
/*
4+
Copyright The containerd Authors.
5+
6+
Licensed under the Apache License, Version 2.0 (the "License");
7+
you may not use this file except in compliance with the License.
8+
You may obtain a copy of the License at
9+
10+
http://www.apache.org/licenses/LICENSE-2.0
11+
12+
Unless required by applicable law or agreed to in writing, software
13+
distributed under the License is distributed on an "AS IS" BASIS,
14+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
See the License for the specific language governing permissions and
16+
limitations under the License.
17+
*/
18+
19+
package opts
20+
21+
import (
22+
"context"
23+
"errors"
24+
"strings"
25+
26+
"github.com/containerd/containerd/containers"
27+
"github.com/containerd/containerd/oci"
28+
imagespec "github.com/opencontainers/image-spec/specs-go/v1"
29+
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
30+
"golang.org/x/sys/windows"
31+
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
32+
)
33+
34+
func escapeAndCombineArgsWindows(args []string) string {
35+
escaped := make([]string, len(args))
36+
for i, a := range args {
37+
escaped[i] = windows.EscapeArg(a)
38+
}
39+
return strings.Join(escaped, " ")
40+
}
41+
42+
// WithProcessCommandLineOrArgsForWindows sets the process command line or process args on the spec based on the image
43+
// and runtime config
44+
// If image.ArgsEscaped field is set, this function sets the process command line and if not, it sets the
45+
// process args field
46+
func WithProcessCommandLineOrArgsForWindows(config *runtime.ContainerConfig, image *imagespec.ImageConfig) oci.SpecOpts {
47+
if image.ArgsEscaped {
48+
return func(ctx context.Context, client oci.Client, c *containers.Container, s *runtimespec.Spec) (err error) {
49+
// firstArgFromImg is a flag that is returned to indicate that the first arg in the slice comes from either the
50+
// image Entrypoint or Cmd. If the first arg instead comes from the container config (e.g. overriding the image values),
51+
// it should be false. This is done to support the non-OCI ArgsEscaped field that Docker used to determine how the image
52+
// entrypoint and cmd should be interpreted.
53+
//
54+
args, firstArgFromImg, err := getArgs(image.Entrypoint, image.Cmd, config.GetCommand(), config.GetArgs())
55+
if err != nil {
56+
return err
57+
}
58+
59+
var cmdLine string
60+
if image.ArgsEscaped && firstArgFromImg {
61+
cmdLine = args[0]
62+
if len(args) > 1 {
63+
cmdLine += " " + escapeAndCombineArgsWindows(args[1:])
64+
}
65+
} else {
66+
cmdLine = escapeAndCombineArgsWindows(args)
67+
}
68+
69+
return oci.WithProcessCommandLine(cmdLine)(ctx, client, c, s)
70+
}
71+
}
72+
// if ArgsEscaped is not set
73+
return func(ctx context.Context, client oci.Client, c *containers.Container, s *runtimespec.Spec) (err error) {
74+
args, _, err := getArgs(image.Entrypoint, image.Cmd, config.GetCommand(), config.GetArgs())
75+
if err != nil {
76+
return err
77+
}
78+
return oci.WithProcessArgs(args...)(ctx, client, c, s)
79+
}
80+
}
81+
82+
// getArgs is used to evaluate the overall args for the container by taking into account the image command and entrypoints
83+
// along with the container command and entrypoints specified through the podspec if any
84+
func getArgs(imgEntrypoint []string, imgCmd []string, ctrEntrypoint []string, ctrCmd []string) ([]string, bool, error) {
85+
//nolint:dupword
86+
// firstArgFromImg is a flag that is returned to indicate that the first arg in the slice comes from either the image
87+
// Entrypoint or Cmd. If the first arg instead comes from the container config (e.g. overriding the image values),
88+
// it should be false.
89+
// Essentially this means firstArgFromImg should be true iff:
90+
// Ctr entrypoint ctr cmd image entrypoint image cmd firstArgFromImg
91+
// --------------------------------------------------------------------------------
92+
// nil nil exists nil true
93+
// nil nil nil exists true
94+
95+
// This is needed to support the non-OCI ArgsEscaped field used by Docker. ArgsEscaped is used for
96+
// Windows images to indicate that the command has already been escaped and should be
97+
// used directly as the command line.
98+
var firstArgFromImg bool
99+
entrypoint, cmd := ctrEntrypoint, ctrCmd
100+
// The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go
101+
// TODO(random-liu): Clearly define the commands overwrite behavior.
102+
if len(entrypoint) == 0 {
103+
// Copy array to avoid data race.
104+
if len(cmd) == 0 {
105+
cmd = append([]string{}, imgCmd...)
106+
if len(imgCmd) > 0 {
107+
firstArgFromImg = true
108+
}
109+
}
110+
if entrypoint == nil {
111+
entrypoint = append([]string{}, imgEntrypoint...)
112+
if len(imgEntrypoint) > 0 || len(ctrCmd) == 0 {
113+
firstArgFromImg = true
114+
}
115+
}
116+
}
117+
if len(entrypoint) == 0 && len(cmd) == 0 {
118+
return nil, false, errors.New("no command specified")
119+
}
120+
return append(entrypoint, cmd...), firstArgFromImg, nil
121+
}

pkg/cri/opts/spec_windows_opts.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,11 @@ import (
2424
"sort"
2525
"strings"
2626

27-
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
28-
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
29-
3027
"github.com/containerd/containerd/containers"
3128
"github.com/containerd/containerd/oci"
3229
osinterface "github.com/containerd/containerd/pkg/os"
30+
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
31+
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
3332
)
3433

3534
// namedPipePath returns true if the given path is to a named pipe.

pkg/cri/sbserver/container_create.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -730,9 +730,8 @@ func (c *criService) buildWindowsSpec(
730730
extraMounts []*runtime.Mount,
731731
ociRuntime config.Runtime,
732732
) (_ []oci.SpecOpts, retErr error) {
733-
specOpts := []oci.SpecOpts{
734-
customopts.WithProcessArgs(config, imageConfig),
735-
}
733+
var specOpts []oci.SpecOpts
734+
specOpts = append(specOpts, customopts.WithProcessCommandLineOrArgsForWindows(config, imageConfig))
736735

737736
// All containers in a pod need to have HostProcess set if it was set on the pod,
738737
// and vice versa no containers in the pod can be HostProcess if the pods spec

pkg/cri/server/container_create_windows.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,8 @@ func (c *criService) containerSpec(
5151
extraMounts []*runtime.Mount,
5252
ociRuntime config.Runtime,
5353
) (*runtimespec.Spec, error) {
54-
specOpts := []oci.SpecOpts{
55-
customopts.WithProcessArgs(config, imageConfig),
56-
}
54+
var specOpts []oci.SpecOpts
55+
specOpts = append(specOpts, customopts.WithProcessCommandLineOrArgsForWindows(config, imageConfig))
5756

5857
// All containers in a pod need to have HostProcess set if it was set on the pod,
5958
// and vice versa no containers in the pod can be HostProcess if the pods spec

0 commit comments

Comments
 (0)