Pod Security Review Feedback

[mesutcelik]

• Why do we have two ServiceAccount definition? 1 2
• Is this if condition relevant as runAsUser is always set?
• fsgroup is not part of SecurityContext. see the doc
• Do you think we should add RunAsGroup field?

[mesutcelik]

I see some of them already mentioned in the PR
#30

[leszko]

• Why do we have two ServiceAccount definition? 1 2

This is fine I guess. One serviceAccountName is used for the resource StatefulSet. The other serviceAccountName is used for the resource Pod.

• Is this if condition relevant as runAsUser is always set?

It looks fine. If it's empty then it's not set. I think that in YAML empty field is the same as not set field.

• fsgroup is not part of SecurityContext. see the doc

Yes, you're right, it was removed in #37 .

• Do you think we should add RunAsGroup field?

Yeah, I think we could add it. For clarity I've created a separate GH Issue #41

[mesutcelik]

This is fine I guess. One serviceAccountName is used for the resource StatefulSet. The other serviceAccountName is used for the resource Pod.

I see that caused some lint errors. Apparently, @dacleyra has removed one of them in the PR
https://github.com/hazelcast/charts/pull/30/files#diff-f68bdae4b1dd62c1742d595338dba9c8L101

It looks fine. If it's empty then it's not set. I think that in YAML empty field is the same as not set field.

What happens if we just remove if condition and move runAsNonRoot: true just under runAsUser?

[leszko]

I see that caused some lint errors. Apparently, @dacleyra has removed one of them in the PR
https://github.com/hazelcast/charts/pull/30/files#diff-f68bdae4b1dd62c1742d595338dba9c8L101

Actually you're right. That's the fix: #42

What happens if we just remove if condition and move runAsNonRoot: true just under runAsUser?

If runAsUser is not defined, then you run as a root. So you don't want to have runAsNonRoot defined. That's my understanding. For example, you mount a volume which requires root permissions then you need to run your container as a root. Does it make sense?

[mesutcelik]

I am probably focusing on readability... I feel refactoring into following would be more compact. Is there any problem with the following statement?

       securityContext:
          {{- if .Values.securityContext.runAsUser }}
          runAsUser: {{ .Values.securityContext.runAsUser }}
          runAsNonRoot: true
          {{- end }} 
          readOnlyRootFilesystem: true
          allowPrivilegeEscalation: false

[leszko]

@mesutcelik You're change is not just about readability. But I guess it's fine, we should never need to write to the root filesystem or allow priviledge escalation, even if you run as a root. I've created a PR to fix it. PTAL.

#43