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

Add AppArmor validation logic #29812

Merged
merged 1 commit into from Aug 11, 2016

Conversation

timstclair
Copy link

@timstclair timstclair commented Jul 30, 2016

The validation checks the prerequisites described in the AppArmor proposal.

In order to unblock the AppArmor implementation from waiting on the APIs to merge, this PR uses 2 helper stubs for handling the Pod API.


This change is Reviewable

@timstclair timstclair added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 30, 2016
@timstclair timstclair added this to the v1.4 milestone Jul 30, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 30, 2016
@timstclair
Copy link
Author

Rebased and updated boilerplate (again!).

@timstclair
Copy link
Author

Reassigning review to Dawn since I'm blocked on this.

@timstclair timstclair assigned dchen1107 and unassigned jessfraz Aug 3, 2016
@timstclair
Copy link
Author

/cc @Amey-D

return fmt.Errorf("could not read loaded profiles: %v", err)
}

for _, container := range pod.Spec.Containers {
Copy link
Member

Choose a reason for hiding this comment

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

If pod has initContainers, should we validate its profile too?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Done.

@timstclair
Copy link
Author

@dchen1107 ping :)

return nil
}

func (v *validator) getLoadedProfiles() (map[string]bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if the format here is guaranteed

Copy link
Author

Choose a reason for hiding this comment

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

I looked into it a bit, and couldn't find the profiles format documented anywhere (AppArmor documentation seems scarce), but I did find the kernel implementation, and the parsing done by the aa-status tool. I updated the parsing to be a bit more precise.

@timstclair
Copy link
Author

Thanks @jfrazelle

@jessfraz
Copy link
Contributor

LGTM

@jessfraz jessfraz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2016
@dchen1107
Copy link
Member

LGTM overall except one comment which should be addressed through doc, and outside this pr:

Based on current proposed design, app profile is opaque to Kubernetes, including kubelet, which is good. But ubuntu apparmor has more features (more "hooks") than the upstream one which we are validating today through GCI image. But if the customers bring apparmor profiles from their ubuntu installations, in which case the profiles may not work correctly on GKE/GCI. This is a portability issue, and we should document it.

Please squash the commits, we are ready to go.

@dchen1107 dchen1107 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2016
@dchen1107
Copy link
Member

@jfrazelle Thanks for reviewing this. I temporarily removed lgtm label so that @timstclair can have enough time to squash his commits. :-) Tim, please feel free to readd the label once you are done with squash.

@timstclair
Copy link
Author

Squashed. Thanks all!

@timstclair timstclair added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2016
@jessfraz
Copy link
Contributor

oh so sorry!!

On Wed, Aug 10, 2016 at 3:08 PM, Dawn Chen notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle Thanks for reviewing this. I
temporarily removed lgtm label so that @timstclair
https://github.com/timstclair can have enough time to squash his
commits. :-) Tim, please feel free to readd the label once you are done
with squash.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29812 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABYNbEa2ZJvkOHzkR8OTyqYTUfsumdozks5qekvtgaJpZM4JYrDW
.

@timstclair
Copy link
Author

Rebased.

@timstclair timstclair added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 11, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 11, 2016

GCE e2e build/test passed for commit bdc306b.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 11, 2016

GCE e2e build/test passed for commit bdc306b.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 2c28b88 into kubernetes:master Aug 11, 2016
k8s-github-robot pushed a commit that referenced this pull request Aug 16, 2016
Automatic merge from submit-queue

Implement AppArmor Kubelet support

Includes PR #29812

Implements the Kubelet logic for AppArmor based on the alpha API proposed [here](https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/apparmor.md). Also adds an E2E test, and I ran manual tests.

Remaining work: PodSecurityPolicy support, profile loader daemon, documentation, (maybe) beta API.

/cc @jfrazelle @Amey-D @kubernetes/sig-node 

*Note on release-note-none: I am implementing AppArmor over multiple PRs. I will submit a single release note once the implementation is done to cover all of them.*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants