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

[KEP-127] Add documentation about user namespaces and PSS #43803

Merged

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Nov 3, 2023

@k8s-ci-robot k8s-ci-robot added this to the 1.29 milestone Nov 3, 2023
Copy link

netlify bot commented Nov 3, 2023

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

Name Link
🔨 Latest commit 4e156c7
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6565c3d12bcec900080b124b

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 3, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 3, 2023
@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 Nov 3, 2023
@dipesh-rawat
Copy link
Member

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Nov 3, 2023
sftim
sftim previously requested changes Nov 3, 2023
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.

This change is actually about Pod security admission, right?

I think we might want to make an associated change to https://kubernetes.io/docs/concepts/security/pod-security-standards/ to explain what the standards are. These standards are not behind a feature gate, even if our in-tree enforcement mechanism is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the change should really be made to https://kubernetes.io/docs/concepts/security/pod-security-admission/, and then have a small section in the user namespaces concept that links to the relevant part of the PSA docs.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean pod security admission or pod security standards? That page you link doesn't seem related, but this one (about pod security standards) does: https://kubernetes.io/docs/concepts/security/pod-security-standards/

Can you explain why pod security admisions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, because that's where the feature gate has an effect.

Think about it like this: imagine we were adding support for TLS 1.3; we'd document that in our pages about configuring transport layer security and maybe around setting which TLS versions were supported. But we wouldn't propose an update to https://datatracker.ietf.org/doc/html/rfc8446

On the other hand, if someone does want to revise the TLS standard, they don't put the update to the standard behind a feature gate. But they might have a feature gate to - eg - enable the RSASSA-PSS signature scheme.

I don't really understand how you feature gate a standard.

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

@sftim PTAL. Fixed all the issues, some questions remain open.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean pod security admission or pod security standards? That page you link doesn't seem related, but this one (about pod security standards) does: https://kubernetes.io/docs/concepts/security/pod-security-standards/

Can you explain why pod security admisions?

content/en/docs/concepts/workloads/pods/user-namespaces.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/pods/user-namespaces.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/pods/user-namespaces.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/pods/user-namespaces.md Outdated Show resolved Hide resolved
@rata rata force-pushed the dev-1.29-user-namespaces-pss branch from 38b4d39 to 7eb729d Compare November 9, 2023 13:58
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 9, 2023
@sftim
Copy link
Contributor

sftim commented Nov 9, 2023

We don't (and IMO shouldn't) have a feature gate that controls Pod Security Standards. That wouldn't fit with what I think standards are.

An analogy: the IEC-60320 standard (for electrical connectors) is one standard that a connector either complies with or not. If you turned on a toggle switch on some appliance, the cable you plug into it either would or wouldn't comply with the standard just the same.

People will want to know what standard their Pod manifest complies with, and if we tell them the answer depends on the cluster where they run it, they might wonder why we (Kubernetes) took that approach.
However, if it really is about enforcement, fine. Lots of new standards or revisions come in with an exception where existing things are still tolerated; there are real-world analogies aplenty.


I've tried to build a mental model where the explanation in this PR makes sense, and I can't. I think we should change at least the explanation and possibly some of the meaning too.
Want some synchronous time on this? Message me on Kubernetes Slack and I'll share a Calendly link, or we can sort out a call some other way.

@rata
Copy link
Member

rata commented Nov 9, 2023

Well, the PR is merged and the KEP too, with all the approvals in place. I think we just need to document it.

But if you feel strongly about it, I'll let you discuss it with @saschagrunert when he is back next week. He is the one doing all the work here, so he should be involved in the discussion.

@giuseppe
Copy link
Member

giuseppe commented Nov 9, 2023

I am not sure that the website repository is the place to discuss such design issues which were already discussed and agreed upon.

So please, let's just stick to the fact that this PR is about adding documentation for an existing feature.

@saschagrunert
Copy link
Member Author

I agree that we should focus on the docs within this PR rather than questioning the whole KEP.

@sftim
Copy link
Contributor

sftim commented Nov 14, 2023

I see that https://github.com/kubernetes/enhancements/blob/fa647b424a0b11316e8e31274e50332b14434f9c/keps/sig-node/127-user-namespaces/README.md#pod-security-standards-pss-integration does say that the pod security standards will be gated.

OK, we should document that.

However, we'll need to make it really clear to users what this means. It doesn't make sense to me, and I'm confident I won't be the only puzzled person.

If I get time, I will also PR an update to the KEP.

@sftim sftim dismissed their stale review November 14, 2023 10:22

It turns out the KEP also specifies the problematic framing

@drewhagen
Copy link
Member

Hello @saschagrunert @onlydole @tengqm @rata @sftim 👋,

Friendly reminder that the deadline to have docs merged is next Tuesday 28th November 2023. If this feature needs documentation and the docs are not ready, the feature may be removed from the milestone.

Let me know if you have any questions. Thank you!

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.

If folks are willing to make the changes I've proposed, that might be enough.

Should we also add explanatory text to https://kubernetes.io/docs/concepts/security/pod-security-standards/? I really think we should do, even for alpha.

@saschagrunert saschagrunert force-pushed the dev-1.29-user-namespaces-pss branch from f3ede39 to cf53f8c Compare November 28, 2023 08:44
@saschagrunert
Copy link
Member Author

saschagrunert commented Nov 28, 2023

If folks are willing to make the changes I've proposed, that might be enough.

Thanks! I incorporated the changes as suggested.

Should we also add explanatory text to https://kubernetes.io/docs/concepts/security/pod-security-standards/? I really think we should do, even for alpha.

Can I follow-up on that? I assume linking back with a note would be enough, but having that as part of a separate PR would allow us more time.

@sftim
Copy link
Contributor

sftim commented Nov 28, 2023

This isn't passing tests. LGTM though.

@sftim
Copy link
Contributor

sftim commented Nov 28, 2023

I assume linking back with a note would be enough, but having that as part of a separate PR would allow us more time.

Sure, I was also thinking a separate PR.

Adding required documentation for
[KEP-127](kubernetes/enhancements#127).

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Co-authored-by: Tim Bannister <tim@scalefactory.com>
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert saschagrunert force-pushed the dev-1.29-user-namespaces-pss branch from cf53f8c to 4e156c7 Compare November 28, 2023 10:41
@saschagrunert
Copy link
Member Author

This isn't passing tests. LGTM though.

Green now :)

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.

/lgtm

Thanks

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

LGTM label has been added.

Git tree hash: 5b846f798373c2dafd4c6013fc587ccb2c7643af

@sftim
Copy link
Contributor

sftim commented Nov 28, 2023

/assign @kbhawkey

per #43803 (comment)

@reylejano
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

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 Nov 28, 2023
@k8s-ci-robot k8s-ci-robot merged commit 8a29190 into kubernetes:dev-1.29 Nov 28, 2023
6 checks passed
- `UserNamespacesPodSecurityStandards`: Enable Pod Security Standards policies relaxation for pods
that run with namespaces. You must set the value of this feature gate consistently across all nodes in
your cluster, and you must also enable `UserNamespacesSupport` to use this feature.
See [User Namespaces](/docs/concepts/workloads/pods/user-namespaces/#integration-with-pod-security-standards) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. Should be:

-  See [User Namespaces](/docs/concepts/workloads/pods/user-namespaces/#integration-with-pod-security-standards) for more details.
+  See [User Namespaces](/docs/concepts/workloads/pods/user-namespaces/#integration-with-pod-security-admission-checks) for more details.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing in the follow-up PR.

@saschagrunert saschagrunert deleted the dev-1.29-user-namespaces-pss branch November 29, 2023 07:55
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

9 participants