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.

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

Fix AppArmor profile Apply() function to correctly handle an "Unconfined" mode #8103

Conversation

roman-kiselenko
Copy link
Member

@roman-kiselenko roman-kiselenko commented May 1, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

When applying a given AppArmor profile, the Apply() function should correctly handle the "Unconfined" profile type for both fields, the Apparmor (which is a SecurityProfile type) and ApparmorProfile (a bare string), of the LinuxContainerSecurityContext type.

Related:

Which issue(s) this PR fixes:

Fixes #8080.

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

None

Fix AppArmour profile Apply() function to correctly handle an "Unconfined" mode.

@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. labels May 1, 2024
@openshift-ci openshift-ci bot requested review from klihub and wgahnagl May 1, 2024 13:21
@roman-kiselenko
Copy link
Member Author

@kwilczynski
Copy link
Member

/cc @saschagrunert @kwilczynski @haircommander

@roman-kiselenko, feel free to use @cri-o/cri-o-maintainers handle here. It casts a wider net of maintainers.

@kwilczynski kwilczynski changed the title Apply() function doesn't seem to handle the case for "Unconfined" mode Fix AppArmour profile apply function to correctly handle an "Unconfined" mode May 1, 2024
@kwilczynski kwilczynski changed the title Fix AppArmour profile apply function to correctly handle an "Unconfined" mode Fix AppArmor profile apply function to correctly handle an "Unconfined" mode May 1, 2024
@kwilczynski kwilczynski changed the title Fix AppArmor profile apply function to correctly handle an "Unconfined" mode Fix AppArmor profile Apply() function to correctly handle an "Unconfined" mode May 1, 2024
@kwilczynski
Copy link
Member

@roman-kiselenko, I've made some changes to the subject and description (feel free to edit this one as needed).

The commit subjects should be as imperative as possible, within reason. That said, I cannot adjust the commit subject line itself, so this is something you could do, if you want and have a moment.

Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.70%. Comparing base (8b966a8) to head (d914961).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8103      +/-   ##
==========================================
+ Coverage   49.67%   49.70%   +0.02%     
==========================================
  Files         153      153              
  Lines       16826    16827       +1     
==========================================
+ Hits         8359     8364       +5     
+ Misses       7423     7420       -3     
+ Partials     1044     1043       -1     

@kwilczynski
Copy link
Member

/hold

Waiting for the commit message to be updated.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2024
@roman-kiselenko roman-kiselenko force-pushed the bugfix/apparmor-profile-unconfined branch from 07670a1 to e724439 Compare May 2, 2024 08:09
@kwilczynski
Copy link
Member

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2024
@kwilczynski
Copy link
Member

/retest

@roman-kiselenko
Copy link
Member Author

/hold

I'm going to rebase after merge

#7881

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2024
@roman-kiselenko
Copy link
Member Author

/unhold

There is merged v1.30 version #8094

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024
…ined" mode.

Signed-off-by: roman-kiselenko <roman.kiselenko.dev@gmail.com>
@roman-kiselenko roman-kiselenko force-pushed the bugfix/apparmor-profile-unconfined branch from e724439 to d914961 Compare May 2, 2024 16:12
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024
@haircommander
Copy link
Member

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2024
Copy link
Contributor

openshift-ci bot commented May 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, roman-kiselenko

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2024
@haircommander haircommander removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2024
@haircommander haircommander added this to the 1.30 milestone May 2, 2024
@haircommander
Copy link
Member

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit c4a0387 into cri-o:main May 2, 2024
71 checks passed
@jadedeane
Copy link

jadedeane commented May 8, 2024

Despite seemingly correct annotations, still seeing 'unconfined' return:

empty localhost AppArmor profile is forbidden

@kwilczynski
Copy link
Member

@jadedeane, please reply to and even possibly re-open the following:

@roman-kiselenko roman-kiselenko deleted the bugfix/apparmor-profile-unconfined branch May 8, 2024 05:53
@jadedeane
Copy link

@kwilczynski apologies, sorted moving up to 1.31.0~dev-2.1.

@kwilczynski
Copy link
Member

@jadedeane, no worries.

So, just to confirm: you are all good here? Everything works as expected?

@jadedeane
Copy link

@jadedeane, no worries.

So, just to confirm: you are all good here? Everything works as expected?

Yes, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to run pods with apparmor type "Unconfined"
5 participants