Fix seccomp notify inconsistencies#1096
Merged
vbatts merged 2 commits intoopencontainers:masterfrom Mar 19, 2021
Merged
Conversation
This wasn't catched before, even though we had working patches for containerd and runc in advance, as neither containerd nor runc really use these consts. In the spec this field is a string[1] and therefore when containerd parses with json.Unmarshall[2] it works just fine. With runc is not used either, as it uses a different struct in the libcontainer directory[3]. Therefore, even with patches using this change, this const definition was missed as it is not used by the patches. [1]: https://github.com/opencontainers/runtime-spec/blob/a8c4a9ee0f6b5a0b994c5c23c68725394e2b0d9d/specs-go/config.go#L641 [2]: https://github.com/containerd/containerd/blob/8dbe53a2a930af3631229e4d92cf839b64ee5a38/contrib/seccomp/seccomp.go#L36-L40 [3]: https://github.com/opencontainers/runc/pull/2682/files#diff-9915e69bab45a993d366aad4a7d47459d73ec4304b7c33942f197dd221673376R51 [4]: https://github.com/opencontainers/runtime-spec/blob/a8c4a9ee0f6b5a0b994c5c23c68725394e2b0d9d/specs-go/config.go#L614 Signed-off-by: Rodrigo Campos <[email protected]>
Commit "Add Seccomp Notify support" (58798e7) just added SECCOMP_FILTER_FLAG_NEW_LISTENER to the schema and not to the list of flags in config-linux.md. However, it was a mistake to add them to the schema, as the user will never really need to specify that flag. Signed-off-by: Rodrigo Campos <[email protected]>
rata
force-pushed
the
rata_seccomp_listenerpath
branch
from
8b43023 to
0f84938
Compare
Member
Author
|
@giuseppe Thanks! Pushed another fix to another inconsistency, removing SECCOMP_FILTER_FLAG_NEW_LISTENER from the valid flags. That should have never been added there in the first place 🙈 |
Member
|
LGTM |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In PR #1074 I missed some consts, sorry!
Funny thing is we didn't catch them even when we used this in runc and containerd patches because containerd just passes a string (see commit for a detailed explanation) and runc doesn't use these constants (again, full explanation in the commit msg).
cc @vbatts @giuseppe as you LGTM to #1074.