Skip to content

Conversation

@hasheddan
Copy link
Contributor

@hasheddan hasheddan commented May 28, 2020

Signed-off-by: hasheddan [email protected]

This is adds documentation for the Seccomp GA KEP: kubernetes/enhancements#135

Tasks still to be completed:

  • Update /content/en/docs/concepts/security/pod-security-standards.md
  • Update /content/en/docs/tasks/configure-pod-container/security-context.md with a short section on setting seccompProfile fields
  • Update various examples through documentation that use seccomp annotations
  • Add full guide in /content/en/docs/tutorials/clusters/seccomp.md

/cc @saschagrunert @pjbgf @evrardjp

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 28, 2020
@k8s-ci-robot
Copy link
Contributor

@hasheddan: GitHub didn't allow me to request PR reviews from the following users: evrardjp.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Signed-off-by: hasheddan [email protected]

This is a WIP for adding documentation for the Seccomp GA KEP: kubernetes/enhancements#135

Tasks still to be completed:

  • Update /content/en/docs/concepts/security/pod-security-standards.md
  • Update /content/en/docs/tasks/configure-pod-container/security-context.md with a short section on setting seccompProfile fields
  • Update various examples through documentation that use seccomp annotations
  • Add full guide in /content/en/docs/tutorials/clusters/seccomp.md

/cc @saschagrunert @pjbgf @evrardjp

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels May 28, 2020
@netlify
Copy link

netlify bot commented May 28, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 3582c8d

https://deploy-preview-21278--kubernetes-io-master-staging.netlify.app

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

@hasheddan thanks for pushing ahead with the seccomp documentation. 👍

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Thank you for starting with this @hasheddan 🙏 !

@savitharaghunathan
Copy link
Member

Hi @hasheddan, Thank you for the PR. If this is a part of release 1.19, which I assume it is, can you open this PR against dev-1.19 branch, please? Thanks!

@hasheddan
Copy link
Contributor Author

@savitharaghunathan sure thing! Thanks for letting me know!

@hasheddan hasheddan changed the base branch from master to dev-1.19 June 1, 2020 13:10
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Jun 1, 2020

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 3ad7ea7

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5f15e80f1dd204000701b67b

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 1, 2020
@sftim
Copy link
Contributor

sftim commented Jun 2, 2020

/milestone 1.19
AIUI

@k8s-ci-robot k8s-ci-robot added this to the 1.19 milestone Jun 2, 2020
@sftim
Copy link
Contributor

sftim commented Jun 28, 2020

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jun 28, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Here's some early feedback.

As well as the suggested rewording, I have a question.

What happens to the --seccomp-profile-root command line setting to the kubelet? What if I set this to, eg, /run/foo in 1.19 - does that error out? Does the kubelet silently ignore the command line setting?

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 1, 2020
Comment on lines 619 to 622
`--seccomp-profile-root` flag on the Kubelet. (Note: this flag has been
deprecated in v1.19 and will be removed in v1.23. The seccomp root path will
then be derived from the kubelet root path, which is defined by `--root-dir`.
The current default value is `<root-dir>/seccomp`.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`--seccomp-profile-root` flag on the Kubelet. (Note: this flag has been
deprecated in v1.19 and will be removed in v1.23. The seccomp root path will
then be derived from the kubelet root path, which is defined by `--root-dir`.
The current default value is `<root-dir>/seccomp`.)
`--seccomp-profile-root` flag on the kubelet.
{{< note >}}
Kubernetes v1.19 deprecated the `--seccomp-profile-root` flag, which will be removed in
Kubernetes v1.23. The seccomp root path will then be derived from the kubelet root path,
which is defined by `--root-dir`. The current default value is `<root-dir>/seccomp`.
{{< /note >}}

or, with avoid statements about the future in mind:

Suggested change
`--seccomp-profile-root` flag on the Kubelet. (Note: this flag has been
deprecated in v1.19 and will be removed in v1.23. The seccomp root path will
then be derived from the kubelet root path, which is defined by `--root-dir`.
The current default value is `<root-dir>/seccomp`.)
the seccomp root path for that node. The default seccomp root path is a directory
named `seccomp` directly beneath the kubelet's root directory (specified via the
`--root-dir` command line flag to the kubelet).
{{< note >}}
The `--seccomp-profile-root` flag (deprecated since Kubernetes v1.19) allowed you to configure
the seccomp root path for a node. To use the defined behavior, leave that flag unset so that the
kubelet uses its default.
{{< /note >}}

Copy link
Contributor

Choose a reason for hiding this comment

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

@hasheddan , avoid statements/promises/predictions about the future.

---
reviewers:
- stclair
title: AppArmor
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider revising this title in the PR that introduces a Seccomp tutorial.

@hasheddan
Copy link
Contributor Author

@sftim how would opening a separate PR work with docs freeze? This PR was marked ready for review before the deadline but a new one would not have been.

@sftim
Copy link
Contributor

sftim commented Jul 14, 2020

@sftim how would opening a separate PR work with docs freeze?

@hasheddan the website itself uses a continuous deployment pipeline. To get your change into a release (v1.19) you must document it, and this documentation has an impending deadline for when your feature documentation must be ready. But, you can split this PR into a minimum-viable amount of documentation and a separate PR with the tutorial in it, both targeting dev-1.19.

Once #21278 (this PR) merges, and a putative separate PR for adding the seccomp tutorial has /lgtm, then that separate tutorial PR is good to merge into dev-1.19, ahead of the release. If the separate tutorial misses the opportunity to merge into the dev-1.19 branch you can change its merge target to master, after v1.19 is released, and it'll still be ready for approval, automated merge, and publishing.

Essentially: whilst it's nice to add that tutorial for using Seccomp, not adding that tutorial doesn't block or delay the v1.19 release.

@savitharaghunathan & @kubernetes/sig-docs-leads does that sound right? If I've got a detail wrong there please jump in.

@sftim sftim dismissed their stale review July 14, 2020 22:43

Discussed in comment history

## {{% heading "prerequisites" %}}

In order to complete all steps in this tutorial, you must install
[kind](https://kind.sigs.k8s.io/docs/user/quick-start/) and
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it straightforward to port the tutorial to use minikube?


## Create Pod with Seccomp Profile for Syscall Auditing

Next we want to apply our `audit.json` profile, which will log all syscalls of
Copy link
Contributor

Choose a reason for hiding this comment

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

Next , apply the audit.json profile. The audit.json profile logs all syscalls of the process to a new Pod.

Comment on lines 109 to 120
If you are using a pre-v1.19 Kubernetes version, download this manifest as
`audit-pod.yaml`.

{{< codenew file="pods/security/seccomp/alpha/audit-pod.yaml" >}}

If using Kubernetes v1.19 or later, download this manifest as `audit-pod.yaml`.

{{< codenew file="pods/security/seccomp/ga/audit-pod.yaml" >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using tabs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

A viable alternative: get a version of this tutorial for v1.18 merged into master, and then revise it for v1.19

In the v1.19 and later versions, tell the reader to look at the v1.18 version if they're using an older cluster.

audit-pod 1/1 Running 0 30s
```

We want to be able to interact with this endpoint exposed by this container, so
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, clean up we and let's:
To interact with this endpoint exposed by this container, create a NodePort service that allows access
to the endpoint from inside your kind control plane container.

```

Now we will be able to `curl` the endpoint from inside the kind control plane
container at the port exposed by this Service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Clean up: Now we will be able ...

```
docker exec -it 6a96207fed4b curl localhost:32373
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the output?
If so, could add text before the code block to explain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a command, added some text to indicate so 👍


{{< codenew file="pods/security/seccomp/alpha/default-pod.yaml" >}}

If using Kubernetes v1.19 or later, set the field.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Set the field?

@kbhawkey
Copy link
Contributor

Looking at this again.

@kbhawkey
Copy link
Contributor

kbhawkey commented Jul 16, 2020

Hello @hasheddan . Thanks for this PR.
I looked through the changes again. If you have time, you could fix the style and other small issues.
I don't have a strong opinion about which cluster setup tool to feature with the examples.
I'd be in favor of showing both kind and minikube paths. I am okay with showing kind now and updating later.
For me, the biggest challenge with this tutorial is displaying the alpha and GA manifests together.
How difficult would it be to split the tutorial, creating two versions (alpha, GA) of the tutorial?
I imagine this would not be too difficult.

@kbhawkey
Copy link
Contributor

@savitharaghunathan , What is the absolute last day/time for accepting 1.19 docs changes?

@savitharaghunathan
Copy link
Member

@savitharaghunathan , What is the absolute last day/time for accepting 1.19 docs changes?

@kbhawkey It is EOD PST time today :) However, this particular PR has obtained approvals to be merged after the deadline.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2020
@hasheddan
Copy link
Contributor Author

@sftim @kbhawkey thank you for your thoughtful reviews and your patience! I believe I have now addressed all comments, and have made the explanation of using both GA and alpha seccomp cleaner by using tabs. I have not yet moved the tutorial from kind to minikube, and would advocate for keeping kind and adding minikube because I think the kind setup is useful information. Seccomp should work on any Kubernetes cluster where it is enabled, so it does not make a huge difference what tool or service is used. I am starting work on adding minikube support here, but please let me know if you think it is not necessary. Thanks again for your work!

@hasheddan
Copy link
Contributor Author

@zacharysarah commits squashed 👍

@zacharysarah
Copy link
Contributor

/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 Jul 20, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zacharysarah

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 Jul 20, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3841d72 into kubernetes:dev-1.19 Jul 20, 2020
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.