Skip to content
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.

Sign up for GitHub

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

Jump to bottom

vendor: bump seccomp/libseccomp-golang to f33da4d #3465

Merged
merged 3 commits into from
May 4, 2022
Merged

vendor: bump seccomp/libseccomp-golang to f33da4d #3465

merged 3 commits into from
May 4, 2022

Conversation

crazy-max
Copy link
Contributor

fixes clang issue seccomp/libseccomp-golang#90.

@thaJeztah there are other changes while vendoring it seccomp/libseccomp-golang@3879420...f33da4d. not sure about the impact of them on runc.

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@thaJeztah
Copy link
Member

Looks like there's some uses of libseccomp.ActKill that need to be updated locally;

Error: SA1019: libseccomp.ActKill is deprecated: use ActKillThread  (staticcheck)
  Error: SA1019: libseccomp.ActKill is deprecated: use ActKillThread  (staticcheck)
  
  Error: issues found

thaJeztah
thaJeztah reviewed May 3, 2022
View reviewed changes
libcontainer/seccomp/seccomp_linux.go
@@ -113,8 +113,8 @@ func InitSeccomp(config *configs.Seccomp) (int, error) {
// Convert Libcontainer Action to Libseccomp ScmpAction
func getAction(act configs.Action, errnoRet *uint) (libseccomp.ScmpAction, error) {
switch act {
case configs.Kill:
return libseccomp.ActKill, nil
case configs.Kill, configs.KillThread:
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if one of configs.Kill also should be deprecated on this side 🤔 (not for this PR, just thinking out loud?)

Copy link
Contributor

Choose a reason for hiding this comment

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

We have the same problem in libcontainer/config -- Kill and KillThread are two distinct constants. Guess it makes sense to deprecate the former.

Copy link
Member

Choose a reason for hiding this comment

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

Yup; that was my thought as well

I'm ok doing that separately, but something we should probably do

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh well, it's a bit bigger than I thought; see #3466

@crazy-max feel free to cherry-pick that commit to here.

I also think we should really deprecate ACT_KILL, but it should be done in the runtime-spec repo first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kolyshkin

feel free to cherry-pick that commit to here.

done

@thaJeztah
Copy link
Member

@crazy-max can you swap the order of the commits (assuming both ActKillThread and ActKill were present before updating; if not, then it's best to squash the commits to preserve git bisect. 😅

@crazy-max
fix deprecated ActKill
29a56b5 
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
vendor: bump seccomp/libseccomp-golang to f33da4d
df2bc13 
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max marked this pull request as ready for review May 3, 2022 12:04
@crazy-max crazy-max requested a review from thaJeztah May 3, 2022 14:16
thaJeztah
thaJeztah previously approved these changes May 3, 2022
View reviewed changes
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.

LGTM

@kolyshkin PTAL

@kolyshkin @crazy-max
libct/seccomp/config: add missing KillThread, KillProcess
68427f3 
OCI spec added SCMP_ACT_KILL_THREAD and SCMP_ACT_KILL_PROCESS almost two
years ago ([1], [2]), but runc support was half-finished [3].

Add these actions, and modify the test case to check them.

In addition, "runc features" now lists the new actions.

[1] opencontainers/runtime-spec#1044
[2] opencontainers/runtime-spec#1064
[3] https://github.com/opencontainers/runc/pulls/3204

Fixes: 4a4d4f1
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit e74fdeb)
kolyshkin
kolyshkin approved these changes May 4, 2022
View reviewed changes
Copy link
Contributor

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

LGTM

@kolyshkin kolyshkin mentioned this pull request May 4, 2022
Closed
@kolyshkin kolyshkin requested a review from thaJeztah May 4, 2022 18:15
@kolyshkin
Copy link
Contributor

@thaJeztah I see your LGTM, but you probably forgot to actually "Accept" the PR

thaJeztah
thaJeztah approved these changes May 4, 2022
View reviewed changes
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.

whoops! probably picked "comment" instead of "approve" 😂

LGTM (for real this time)

@thaJeztah thaJeztah merged commit da6f3b0 into opencontainers:main May 4, 2022
@crazy-max crazy-max deleted the update-libseccomp branch May 10, 2022 16:18
@crazy-max crazy-max mentioned this pull request May 10, 2022
Merged
@kolyshkin kolyshkin mentioned this pull request May 19, 2022
Merged
@crazy-max
Copy link
Contributor Author

@thaJeztah @kolyshkin is there a milestone that includes this PR to fix the clang issue? cc @tonistiigi

@kolyshkin kolyshkin added kind/dependency backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 labels May 20, 2022
@kolyshkin kolyshkin mentioned this pull request May 20, 2022
Merged
@kolyshkin kolyshkin added backport/done/1.1 A PR in main branch which was backported to release-1.1 and removed backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 labels Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@kolyshkin kolyshkin kolyshkin approved these changes

Assignees
No one assigned
Labels
backport/done/1.1 A PR in main branch which was backported to release-1.1 kind/dependency
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@crazy-max @thaJeztah @kolyshkin