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

Fix apparmor profile to work with COS Linux used by GKE #2541

Merged
merged 8 commits into from
Nov 11, 2024
Merged

Fix apparmor profile to work with COS Linux used by GKE #2541

merged 8 commits into from
Nov 11, 2024

Conversation

ccojocar
Copy link
Contributor

@ccojocar ccojocar commented Nov 8, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

These are a number of fixes required in order to get working the apparmor profile recording and installation with COS Linux used by GKE.

The fixes include:

  • Update the options of go-apparmor package to assume that the container runs into the host pid namespace. Without this, the go-apparmor package
    will try to auto-detect if a container runs into the host namespace but this will require the Linux kernel to be built with CONFIG_SCHED_DEBUG.
    This is not the case for COS Linux used by the GKE. This check is not required in a Kubernetes environment since typically the containers run
    into the Host PID namespace. For more details see the fix in Add an option to skip the check that a container runs into the host PID namespace pjbgf/go-apparmor#30.
  • Filter out the invalid file paths such as (deleted) returned by the Linux kernel. These should not land into the apparmor profile.

Which issue(s) this PR fixes:

fixes #2462

Does this PR have test?

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

saschagrunert and others added 4 commits November 5, 2024 10:26
@saschagrunert
Update runc to v1.2.1 and crun to v1.18.2
9605eca 
Signed-off-by: Sascha Grunert <[email protected]>
@ccojocar
Filter out invalid file path which shouldn't land into the apparmor p…
05a2def 
…rofile

Change-Id: I14fbf59d58d7617386578a3bb410dfe3fd0d492f
Signed-off-by: Cosmin Cojocar <[email protected]>
@ccojocar
Update go-apparmor to main version
e92593e 
Update the go-apparmor to main version to include the fix
pjbgf/go-apparmor#30

Change-Id: I45997ac722b830b9589751db034f9e89ba8526e4
Signed-off-by: Cosmin Cojocar <[email protected]>
@ccojocar
Update the apparmor builder options to include the logger and to assu…
4810720 
…me the host pid namespace

This assumes that the container runs into the host pid namespace, which
is typically the case in Kubernetes. Otherwise the go-apparmor will
auto-detect this and that check will require that the Linux kernel was
compiled with CONFIG_SCHED_DEBUG. Disabiling this check will ensure that
the apparmor works with Linux distributions which don't have this kernel
option active such as COS used by GKE.

Change-Id: I1435b63d2f9c5b8d8f527ef1d77dcc2b9cb74bc9
Signed-off-by: Cosmin Cojocar <[email protected]>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 8, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 8, 2024
@ccojocar
Fix typo
3e29815 
Change-Id: Ie4f329ea92c2548266311d500d553ccb22537d8e
Signed-off-by: Cosmin Cojocar <[email protected]>
@ccojocar ccojocar requested review from saschagrunert and removed request for jhrozek and Vincent056 November 8, 2024 11:41
@ccojocar
Copy link
Contributor Author

ccojocar commented Nov 8, 2024

/test all

1 similar comment
@ccojocar
Copy link
Contributor Author

ccojocar commented Nov 8, 2024

/test all

@ccojocar
Use the exist? method in Vagrantfile to avoid undifined method exists?
4a5c147 
Change-Id: I0181d6fd17ecae835c2ec2dbf1971b6eda87bdaf
Signed-off-by: Cosmin Cojocar <[email protected]>
@ccojocar
Copy link
Contributor Author

@saschagrunert I fixed also the vagrant issue in this pull request.

@saschagrunert
Copy link
Member

@saschagrunert I fixed also the vagrant issue in this pull request.

That's awesome, thank you!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2024
@saschagrunert
Copy link
Member

@ccojocar we have to update runc/crun in CI, but I can follow-up on that in #2535

@ccojocar
Update runc and crun versions in the examples
870372f 
Change-Id: I14955f4a2568babe8f24c5a3664f0a26c34fc02c
Signed-off-by: Cosmin Cojocar <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2024
@ccojocar
Copy link
Contributor Author

we have to update runc/crun in CI, but I can follow-up on that in #2535

I just bumped the versions in the examples to get the test passing. Should I merge your branch into this?

@saschagrunert
Copy link
Member

we have to update runc/crun in CI, but I can follow-up on that in #2535

I just bumped the versions in the examples to get the test passing. Should I merge your branch into this?

Yes this would work as well.

@ccojocar
Merge branch 'runc-crun' into apparmor-cos
a6f8521 
Change-Id: I1bf38a8e11e3603ab24370fac819889e7fb4290d
@k8s-ci-robot
Copy link
Contributor

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 11, 2024
@ccojocar
Copy link
Contributor Author

Yes this would work as well.

I merged your branch. Hopefully now it will pass.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ccojocar, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ccojocar,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5d3e339 into main Nov 11, 2024
28 checks passed
@k8s-ci-robot k8s-ci-robot deleted the apparmor-cos branch November 11, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

Assignees

@saschagrunert saschagrunert

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AppArmor Profile Loading on GKE COS failed with "open /proc/1/sched: no such file or directory"
3 participants
@ccojocar @saschagrunert @k8s-ci-robot