Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Conversation

@miaoyq
Copy link
Member

@miaoyq miaoyq commented Oct 3, 2017

Fixes #314
/cc @Random-Liu

Signed-off-by: Yanqiang Miao [email protected]

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

"should not return error if apparmor is unconfined when apparmor is not supported": {
profile: unconfinedProfile,
disable: true,
expectErr: true,
Copy link
Member

Choose a reason for hiding this comment

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

The text is correct but the expectErr should be false here..

Copy link
Member

Choose a reason for hiding this comment

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

Agree.

case runtimeDefault:
// TODO (mikebrow): delete created apparmor default profile
return apparmor.WithDefaultProfile(appArmorDefaultProfileName), nil
// TODO(random-liu): Should support "unconfined" after kubernetes#52395 lands.
Copy link
Member

Choose a reason for hiding this comment

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

unconfined means not applying any profile.
Please reference the implementation in seccomp, and do similar thing.

Copy link
Member

Choose a reason for hiding this comment

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

agree...

disable: true,
expectErr: true,
},
"should set default apparmor when apparmor is unconfined": {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong assumption.

Copy link
Member

Choose a reason for hiding this comment

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

agree

"should not return error if apparmor is unconfined when apparmor is not supported": {
profile: unconfinedProfile,
disable: true,
expectErr: true,
Copy link
Member

Choose a reason for hiding this comment

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

Agree.

@Random-Liu
Copy link
Member

@miaoyq Can you also update corresponding CRI validation test in another PR? Thanks!

@Random-Liu
Copy link
Member

Random-Liu commented Oct 4, 2017

Closes #320.

Updating Kubernetes also fixes #320.

@miaoyq
Copy link
Member Author

miaoyq commented Oct 4, 2017

@Random-Liu @mikebrow Thanks for review, I have not fully understand the meaning of unconfined.
Have addressed all comments, PTAL.

@miaoyq
Copy link
Member Author

miaoyq commented Oct 4, 2017

@miaoyq Can you also update corresponding CRI validation test in another PR? Thanks!

@Random-Liu OK

Signed-off-by: Yanqiang Miao <[email protected]>
@Random-Liu
Copy link
Member

Random-Liu commented Oct 4, 2017

LGTM

/cc @mikebrow

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

@Random-Liu Random-Liu merged commit 23b8330 into containerd:master Oct 4, 2017
@miaoyq
Copy link
Member Author

miaoyq commented Oct 6, 2017

@miaoyq Can you also update corresponding CRI validation test in another PR? Thanks!

@Random-Liu You have finished this in #329
I didn't seem to get what you means, I have thought that you made me to update corresponding CRI validation test in cri-tools 😓

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants