-
Notifications
You must be signed in to change notification settings - Fork 8
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
Run livenessprobe
as non-root + Run all possible sidecars as nonRootGroup
#65
Conversation
1303135
to
f917c77
Compare
c97c602
to
f411175
Compare
👍 Coverage increased from [%] to [68.175%] |
👍 Coverage increased from [%] to [68.725%] |
👍 Coverage increased from [%] to [68.3375%] |
086b1ae
to
2337d8c
Compare
👍 Coverage increased from [%] to [68.0125%] |
👍 Coverage increased from [%] to [68.9875%] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments
livenessprobe
as non-root + Run all possible sidecars as nonRootGroup
👍 Coverage increased from [%] to [69.35%] |
👍 Coverage increased from [%] to [69.55%] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prankulmahajan in case of any error on this setup the driver wont be running ?
5c866b0
to
5244c2d
Compare
Signed-off-by: Prankul <[email protected]>
5244c2d
to
9484797
Compare
👍 Coverage increased from [%] to [82.0222%] |
👍 Coverage increased from [%] to [82.2556%] |
👍 Coverage increased from [%] to [81.9111%] |
👍 Coverage increased from [%] to [82.2556%] |
👍 Coverage increased from [%] to [82.2556%] |
👍 Coverage increased from [%] to [82.2556%] |
👍 Coverage increased from [%] to [82.2556%] |
Test Results: ROKS
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case to be verified
- IKS and ROKS (rhel and rhcos)
- Migration case/existing users
[Edit]: Added test results below
👍 Coverage increased from [%] to [82.7333%] |
Migration/Existing driver users
|
ROKS with Coreos
|
👍 Coverage increased from [%] to [82.3222%] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Detailed analysis and test results in GHE https://github.ibm.com/alchemy-containers/armada-storage/issues/5301
This PR aims to do the following,
NonRootGroup
. Currently, though the users are non-root, the group is still being used as root.livenessprobe
sidecar as non-root user/group.To achieve the 2nd point, had to change file permissions of CSI socket created by node server.
The socket file should have read and write permissions for access. i.e min access required is 600/660 depending on who is accessing the socket file
This PR changes the group of the csi socket from root to non-root(set 2121 in deployment configMap) and update permissions to 660. Setting "660" allows the root user and the non-root group to access the socket as expected.
Test functionality and logs