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

Improve 'Seccomp defaulting' feature name #35121

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Jul 18, 2022

We're now rephrasing those two paragraphs to avoid confusing readers.

Follow-up on #34640 (comment)

@netlify
Copy link

netlify bot commented Jul 18, 2022

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 641a8e2
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/62d6575ee7041c000884406f

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2022
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 18, 2022
@k8s-ci-robot k8s-ci-robot requested review from hasheddan and pjbgf July 18, 2022 08:30
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jul 18, 2022
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.

Good shout! It does make it clearer for the reader.

/lgtm

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

LGTM label has been added.

Git tree hash: e41ab89e2948f61b2c8cf0e79b3e3dcceb7b53be

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.

As much as possible, we should write the docs as if the feature is already GA. We still need to mention the name of the related feature gate until graduation to stable, but we should try to minimize those mentions.

Writing as if the feature is stable makes it easier to adapt at graduation time, and helps makes those docs closer to our timeless docs ideal.

@sftim
Copy link
Contributor

sftim commented Jul 18, 2022

I'm not going to /lgtm cancel this, but I do disagree with exposing the feature gate name as jargon that we expect readers to recognize.

@pjbgf
Copy link
Member

pjbgf commented Jul 18, 2022

we should write the docs as if the feature is already GA

I think this may be an exception to the rule, given that this functionality is transitional by nature and will become a non-feature as soon as it GAs.

Given the security implications of assuming this is enabled when it may not be, I also don't think the timeless docs approach should apply, or we risk leaving users that potentially don't know the ins and outs of Seccomp worse off.

But I am keen to hear what other folks may think about this.

@sftim
Copy link
Contributor

sftim commented Jul 18, 2022

will become a non-feature as soon as it GAs

I don't understand that, to be honest.

  • We should clearly document how Kubernetes in respect of the behavior that the feature gate affects (the default seccomp profile for Pods that don't explicitly specify one). This is the key thing to document.
    For example, I'd expect the default seccomp profile to be mentioned in https://kubernetes.io/docs/tasks/configure-pod-container/security-context/
  • Whilst the feature gate exists, we should also document how to toggle the feature and the implications of doing so. Pages that mention the default seccomp profile can link to the docs about configuring what that default is.
    • Once the feature graduates, we remove this part but leave in the docs that explain the default seccomp profile.

Comment on lines 204 to 209
Seccomp defaulting for Pods is a beta feature in Kubernetes {{< skew currentVersion >}},
SeccompDefault for Pods is a beta feature in Kubernetes {{< skew currentVersion >}},
and the corresponding `SeccompDefault` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
is enabled by default. However, you still need to enable this defaulting for each node where
you would like to use it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like:

Kubernetes {{< skew currentVersion >}} lets you configure the seccomp profile that applies when the
spec for a Pod doesn't define a specific seccomp profile. This is a beta feature and …

Copy link
Member Author

Choose a reason for hiding this comment

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

Good, thank you for the input. I changed the paragraphs a bit.

@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, 2022
@k8s-ci-robot k8s-ci-robot requested a review from pjbgf July 19, 2022 07:03
We're now rephrasing those two paragraphs to avoid confusing readers.

Signed-off-by: Sascha Grunert <[email protected]>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 19, 2022
@saschagrunert saschagrunert changed the title Fix SeccompDefault feature name Improve 'Seccomp defaulting' feature name Jul 19, 2022
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.

Thanks

/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 19, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 77a127835ffb589f0c4dd2603e54fc131235c940

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pjbgf, sftim

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 19, 2022
@k8s-ci-robot k8s-ci-robot merged commit b61dfcd into kubernetes:dev-1.25 Jul 19, 2022
@sftim
Copy link
Contributor

sftim commented Jul 19, 2022

I didn't notice that this targeted dev-1.25. It's fine, but we could actually have targeted main here.

Also OK to cherry-pick this to make an equivalent change to main.

@saschagrunert saschagrunert deleted the dev-1.25 branch July 19, 2022 08:40
@saschagrunert
Copy link
Member Author

I didn't notice that this targeted dev-1.25. It's fine, but we could actually have targeted main here.

Also OK to cherry-pick this to make an equivalent change to main.

Changing it in main would cause merge conflicts later on, right?

@sftim
Copy link
Contributor

sftim commented Jul 25, 2022

Git can automatically solve a merge if we cherry pick these exact changes.

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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants