-
Notifications
You must be signed in to change notification settings - Fork 388
allow to disable fsgroup, when installing to openshift #528
Conversation
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.
Hi - thank you for this PR, it is great to get additional feedback on OpenShift use-cases.
Since we do not have official support for OpenShift yet, but are actively working toward it, it would be great if for now we change the naming conventions a bit.
A few comments before merging :
-
Since we do not have official support for OpenShift, let's rename the flag in values.yaml to something like
disableFsGroupSecurityContext
and put it in theserver:
stanza rather than global, and reflect its new name and location in theserver-statefulset.yaml
template. -
Could you kindly add some additional information in the comments for the changes in values.yaml? Maybe something such as:
disableFsGroupSecurityContext disables setting the fsGroup securityContext for the server statefulset, this is required when using the OpenShift platform as this is an invalid setting.
and/or more information if you have it. -
We generally try to include a new bats test anytime a new flag is introduced. Could you please update
test/unit/server-statefulset.bats
with one?
I think this one might do the trick, but feel free to be more descriptive :
#--------------------------------------------------------------------
# server.disableFsGroupSecurityContext
@test "server/StatefulSet: can disable fsGroup security context settings" {
cd `chart_dir`
local actual=$(helm template \
-s templates/server-statefulset.yaml \
--set 'server.disableFsGroupSecurityContext=true' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.securityContext' | tee /dev/stderr)
[ "${actual}" = "null" ]
}
@test "server/StatefulSet: default fsGroup security context settings fsGroup: 1000" {
cd `chart_dir`
local actual=$(helm template \
-s templates/server-statefulset.yaml \
--set 'server.disableFsGroupSecurityContext=false' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.securityContext.fsGroup' | tee /dev/stderr)
[ "${actual}" = "1000" ]
}
Applying freview feedback
Hi Thank you for the review. I've applied the changes as requested. Initially I've named the property openshift, to have it work the same as it is implemented in the vault-helm. Once the chart officially, the fsGroup property could probably be changed back to an openshift flag. Regards, |
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.
This looks great.
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.
Looks great, thank you for the PR!
When trying to install this chart to OpenShift, the StatefulSet fails, since the fsGroup is set by default to 1000, which is not valid in OpenShift by default.
Adding this option allows the fsGroup not to be set, when running in OpenShift.