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

AppArmor fields API #123435

Merged
merged 12 commits into from Mar 6, 2024
Merged

AppArmor fields API #123435

merged 12 commits into from Mar 6, 2024

Conversation

tallclair
Copy link
Member

@tallclair tallclair commented Feb 22, 2024

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Implement the API outlined in KEP-24: AppArmor, which converts the AppArmor annotations to SecurityContext fields.

Which issue(s) this PR fixes:

For kubernetes/enhancements#24

Special notes for your reviewer:

A bunch of this code was copied / adapted from the similar set of changes to migrate Seccomp annotations to fields:

  1. seccomp GA - Add new seccomp fields and update kubelet to use them #91381
  2. Add seccomp GA version skew for pods #91408

This PR does not include the following, which will be addressed in a follow up:

  • Warnings on deprecated annotation usage
  • Feature flag flip

Also, I decided to make a slight change to the Kubelet static pod behavior from what is described in https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/24-apparmor/README.md#kubelet-fallback: Rather than applying the annotation/field sync to static pods, the Kubelet just falls back to the annotation value when computing the AppArmor profile for a container.

Does this PR introduce a user-facing change?

- AppArmor profiles can now be configured through fields on the PodSecurityContext and container SecurityContext.
- The beta AppArmor annotations are deprecated.
- AppArmor status is no longer included in the node ready condition

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/24-apparmor/

/sig node
/assign @liggitt @dchen1107 @SergeyKanzhelev

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 22, 2024
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 22, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 22, 2024
@k8s-ci-robot k8s-ci-robot added area/apiserver area/code-generation area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubelet area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 22, 2024
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@alexzielenski
Copy link
Contributor

/remove-sig api-machinery
/sig node

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 22, 2024
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 22, 2024
@liggitt
Copy link
Member

liggitt commented Feb 22, 2024

/label api-review

@dchen1107
Copy link
Member

/test pull-kubernetes-e2e-gce

Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

gate changes in the APIserver look good, one question on the kubelet side

e2e test failure is related:

failed [FAILED] Error creating Pod: Pod "test-apparmor-h56bd" is invalid: metadata.annotations[container.apparmor.security.beta.kubernetes.io/test]: Invalid value: "container.apparmor.security.beta.kubernetes.io/e2e-apparmor-test-apparmor-6537": invalid AppArmor profile name: "container.apparmor.security.beta.kubernetes.io/e2e-apparmor-test-apparmor-6537"

Comment on lines +57 to +59
if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmorFields) {
return getProfileFromPodAnnotations(pod.Annotations, container.Name)
}
Copy link
Member

Choose a reason for hiding this comment

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

if the gate controls whether we let data into the field, and make the kubelet trigger off of presence of the field alone, would that work? we get control over the field via the gate in kube-apiserver, and skewed kubelets honor the field starting with 1.30

Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed for static pods. Probably not necessary, but seems worth including for completeness.

@MaryamTavakkoli
Copy link

/milestone v1.30

@k8s-ci-robot
Copy link
Contributor

@MaryamTavakkoli: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.30

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.

@tallclair
Copy link
Member Author

/milestone v1.30

@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Mar 6, 2024
@tallclair
Copy link
Member Author

My IDE ran into a bunch of problems when I was doing the annotation rename, and missed a bunch of cases. When I went to manually clean it up, I accidentally mixed up the container key prefix & localhost value prefix, but didn't realize until I'd already squashed the commits. What a mess! I think it should be all fixed up now (including the failing e2e test).

@tallclair
Copy link
Member Author

/retest

@liggitt
Copy link
Member

liggitt commented Mar 6, 2024

/lgtm
/approve

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

LGTM label has been added.

Git tree hash: badfa862d5104905d8f3788cba76657267844663

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tallclair

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2024
@k8s-ci-robot k8s-ci-robot merged commit bd25605 into kubernetes:master Mar 6, 2024
16 checks passed
SIG Node CI/Test Board automation moved this from Archive-it to Done Mar 6, 2024
SIG Node PR Triage automation moved this from Needs Reviewer to Done Mar 6, 2024
@alculquicondor
Copy link
Member

The beta AppArmor annotations are deprecated.

@tallclair was it marked as deprecated or fully removed?

We got a report of someone running into problems where the apiserver would (I think) try to convert the annotation into a pod security context:

Forbidden: pod updates may not change fields other than `spec.containers[*].image`,`spec.initContainers[*].image`,`spec.activeDeadlineSeconds`,`spec.tolerations` (only additions to existing tolerations),`spec.terminationGracePeriodSecon
ds` (allow it to be set to 1 if it was previously negative)

Is this a bug or should it be documented as ACTION REQUIRED?

@liggitt
Copy link
Member

liggitt commented Apr 24, 2024

The AppArmor annotation → pod security context translation only happens on pod create, not on update.

I suspect someone updating the pod is doing a read / dropping the apparmor fields / update and so the server is preventing them from modifying the pod.

I would strongly recommend they use a patch to modify just the scheduling gate field on update.

@alculquicondor
Copy link
Member

Oh ok, so that's because we are using the 1.28 libraries. But yeah, I agree that patch/apply would be more reliable. Let me follow up on that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/code-generation area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.30
Archived in project
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants