[release/1.6 backport] Revert apparmor_parser regression
#8087
Conversation
This reverts commit 1acca8b. As stated in the Godoc, this function is intended to check for presence of `apparmor_parser`. Changing this regressed the public API of containerd, and directly contradicts the way that this function is consumed inside of containerd itself: * https://github.com/containerd/containerd/blob/fdfdc9bfc0f865a43c88171110615d1510fad3bc/pkg/apparmor/apparmor.go#L20 * https://github.com/containerd/containerd/blob/fdfdc9bfc0f865a43c88171110615d1510fad3bc/pkg/cri/sbserver/helpers_linux.go#L85 * https://github.com/containerd/containerd/blob/fdfdc9bfc0f865a43c88171110615d1510fad3bc/pkg/cri/server/helpers_linux.go#L144 This has lead to a number of painful regressions and attempted fixes in Moby: * moby/moby#44900 * moby/moby#44902 * moby/moby#44970 While reverting this late into the life of 1.6 and at the start of the life of 1.7 is likely painful, I think this is ultimately the best path to take, as containerd is subject to the same failure to start containers with an AppArmor kernel when `apparmor_parser` is missing as Moby. Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com> (cherry picked from commit a326510) Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com>
Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com> (cherry picked from commit d33a43c) Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com>
|
Hi @neersighted. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
neersighted
changed the title
apparmor_parser regression
|
/ok-to-test |
containerd 1.6.18 Welcome to the v1.6.18 release of containerd! The eighteenth patch release for containerd 1.6 includes fixes for CVE-2023-25153 and CVE-2023-25173 along with a security update for Go. * **Fix OCI image importer memory exhaustion** ([GHSA-259w-8hf6-59c2](GHSA-259w-8hf6-59c2)) * **Fix supplementary groups not being set up properly** ([GHSA-hmfx-3pcx-653p](GHSA-hmfx-3pcx-653p)) * **Revert removal of `/sbin/apparmor_parser` check** ([#8087](containerd/containerd#8087)) * **Update Go to 1.19.6** ([#8111](containerd/containerd#8111)) See the changelog for complete list of changes Please try out the release binaries and report any issues at https://github.com/containerd/containerd/issues. * Akihiro Suda * Derek McGowan * Ye Sijun * Samuel Karp * Bjorn Neergaard * Wei Fu * Brian Goff * Iceber Gu * Kazuyoshi Kato * Phil Estes * Swagat Bora <details><summary>24 commits</summary> <p> * [release/1.6] Prepare release notes for v1.6.18 ([#8118](containerd/containerd#8118)) * [`44e61d764`](containerd/containerd@44e61d7) Add release notes for v1.6.18 * Github Security Advisory [GHSA-hmfx-3pcx-653p](GHSA-hmfx-3pcx-653p) * [`286a01f35`](containerd/containerd@286a01f) oci: fix additional GIDs * [`301823453`](containerd/containerd@3018234) oci: fix loop iterator aliasing * [`0070ab70f`](containerd/containerd@0070ab7) oci: skip checking gid for WithAppendAdditionalGroups * [`16d52de64`](containerd/containerd@16d52de) refactor: reduce duplicate code * [`b45e30292`](containerd/containerd@b45e302) add WithAdditionalGIDs test * [`0a06c284a`](containerd/containerd@0a06c28) add WithAppendAdditionalGroups helper * Github Security Advisory [GHSA-259w-8hf6-59c2](GHSA-259w-8hf6-59c2) * [`84936fd1f`](containerd/containerd@84936fd) importer: stream oci-layout and manifest.json * [1.6] Add fallback for windows platforms without osversion ([#8106](containerd/containerd#8106)) * [`b327af6a4`](containerd/containerd@b327af6) Add fallback for windows platforms without osversion * [release/1.6] Go 1.19.6 ([#8111](containerd/containerd#8111)) * [`54ead5b7b`](containerd/containerd@54ead5b) Go 1.19.6 * [release/1.6] ctr/run: flags --detach and --rm cannot be specified together ([#8094](containerd/containerd#8094)) * [`2b4b35ab4`](containerd/containerd@2b4b35a) ctr/run: flags --detach and --rm cannot be specified together * [release/1.6] Fix retry logic within devmapper device deactivation ([#8088](containerd/containerd#8088)) * [`d5284157b`](containerd/containerd@d528415) Fix retry logic within devmapper device deactivation * [release/1.6 backport] Revert `apparmor_parser` regression ([#8087](containerd/containerd#8087)) * [`624ff636b`](containerd/containerd@624ff63) pkg/apparmor: clarify Godoc * [`3a0a35b36`](containerd/containerd@3a0a35b) Revert "Don't check for apparmor_parser to be present" * [release/1.6] CI: skip some jobs when `repo != containerd/containerd` ([#8083](containerd/containerd#8083)) * [`664a938a3`](containerd/containerd@664a938) CI: skip some jobs when `repo != containerd/containerd` </p> </details> This release has no dependency changes Previous release can be found at [v1.6.17](https://github.com/containerd/containerd/releases/tag/v1.6.17)
|
There are many issues and commits for this apparmor regression. Is this regression really fully resolved? My 'apt full-upgrade' upgraded this: Log: My workaround was to 'apt install apparmor' + reboot. |
|
This pull request is not in a tagged version yet. |
|
Ah! Thanks! Sorry! PS: |
|
Does "official Debian repos" mean the Docker CE repos, or the ones provided by Debian themselves? In either case, this is a change in containerd and not Moby (Docker) as shown in your logs. The fix is in a tagged version of containerd (so the version of this issue in containerd is fixed); but in Moby, we use containerd as a library. You can click on the linked pull request in moby/moby: moby/moby#44982 You can look at the backport milestone and see In short, we're preparing another release; if you use Docker CE, it should show up in Docker CE 23.0.2. If you are depending on the official Debian repos (which from your comment, you are not), you are at the mercy of Debian to release new versions or backport fixes (but again, that appears not to apply here). |
|
Thanks! Ah, yes, the Debian machine has an apt-source to download.docker.com. |
flannel was failing to start on sandbox nodes, causing the node to be ina NotReady state because networking was not ready. The pod description had this event: "Error: failed to create containerd container: get apparmor_parser version: exec: "apparmor_parser": executable file not found in $PATH" This appears to be related to some changes going on in containerd: containerd/containerd#8087
* Replaces old epoxy extension services with new server The token-server and bmc-store-password ePoxy extensions are now replaced by a new ePoxy "extension server." Instead of individual extension container images, they are now all combined into a single binary and container image that listens on a single port. * Uses date string versions for images not in mlab-oti See long comment in change set for details * Deploys a single ePoxy extension server Previously there were separate token-server and bmc-store-password containers and systemd units. These have now been combined into a single extension server that listens on a single port. * Fixes nameing violation for virtual images * Installs apparmor package flannel was failing to start on sandbox nodes, causing the node to be ina NotReady state because networking was not ready. The pod description had this event: "Error: failed to create containerd container: get apparmor_parser version: exec: "apparmor_parser": executable file not found in $PATH" This appears to be related to some changes going on in containerd: containerd/containerd#8087 * Fixes a syntax error in mount-data-api.sh * Fixes ordering of control plane services The create-control-plane.service is supposed to run _after_ mount-data-api, but that ordering was broken because the name of the service changed and I failed to update the "After" block with the new name. * Don't fail the build when cluster vesion not available If the query to the live cluster for its version fails, then don't bother doing any version checking. The live cluster may not even exist, and possibly needs the images from this build so that it can be created. * Redundant check for working cluster before init Adds an additional, redundant check for the existence of /etc/kubernetes/admin.conf before initializing the cluster. A bug in our config caused the service unit to run even though that file existed, and kubeadm overwrote numerous things before finally erroring out. Can't hurt to add the additional check in this file. For nodes joining the cluster, wait for 90s (up from 60s) before trying to join to give the primary control plane node time to finish setting everything up. I discovered that 60s was not quite enough, and nodes joining the control plane might get a connection refused from the primary API endpoint. * Don't mkdir /etc/kubernetes/manifests on API machines On control plane machines, /etc/kubernetes is supposed to be a symlink to /mnt/cluster-data/kubernetes. When /etc/kubernetes already exists as a regular dir, then ln creates a symlink inside /etc/kubernetes, breaking the configuration and breakage of the create-control-plane service. Anyway, on control plane nodes that directory will be created automatically by kubeadm. * Makes setup_k8s.sh parse allocate_k8s_token v2 data ePoxy extension allocate_k8s_token V2 returns all the data needed to join the cluster. This commit removes all templating from setup_k8s.sh and moves it into the physical image filesystem. It is now a static script which can fetch everything it needs from allocate_k8s_token V2. * Makes setup_k8s.sh executable * Refactors the join-cluster.sh script Previously, the script assumed that all VMs were going to be part of a MIG. We have decided to have a hybrid approach with both MIGs and standard VMs, which required a few changes. Additionally, configure the script to the V2 allocate_k8s_token ePoxy extension, which returns all the data needed to join the cluster, not just the token. This also required some refactoring of the code.
apparmor_parserregression #8086This reverts commit 1acca8b.
As stated in the Godoc, this function is intended to check for presence of
apparmor_parser. Changing this regressed the public API of containerd, and directly contradicts the way that this function is consumed inside of containerd itself:containerd/pkg/apparmor/apparmor.go
Line 20 in fdfdc9b
containerd/pkg/cri/sbserver/helpers_linux.go
Line 85 in fdfdc9b
containerd/pkg/cri/server/helpers_linux.go
Line 144 in fdfdc9b
This has lead to a number of painful regressions and attempted fixes in Moby:
While reverting this late into the life of 1.6 and at the start of the life of 1.7 is likely painful, I think this is ultimately the best path to take, as containerd is subject to the same failure to start containers with an AppArmor kernel when
apparmor_parseris missing as Moby.Cherry-pick: #8086