Skip to content

Theop dd/cws instr test #51163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
theop-dd wants to merge 3 commits into
base: main
Choose a base branch
from
+329 −17
Draft

Theop dd/cws instr test #51163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efb9db9
Write on base folder if possible
theop-dd May 7, 2026
1deb1b9
add tests
theop-dd May 12, 2026
b4f71ae
Add a check to resolve container name
theop-dd May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 55 additions & 17 deletions pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"fmt"
"path/filepath"
"strconv"
"strings"
"time"

"github.com/wI2L/jsondiff"
Expand Down Expand Up @@ -447,22 +448,59 @@ func containerHasReadonlyRootfs(container corev1.Container) bool {
return false
}

func (ci *CWSInstrumentation) hasReadonlyRootfs(pod *corev1.Pod, container string) bool {
func findContainerByName(pod *corev1.Pod, container string) *corev1.Container {
// check in the init containers
for _, c := range pod.Spec.InitContainers {
for i, c := range pod.Spec.InitContainers {
if c.Name == container {
return containerHasReadonlyRootfs(c)
return &pod.Spec.InitContainers[i]
}
}

// check the other containers
for _, c := range pod.Spec.Containers {
for i, c := range pod.Spec.Containers {
if c.Name == container {
return containerHasReadonlyRootfs(c)
return &pod.Spec.Containers[i]
}
}
return nil
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Include ephemeral containers in readonly-path lookup

injectCWSCommandInstrumentation now resolves exec.Container via podcmd.FindOrDefaultContainerByName, which supports ephemeral containers, but isDirectoryReadOnly calls findContainerByName, and that helper only scans init + regular containers before returning nil. In RemoteCopy mode with mount_volume=false, exec requests targeting an ephemeral container will therefore be treated as read-only and rejected (cwsInvalidInput path) even when the target directory is writable. This is a functional regression for ephemeral-container exec flows.

Useful? React with 👍 / 👎.

}

return false
func volumeMountContainsPath(mountPath, target string) bool {
mountPath = filepath.Clean(mountPath)
target = filepath.Clean(target)
if mountPath == target {
return true
}
if mountPath == "/" {
return true
}
return strings.HasPrefix(target, mountPath+"/")
}

func (ci *CWSInstrumentation) isDirectoryReadOnly(pod *corev1.Pod, containerName, directory string) bool {
container := findContainerByName(pod, containerName)
if container == nil {
return true
}
// find the most specific VolumeMount that contains the directory
var matched *corev1.VolumeMount
matchedLen := -1
for i, mnt := range container.VolumeMounts {
mountPath := filepath.Clean(mnt.MountPath)
if !volumeMountContainsPath(mountPath, directory) {
continue
}
if len(mountPath) > matchedLen {
matchedLen = len(mountPath)
matched = &container.VolumeMounts[i]
}
}
// if we found a VolumeMount, check its ReadOnly flag
if matched != nil {
return matched.ReadOnly
}
// otherwise, fall back to the container's SecurityContext
return containerHasReadonlyRootfs(*container)
}

func (ci *CWSInstrumentation) getPod(apiClient kubernetes.Interface, name string, ns string) (*corev1.Pod, error) {
Expand Down Expand Up @@ -543,6 +581,14 @@ func (ci *CWSInstrumentation) injectCWSCommandInstrumentation(exec *corev1.PodEx

var cwsInstrumentationRemotePath string

// check if the input container exists, or select the default one to which the user will be redirected
container, err := podcmd.FindOrDefaultContainerByName(pod, exec.Container, true, nil)
if err != nil {
log.Errorf("Ignoring exec request into %s: invalid container: %v", mutatecommon.PodString(pod), err)
metrics.CWSExecMutationAttempts.Inc(ci.mode.String(), "false", cwsInvalidInputContainerReason)
return false, errors.New(metrics.InvalidInput)
}

switch ci.mode {
case InitContainer:
cwsInstrumentationRemotePath = filepath.Join(cwsMountPath, "cws-instrumentation")
Expand All @@ -566,10 +612,10 @@ func (ci *CWSInstrumentation) injectCWSCommandInstrumentation(exec *corev1.PodEx
}
cwsInstrumentationRemotePath = filepath.Join(cwsMountPath, cwsInstrumentationRemotePath)
} else {
// check if the target pod has a read only filesystem
if readOnly := ci.hasReadonlyRootfs(pod, exec.Container); readOnly {
// if we're not using a shared volume, we need to make sure the directory is writable
if ci.isDirectoryReadOnly(pod, container.Name, ci.directoryForRemoteCopy) {
// readonly rootfs containers can't be instrumented
log.Errorf("Ignoring exec request into %s, container %s has read only rootfs. Try enabling admission_controller.cws_instrumentation.remote_copy.mount_volume", mutatecommon.PodString(pod), exec.Container)
log.Errorf("Ignoring exec request into %s, directory %q is not writable in container %s that has readonly rootfs. Try enabling admission_controller.cws_instrumentation.remote_copy.mount_volume", mutatecommon.PodString(pod), ci.directoryForRemoteCopy, container.Name)
metrics.CWSExecMutationAttempts.Inc(ci.mode.String(), "false", cwsReadonlyFilesystemReason)
return false, errors.New(metrics.InvalidInput)
}
Expand Down Expand Up @@ -598,14 +644,6 @@ func (ci *CWSInstrumentation) injectCWSCommandInstrumentation(exec *corev1.PodEx
return false, errors.New(metrics.InvalidInput)
}

// check if the input container exists, or select the default one to which the user will be redirected
container, err := podcmd.FindOrDefaultContainerByName(pod, exec.Container, true, nil)
if err != nil {
log.Errorf("Ignoring exec request into %s: invalid container: %v", mutatecommon.PodString(pod), err)
metrics.CWSExecMutationAttempts.Inc(ci.mode.String(), "false", cwsInvalidInputContainerReason)
return false, errors.New(metrics.InvalidInput)
}

// copy CWS instrumentation directly to the target container
if err := ci.injectCWSCommandInstrumentationRemoteCopy(pod, container.Name, cwsInstrumentationLocalPath, cwsInstrumentationRemotePath); err != nil {
log.Warnf("Ignoring exec request into %s: remote copy failed: %v", mutatecommon.PodString(pod), err)
Expand Down
274 changes: 274 additions & 0 deletions pkg/clusteragent/admission/mutate/cwsinstrumentation/cws_instrumentation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1085,3 +1085,277 @@ func Test_initCWSInitContainerResources(t *testing.T) {
})
}
}

// boolPtr returns a pointer to the provided bool value.
func boolPtr(b bool) *bool {
return &b
}

// newPodWithContainer returns a pod whose single regular container is built
// from the provided container.
func newPodWithContainer(c corev1.Container) *corev1.Pod {
return &corev1.Pod{
Spec: corev1.PodSpec{
Containers: []corev1.Container{c},
},
}
}

func Test_isDirectoryReadOnly(t *testing.T) {
const (
targetContainer = "target"
directory = "/tmp"
)

tests := []struct {
name string
pod *corev1.Pod
container string
directory string
want bool
}{
{
name: "no VolumeMount, no SecurityContext -> writable",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
}),
container: targetContainer,
directory: directory,
want: false,
},
{
name: "no VolumeMount, ReadOnlyRootFilesystem=false -> writable",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(false),
},
}),
container: targetContainer,
directory: directory,
want: false,
},
{
name: "no VolumeMount, ReadOnlyRootFilesystem=true -> read-only (fallback)",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
}),
container: targetContainer,
directory: directory,
want: true,
},
{
name: "VolumeMount on /tmp writable wins over RO rootfs (system-probe case)",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
VolumeMounts: []corev1.VolumeMount{
{Name: "tmpdir", MountPath: "/tmp", ReadOnly: false},
},
}),
container: targetContainer,
directory: directory,
want: false,
},
{
name: "VolumeMount on /tmp read-only wins over writable rootfs",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
VolumeMounts: []corev1.VolumeMount{
{Name: "tmpdir", MountPath: "/tmp", ReadOnly: true},
},
}),
container: targetContainer,
directory: directory,
want: true,
},
{
name: "VolumeMount on parent path / covers /tmp",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
VolumeMounts: []corev1.VolumeMount{
{Name: "rootfs", MountPath: "/", ReadOnly: false},
},
}),
container: targetContainer,
directory: directory,
want: false,
},
{
name: "most specific VolumeMount wins: / rw + /tmp ro -> /tmp ro",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
VolumeMounts: []corev1.VolumeMount{
{Name: "rootfs", MountPath: "/", ReadOnly: false},
{Name: "tmpdir", MountPath: "/tmp", ReadOnly: true},
},
}),
container: targetContainer,
directory: directory,
want: true,
},
{
name: "sibling VolumeMount does not cover /tmp (no false positive)",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
VolumeMounts: []corev1.VolumeMount{
{Name: "tmpfoo", MountPath: "/tmpfoo", ReadOnly: false},
},
}),
container: targetContainer,
directory: directory,
want: true,
},
{
name: "child VolumeMount does not cover parent /tmp",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
VolumeMounts: []corev1.VolumeMount{
{Name: "subdir", MountPath: "/tmp/foo", ReadOnly: false},
},
}),
container: targetContainer,
directory: directory,
want: true,
},
{
name: "VolumeMount mountPath with trailing slash is normalized",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
VolumeMounts: []corev1.VolumeMount{
{Name: "tmpdir", MountPath: "/tmp/", ReadOnly: false},
},
}),
container: targetContainer,
directory: directory,
want: false,
},
{
name: "container name does not exist -> read-only (safe default)",
pod: newPodWithContainer(corev1.Container{
Name: "other",
VolumeMounts: []corev1.VolumeMount{
{Name: "tmpdir", MountPath: "/tmp", ReadOnly: false},
},
}),
container: targetContainer,
directory: directory,
want: true,
},
{
name: "init container is also inspected",
pod: &corev1.Pod{
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{
Name: targetContainer,
VolumeMounts: []corev1.VolumeMount{
{Name: "tmpdir", MountPath: "/tmp", ReadOnly: false},
},
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
},
},
},
},
container: targetContainer,
directory: directory,
want: false,
},
{
name: "non-default directory falls back when no mount covers it",
pod: newPodWithContainer(corev1.Container{
Name: targetContainer,
SecurityContext: &corev1.SecurityContext{
ReadOnlyRootFilesystem: boolPtr(true),
},
VolumeMounts: []corev1.VolumeMount{
{Name: "tmpdir", MountPath: "/tmp", ReadOnly: false},
},
}),
container: targetContainer,
directory: "/var/run",
want: true,
},
}

ci := &CWSInstrumentation{}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := ci.isDirectoryReadOnly(tt.pod, tt.container, tt.directory)
require.Equal(t, tt.want, got)
})
}
}

func Test_volumeMountContainsPath(t *testing.T) {
tests := []struct {
name string
mountPath string
target string
want bool
}{
{"exact match", "/tmp", "/tmp", true},
{"mountPath is parent", "/", "/tmp", true},
{"mountPath is multi-level parent", "/var", "/var/run/foo", true},
{"mountPath equals target with trailing slash", "/tmp/", "/tmp", true},
{"target with trailing slash", "/tmp", "/tmp/", true},
{"sibling prefix is not a match", "/tmpfoo", "/tmp", false},
{"target sibling of mount", "/tmp", "/tmpfoo", false},
{"child mount does not cover parent", "/tmp/foo", "/tmp", false},
{"unrelated paths", "/var", "/tmp", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require.Equal(t, tt.want, volumeMountContainsPath(tt.mountPath, tt.target))
})
}
}

func Test_findContainerByName(t *testing.T) {
pod := &corev1.Pod{
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{Name: "init"},
},
Containers: []corev1.Container{
{Name: "app"},
{Name: "sidecar"},
},
},
}

t.Run("found in regular containers", func(t *testing.T) {
c := findContainerByName(pod, "sidecar")
require.NotNil(t, c)
require.Equal(t, "sidecar", c.Name)
})

t.Run("found in init containers", func(t *testing.T) {
c := findContainerByName(pod, "init")
require.NotNil(t, c)
require.Equal(t, "init", c.Name)
})

t.Run("not found returns nil", func(t *testing.T) {
require.Nil(t, findContainerByName(pod, "missing"))
})
}
Loading