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.

Sign up for GitHub

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

Jump to bottom

KEP-3169: Fine-grained SupplementalGroups control #3620

Merged
merged 4 commits into from
Feb 9, 2023
Merged

KEP-3169: Fine-grained SupplementalGroups control #3620

merged 4 commits into from
Feb 9, 2023
+1,080 −0

Conversation

everpeace
Copy link
Contributor

@everpeace everpeace commented Oct 17, 2022 •
edited
Loading

  • One-line PR description: the first draft of the KEP. This proposes a new API surface to control how supplemental groups are applied in the container
  • Other comments: This needs to update CRI API and will impact CRI implementations. I'm not sure how I proceed to implement it. I really appreciated some comments/guidance from active/experienced contributors.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Oct 17, 2022
@k8s-ci-robot k8s-ci-robot added 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. labels Oct 17, 2022
@everpeace everpeace mentioned this pull request Oct 17, 2022
Open
24 tasks
@everpeace everpeace force-pushed the 3619-supplemental-groups-policy branch from e3ce6f7 to 14a1169 Compare October 17, 2022 04:41
@everpeace everpeace changed the title [WIP] KEP-3169: Fine grained SupplementalGroups control [WIP] KEP-3169: Fine-grained SupplementalGroups control Oct 17, 2022
@everpeace everpeace force-pushed the 3619-supplemental-groups-policy branch 2 times, most recently from 65e4c29 to d1617cd Compare October 17, 2022 07:06
@everpeace everpeace mentioned this pull request Oct 19, 2022
Closed
@everpeace everpeace force-pushed the 3619-supplemental-groups-policy branch from d1617cd to 0e57ad2 Compare October 21, 2022 08:24
@everpeace everpeace changed the title [WIP] KEP-3169: Fine-grained SupplementalGroups control KEP-3169: Fine-grained SupplementalGroups control Nov 8, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2022
everpeace
everpeace commented Nov 8, 2022
View reviewed changes
Copy link
Contributor Author

@everpeace everpeace left a comment

Choose a reason for hiding this comment

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

@dchen1107 @derekwaynecarr would you mind reviewing this??

This needs to update CRI API and will impact CRI implementations. I'm not sure how I proceed to implement it. I really appreciated some comments/guidance from you.

@derekwaynecarr
Copy link
Member

discussed in Nov 29 sig-node call, @mrunalp will take a pass.

/assign @mrunalp

everpeace
everpeace commented Dec 14, 2022
View reviewed changes
Copy link
Contributor Author

@everpeace everpeace left a comment

Choose a reason for hiding this comment

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

@mrunalp I updated the branch. PTAL.

keps/sig-node/3619-supplemental-groups-policy/README.md Show resolved Hide resolved
@everpeace everpeace requested review from mrunalp and removed request for derekwaynecarr and dchen1107 January 4, 2023 15:30
@thockin
Copy link
Member

thockin commented Feb 9, 2023

But I don't think we agree upon the design yet, and please come to SIG node discussing the design. Thanks!

I'm confused - what part of this is left unresolved?

@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, everpeace, johnbelamaric, mrunalp, SergeyKanzhelev

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 Feb 9, 2023
everpeace and others added 2 commits February 9, 2023 11:16
@everpeace @SergeyKanzhelev
First draft of KEP-3169: Fine-grained SupplementalGroups control
ba195b6 
This KEP roughly introduces belows in Kubernetes API:
- 'PodSecurityContext.SupplementalGroupsPolicy' to control which groups are attached to the container process, and
- 'ContainerStatus.User' so that user know which identities(uid, gid, supplemental groups) are ACTUALLY attached to the container process.
The corresponding changes are also proposed in CRI.

Co-authored-by: Sergey Kanzhelev <[email protected]>
@everpeace
add PRR approval request file and filled out PRR questionnaire just f…
b598ea5 
…or alpha relevant sections.
@everpeace
Copy link
Contributor Author

I will squash my commits. Squash will also remove do-not-merge/invalid-commit-message label.

@everpeace everpeace force-pushed the 3619-supplemental-groups-policy branch from 8b3e39b to b598ea5 Compare February 9, 2023 02:21
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Feb 9, 2023
@everpeace
Copy link
Contributor Author

But I don't think we agree upon the design yet, and please come to SIG node discussing the design. Thanks!

I'm in Tokyo. It's very difficult for me to attend the SIG node meeting because of the timezone (the meeting is 3am in my time😭).

I'd also like to know which part is unclear for merging this PR? I'm glad to address/answer it.

@everpeace
Copy link
Contributor Author

everpeace commented Feb 9, 2023 •
edited
Loading

Oh, my squash wiped out lgtm label....🙇

@thockin @mrunalp sorry, would you mind giving /lgtm again 🙇?

@dchen1107 I will keep do-not-merge/hold label once I got your response.

edited: I noticed dchen1107 already gave /approve to the PR in #3620 (comment). So, I'm going to unhold the PR.

@everpeace
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2023
@everpeace
Copy link
Contributor Author

@thockin @mrunalp I flipped the kep status to 'implementable' as part of tasks to satisfy KEP freeze requirements.

@SergeyKanzhelev
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2023
@k8s-ci-robot k8s-ci-robot merged commit c20aee7 into kubernetes:master Feb 9, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Feb 9, 2023
@everpeace everpeace deleted the 3619-supplemental-groups-policy branch February 9, 2023 04:00
@everpeace everpeace mentioned this pull request Feb 9, 2023
Merged
5 tasks
@AkihiroSuda
Copy link
Member

@everpeace

FYI: containerd fixed an issue related to supplemental GIDs.
GHSA-hmfx-3pcx-653p
containerd/containerd@286a01f

I don't think this change affects this KEP, but please confirm.

@everpeace
Copy link
Contributor Author

Thanks for the info.

I don't think this change affects this KEP, but please confirm.

Confirmed. IIUC, the issue is caused by container runtimes not duplicating the primary group in the supplementary groups in some situations. This means that the primary group MUST always be duplicated into the supplementary groups, right?

Then, although the fix does not affect the KEP as described, I would say we would need to update API description of SupplementalGroups so that it clarifies the behavior.

@AkihiroSuda
Copy link
Member

This means that the primary group MUST always be duplicated into the supplementary groups, right?

Right.

@thockin
Copy link
Member

thockin commented Apr 7, 2023

So this KEP got approved, but no PR merged for 1.27, right?

Will we revisit for 1.28?

@everpeace
Copy link
Contributor Author

So this KEP got approved, but no PR merged for 1.27, right?

No, sorry. I opened one cleanup PR in containerd side. But this was just approved and not merged.

containerd/containerd#8136

Will we revisit for 1.28?

Yes, I would like to work on this for 1.28.

@npolshakova npolshakova mentioned this pull request Jun 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyrickard jeremyrickard jeremyrickard left review comments

@jonjohnsonjr jonjohnsonjr jonjohnsonjr left review comments

@mikebrow mikebrow mikebrow left review comments

@wojtek-t wojtek-t wojtek-t left review comments

@johnbelamaric johnbelamaric johnbelamaric left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@thockin thockin thockin left review comments

@tianon tianon tianon left review comments

@vbatts vbatts vbatts left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

@mrunalp mrunalp

@thockin thockin

@SergeyKanzhelev SergeyKanzhelev

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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.