From fdc20de500452d16d9611e42d473fbd7dc1aa0ae Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Fri, 19 Jan 2024 11:33:55 +0100 Subject: [PATCH 1/4] kubelet/userns: Wrap error message Most error messages are properly wrapped already, but this was missing. The kubelet logs will show something like this now: E0201 12:00:03.505680 3007049 run.go:74] "command failed" err="failed to run Kubelet: failed to create kubelet: record pod mappings: create user namespace store: mkdir XXX: permission denied" Before this commit, the message was not so clear: E0120 16:02:40.484404 474711 run.go:74] "command failed" err="failed to run Kubelet: failed to create kubelet: mkdir XXX: permission denied" Signed-off-by: Rodrigo Campos --- pkg/kubelet/userns/userns_manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/userns/userns_manager.go b/pkg/kubelet/userns/userns_manager.go index 95b08184c95..662479e6d96 100644 --- a/pkg/kubelet/userns/userns_manager.go +++ b/pkg/kubelet/userns/userns_manager.go @@ -157,7 +157,7 @@ func MakeUserNsManager(kl userNsPodsManager) (*UsernsManager, error) { for _, podUID := range found { klog.V(5).InfoS("reading pod from disk for user namespace", "podUID", podUID) if err := m.recordPodMappings(podUID); err != nil { - return nil, err + return nil, fmt.Errorf("record pod mappings: %w", err) } } From 0f7b9cc4f5393b5c5da48591bcd5c4060553381c Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Thu, 1 Feb 2024 11:33:39 +0100 Subject: [PATCH 2/4] pkg/kubelet/userns: Simplify error messages The error we are wrapping is already verbose, let's just use minimal wrapping as it is usually the case in go code. Note that the error on parseUserNsFileAndRecord() can be returned to the user, so we added some context about user namespace. Otherwise, an error to parse the json would not be clear to which of all the json the kubelet parses it refers to. Signed-off-by: Rodrigo Campos --- pkg/kubelet/userns/userns_manager.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/kubelet/userns/userns_manager.go b/pkg/kubelet/userns/userns_manager.go index 662479e6d96..8e333de7905 100644 --- a/pkg/kubelet/userns/userns_manager.go +++ b/pkg/kubelet/userns/userns_manager.go @@ -96,7 +96,7 @@ func (m *UsernsManager) writeMappingsToFile(pod types.UID, userNs userNamespace) fstore, err := utilstore.NewFileStore(dir, &utilfs.DefaultFs{}) if err != nil { - return err + return fmt.Errorf("create user namespace store: %w", err) } if err := fstore.Write(mappingsFile, data); err != nil { return err @@ -123,7 +123,7 @@ func (m *UsernsManager) readMappingsFromFile(pod types.UID) ([]byte, error) { dir := m.kl.GetPodDir(pod) fstore, err := utilstore.NewFileStore(dir, &utilfs.DefaultFs{}) if err != nil { - return nil, err + return nil, fmt.Errorf("create user namespace store: %w", err) } return fstore.Read(mappingsFile) } @@ -151,7 +151,7 @@ func MakeUserNsManager(kl userNsPodsManager) (*UsernsManager, error) { if os.IsNotExist(err) { return &m, nil } - return nil, fmt.Errorf("user namespace manager can't read pods from disk: %w", err) + return nil, fmt.Errorf("read pods from disk: %w", err) } for _, podUID := range found { @@ -308,7 +308,7 @@ func (m *UsernsManager) releaseWithLock(pod types.UID) { func (m *UsernsManager) parseUserNsFileAndRecord(pod types.UID, content []byte) (userNs userNamespace, err error) { if err = json.Unmarshal([]byte(content), &userNs); err != nil { - err = fmt.Errorf("can't parse file: %w", err) + err = fmt.Errorf("invalid user namespace mappings file: %w", err) return } From a56d483df069fc44acbd5e61fc04b9d95e86f155 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Fri, 19 Jan 2024 16:29:05 +0100 Subject: [PATCH 3/4] kubelet/userns: Use t.TempDir() These tests will create the userns record mapping file, so let's use a temporal directory for that. Without specifying one, by mistake we were using the "/tmp/non-existant-dir.This-is-not-used-in-tests/" directory. Signed-off-by: Rodrigo Campos --- pkg/kubelet/userns/userns_manager_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/kubelet/userns/userns_manager_test.go b/pkg/kubelet/userns/userns_manager_test.go index d2156dd4c6d..58e2df9d586 100644 --- a/pkg/kubelet/userns/userns_manager_test.go +++ b/pkg/kubelet/userns/userns_manager_test.go @@ -297,6 +297,7 @@ func TestCleanupOrphanedPodUsernsAllocations(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { testUserNsPodsManager := &testUserNsPodsManager{ + podDir: t.TempDir(), podList: tc.listPods, } m, err := MakeUserNsManager(testUserNsPodsManager) From cae710d9e9615447dd609d40a47651890540ccdf Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Fri, 19 Jan 2024 16:18:12 +0100 Subject: [PATCH 4/4] kublet/userns: Test error messages on init failures This adds a test for the just added wrapping error message, as well as for the other already present error messages that initialization can fail with. Signed-off-by: Rodrigo Campos --- pkg/kubelet/userns/userns_manager_test.go | 36 +++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/pkg/kubelet/userns/userns_manager_test.go b/pkg/kubelet/userns/userns_manager_test.go index 58e2df9d586..cf23967405d 100644 --- a/pkg/kubelet/userns/userns_manager_test.go +++ b/pkg/kubelet/userns/userns_manager_test.go @@ -18,6 +18,7 @@ package userns import ( "fmt" + "os" "testing" "github.com/stretchr/testify/assert" @@ -360,3 +361,38 @@ func TestRecordMaxPods(t *testing.T) { err = m.record(types.UID(fmt.Sprintf("%d", maxPods+1)), uint32((maxPods+1)*65536), 65536) assert.Error(t, err) } + +type failingUserNsPodsManager struct { + testUserNsPodsManager +} + +func (m *failingUserNsPodsManager) ListPodsFromDisk() ([]types.UID, error) { + return nil, os.ErrPermission +} + +func TestMakeUserNsManagerFailsListPod(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, pkgfeatures.UserNamespacesSupport, true)() + + testUserNsPodsManager := &failingUserNsPodsManager{} + _, err := MakeUserNsManager(testUserNsPodsManager) + assert.Error(t, err) + assert.ErrorContains(t, err, "read pods from disk") +} + +func TestMakeUserNsManagerFailsPodRecord(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, pkgfeatures.UserNamespacesSupport, true)() + + testUserNsPodsManager := &testUserNsPodsManager{ + podList: []types.UID{"pod-1", "pod-2"}, + podDir: t.TempDir(), + } + + // Remove read/execute permissions from this directory. + if err := os.Chmod(testUserNsPodsManager.podDir, 0222); err != nil { + t.Fatal(err) + } + + _, err := MakeUserNsManager(testUserNsPodsManager) + assert.Error(t, err) + assert.ErrorContains(t, err, "record pod mappings") +}