Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

kubelet: set terminationMessagePath perms to 0660 #108076

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
Loading
from
Open

kubelet: set terminationMessagePath perms to 0660 #108076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6619c9
kubelet: set terminationMessagePath perms to 0660
skrobul Jan 18, 2022
b6beb97
node runtime test: use non-root group
skrobul Apr 4, 2023
fd5a394
kuberuntime: change ownership of the termination-log file
skrobul Apr 4, 2023
9cbab5b
kuberuntime: add nil check for SecurityContext
skrobul Apr 28, 2023
c4a75ea
kuberuntime: linter updates
skrobul Dec 18, 2024
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
29 changes: 26 additions & 3 deletions 29 pkg/kubelet/kuberuntime/kuberuntime_container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(ctx context.Context,
Annotations: newContainerAnnotations(container, pod, restartCount, opts),
Devices: makeDevices(opts),
CDIDevices: makeCDIDevices(opts),
Mounts: m.makeMounts(opts, container),
Mounts: m.makeMounts(opts, container, pod),
LogPath: containerLogsPath,
Stdin: container.Stdin,
StdinOnce: container.StdinOnce,
Expand Down Expand Up @@ -405,7 +405,7 @@ func makeCDIDevices(opts *kubecontainer.RunContainerOptions) []*runtimeapi.CDIDe
}

// makeMounts generates container volume mounts for kubelet runtime v1.
func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerOptions, container *v1.Container) []*runtimeapi.Mount {
func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerOptions, container *v1.Container, pod *v1.Pod) []*runtimeapi.Mount {
volumeMounts := []*runtimeapi.Mount{}

for idx := range opts.Mounts {
Expand Down Expand Up @@ -441,10 +441,33 @@ func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerO
// open(2) to create the file, so the final mode used is "mode &
// ~umask". But we want to make sure the specified mode is used
// in the file no matter what the umask is.
if err := m.osInterface.Chmod(containerLogPath, 0666); err != nil {
if err := m.osInterface.Chmod(containerLogPath, 0660); err != nil {
Copy link
Member

@rata rata Dec 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The PR mentions the terminationMessagePath that is defined a few lines below: https://github.com/kubernetes/kubernetes/pull/108076/files#diff-d86ffc24d8e4c9deafd3e39e8f4ee3546ee46a31dcea8ea6526443b3f4bffa28R420

How is it that changing the containerLogPath affects the terminationMessagePath?

Copy link
Author

Choose a reason for hiding this comment

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

ok, so my goal is to get rid of the world writeable file regardless of how is it called. The error message on line 406 suggests that termination-log file is the same as containerLogPath - is it not?

The terminationMessagePath as I understand it is an absolute path that is used when mounting the volume and
it has not much to do with what is known as the concept of termination message despite the name. In fact, even the error message on 415 refers to containerLogPath rather than terminationMessagePath.

At this point I am bit confused about this code - if that's not the line responsible for creating the empty world writable files like this:

# ls -la /var/lib/kubelet/pods/fa852268-2261-4ba4-9aaf-a4eaa98d1dfe/containers/kube-proxy/a62b249a
-rw-rw-rw- 1 root root 0 Mar 20 12:30 /var/lib/kubelet/pods/fa852268-2261-4ba4-9aaf-a4eaa98d1dfe/containers/kube-proxy/a62b249a

then what is?

Copy link
Member

Choose a reason for hiding this comment

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

It might very well be the same, I'm unsure. But some explanation on the commit msg would be helpful so it is easier to review, you probably figured that out when writing the PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Ohh, I see it now. It seems a bind mount is created a few lines below where the source is the host path (containerLogPath) and the destination is the terminationMessagePath.

See here: https://github.com/kubernetes/kubernetes/pull/108076/files#diff-d86ffc24d8e4c9deafd3e39e8f4ee3546ee46a31dcea8ea6526443b3f4bffa28L422-L426

So this is solved :)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I didn't want to make any statements I was not sure about - initially I also had thought that containerLogPath referred to a log with whatever is printed out to container's standard output, but that doesn't seem to be the case.

Either way, big thank you for the review and looking into this. Apologies it took me so long to respond, I have missed the notification email.

utilruntime.HandleError(fmt.Errorf("unable to set termination-log file permissions %q: %v", containerLogPath, err))
}

var containerUID, containerGID int
if container.SecurityContext != nil && container.SecurityContext.RunAsUser != nil {
containerUID = int(*container.SecurityContext.RunAsUser)
} else if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.RunAsUser != nil {
containerUID = int(*pod.Spec.SecurityContext.RunAsUser)
} else {
containerUID = 0
}

if container.SecurityContext != nil && container.SecurityContext.RunAsGroup != nil {
containerGID = int(*container.SecurityContext.RunAsGroup)
} else if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.RunAsGroup != nil {
containerGID = int(*pod.Spec.SecurityContext.RunAsGroup)
} else {
containerGID = 0
}

if goruntime.GOOS != "windows" {
if err := os.Chown(containerLogPath, containerUID, containerGID); err != nil {
utilruntime.HandleError(fmt.Errorf("unable to chown termination-log file: %q: %w", containerLogPath, err))
}
}

// Volume Mounts fail on Windows if it is not of the form C:/
containerLogPath = volumeutil.MakeAbsolutePath(goruntime.GOOS, containerLogPath)
terminationMessagePath := volumeutil.MakeAbsolutePath(goruntime.GOOS, container.TerminationMessagePath)
Expand Down
2 changes: 1 addition & 1 deletion 2 pkg/kubelet/kuberuntime/kuberuntime_container_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func makeExpectedConfig(m *kubeGenericRuntimeManager, pod *v1.Pod, containerInde
Labels: newContainerLabels(container, pod),
Annotations: newContainerAnnotations(container, pod, restartCount, opts),
Devices: makeDevices(opts),
Mounts: m.makeMounts(opts, container),
Mounts: m.makeMounts(opts, container, pod),
LogPath: containerLogsPath,
Stdin: container.Stdin,
StdinOnce: container.StdinOnce,
Expand Down
2 changes: 2 additions & 0 deletions 2 test/e2e/common/node/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ while true; do sleep 1; done
ginkgo.Context("on terminated container", func() {
rootUser := int64(0)
nonRootUser := int64(10000)
nonRootGroup := int64(10000)
adminUserName := "ContainerAdministrator"
nonAdminUserName := "ContainerUser"

Expand Down Expand Up @@ -204,6 +205,7 @@ while true; do sleep 1; done
container.SecurityContext.WindowsOptions = &v1.WindowsSecurityContextOptions{RunAsUserName: &nonAdminUserName}
} else {
container.SecurityContext.RunAsUser = &nonRootUser
container.SecurityContext.RunAsGroup = &nonRootGroup
}
matchTerminationMessage(ctx, container, v1.PodSucceeded, gomega.Equal("DONE"))
})
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.