Skip to content

pkg/cri/server: remove dependency on libcontainer/apparmor, libcontainer/utils#4715

Merged
mxpv merged 1 commit intocontainerd:masterfrom
thaJeztah:remove_libcontainer_apparmor
Nov 18, 2020
Merged

pkg/cri/server: remove dependency on libcontainer/apparmor, libcontainer/utils#4715
mxpv merged 1 commit intocontainerd:masterfrom
thaJeztah:remove_libcontainer_apparmor

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 10, 2020

relates to #4678 (comment)
relates to opencontainers/runc#2554
relates to moby/moby#5534

recent versions of libcontainer/apparmor simplified the AppArmor
check to only check if the host supports AppArmor, but no longer
checks if apparmor_parser is installed, or if we're running
docker-in-docker;

opencontainers/runc@bfb4ea1

The apparmor_parser binary is not really required for a system to run
AppArmor from a runc perspective. How to apply the profile is more in
the responsibility of higher level runtimes like Podman and Docker,
which may do the binary check on their own.

This patch copies the logic from libcontainer/apparmor, and
restores the additional checks.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 10, 2020

interesting

Requested golangci-lint 'v1.29', using 'v1.29.0', calculation took 7969ms
Installing golangci-lint v1.29.0...
Downloading https://github.com/golangci/golangci-lint/releases/download/v1.29.0/golangci-lint-1.29.0-darwin-amd64.tar.gz ...

calculation took 7969ms 🙀 that's rather long to change v1.29 to v1.29.0, wondering changing to v1.29.0 would save that step.

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 10, 2020 

  Running [/Users/runner/golangci-lint-1.29.0-darwin-amd64/golangci-lint run --out-format=github-actions --path-prefix=src/github.com/containerd/containerd --timeout=5m] in [/Users/runner/work/containerd/containerd/src/github.com/containerd/containerd] ...
  Error: `hostSupportsAppArmor` is unused (deadcode)

🤔 false positive?

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from 20c095c to a6ca3fa Compare November 10, 2020 10:34
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from a6ca3fa to b05b5d4 Compare November 10, 2020 10:49
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from b05b5d4 to eda49e3 Compare November 10, 2020 11:00
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah thaJeztah mentioned this pull request Nov 10, 2020
Merged
2 tasks
@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from eda49e3 to 9eb4918 Compare November 10, 2020 11:34
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 10, 2020 •
edited
Loading

Hmmm... failures look relevant; is it the `sync.Once that's problematic?

Details 
Summarizing 2 Failures:
[Fail] [k8s.io] AppArmor runtime should support apparmor [It] should enforce a profile blocking writes 
/home/runner/work/containerd/containerd/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/apparmor_linux.go:121
[Fail] [k8s.io] AppArmor runtime should support apparmor [It] should enforce a permissive profile 
/home/runner/work/containerd/containerd/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/apparmor_linux.go:121

edit: never mind; silly mistake; flipped a bool

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from 9eb4918 to b38e28e Compare November 10, 2020 12:00
thaJeztah
thaJeztah commented Nov 10, 2020
View reviewed changes
pkg/cri/server/helpers_linux.go Outdated
Copy link
Member Author

@thaJeztah thaJeztah Nov 10, 2020

Choose a reason for hiding this comment

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

FWIW; I was in doubt wether to add the os.Getenv() here, or in hostSupportsAppArmor(); happy to move it there

Copy link
Member

@dmcgowan dmcgowan Nov 11, 2020

Choose a reason for hiding this comment

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

I think its cleaner to put it in the function and keep the original logic ordering.

Copy link
Member Author

@thaJeztah thaJeztah Nov 12, 2020

Choose a reason for hiding this comment

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

@dmcgowan updated; PTAL

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from b38e28e to ce831a0 Compare November 10, 2020 12:58
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 10, 2020 •
edited
Loading

Is there a time-limit on tests? Looks like one was cancelled after 15 minutes;

Ran 76 of 83 Specs in 61.066 seconds
SUCCESS! -- 76 Passed | 0 Failed | 0 Pending | 7 Skipped


Ginkgo ran 1 suite in 1m1.131860577s
Test Suite Passed
PASS
Error: The operation was canceled.

edit: rebased to trigger CI again

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from ce831a0 to 7ff1502 Compare November 10, 2020 15:19
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from 7ff1502 to 9d52370 Compare November 10, 2020 19:53
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from 9d52370 to 8c77bce Compare November 10, 2020 22:35
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 10, 2020

Build succeeded.

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 10, 2020

yay, green

@AkihiroSuda @crosbymichael ptal

AkihiroSuda
AkihiroSuda reviewed Nov 11, 2020
View reviewed changes
pkg/cri/server/helpers_linux.go Outdated
Copy link
Member

@AkihiroSuda AkihiroSuda Nov 11, 2020

Choose a reason for hiding this comment

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

This is from 2014... Do we really need it?

Copy link
Member Author

@thaJeztah thaJeztah Nov 11, 2020

Choose a reason for hiding this comment

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

Apparently we do opencontainers/runc#2554 (comment)

@thaJeztah
pkg/cri/server: remove dependency on libcontainer/apparmor, libcontai…
eba94a1 
…ner/utils

recent versions of libcontainer/apparmor simplified the AppArmor
check to only check if the host supports AppArmor, but no longer
checks if apparmor_parser is installed, or if we're running
docker-in-docker;

opencontainers/runc@bfb4ea1

> The `apparmor_parser` binary is not really required for a system to run
> AppArmor from a runc perspective. How to apply the profile is more in
> the responsibility of higher level runtimes like Podman and Docker,
> which may do the binary check on their own.

This patch copies the logic from libcontainer/apparmor, and
restores the additional checks.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the remove_libcontainer_apparmor branch from 8c77bce to eba94a1 Compare November 12, 2020 14:42
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 12, 2020

Build succeeded.

dmcgowan
dmcgowan approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 13, 2020

@dmcgowan good to go?

@mxpv mxpv merged commit 2837fb3 into containerd:master Nov 18, 2020
@thaJeztah thaJeztah deleted the remove_libcontainer_apparmor branch November 18, 2020 22:36
@thaJeztah thaJeztah mentioned this pull request Feb 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@mxpv mxpv mxpv approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @dmcgowan @mxpv @AkihiroSuda