diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index ade4ed26a..2d8a05560 100644 --- a/pkg/server/container_create.go +++ b/pkg/server/container_create.go @@ -35,7 +35,6 @@ import ( imagespec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/opencontainers/runc/libcontainer/devices" runtimespec "github.com/opencontainers/runtime-spec/specs-go" - "github.com/opencontainers/runtime-tools/generate" "github.com/opencontainers/runtime-tools/validate" "github.com/opencontainers/selinux/go-selinux/label" "github.com/pkg/errors" @@ -76,6 +75,7 @@ func init() { // CreateContainer creates a new container in the given PodSandbox. func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateContainerRequest) (_ *runtime.CreateContainerResponse, retErr error) { config := r.GetConfig() + logrus.Debugf("Container config %+v", config) sandboxConfig := r.GetSandboxConfig() sandbox, err := c.sandboxStore.Get(r.GetPodSandboxId()) if err != nil { @@ -524,7 +524,7 @@ func (c *criService) generateContainerMounts(sandboxID string, config *runtime.C // setOCIProcessArgs sets process args. It returns error if the final arg list // is empty. -func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error { +func setOCIProcessArgs(g *generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error { command, args := config.GetCommand(), config.GetArgs() // The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go // TODO(random-liu): Clearly define the commands overwrite behavior. @@ -546,7 +546,7 @@ func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, i // addImageEnvs adds environment variables from image config. It returns error if // an invalid environment variable is encountered. -func addImageEnvs(g *generate.Generator, imageEnvs []string) error { +func addImageEnvs(g *generator, imageEnvs []string) error { for _, e := range imageEnvs { kv := strings.SplitN(e, "=", 2) if len(kv) != 2 { @@ -557,7 +557,7 @@ func addImageEnvs(g *generate.Generator, imageEnvs []string) error { return nil } -func setOCIPrivileged(g *generate.Generator, config *runtime.ContainerConfig) error { +func setOCIPrivileged(g *generator, config *runtime.ContainerConfig) error { // Add all capabilities in privileged mode. g.SetupPrivileged(true) setOCIBindMountsPrivileged(g) @@ -578,7 +578,7 @@ func clearReadOnly(m *runtimespec.Mount) { } // addDevices set device mapping without privilege. -func (c *criService) addOCIDevices(g *generate.Generator, devs []*runtime.Device) error { +func (c *criService) addOCIDevices(g *generator, devs []*runtime.Device) error { spec := g.Config for _, device := range devs { path, err := c.os.ResolveSymbolicLink(device.HostPath) @@ -610,7 +610,7 @@ func (c *criService) addOCIDevices(g *generate.Generator, devs []*runtime.Device } // addDevices set device mapping with privilege. -func setOCIDevicesPrivileged(g *generate.Generator) error { +func setOCIDevicesPrivileged(g *generator) error { spec := g.Config hostDevices, err := devices.HostDevices() if err != nil { @@ -641,7 +641,7 @@ func setOCIDevicesPrivileged(g *generate.Generator) error { } // addOCIBindMounts adds bind mounts. -func (c *criService) addOCIBindMounts(g *generate.Generator, mounts []*runtime.Mount, mountLabel string) error { +func (c *criService) addOCIBindMounts(g *generator, mounts []*runtime.Mount, mountLabel string) error { // Sort mounts in number of parts. This ensures that high level mounts don't // shadow other mounts. sort.Sort(orderedMounts(mounts)) @@ -746,7 +746,7 @@ func (c *criService) addOCIBindMounts(g *generate.Generator, mounts []*runtime.M return nil } -func setOCIBindMountsPrivileged(g *generate.Generator) { +func setOCIBindMountsPrivileged(g *generator) { spec := g.Config // clear readonly for /sys and cgroup for i, m := range spec.Mounts { @@ -762,7 +762,7 @@ func setOCIBindMountsPrivileged(g *generate.Generator) { } // setOCILinuxResourceCgroup set container cgroup resource limit. -func setOCILinuxResourceCgroup(g *generate.Generator, resources *runtime.LinuxContainerResources) { +func setOCILinuxResourceCgroup(g *generator, resources *runtime.LinuxContainerResources) { if resources == nil { return } @@ -775,7 +775,7 @@ func setOCILinuxResourceCgroup(g *generate.Generator, resources *runtime.LinuxCo } // setOCILinuxResourceOOMScoreAdj set container OOMScoreAdj resource limit. -func setOCILinuxResourceOOMScoreAdj(g *generate.Generator, resources *runtime.LinuxContainerResources, restrictOOMScoreAdjFlag bool) error { +func setOCILinuxResourceOOMScoreAdj(g *generator, resources *runtime.LinuxContainerResources, restrictOOMScoreAdjFlag bool) error { if resources == nil { return nil } @@ -805,7 +805,7 @@ func getOCICapabilitiesList() []string { } // Adds capabilities to all sets relevant to root (bounding, permitted, effective, inheritable) -func addProcessRootCapability(g *generate.Generator, c string) error { +func addProcessRootCapability(g *generator, c string) error { if err := g.AddProcessCapabilityBounding(c); err != nil { return err } @@ -822,7 +822,7 @@ func addProcessRootCapability(g *generate.Generator, c string) error { } // Drops capabilities to all sets relevant to root (bounding, permitted, effective, inheritable) -func dropProcessRootCapability(g *generate.Generator, c string) error { +func dropProcessRootCapability(g *generator, c string) error { if err := g.DropProcessCapabilityBounding(c); err != nil { return err } @@ -839,7 +839,7 @@ func dropProcessRootCapability(g *generate.Generator, c string) error { } // setOCICapabilities adds/drops process capabilities. -func setOCICapabilities(g *generate.Generator, capabilities *runtime.Capability) error { +func setOCICapabilities(g *generator, capabilities *runtime.Capability) error { if capabilities == nil { return nil } @@ -885,7 +885,7 @@ func setOCICapabilities(g *generate.Generator, capabilities *runtime.Capability) } // setOCINamespaces sets namespaces. -func setOCINamespaces(g *generate.Generator, namespaces *runtime.NamespaceOption, sandboxPid uint32) { +func setOCINamespaces(g *generator, namespaces *runtime.NamespaceOption, sandboxPid uint32) { g.AddOrReplaceLinuxNamespace(string(runtimespec.NetworkNamespace), getNetworkNamespace(sandboxPid)) // nolint: errcheck g.AddOrReplaceLinuxNamespace(string(runtimespec.IPCNamespace), getIPCNamespace(sandboxPid)) // nolint: errcheck g.AddOrReplaceLinuxNamespace(string(runtimespec.UTSNamespace), getUTSNamespace(sandboxPid)) // nolint: errcheck diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go index 6828c0e3d..040f42757 100644 --- a/pkg/server/container_create_test.go +++ b/pkg/server/container_create_test.go @@ -450,8 +450,9 @@ func TestContainerSpecCommand(t *testing.T) { } { config, _, imageConfig, _ := getCreateContainerTestData() - g, err := generate.New("linux") + og, err := generate.New("linux") assert.NoError(t, err) + g := newCustomGenerator(og) config.Command = test.criEntrypoint config.Args = test.criArgs imageConfig.Entrypoint = test.imageEntrypoint @@ -664,8 +665,9 @@ func TestPrivilegedBindMount(t *testing.T) { }, } { t.Logf("TestCase %q", desc) - g, err := generate.New("linux") + og, err := generate.New("linux") assert.NoError(t, err) + g := newCustomGenerator(og) c := newTestCRIService() c.addOCIBindMounts(&g, nil, "") if test.privileged { @@ -773,8 +775,9 @@ func TestMountPropagation(t *testing.T) { }, } { t.Logf("TestCase %q", desc) - g, err := generate.New("linux") + og, err := generate.New("linux") assert.NoError(t, err) + g := newCustomGenerator(og) c := newTestCRIService() c.os.(*ostesting.FakeOS).LookupMountFn = test.fakeLookupMountFn err = c.addOCIBindMounts(&g, []*runtime.Mount{test.criMount}, "") diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index 6394ad8e3..51e4a8d0c 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -398,10 +398,51 @@ func buildLabels(configLabels map[string]string, containerType string) map[strin } // newSpecGenerator creates a new spec generator for the runtime spec. -func newSpecGenerator(spec *runtimespec.Spec) generate.Generator { +func newSpecGenerator(spec *runtimespec.Spec) generator { g := generate.NewFromSpec(spec) g.HostSpecific = true - return g + return newCustomGenerator(g) +} + +// generator is a custom generator with some functions overridden +// used by the cri plugin. +// TODO(random-liu): Upstream this fix. +type generator struct { + generate.Generator + envCache map[string]int +} + +func newCustomGenerator(g generate.Generator) generator { + cg := generator{ + Generator: g, + envCache: make(map[string]int), + } + if g.Config != nil && g.Config.Process != nil { + for i, env := range g.Config.Process.Env { + kv := strings.SplitN(env, "=", 2) + cg.envCache[kv[0]] = i + } + } + return cg +} + +// AddProcessEnv overrides the original AddProcessEnv. It uses +// a map to cache and override envs. +func (g *generator) AddProcessEnv(key, value string) { + if len(g.envCache) == 0 { + // Call AddProccessEnv once to initialize the spec. + g.Generator.AddProcessEnv(key, value) + g.envCache[key] = 0 + return + } + spec := g.Config + env := fmt.Sprintf("%s=%s", key, value) + if idx, ok := g.envCache[key]; !ok { + spec.Process.Env = append(spec.Process.Env, env) + g.envCache[key] = len(spec.Process.Env) - 1 + } else { + spec.Process.Env[idx] = env + } } func getPodCNILabels(id string, config *runtime.PodSandboxConfig) map[string]string { diff --git a/pkg/server/helpers_test.go b/pkg/server/helpers_test.go index 5d33cd127..c0eb29839 100644 --- a/pkg/server/helpers_test.go +++ b/pkg/server/helpers_test.go @@ -25,6 +25,7 @@ import ( runcoptions "github.com/containerd/containerd/runtime/v2/runc/options" "github.com/docker/distribution/reference" imagedigest "github.com/opencontainers/go-digest" + runtimespec "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" @@ -321,3 +322,87 @@ func TestRestrictOOMScoreAdj(t *testing.T) { require.NoError(t, err) assert.Equal(t, got, current+1) } + +func TestCustomGenerator(t *testing.T) { + for desc, test := range map[string]struct { + existing []string + kv [][2]string + expected []string + expectNil bool + }{ + "empty": { + expectNil: true, + }, + "single env": { + kv: [][2]string{ + {"a", "b"}, + }, + expected: []string{"a=b"}, + }, + "multiple envs": { + kv: [][2]string{ + {"a", "b"}, + {"c", "d"}, + {"e", "f"}, + }, + expected: []string{ + "a=b", + "c=d", + "e=f", + }, + }, + "env override": { + kv: [][2]string{ + {"k1", "v1"}, + {"k2", "v2"}, + {"k3", "v3"}, + {"k3", "v4"}, + {"k1", "v5"}, + {"k4", "v6"}, + }, + expected: []string{ + "k1=v5", + "k2=v2", + "k3=v4", + "k4=v6", + }, + }, + "existing env": { + existing: []string{ + "k1=v1", + "k2=v2", + "k3=v3", + }, + kv: [][2]string{ + {"k3", "v4"}, + {"k2", "v5"}, + {"k4", "v6"}, + }, + expected: []string{ + "k1=v1", + "k2=v5", + "k3=v4", + "k4=v6", + }, + }, + } { + t.Logf("TestCase %q", desc) + var spec *runtimespec.Spec + if len(test.existing) > 0 { + spec = &runtimespec.Spec{ + Process: &runtimespec.Process{ + Env: test.existing, + }, + } + } + g := newSpecGenerator(spec) + for _, kv := range test.kv { + g.AddProcessEnv(kv[0], kv[1]) + } + if test.expectNil { + assert.Nil(t, g.Config) + } else { + assert.Equal(t, test.expected, g.Config.Process.Env) + } + } +} diff --git a/pkg/server/instrumented_service.go b/pkg/server/instrumented_service.go index e7c4a97a6..88e05f266 100644 --- a/pkg/server/instrumented_service.go +++ b/pkg/server/instrumented_service.go @@ -52,7 +52,7 @@ func (in *instrumentedService) RunPodSandbox(ctx context.Context, r *runtime.Run if err := in.checkInitialized(); err != nil { return nil, err } - logrus.Infof("RunPodSandbox with config %+v", r.GetConfig()) + logrus.Infof("RunPodsandbox for %+v", r.GetConfig().GetMetadata()) defer func() { if err != nil { logrus.WithError(err).Errorf("RunPodSandbox for %+v failed, error", r.GetConfig().GetMetadata()) @@ -142,8 +142,8 @@ func (in *instrumentedService) CreateContainer(ctx context.Context, r *runtime.C if err := in.checkInitialized(); err != nil { return nil, err } - logrus.Infof("CreateContainer within sandbox %q with container config %+v and sandbox config %+v", - r.GetPodSandboxId(), r.GetConfig(), r.GetSandboxConfig()) + logrus.Infof("CreateContainer within sandbox %q for container %+v", + r.GetPodSandboxId(), r.GetConfig().GetMetadata()) defer func() { if err != nil { logrus.WithError(err).Errorf("CreateContainer within sandbox %q for %+v failed", diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index aad88a46e..b5825c8e9 100644 --- a/pkg/server/sandbox_run.go +++ b/pkg/server/sandbox_run.go @@ -28,6 +28,7 @@ import ( "github.com/containerd/containerd/oci" cni "github.com/containerd/go-cni" "github.com/containerd/typeurl" + "github.com/davecgh/go-spew/spew" imagespec "github.com/opencontainers/image-spec/specs-go/v1" runtimespec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" @@ -55,6 +56,7 @@ func init() { // the sandbox is in ready state. func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandboxRequest) (_ *runtime.RunPodSandboxResponse, retErr error) { config := r.GetConfig() + logrus.Debugf("Sandbox config %+v", config) // Generate unique id and name for the sandbox and reserve the name. id := util.GenerateID() @@ -149,7 +151,7 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox if err != nil { return nil, errors.Wrap(err, "failed to generate sandbox container spec") } - logrus.Debugf("Sandbox container spec: %+v", spec) + logrus.Debugf("Sandbox container %q spec: %#+v", id, spew.NewFormatter(spec)) var specOpts []oci.SpecOpts userstr, err := generateUserString(