Skip to content

Move functionality from github.com/docker/docker/profiles/seccomp #189

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsternberg
Copy link

Moves the github.com/docker/docker/profiles/seccomp package into
github.com/moby/sys/seccomp.

Moves the `github.com/docker/docker/profiles/seccomp` package into
`github.com/moby/sys/seccomp`.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should discuss if this should go in this repo or a separate one together with AppArmor.

At least we must preserve history, because there's a lot of context in the changes in those profile.

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, here in moby/sys we have low-level stuff which do not have tons of external dependencies (some of them depend on others in this repo, some need x/sys/unix, and that's all). That alone makes me think this is not the best place.

Having said that, it might make sense to create kernelversion package here, for example.

If there's something that can be added to https://github.com/seccomp/libseccomp-golang (which I happen to be a maintainer of), I'm also open to suggestions.

@thaJeztah
Copy link
Member

Thanks!

w.r.t. the "kernel version parsing"; possibly? The reason it was made internal is that we originally had a kernel-parsing package in moby, but we didn't want it to be an all-purpose one (people started to contribute "macOS kernel version parsing" and the like). I also moved it internal into this package to try and keep it self-contained with the possibility of moving the seccomp and AppArmor profiles to a separate module.

(it looks like gotest.tools made its way back into the package for some assertions, which we definitely should look if strictly needed, and otherwise just use stdlib for this again)

For a bit of context; we are currently working on trying to fix the circular dependency between BuildKit and Moby; we're almost there, but the seccomp profile was one of the last remaining ones. The reason BuildKit uses the seccomp profile from Moby is so that we make sure the profile is "in sync" with the one used by Moby itself; before that we used the profile from containerd (the containerd profile started as a fork of the one in Moby); while we TRY to keep those profiles in sync as well, it sometimes differed slightly with Moby (see moby/buildkit#1807).

An "easy" temporary solution would be for BuildKit to switch back to the containerd profile; however, there may be cases where Moby has changes in the profile that have not (yet) made their way into the containerd release.

All that said; there's been various discussions related to the profile, because (as shown above), we try to keep them all in sync, and having multiple forks doesn't help there.

So my ideal would be

  • for the profile(s) (seccomp, apparmor) to live in a separate module
  • align some of the differences in implementation (I think the containerd one had a slightly nicer interface to use, which basically was "apply this to this OCI spec", but I'd have to look at that again)
  • the moby package also defines a file-format (JSON), which I'm not sure the containerd package provides; not strictly critical for the seccomp profile as a whole, but probably also OK to keep as part of the same module (but could be a package in it)

And with the above, both Moby, BuildKit, and containerd could use the same module for the seccomp profile; also allowing the profile to have its own versioning (something to discuss though if we want that based on the module version, or to have some "version" selector for applying the profile if there's possibly changes in the profile that could break some scenarios).

One of the things we SHOULD look at is to finish that ever-lasting issue of the EPERM as default (I have an internal ticket related to that, and probably should put it in the Moby issue tracker);

☝️ whatever we do on that matter, we should try to preserve git history for all of those changes, as there's various cases where history contains very relevant information (why was a syscall added / removed? what was the motivation and decision around it?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants