Hi @wzshiming. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 45s (non-voting)
• containerd-test-arm64 : SUCCESS in 5m 30s (non-voting)
• containerd-integration-test-arm64 : FAILURE in 20m 15s (non-voting)

Just adding a note here: The comment here was read backwards. It was meant to be understood as "this check is like the one in libcontainer, but that one doesn't check this thing that we additionally do."

This caused a regression in Moby as containerd stopped checking for apparmor_parser as its Godoc stated/still states. The comment was worded poorly, but if the plan is to move forward with this/not revert (as Moby is likely the only consumer that ever cared about the distinction and it wasn't caught up to this point), we probably should just update the Godoc for the new reality and add some errata to the release notes.

Also it looks like we knew about this 18 months ago: #5557 (comment)

@thaJeztah:

Ah, hmm.. yes, I think #5519 is wrong, as this implementation was explicitly different than the one in libcontainer, because containerd (and moby/docker) need to set up / load the profile, whereas runc only has to apply it.

Yes. Ran into this yesterday.

Assuming that you have a working apparmor just because the module was compiled in, does not seem appropriate here.

• Either you assume that the kernel calls work, and you use those.
• Or you check that the userland tools work/exist, and you use those.

we probably should just update the Godoc for the new reality and add some errata to the release notes.

This hybrid where we check for A and do B is wrong. Just updating the comments without removing all Command("apparmor_parser...) is not sufficient.

So in a (slightly larger) nutshell;

• libcontainer originated from docker, and later became the foundation for runc because of this, libcontainer was carrying some amount of code / logic / complexity that made sense as part of "higher level" runtimes, but not needed for runc itself, which sometimes meant runc was more heavyweight than needed
• removing such code would involve breaking changes (either in signature or behavior), so the project advertised libcontainer to be considered "unstable" (not for external use, expect breaking changes) with the intent to make it internal only (move most packages into internal opencontainers/runc#3028)
• containerd still depended on many parts of runc/libcontainer which initially also meant that the libcontainer version was coupled with the runc binary version used
• which, when updating to runc 1.0.0-rc93, resulted in a breaking change, as runc removed the check for apparmor_parser [draft] update to runc v1.0.0-rc92-215-gcf6c0741 #4678 (comment)
• to keep the behavior (needed for containerd), the logic that was previously in runc, was now moved to containerd itself pkg/cri/server: remove dependency on libcontainer/apparmor, libcontainer/utils #4715
• followed by other changes to reduce the dependency on runc/libcontainer (e.g. further reduce uses of libcontainer and update runc v1.0.0-rc93 #4717), and explicitly decoupling the runc binary version and library (libcontainer) version (Separate runc binary version from libcontainer version, and remove obsolete build-tags #5036)

So, yes, the changes in #4715 where specifically intended to preserve the old behavior as it was needed for containerd (CRI); the function was named hostSupportsAppArmor() to be more explicit on intent (but naming is hard), and kept internal to pkg/cri/server/, as the check was specific for that purpose (not a general purpose AppArmor check) at the time (but later move to pkg/apparmor and exported in #4811).

The changes in this PR;

• reverted that, and aligned the function with the runc equivalent (possibly partially due it now in pkg/apparmor, so to be considered more "general purpose", and the slightly ambiguous GoDoc)
• removed the reference to moby/moby@de191e8, which was left on purpose pkg/cri/server: remove dependency on libcontainer/apparmor, libcontainer/utils #4715 (comment)

Not sure about the background, but I think @wdoekes is perfectly correct, regardless what the background is:

• First of all, the code comments above the function now do not match what the function does: If the function intentionally is meant to only check whether the kernel feature is available, the comment should reflect this and not still state the previous stage with the reference to libcontainer.
• Second, if containerd uses this function as condition whether to call apparmor_parser or not, then either a new function needs to be added or this commit reverted, since then obviously containerd uses it differently than libcontainer which makes it wrong to align both. If it is only Docker/moby which runs apparmor_parser, then apparmor: Check if apparmor_parser is available moby/moby#44902 seems to be the right thing.

If it is only Docker/moby which runs apparmor_parser

Looks like containerd also uses it (apparmor_parser);

which gets invoked by

(and I think this PR was what led to CI failures investigated in #5557)

Generally it seems like hostSupports and other module/package API functions are used not by containerd itself/internally but only by other applications which use containerd, like Docker, to load/control modules, right? Changes to an API hence should be done with care to not break things elsewhere without a major version increment and release note. But, the hostSupports function seems to be generic for all packages/modules to tell API users whether it is supported or not and hence whether it can be loaded, including usage of the other functions it provides, right? So in this case, since using the AppArmor package/module implies the ability to use apparmor_parser, hostSupports now seems to be able to simply tell the wrong thing without checking whether this tool is actually available. The previous behaviour looks pretty correct, since both, the kernel feature as well as the userland tool are required to use this pkg/module/API.

The million dollar question is what does hostSupports mean. Does it mean "we can use AppArmor profiles (aka the parser + kernel are both present)," or does it mean "kernel is aware of AppArmor?"

If containerd depends on apparmor_parser for anything, it seems like this change should be reverted and the original meaning of the function kept. If containerd needs to only check that the kernel supports AppArmor in other places, it seems like a new check should be added that is much more narrowly scoped. That could reduce containerd's dependence on apparmor_parser to only the places where it is truly needed.