add seccomp profile, hooks dir override for containerd #8044

drahnr · 2022-02-06T16:59:31Z

Allow to pass an explicit seccomp profile in json format, to override the default abilities for the worker.

This allows for a more fine gained control to, i.e. allow simplistic passthrough of GPUs and other worker specific devices.

For some devices, pre and post hooks are required to be used with i.e. containerd. The client has to pass these on. As such, the worker has to be instructed to pass them on to the container runtime. (WIP)

Changes proposed by this PR

ref #6912

comply to all contributing guidelines
add flag for seccomp filter override
allow hooks dir override
add a test case

Notes to reviewer

I am new to golang, if there are any idioms I am violating, happy to correct!

Release Note

Adds a worker cli option to override the seccomp filter
Adds a worker containerd cli option to pass on a oci hooks dir, for i.e. nvidia gpu mapping

xtremerui · 2022-09-07T19:42:19Z

Hi @drahnr , I have noticed the commit history is a bit messing up.

To fix it, the easiest way IMO is

git checkout a new branch from latest master
git cherry-pick your commits that related to this PR
git push your new branch and create a PR from that

Later, if you want to get updates from master branch, you could

git fetch master
git rebase master into your working branch

drahnr · 2022-09-11T10:52:27Z

@xtremerui I am not sure why the unit and watsjs tests fail, to my knowledge the files I changed should not affect these checks. I can't seem to access the test result from the run when trying to login with my gh account. It's either errors or is loading indefinitely.

xtremerui · 2022-09-12T04:23:35Z

The error in unit test:

# github.com/concourse/concourse/worker/runtime
worker/runtime/backend_test.go:551:32: unexpected comma; expecting ]
FAIL	github.com/concourse/concourse/worker/runtime [setup failed]
FAIL

you can join contributors team in concourse/governance to view the build log

drahnr · 2022-09-12T17:01:14Z

@xtremerui I am not entirely sure what's happening, but the CI is gimping out on various, seemingly unrelated ends and I don't even know where to begin to debug.

taylorsilva · 2022-09-12T20:52:12Z

@drahnr In case you're still stuck, you can reproduce locally by running:

go test -v ./worker/runtime/spec

I got the same result as what's in CI

┌─[drahnr/master*][~/workspace/concourse/concourse]
└─▪ go test -v ./worker/runtime/spec
go: downloading github.com/stretchr/testify v1.8.0
=== RUN   TestSuite
=== RUN   TestSuite/TestContainerSpec
=== RUN   TestSuite/TestContainerSpec/defaults
=== RUN   TestSuite/TestContainerSpec/privileged_mounts
=== RUN   TestSuite/TestContainerSpec/env_with_path_already_configured
=== RUN   TestSuite/TestContainerSpec/mounts
=== RUN   TestSuite/TestContainerSpec/seccomp_is_not_empty_for_unprivileged
=== CONT  TestSuite
    spec_test.go:471:
                Error Trace:    /home/taylor/workspace/concourse/concourse/worker/runtime/spec/spec_test.go:471
                                                        /home/taylor/workspace/concourse/concourse/worker/runtime/spec/spec_test.go:526
                Error:          Should NOT be empty, but was &{ <nil> [] []   []}
                Test:           TestSuite
=== CONT  TestSuite/TestContainerSpec/seccomp_is_not_empty_for_unprivileged
    testing.go:1336: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
=== CONT  TestSuite/TestContainerSpec
    testing.go:1336: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
--- FAIL: TestSuite (0.00s)
    --- FAIL: TestSuite/TestContainerSpec (0.00s)
        --- PASS: TestSuite/TestContainerSpec/defaults (0.00s)
        --- PASS: TestSuite/TestContainerSpec/privileged_mounts (0.00s)
        --- PASS: TestSuite/TestContainerSpec/env_with_path_already_configured (0.00s)
        --- PASS: TestSuite/TestContainerSpec/mounts (0.00s)
        --- FAIL: TestSuite/TestContainerSpec/seccomp_is_not_empty_for_unprivileged (0.00s)
FAIL
FAIL    github.com/concourse/concourse/worker/runtime/spec      0.006s
FAIL

taylorsilva · 2022-09-12T20:52:51Z

Looks like your undo commit fixed all the unrelated stuff :)

drahnr · 2022-09-12T21:08:35Z

Sorry for the noise, new language (it's rather painful coming from rust).

How's the logging supposed to be implemented, the current fmt.Println is not ok I assume? I'd be very happy to get some input.

drahnr · 2022-09-14T08:44:24Z

@xtremerui / @taylorsilva I'd appreciate a review :)

drahnr · 2022-09-24T11:35:37Z

Gentle ping

xtremerui · 2022-09-26T22:18:05Z

@drahnr thx for the PR. Just want to make sure the feature is completely optional as we want to make v7.9 as a good upgrade for existing user without worring about behaviour changes.

drahnr · 2022-09-27T10:14:36Z

Just did another worker run, for both using the newly added flags and not using them. Either way works fine.

So as far as I can tell, this is fine to be merged. (I am still not convinced using println is the right appraoch, I'd be in need of some input here, otherwise good to go)

drahnr · 2022-10-05T14:20:17Z

Gentle ping

drahnr · 2022-11-08T14:18:24Z

Is there anything that's left or missing to do?

xtremerui · 2022-11-08T14:55:56Z

@drahnr So far it is just in pending for reviewing.

xtremerui

Thank you for the PR. Just some nits then we are good to go.

worker/runtime/backend.go

xtremerui · 2022-11-28T21:30:14Z

worker/runtime/backend.go

+		}
+		fmt.Println("Using seccomp rules from path by default:", b.seccompProfilePath)
+		b.seccompProfile = profile
+	} else {


Not needed.

The else case is needed, unless there is some lang feature I am not aware of? It's the only place where seccompProfile is initialized if no arg was given.

so if b.seccompProfilePath is not set, it will skip line 213 to 224, and exec line 225 and then line 228. That is the same as you remove line 224 and 226. Maybe I am not clear here in first place sorry about that.

I actually meant keeping line 225 while removing line 224 and 226.

Wouldn't removing the else cause b.seccompProfile to always be set to the default?

I agree with @wailashi see, next comment

worker/workercmd/worker_linux.go

worker/runtime/backend.go

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

This reverts commit a78e6b3. Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

drahnr · 2022-12-19T20:54:28Z

gentle 🎄 / ☃️ ping

drahnr · 2023-01-19T10:27:26Z

gentle ☃️ ping

xtremerui · 2023-01-19T14:42:04Z

worker/runtime/backend.go

+	} else {
+		b.seccompProfile = bespec.GetDefaultSeccompProfile()
+	}
+


Suggested change

} else {

b.seccompProfile = bespec.GetDefaultSeccompProfile()

}

b.seccompProfile = bespec.GetDefaultSeccompProfile()

I do not understand the proposed changeset.

The if branch covers and fills b.seccompProfile from the provided path or fails, the else branch should fill it ( b.seccompProfile) with the default seccomp profile bspec.GetDefaultSeccompProfile().

If the else case is removed, it will override the loaded seccomp profile with the default one.

Either I do not understand the flow of go code, or I am missing some impl detail, right now I do not see how this would work and why the else branch is not needed. Could you clarify please?

oh my mistake here. I thought we either want to always set it to default or the provided one. If thats not the case then we are good here.

We always want to set it either explicitly by loading from file, or using the default one. If you want to avoid the else branch we'd have to move b.seccompProfile = bespec.GetDefaultSeccompProfile() up before the if branch

drahnr requested a review from a team as a code owner February 6, 2022 16:59

drahnr force-pushed the master branch 3 times, most recently from 6ac7333 to 84539f9 Compare March 23, 2022 08:43

EstebanFS assigned xtremerui Mar 28, 2022

drahnr force-pushed the master branch from d4a9589 to df79596 Compare September 6, 2022 15:02

drahnr force-pushed the master branch 3 times, most recently from 3a27c9c to aca0458 Compare September 9, 2022 12:52

xtremerui added the enhancement label Sep 9, 2022

drahnr force-pushed the master branch from cff8406 to 86c85f5 Compare September 12, 2022 09:17

drahnr force-pushed the master branch from 521b176 to e9b5950 Compare September 12, 2022 21:04

xtremerui added this to the v7.9.0 milestone Nov 25, 2022

drahnr mentioned this pull request Nov 28, 2022

influxdb metrics client - update to 2.0 #8633

Open

xtremerui reviewed Nov 29, 2022

View reviewed changes

drahnr commented Dec 1, 2022

View reviewed changes

worker/runtime/backend.go Outdated Show resolved Hide resolved

drahnr commented Dec 1, 2022

View reviewed changes

worker/runtime/backend.go Outdated Show resolved Hide resolved

drahnr commented Dec 1, 2022

View reviewed changes

worker/runtime/backend.go Outdated Show resolved Hide resolved

drahnr commented Dec 1, 2022

View reviewed changes

worker/runtime/backend.go Outdated Show resolved Hide resolved

drahnr force-pushed the master branch from 4db742d to a6f02da Compare December 1, 2022 13:06

drahnr modified the milestones: v7.9.0, v7.9.1 Dec 1, 2022

drahnr added 12 commits December 1, 2022 16:52

add seccomp profile

b25d65b

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

use non-standard ports

97a82d0

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

add oci hooks dir

ad9354b

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

fixins

0923bf9

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

go casing is not my ting

e2ea53e

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

proper hook loading

03af84f

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

fix syntax errors

425375b

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

redudant types

6ccdeb0

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

Revert "use non-standard ports"

9ff86d4

This reverts commit a78e6b3. Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

undo

7402293

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

fixup test

4aa6ab8

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

Remove print statements

fefa1b4

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>

drahnr force-pushed the master branch from a6f02da to fefa1b4 Compare December 1, 2022 15:52

drahnr requested a review from xtremerui December 1, 2022 15:53

xtremerui reviewed Jan 19, 2023

View reviewed changes

xtremerui approved these changes Jan 19, 2023

View reviewed changes

xtremerui merged commit c533e5c into concourse:master Jan 19, 2023

This was referenced Feb 1, 2023

add worker containerd config concourse/concourse-chart#323

Merged

add worker containerd config concourse/concourse-bosh-release#170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add seccomp profile, hooks dir override for containerd #8044

add seccomp profile, hooks dir override for containerd #8044

drahnr commented Feb 6, 2022 •

edited

xtremerui commented Sep 7, 2022

drahnr commented Sep 11, 2022

xtremerui commented Sep 12, 2022

drahnr commented Sep 12, 2022

taylorsilva commented Sep 12, 2022

taylorsilva commented Sep 12, 2022

drahnr commented Sep 12, 2022 •

edited

drahnr commented Sep 14, 2022

drahnr commented Sep 24, 2022

xtremerui commented Sep 26, 2022

drahnr commented Sep 27, 2022

drahnr commented Oct 5, 2022

drahnr commented Nov 8, 2022

xtremerui commented Nov 8, 2022

xtremerui left a comment

xtremerui Nov 28, 2022

drahnr Dec 1, 2022 •

edited

xtremerui Jan 19, 2023

wailashi Jan 19, 2023

drahnr Jan 19, 2023

drahnr commented Dec 19, 2022

drahnr commented Jan 19, 2023 •

edited

xtremerui Jan 19, 2023

drahnr Jan 19, 2023 •

edited

xtremerui Jan 19, 2023

drahnr Jan 19, 2023

add seccomp profile, hooks dir override for containerd #8044

add seccomp profile, hooks dir override for containerd #8044

Conversation

drahnr commented Feb 6, 2022 • edited

Changes proposed by this PR

Notes to reviewer

Release Note

xtremerui commented Sep 7, 2022

drahnr commented Sep 11, 2022

xtremerui commented Sep 12, 2022

drahnr commented Sep 12, 2022

taylorsilva commented Sep 12, 2022

taylorsilva commented Sep 12, 2022

drahnr commented Sep 12, 2022 • edited

drahnr commented Sep 14, 2022

drahnr commented Sep 24, 2022

xtremerui commented Sep 26, 2022

drahnr commented Sep 27, 2022

drahnr commented Oct 5, 2022

drahnr commented Nov 8, 2022

xtremerui commented Nov 8, 2022

xtremerui left a comment

Choose a reason for hiding this comment

xtremerui Nov 28, 2022

Choose a reason for hiding this comment

drahnr Dec 1, 2022 • edited

Choose a reason for hiding this comment

xtremerui Jan 19, 2023

Choose a reason for hiding this comment

wailashi Jan 19, 2023

Choose a reason for hiding this comment

drahnr Jan 19, 2023

Choose a reason for hiding this comment

drahnr commented Dec 19, 2022

drahnr commented Jan 19, 2023 • edited

xtremerui Jan 19, 2023

Choose a reason for hiding this comment

drahnr Jan 19, 2023 • edited

Choose a reason for hiding this comment

xtremerui Jan 19, 2023

Choose a reason for hiding this comment

drahnr Jan 19, 2023

Choose a reason for hiding this comment

drahnr commented Feb 6, 2022 •

edited

drahnr commented Sep 12, 2022 •

edited

drahnr Dec 1, 2022 •

edited

drahnr commented Jan 19, 2023 •

edited

drahnr Jan 19, 2023 •

edited