Fix the wrong localhost seccomp path of CRI #55450

feiskyer · 2017-11-10T02:25:59Z

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:

Fix CRI localhost seccomp path in format localhost//profileRoot/profileName.

feiskyer · 2017-11-10T02:26:10Z

/assign @yujuhong

feiskyer · 2017-11-10T02:56:49Z

/retest

feiskyer · 2017-11-10T05:37:27Z

/retest

mtaufen · 2017-11-13T03:14:20Z

pkg/kubelet/dockershim/helpers_linux.go

@@ -62,7 +62,8 @@ 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/")


Not sure you end up with an absolute path here. TrimPrefix will cut the / too, and you're left with a path that has no leading /. From what I can tell, filepath.FromSlash does not add a leading /. https://play.golang.org/p/_A6DBoSG1V

@mtaufen yep, it doesn't add a leading '/'. And there are indeed issues about seccomp path.

localhost/ is missing because of following code:

kubernetes/pkg/kubelet/kuberuntime/helpers.go

Lines 278 to 282 in 72a2b0b

if strings.HasPrefix(profile, "localhost/") {

name := strings.TrimPrefix(profile, "localhost/")

fname := filepath.Join(m.seccompProfileRoot, filepath.FromSlash(name))

return fname

}

will add a new commit to fix this.

feiskyer · 2017-11-13T05:26:13Z

@yujuhong @mtaufen Added a new commit to fix CRI localhost seccomp path in format localhost//profileRoot/profileName.

k8s-cherrypick-bot · 2017-11-13T05:29:07Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

mtaufen · 2017-11-14T02:30:03Z

pkg/kubelet/dockershim/helpers_linux.go

@@ -62,7 +62,8 @@ 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/")
 	file, err := ioutil.ReadFile(filepath.FromSlash(fname))


Can we also add validation that fname is absolute, and return an error if it is not?

something like this

import "path/filepath" if !filepath.IsAbs(fname) { return nil, fmt.Errorf("seccomp profile path must be absolute, but got relative path %q", fname) }

@mtaufen Good catch. Will add.

feiskyer · 2017-11-15T04:25:47Z

/retest

feiskyer · 2017-11-16T02:12:07Z

/retest

yujuhong · 2017-11-21T00:32:55Z

@feiskyer what's the expected format of the annotation value? Is it localhost/<full path to profile>, e.g., localhost//foo/bar/profile? Or is it localhost/foo/bar/profile?

feiskyer · 2017-11-21T01:24:47Z

@yujuhong The path in the kubernetes annotation is a relative path, and root directory is set by kubelet's --seccomp-profile-root option (default to "/var/lib/kubelet/seccomp"). We converted it to absolute path in CRI.

feiskyer · 2017-11-21T02:23:22Z

/retest

yujuhong · 2017-11-21T18:26:25Z

pkg/kubelet/dockershim/helpers_linux_test.go

@@ -55,26 +58,26 @@ func TestGetSeccompSecurityOpts(t *testing.T) {
 }

 func TestLoadSeccompLocalhostProfiles(t *testing.T) {
+	tmpdir, err := ioutil.TempDir("", "seccomp-local-profile-test")
+	assert.NoError(t, err)


s/assert/require

assert is not fatal and the test will continue.

yujuhong · 2017-11-21T18:26:28Z

pkg/kubelet/dockershim/helpers_linux_test.go

+	defer os.RemoveAll(tmpdir)
+	testProfile := `{"foo": "bar"}`
+	err = ioutil.WriteFile(filepath.Join(tmpdir, "test"), []byte(testProfile), 0644)
+	assert.NoError(t, err)


s/assert/require

yujuhong · 2017-11-21T18:26:56Z

pkg/kubelet/dockershim/helpers_linux_test.go

-		seccompProfile: "localhost/fixtures/seccomp/sub/subtest",
-		expectedOpts:   []string{`seccomp={"abc":"def"}`},
-		expectErr:      false,
+		msg:            "Non-exist profile should return error",


nit: s/Non-exist/Non-existent

yujuhong · 2017-11-21T18:27:23Z

pkg/kubelet/kuberuntime/helpers_test.go

+
+func TestGetSeccompProfileFromAnnotations(t *testing.T) {
+	_, _, m, err := createTestRuntimeManager()
+	assert.NoError(t, err)


s/assert/require

tallclair · 2017-11-21T23:08:48Z

/unassign
defer to yuju & michael

feiskyer · 2017-11-22T02:51:13Z

@yujuhong Addressed comments. PTAL

yujuhong · 2017-11-22T19:37:35Z

/lgtm

k8s-github-robot · 2017-11-22T19:37:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, yujuhong

Associated issue: 55359

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [yujuhong]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jberkus · 2017-11-22T23:52:15Z

/priority critical-urgent

/remove-priority important-soon

adjusting priorities for code freeze

k8s-github-robot · 2017-11-23T01:28:53Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@Random-Liu @feiskyer @yujuhong

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/node: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-github-robot · 2017-11-23T05:48:43Z

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 here.

…50-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #55450 Cherry pick of #55450 on release-1.8. #55450: Fix incorrect localhost seccomp profile path

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 10, 2017

k8s-github-robot assigned yifan-gu and dims Nov 10, 2017

k8s-ci-robot assigned yujuhong Nov 10, 2017

feiskyer unassigned dims and yifan-gu Nov 10, 2017

mtaufen reviewed Nov 13, 2017

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 13, 2017

feiskyer changed the title ~~Fix the wrong seccomp path comment~~ Fix the wrong localhost seccomp path Nov 13, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 13, 2017

feiskyer changed the title ~~Fix the wrong localhost seccomp path~~ Fix the wrong localhost seccomp path of CRI Nov 13, 2017

feiskyer added the cherrypick-candidate label Nov 13, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Nov 13, 2017

feiskyer assigned Random-Liu Nov 13, 2017

mtaufen reviewed Nov 14, 2017

View reviewed changes

feiskyer force-pushed the seccomp-path branch 3 times, most recently from f2d58e8 to d03629a Compare November 14, 2017 08:40

feiskyer added the kind/bug Categorizes issue or PR as related to a bug. label Nov 17, 2017

yujuhong assigned tallclair Nov 21, 2017

yujuhong reviewed Nov 21, 2017

View reviewed changes

k8s-ci-robot unassigned tallclair Nov 21, 2017

feiskyer added 3 commits November 22, 2017 02:49

Fix incorrect localhost seccomp profile path

2a2a875

Verify seccomp absolute path in dockershim

7e21146

Update bazel and remove unused data files

b8469e4

feiskyer force-pushed the seccomp-path branch from f6fa52b to b8469e4 Compare November 22, 2017 02:50

feiskyer added status/approved-for-milestone and removed milestone/needs-approval labels Nov 22, 2017

k8s-github-robot added the milestone/needs-attention label Nov 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 22, 2017

enisoc added the status/in-progress label Nov 23, 2017

feiskyer removed the milestone/needs-attention label Nov 23, 2017

k8s-github-robot merged commit 1fdc688 into kubernetes:master Nov 23, 2017

feiskyer deleted the seccomp-path branch November 23, 2017 06:38

feiskyer mentioned this pull request Nov 23, 2017

Automated cherry pick of #55450 #56279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the wrong localhost seccomp path of CRI #55450

Fix the wrong localhost seccomp path of CRI #55450

feiskyer commented Nov 10, 2017 •

edited

Loading

feiskyer commented Nov 10, 2017

feiskyer commented Nov 10, 2017

feiskyer commented Nov 10, 2017

mtaufen Nov 13, 2017

feiskyer Nov 13, 2017

feiskyer commented Nov 13, 2017

k8s-cherrypick-bot commented Nov 13, 2017

mtaufen Nov 14, 2017

mtaufen Nov 14, 2017

feiskyer Nov 14, 2017

feiskyer commented Nov 15, 2017

feiskyer commented Nov 16, 2017

yujuhong commented Nov 21, 2017

feiskyer commented Nov 21, 2017

feiskyer commented Nov 21, 2017

yujuhong Nov 21, 2017

yujuhong Nov 21, 2017

yujuhong Nov 21, 2017

feiskyer Nov 22, 2017

yujuhong Nov 21, 2017

tallclair commented Nov 21, 2017

feiskyer commented Nov 22, 2017

yujuhong commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

jberkus commented Nov 22, 2017

k8s-github-robot commented Nov 23, 2017

k8s-github-robot commented Nov 23, 2017

	if strings.HasPrefix(profile, "localhost/") {
	name := strings.TrimPrefix(profile, "localhost/")
	fname := filepath.Join(m.seccompProfileRoot, filepath.FromSlash(name))
	return fname
	}

Fix the wrong localhost seccomp path of CRI #55450

Fix the wrong localhost seccomp path of CRI #55450

Conversation

feiskyer commented Nov 10, 2017 • edited Loading

feiskyer commented Nov 10, 2017

feiskyer commented Nov 10, 2017

feiskyer commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Nov 13, 2017

k8s-cherrypick-bot commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Nov 15, 2017

feiskyer commented Nov 16, 2017

yujuhong commented Nov 21, 2017

feiskyer commented Nov 21, 2017

feiskyer commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Nov 21, 2017

feiskyer commented Nov 22, 2017

yujuhong commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

jberkus commented Nov 22, 2017

k8s-github-robot commented Nov 23, 2017

k8s-github-robot commented Nov 23, 2017

feiskyer commented Nov 10, 2017 •

edited

Loading