Merge pull request #55450 from feiskyer/seccomp-path

Automatic merge from submit-queue (batch tested with PRs 55952, 49112, 55450, 56178, 56151). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix the wrong localhost seccomp path of CRI

**What this PR does / why we need it**:

Fix the wrong seccomp path comment.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #55359

**Special notes for your reviewer**:

**Release note**:

```release-note
Fix CRI localhost seccomp path in format localhost//profileRoot/profileName.
```

pkg/kubelet/dockershim/BUILD

@@ -101,8 +101,6 @@ go_test(
         "//conditions:default": [],
     }),
     data = [
-        "fixtures/seccomp/sub/subtest",
-        "fixtures/seccomp/test",
     ],
     importpath = "k8s.io/kubernetes/pkg/kubelet/dockershim",
     library = ":go_default_library",

pkg/kubelet/dockershim/fixtures/seccomp/sub/subtest

@@ -1,3 +0,0 @@
-{
-    "abc": "def"
-}

pkg/kubelet/dockershim/fixtures/seccomp/test

@@ -1,3 +0,0 @@
-{
-    "foo": "bar"
-}

pkg/kubelet/dockershim/helpers_linux.go

@@ -62,7 +62,11 @@ func getSeccompDockerOpts(seccompProfile string) ([]dockerOpt, error) {
 		return nil, fmt.Errorf("unknown seccomp profile option: %s", seccompProfile)
 	}
 
-	fname := strings.TrimPrefix(seccompProfile, "localhost/") // by pod annotation validation, name is a valid subpath
+	// get the full path of seccomp profile when prefixed with 'localhost/'.
+	fname := strings.TrimPrefix(seccompProfile, "localhost/")
+	if !filepath.IsAbs(fname) {
+		return nil, fmt.Errorf("seccomp profile path must be absolute, but got relative path %q", fname)
+	}
 	file, err := ioutil.ReadFile(filepath.FromSlash(fname))
 	if err != nil {
 		return nil, fmt.Errorf("cannot load seccomp profile %q: %v", fname, err)

pkg/kubelet/dockershim/helpers_linux_test.go

@@ -20,9 +20,13 @@ package dockershim
 
 import (
 	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestGetSeccompSecurityOpts(t *testing.T) {
@@ -55,26 +59,31 @@ func TestGetSeccompSecurityOpts(t *testing.T) {
 }
 
 func TestLoadSeccompLocalhostProfiles(t *testing.T) {
+	tmpdir, err := ioutil.TempDir("", "seccomp-local-profile-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpdir)
+	testProfile := `{"foo": "bar"}`
+	err = ioutil.WriteFile(filepath.Join(tmpdir, "test"), []byte(testProfile), 0644)
+	require.NoError(t, err)
+
 	tests := []struct {
 		msg            string
 		seccompProfile string
 		expectedOpts   []string
 		expectErr      bool
 	}{{
-		msg: "Seccomp localhost/test profile",
+		msg:            "Seccomp localhost/test profile should return correct seccomp profiles",
-		// We are abusing localhost for loading test seccomp profiles.
+		seccompProfile: "localhost/" + filepath.Join(tmpdir, "test"),
-		// The profile should be an absolute path while we are using a relative one.
-		seccompProfile: "localhost/fixtures/seccomp/test",
 		expectedOpts:   []string{`seccomp={"foo":"bar"}`},
 		expectErr:      false,
 	}, {
-		msg:            "Seccomp localhost/sub/subtest profile",
+		msg:            "Non-existent profile should return error",
-		seccompProfile: "localhost/fixtures/seccomp/sub/subtest",
+		seccompProfile: "localhost/" + filepath.Join(tmpdir, "fixtures/non-existent"),
-		expectedOpts:   []string{`seccomp={"abc":"def"}`},
+		expectedOpts:   nil,
-		expectErr:      false,
+		expectErr:      true,
 	}, {
-		msg:            "Seccomp non-existent",
+		msg:            "Relative profile path should return error",
-		seccompProfile: "localhost/fixtures/seccomp/non-existent",
+		seccompProfile: "localhost/fixtures/test",
 		expectedOpts:   nil,
 		expectErr:      true,
 	}}

pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go

@@ -33,6 +33,10 @@ import (
 	proberesults "k8s.io/kubernetes/pkg/kubelet/prober/results"
 )
 
+const (
+	fakeSeccompProfileRoot = "/fakeSeccompProfileRoot"
+)
+
 type fakeHTTP struct {
 	url string
 	err error
@@ -78,6 +82,7 @@ func NewFakeKubeRuntimeManager(runtimeService internalapi.RuntimeService, imageS
 		runtimeService:      runtimeService,
 		imageService:        imageService,
 		keyring:             keyring,
+		seccompProfileRoot:  fakeSeccompProfileRoot,
 		internalLifecycle:   cm.NewFakeInternalContainerLifecycle(),
 	}
 

pkg/kubelet/kuberuntime/helpers.go

@@ -273,7 +273,7 @@ func (m *kubeGenericRuntimeManager) getSeccompProfileFromAnnotations(annotations
 	if strings.HasPrefix(profile, "localhost/") {
 		name := strings.TrimPrefix(profile, "localhost/")
 		fname := filepath.Join(m.seccompProfileRoot, filepath.FromSlash(name))
-		return fname
+		return "localhost/" + fname
 	}
 
 	return profile

pkg/kubelet/kuberuntime/helpers_test.go

@@ -17,9 +17,12 @@ limitations under the License.
 package kuberuntime
 
 import (
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtimetesting "k8s.io/kubernetes/pkg/kubelet/apis/cri/testing"
@@ -205,3 +208,100 @@ func TestGetImageUser(t *testing.T) {
 		assert.Equal(t, test.expectedImageUserValues.username, username, "TestCase[%d]", j)
 	}
 }
+
+func TestGetSeccompProfileFromAnnotations(t *testing.T) {
+	_, _, m, err := createTestRuntimeManager()
+	require.NoError(t, err)
+
+	tests := []struct {
+		description     string
+		annotation      map[string]string
+		containerName   string
+		expectedProfile string
+	}{
+		{
+			description:     "no seccomp should return empty string",
+			expectedProfile: "",
+		},
+		{
+			description:     "no seccomp with containerName should return exmpty string",
+			containerName:   "container1",
+			expectedProfile: "",
+		},
+		{
+			description: "pod docker/default seccomp profile should return docker/default",
+			annotation: map[string]string{
+				v1.SeccompPodAnnotationKey: "docker/default",
+			},
+			expectedProfile: "docker/default",
+		},
+		{
+			description: "pod docker/default seccomp profile with containerName should return docker/default",
+			annotation: map[string]string{
+				v1.SeccompPodAnnotationKey: "docker/default",
+			},
+			containerName:   "container1",
+			expectedProfile: "docker/default",
+		},
+		{
+			description: "pod unconfined seccomp profile should return unconfined",
+			annotation: map[string]string{
+				v1.SeccompPodAnnotationKey: "unconfined",
+			},
+			expectedProfile: "unconfined",
+		},
+		{
+			description: "pod unconfined seccomp profile with containerName should return unconfined",
+			annotation: map[string]string{
+				v1.SeccompPodAnnotationKey: "unconfined",
+			},
+			containerName:   "container1",
+			expectedProfile: "unconfined",
+		},
+		{
+			description: "pod localhost seccomp profile should return local profile path",
+			annotation: map[string]string{
+				v1.SeccompPodAnnotationKey: "localhost/chmod.json",
+			},
+			expectedProfile: "localhost/" + filepath.Join(fakeSeccompProfileRoot, "chmod.json"),
+		},
+		{
+			description: "pod localhost seccomp profile with containerName should return local profile path",
+			annotation: map[string]string{
+				v1.SeccompPodAnnotationKey: "localhost/chmod.json",
+			},
+			containerName:   "container1",
+			expectedProfile: "localhost/" + filepath.Join(fakeSeccompProfileRoot, "chmod.json"),
+		},
+		{
+			description: "container localhost seccomp profile with containerName should return local profile path",
+			annotation: map[string]string{
+				v1.SeccompContainerAnnotationKeyPrefix + "container1": "localhost/chmod.json",
+			},
+			containerName:   "container1",
+			expectedProfile: "localhost/" + filepath.Join(fakeSeccompProfileRoot, "chmod.json"),
+		},
+		{
+			description: "container localhost seccomp profile should override pod profile",
+			annotation: map[string]string{
+				v1.SeccompPodAnnotationKey:                            "unconfined",
+				v1.SeccompContainerAnnotationKeyPrefix + "container1": "localhost/chmod.json",
+			},
+			containerName:   "container1",
+			expectedProfile: "localhost/" + filepath.Join(fakeSeccompProfileRoot, "chmod.json"),
+		},
+		{
+			description: "container localhost seccomp profile with unmatched containerName should return empty string",
+			annotation: map[string]string{
+				v1.SeccompContainerAnnotationKeyPrefix + "container1": "localhost/chmod.json",
+			},
+			containerName:   "container2",
+			expectedProfile: "",
+		},
+	}
+
+	for i, test := range tests {
+		seccompProfile := m.getSeccompProfileFromAnnotations(test.annotation, test.containerName)
+		assert.Equal(t, test.expectedProfile, seccompProfile, "TestCase[%d]", i)
+	}
+}