Skip to content

server/*: Fix a bug where the GID is not added to /etc/group when run_as_group is set #8251

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
Merged
merged 1 commit into from
Jun 24, 2024
Merged

server/*: Fix a bug where the GID is not added to /etc/group when run_as_group is set #8251

merged 1 commit into from
Jun 24, 2024

Conversation

@PannagaRao
Copy link
Contributor

@PannagaRao PannagaRao commented Jun 3, 2024 •
edited
Loading

When securityContext of runAsGroup is added the
gid has to be added in /etc/group. Test case for
the same is also being added

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR has changes to add gid [value of RunAsGroup in SecurityContext] under /etc/group

Which issue(s) this PR fixes:

Fixes #8216

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug where the GID is not added to /etc/group when run_as_group is set

@PannagaRao PannagaRao requested a review from mrunalp as a code owner June 3, 2024 20:16
@openshift-ci openshift-ci bot added kind/bug Categorizes issue or PR as related to a bug. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 3, 2024
@openshift-ci openshift-ci bot requested review from hasan4791 and wgahnagl June 3, 2024 20:16
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 3, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 3, 2024

Hi @PannagaRao. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

haircommander
haircommander reviewed Jun 3, 2024
View reviewed changes
@codecov
Copy link

codecov bot commented Jun 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 43.15789% with 54 lines in your changes missing coverage. Please review.

Project coverage is 49.48%. Comparing base (5ab50a9) to head (efd4385).
Report is 98 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8251      +/-   ##
==========================================
- Coverage   49.56%   49.48%   -0.08%     
==========================================
  Files         153      153              
  Lines       16955    17038      +83     
==========================================
+ Hits         8403     8432      +29     
- Misses       7505     7544      +39     
- Partials     1047     1062      +15

haircommander
haircommander reviewed Jun 3, 2024
View reviewed changes
haircommander
haircommander reviewed Jun 3, 2024
View reviewed changes
haircommander
haircommander reviewed Jun 3, 2024
View reviewed changes
haircommander
haircommander reviewed Jun 3, 2024
View reviewed changes
haircommander
haircommander reviewed Jun 7, 2024
View reviewed changes
haircommander
haircommander reviewed Jun 7, 2024
View reviewed changes
@kwilczynski
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 12, 2024
@haircommander
Copy link
Member

/ok-to-test

haircommander
haircommander reviewed Jun 20, 2024
View reviewed changes
server/container_create.go
}

if !genPasswd && !genGroup {
break
Copy link
Member

@haircommander haircommander Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm unsure the "break" here breaks us out of the loop and not the switch statement? we may need a goto (even though I don't like how they look)

@haircommander
Copy link
Member

I think pulling latest and rebasing this PR should fix the test failures

It LGTM besides one remaining question (it wouldn't hurt to have it as-is)
/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2024
@kwilczynski
Copy link
Contributor

/retest

@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 22, 2024
@sohankunkerkar
Copy link
Member

/retest

@kwilczynski
Copy link
Contributor

/hold

Please update both the commit subject and the title of this Pull Request such that it explains what is being added.

We already know that there are "changes" made here, since this is a Pull Request.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2024
@PannagaRao PannagaRao changed the title server/*: Changes made to add gid in /etc/group server/*: Fix a bug where the GID is not added to /etc/group when run_as_group is set Jun 24, 2024
kwilczynski
kwilczynski reviewed Jun 24, 2024
View reviewed changes
utils/utils.go
username = "default"
}
if homedir == "" {
homedir = "/tmp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to set this to /nonexistent?

@cri-o/cri-o-maintainers, thoughts?

@PannagaRao
server/*: Fix bug to add gid in /etc/group
efd4385 
When securityContext of runAsGroup is added the
gid has to be added in /etc/group. This PR has
changes to add gid value under /etc/group
Test case to verify the addition of the same
is also being added

Signed-off-by: PannagaRamamanohara <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2024
@kwilczynski
Copy link
Contributor

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2024
@kwilczynski
Copy link
Contributor

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kwilczynski, PannagaRao, sohankunkerkar

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

@PannagaRao
Copy link
Contributor Author

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit d27c9c0 into cri-o:main Jun 24, 2024
@kwilczynski
Copy link
Contributor

/cherry-pick release-1.30

@kwilczynski
Copy link
Contributor

/cherry-pick release-1.29

@openshift-cherrypick-robot
Copy link

@kwilczynski: new pull request created: #8558

In response to this:

/cherry-pick release-1.30

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-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Sep 5, 2024
Merged
@openshift-cherrypick-robot
Copy link

@kwilczynski: #8251 failed to apply on top of branch "release-1.29":

Applying: server/*: Fix bug to add gid in /etc/group
Using index info to reconstruct a base tree...
M	server/container_create.go
M	test/pod.bats
M	utils/utils.go
M	utils/utils_test.go
Falling back to patching base and 3-way merge...
Auto-merging utils/utils_test.go
Auto-merging utils/utils.go
CONFLICT (content): Merge conflict in utils/utils.go
Auto-merging test/pod.bats
Auto-merging server/container_create.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 server/*: Fix bug to add gid in /etc/group
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.29

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-sigs/prow repository.

@kwilczynski
Copy link
Contributor

OK. Will cherry-pick manually.

This was referenced Sep 6, 2024
Merged
Merged
Merged
Merged
Merged
@github-actions github-actions bot mentioned this pull request Sep 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haircommander haircommander haircommander left review comments

@sohankunkerkar sohankunkerkar sohankunkerkar approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@hasan4791 hasan4791 Awaiting requested review from hasan4791

@wgahnagl wgahnagl Awaiting requested review from wgahnagl

+1 more reviewer

@kwilczynski kwilczynski kwilczynski left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kwilczynski kwilczynski

@haircommander haircommander

@sohankunkerkar sohankunkerkar

@PannagaRao PannagaRao

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Containers runs as a member of group should have its gid in /etc/group inside container

5 participants

@PannagaRao @kwilczynski @haircommander @sohankunkerkar @openshift-cherrypick-robot