Seccomp GA #1747

pjbgf · 2020-05-07T12:00:46Z

Amendments around RuntimeProfile and LocalhostProfile on Tim's initial KEP. The goal remains unchanged: to do the bare minimum to clean up the feature, without blocking future enhancements.

/sig-node
/sig-auth

/priority important-longterm

/assign @liggitt @dchen1107 @derekwaynecarr @tallclair
/cc @jessfraz

saschagrunert

Looks pretty good and I like the deprecation of the docker/default profile.
/lgtm

keps/sig-node/20190717-seccomp-ga.md

tallclair

Thanks for working on this!

keps/sig-node/20190717-seccomp-ga.md

evrardjp

I see this as a positive improvement.

I agree with @tallclair on the need to clarify the LocalhostProfile section, for posterity.

I am looking forward to see more eyes on the Localhost Profile Validation section too.

keps/sig-node/20190717-seccomp-ga.md

pjbgf · 2020-05-12T21:21:19Z

@palnabarun I have added the PRR as you asked. There are a few points TBC, but the majority should be there now.

keps/sig-node/20190717-seccomp-ga.md

palnabarun · 2020-05-15T12:09:54Z

@johnbelamaric Can you please have a look at the PRR questions once? 🙂

tallclair

Added a few clarifications, but this basically lgtm

keps/sig-node/20190717-seccomp-ga.md

tallclair · 2020-05-18T23:58:30Z

/lgtm
/approve

/hold
For @liggitt 's review of the tightening validation approach.

tallclair · 2020-05-18T23:59:13Z

Also @derekwaynecarr for KEP approval.

keps/sig-node/20190717-seccomp-ga.md

liggitt · 2020-05-19T14:58:53Z

keps/sig-node/20190717-seccomp-ga.md

+    // LocalhostProfile must be 100 characters or less, beginning and ending with an alphanumeric 
+    // character ([a-z0-9A-Z]). It may include forward slashes (/), underscores (_), dashes (-) and dots (.) 
+    // between alphanumeric characters.


this is more restrictive than the current annotation... what happens to annotations that do not fit inside these restrictions?

the annotation validation also disallows .. path segments, and does not allow starting with a /

how was the 100 character limit selected?

The character limit selected was mostly to decrease the likelihood of going over the max file path in some systems. After reviewing this point, I realised that the limitation on most FSs is around file name length, which would fit the current rules.

I will revert this, to keep the existing validation and simply formalise it on the documentation.

keps/sig-node/20190717-seccomp-ga.md

liggitt · 2020-05-19T15:24:38Z

keps/sig-node/20190717-seccomp-ga.md

 migrated, the same warning annotation will be added to the controller as for pods:

 ```
-warning.kubernetes.io/seccomp: "Seccomp set through annotations. Support will be dropped in v1.22"
+warning.kubernetes.io/seccomp: "Seccomp set through annotations. Support will be dropped in v1.23"


This is a bit unusual... I'm not sure we want to annotate like this. There's not a great central place to sweep all workload objects that contain pod templates to get them to mutate this correctly.

Would we warn if a workload object had both the annotation and the seccomp field set?

If an update dropped the seccomp annotation, would we clear the warning?

Would tying this to an actual warning make more sense once that mechanism is available in 1.19?

That is reasonable. I have added a reference to the warning mechanism.

Given that today is KEP freeze, and this is the only open point, I wonder whether it would be OK to leave as is (with the reference) and I will follow up with a new PR with the details around the warning?

Reword this to commit to avoid committing to a specific warning method. You can mention possible mechanisms like a metric, an audit annotation, an annotation on the object, an event, or a warning as described in KEP 1693

Added reference to the different mechanisms and removed the previous example.

keps/sig-node/20190717-seccomp-ga.md

derekwaynecarr

I would like to clarify that the flag @liggitt noted is also promoted to GA.

derekwaynecarr · 2020-05-19T19:41:07Z

keps/sig-node/20190717-seccomp-ga.md

+Promoting LocalhostProfile to GA signals to the community that this is a feature they can use and rely on, 
+until a better option materializes. If and when that happens, a new KEP will be created accordingly.


since we are trying to do a kep per feature, any enhancement post GA is new feature and new kep, so agree that we dont need to promise future enhancements.

derekwaynecarr · 2020-05-19T19:52:53Z

keps/sig-node/20190717-seccomp-ga.md

 ## Alternatives

+### Localhost profiles
 The localhost feature currently depends on an alpha Kubelet flag. We could therefore label the


i would like this clarified as well.

the kubelet uses this to build a path like localhost/<some-dir>/<name> when passing the seccompfile path down to sandbox creation flows.

pjbgf · 2020-05-19T20:59:00Z

Covered all the open points and squashed the commits. PTAL @tallclair @derekwaynecarr @liggitt

pjbgf · 2020-05-19T21:20:37Z

keps/sig-node/20190717-seccomp-ga.md

+### Seccomp root path configuration
+
+The existing kubelet (alpha) flag `--seccomp-profile-root` allows for seccomp root path configuration.
+This flag will be marked as deprecated as of v1.19, and will be removed on v1.23.
+The seccomp root path will then be derived from the kubelet root path, which is defined by `--root-dir`.
+


@derekwaynecarr @liggitt @tallclair this is the new paragraph on '--seccomp-profile-root' being deprecated.

nit: note that the default value for the --seccomp-profile-root is <root-dir>/seccomp. So the proposal is to make the default behavior the only behavior.

Added that information to the same section.

I am happy with that.

tallclair · 2020-05-19T21:58:29Z

Spoke with @liggitt offline, and he convinced me that we should drop the PodSecurityPolicy API changes for now. Basically, the PSP implementation needs to be updated to enforce the policy on the new pod fields, but the API on the PSP will still be through annotations. If PSP sticks around, we can always add the API back later. Rather than deleting everything we have about the PSP API, can you just move it to the "alternatives considered" section?

pjbgf · 2020-05-19T22:01:57Z

@tallclair okie dokie, making the changes now.

pjbgf · 2020-05-19T22:13:56Z

@liggitt @tallclair moved the PSP API changes to alternatives.

liggitt · 2020-05-19T22:17:15Z

/lgtm

derekwaynecarr

/approve
/lgtm

k8s-ci-robot · 2020-05-19T22:24:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, pjbgf, 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:

~~keps/sig-node/OWNERS~~ [derekwaynecarr]

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

keps/sig-node/20190717-seccomp-ga.md

pjbgf · 2020-05-19T22:43:08Z

Based on feedback I marked it as implementable.

GA flag. Update validation. Update warnings. Move PSP API to alternatives

tallclair · 2020-05-19T22:50:10Z

/lgtm

I think @derekwaynecarr gets the final say as sig-node lead.

liggitt · 2020-05-20T01:45:59Z

/hold cancel

k8s-ci-robot assigned dchen1107 and derekwaynecarr May 7, 2020

k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label May 7, 2020

k8s-ci-robot assigned liggitt and tallclair May 7, 2020

k8s-ci-robot requested a review from jessfraz May 7, 2020 12:00

saschagrunert approved these changes May 7, 2020

View reviewed changes

keps/sig-node/20190717-seccomp-ga.md Outdated Show resolved Hide resolved

k8s-ci-robot assigned saschagrunert May 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2020

tallclair reviewed May 7, 2020

View reviewed changes

k8s-ci-robot requested a review from liggitt May 7, 2020 20:03

evrardjp reviewed May 8, 2020

View reviewed changes

keps/sig-node/20190717-seccomp-ga.md Outdated Show resolved Hide resolved

keps/sig-node/20190717-seccomp-ga.md Outdated Show resolved Hide resolved

tallclair mentioned this pull request May 9, 2020

Seccomp #135

Closed

22 tasks

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2020

pjbgf requested a review from tallclair May 10, 2020 07:09

tallclair reviewed May 14, 2020

View reviewed changes

keps/sig-node/20190717-seccomp-ga.md Show resolved Hide resolved

pjbgf commented May 14, 2020

View reviewed changes

keps/sig-node/20190717-seccomp-ga.md Show resolved Hide resolved

tallclair reviewed May 15, 2020

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 18, 2020

liggitt reviewed May 19, 2020

View reviewed changes

derekwaynecarr requested changes May 19, 2020

View reviewed changes

pjbgf requested review from derekwaynecarr, liggitt and tallclair May 19, 2020 21:00

pjbgf commented May 19, 2020

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2020

derekwaynecarr approved these changes May 19, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2020

tallclair reviewed May 19, 2020

View reviewed changes

keps/sig-node/20190717-seccomp-ga.md Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2020

pjbgf added 2 commits May 19, 2020 23:45

Add resolution for localhost, runtime profiles and PRR.

efc6554

GA flag. Update validation. Update warnings. Move PSP API to alternatives

Mark as implementable

ebcd355

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2020

k8s-ci-robot merged commit dc72918 into kubernetes:master May 20, 2020

k8s-ci-robot added this to the v1.19 milestone May 20, 2020

pjbgf deleted the seccomp-ga branch May 20, 2020 05:14

pjbgf mentioned this pull request May 20, 2020

KEP for promoting AppArmor to GA #1444

Closed

pjbgf mentioned this pull request Jul 6, 2020

Add documentation for Seccomp GA kubernetes/website#21278

Merged

4 tasks

		Promoting LocalhostProfile to GA signals to the community that this is a feature they can use and rely on,
		until a better option materializes. If and when that happens, a new KEP will be created accordingly.

Seccomp GA #1747

Seccomp GA #1747

Uh oh!

Conversation

pjbgf commented May 7, 2020

Uh oh!

saschagrunert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evrardjp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pjbgf commented May 12, 2020

Uh oh!

Uh oh!

Uh oh!

palnabarun commented May 15, 2020

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tallclair commented May 18, 2020

Uh oh!

tallclair commented May 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

derekwaynecarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjbgf commented May 19, 2020

Uh oh!

Choose a reason for hiding this comment