-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
executor: switch to docker seccomp profile #1807
Conversation
// TODO: Can LCOW support seccomp? Does that even make sense? | ||
return []oci.SpecOpts{seccomp.WithDefaultProfile()}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer this question; https://github.com/containerd/containerd/blob/v1.4.1/contrib/seccomp/seccomp_default_unsupported.go#L1-L26
// +build !linux
func DefaultProfile(sp *specs.Spec) *specs.LinuxSeccomp {
return &specs.LinuxSeccomp{}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was more of a forward-looking pondering. I know it doesn't work now, but I figured if at some point it was implemented, that was the place to hook it up. Although system.SeccompSupported()
doesn't really provide a way to indicate whether it's LCOW or not, so we'd still need some more plumbing to activate that.
The whole LCOW thing seems to be somewhat up-in-the-air anyway, I was going to ignore it until after I had WCOW working, if even then.
var err error | ||
s.Linux.Seccomp, err = seccomp.GetDefaultProfile(s) | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should to the equivalent of what the containerd code does. Probably should add a function to the docker package to directly apply the profile instead of returning it
@tonistiigi @tiborvass thought I'd try to replace the containerd profile so that buildkit and docker would remain in sync; let me know if you think this makes sense. |
While we try to keep the containerd and docker seccomp profiles in sync, they may not always be; this switches the executor to use the docker seccomp profile, so that buildkit (when vendored in docker) will use the same default seccomp profile as is used for containers. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
dd9c3b1
to
a1a85da
Compare
If the Docker and containerd seccomp profiles differ, will this change cause problems for the containerd executor when running against plain containerd (outside Docker)? The Edit: Ah, I think I see. |
"technically" they're fully separate, however, I try to make sure that whenever a change is merged in moby, to also open a pull request in containerd (and vice-versa). So the "syncing" is fully manual 😞 however, I did some preparation / refactor work recently (https://github.com/moby/moby/pulls?q=is%3Apr+author%3Athajeztah+is%3Aclosed+seccomp+in%3Atitle+milestone%3A20.10.0+) to make the seccomp package in moby self contained. Might still need a bit of cleaning up, but possibly we can move it to a separate repository, and make containerd use that as a dependency. That way we can update the profile, and have both containerd and moby/docker use the same profile |
"fully separate" -> the profile in containerd is a fork/copy of the one in docker |
Yeah, that makes sense, and sounds like a good way forward. |
I know the containerd maintainers are not always happy with adding more dependencies, so I may need to try to convince them (and have that package in a clean shape; some code is (currently) docker specific, because docker allows specifying a custom seccomp profile as a JSON, whereas containerd does not have that functionality (only "default" profile, or provide a custom profile through a golang struct) |
While we try to keep the containerd and docker seccomp profiles in sync, they may not always be; this switches the executor to use the docker seccomp profile, so that buildkit (when vendored in docker) will use the same default seccomp profile as is used for containers.