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

Add support for setting the group on a unix domain socket #901

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

ayush933
Copy link
Contributor

Add new optional, immutable string config called unixsocketgroup.
Change the group of the unix socket to unixsocketgroup after bind() if specified.

Manually verified that group is modified as expected and fails on illegal arguments (like non existent group name).

Fixes #873.

@ayush933
Copy link
Contributor Author

Hi, @madolson can you please take a look?

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.33%. Comparing base (109cc21) to head (c8aeaae).
Report is 37 commits behind head on unstable.

Files Patch % Lines
src/anet.c 57.14% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #901      +/-   ##
============================================
- Coverage     70.40%   70.33%   -0.07%     
============================================
  Files           112      112              
  Lines         61465    61518      +53     
============================================
- Hits          43275    43271       -4     
- Misses        18190    18247      +57     
Files Coverage Δ
src/config.c 78.69% <ø> (ø)
src/connection.h 86.74% <ø> (ø)
src/server.c 88.55% <100.00%> (-0.02%) ⬇️
src/server.h 100.00% <ø> (ø)
src/unix.c 76.31% <100.00%> (+0.31%) ⬆️
src/anet.c 74.09% <57.14%> (-1.06%) ⬇️

... and 23 files with indirect coverage changes

Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

It would also be nice to write a test to validate this, probably in other or misc. We can validate both the permissions and group.

@ayush933
Copy link
Contributor Author

Addressed reviews and added UT to test that permissions are set correctly.
Not sure which group to use in tests since using the default root group or creating new group would require sudo.

@ayush933 ayush933 force-pushed the unixSocketGroup branch 2 times, most recently from 33e2274 to f0839bd Compare August 16, 2024 07:18
@ayush933 ayush933 requested a review from madolson August 19, 2024 19:06
@madolson madolson added the major-decision-pending Major decision pending by TSC team label Aug 19, 2024
@madolson
Copy link
Member

@valkey-io/core-team Please vote on this small feature, we can optionally include it in Valkey 8.0 if it's ready for rc2, but not after.

Add new optional, immutable string config called unixsocketgroup.
Change the group of the unix socket to unixsocketgroup after bind() if
specified.

Signed-off-by: Ayush Sharma <[email protected]>
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting on majority approval

@madolson madolson added release-notes This issue should get a line item in the release notes major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Aug 22, 2024
@madolson
Copy link
Member

Since the new test is a bit odd, running the daily against a bunch of different platforms. https://github.com/valkey-io/valkey/actions/runs/10530596918

@madolson madolson merged commit b48596a into valkey-io:unstable Aug 23, 2024
47 checks passed
madolson pushed a commit that referenced this pull request Sep 2, 2024
Add new optional, immutable string config called `unixsocketgroup`. 
Change the group of the unix socket to `unixsocketgroup` after `bind()`
if specified.

Adds tests to validate the behavior.

Fixes #873.

Signed-off-by: Ayush Sharma <[email protected]>
madolson pushed a commit that referenced this pull request Sep 3, 2024
Add new optional, immutable string config called `unixsocketgroup`. 
Change the group of the unix socket to `unixsocketgroup` after `bind()`
if specified.

Adds tests to validate the behavior.

Fixes #873.

Signed-off-by: Ayush Sharma <[email protected]>
pizhenwei added a commit to pizhenwei/valkey that referenced this pull request Oct 13, 2024
Hide 'unixsocketgroup' and 'unixsocketperm' into a Unix socket
specific data structure. A single opaque pointer 'void *priv' is
enough for a listener. Once any new config is added, we don't need
'void *priv2', 'void *priv3' and so on.

Fixes: b48596a ('Add support for setting the group on a unix domain socket (valkey-io#901)')
Cc: Ayush Sharma <[email protected]>
Signed-off-by: zhenwei pi <[email protected]>
pizhenwei added a commit to pizhenwei/valkey that referenced this pull request Oct 13, 2024
Hide 'unixsocketgroup' and 'unixsocketperm' into a Unix socket
specific data structure. A single opaque pointer 'void *priv' is
enough for a listener. Once any new config is added, we don't need
'void *priv2', 'void *priv3' and so on.

Fixes: b48596a ('Add support for setting the group on a unix domain socket (valkey-io#901)')
Cc: Ayush Sharma <[email protected]>
Signed-off-by: zhenwei pi <[email protected]>
pizhenwei added a commit to pizhenwei/valkey that referenced this pull request Oct 13, 2024
Hide 'unixsocketgroup' and 'unixsocketperm' into a Unix socket
specific data structure. A single opaque pointer 'void *priv' is
enough for a listener. Once any new config is added, we don't need
'void *priv2', 'void *priv3' and so on.

Fixes: b48596a ('Add support for setting the group on a unix domain socket (valkey-io#901)')
Cc: Ayush Sharma <[email protected]>
Signed-off-by: zhenwei pi <[email protected]>
pizhenwei added a commit to pizhenwei/valkey that referenced this pull request Oct 13, 2024
Hide 'unixsocketgroup' and 'unixsocketperm' into a Unix socket
specific data structure. A single opaque pointer 'void *priv' is
enough for a listener. Once any new config is added, we don't need
'void *priv2', 'void *priv3' and so on.

Fixes: b48596a ('Add support for setting the group on a unix domain socket (valkey-io#901)')
Cc: Ayush Sharma <[email protected]>
Signed-off-by: zhenwei pi <[email protected]>
pizhenwei added a commit to pizhenwei/valkey that referenced this pull request Oct 18, 2024
Hide 'unixsocketgroup' and 'unixsocketperm' into a Unix socket
specific data structure. A single opaque pointer 'void *priv' is
enough for a listener. Once any new config is added, we don't need
'void *priv2', 'void *priv3' and so on.

Fixes: b48596a ('Add support for setting the group on a unix domain socket (valkey-io#901)')
Cc: Ayush Sharma <[email protected]>
Signed-off-by: zhenwei pi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-approved Major decision approved by TSC team release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NEW] Add support for setting the group on a unix domain socket
6 participants