Allow empty securityContext

[cniackz]

Objective:

Avoid hardcoding values that are not part of any security context constraint and allow Tenant to be deployed without changing scc in OpenShift as Operator logic is currently not allowing empty securityContext for Tenant and this is totally wrong!.

[dvaldivia]

This should be only for an openshift kustomization overlay

[pjuarezd]

Out of the box Openshift install will need to have the userId, groupId and fsGroup assigned in the pods securityContext field, other securityContextConstrains (aka SCC) are optional IMO.

It is also a possibility if we check the namespace annotations for the openshift.io/sa.scc.uid-range={uid}/{gid} and assign its values to the security constraint if that annotation exists.

apiVersion: apps/v1
kind: Deployment
metadata:
 name: tenant
...
   spec:
...
    template:
      spec:
        securityContext:
          runAsUser: {uid}
          runAsGroup: {gid}
          fsGroup: {gid}

If no openshift.io/sa.scc.uid-range={uid}/{gid} annotation exists, then let the defaults as they are now, basically we will rely in the existence of the metadata to identify if the install is Openshift based.

Any Other SecurityContextConstrains that the final setup have we could ask them to set it manually in the securityContext field, but for now this 3 fields are the only ones preventing the containers to start.

According to this Redhat doc https://cloud.redhat.com/blog/a-guide-to-openshift-and-uids, that metadata will allways be there, Openshift Assigns its values based in the SCC that the project have.

[dvaldivia]

the openshift overlay could have this annotations added by default and we could document that this is needed to install on Openshift

[pjuarezd]

the openshift overlay could have this annotations added by default and we could document that this is needed to install on Openshift

I beg to differ, Overlay is good, we should do it, but it only solves operator pods securityContext, not for tenants because the tenant deployment is created on the fly by Operator based on the assignations @cniackz is looking at now.

[cniackz]

Our Operator is currently wrong for OpenShift, we can't clear securityContext and the only way in OpenShift to get this working is by changing the scc which is totally wrong, we should let OpenShift pick those values if not set. So from this PR at least we need to change this function below:

// Builds the security context for containers in a Pool
func poolContainerSecurityContext(pool *miniov2.Pool) *v1.SecurityContext {
	// Default values:
	// By default, values should be totally empty if not provided
	// This is specially needed in OpenShift where scc don't allow hardcoded values
	// from our yamls and if let empty then OCP can pick the values from the constraints defined.
	containerSecurityContext := corev1.SecurityContext{}
	runAsNonRoot := true
	var runAsUser int64 = 1000
	var runAsGroup int64 = 1000

	// Values from pool.SecurityContext ONLY if provided
	if pool.SecurityContext != nil {
		if pool.SecurityContext.RunAsNonRoot != nil {
			runAsNonRoot = *pool.SecurityContext.RunAsNonRoot
		}
		if pool.SecurityContext.RunAsUser != nil {
			runAsUser = *pool.SecurityContext.RunAsUser
		}
		if pool.SecurityContext.RunAsGroup != nil {
			runAsGroup = *pool.SecurityContext.RunAsGroup
		}
        if pool.SecurityContext.RunAsNonRoot != nil || pool.SecurityContext.RunAsUser != nil || pool.SecurityContext.RunAsGroup != nil {
			// Only set values if one is set otherwise let it empty
			containerSecurityContext = corev1.SecurityContext{
				RunAsNonRoot: &runAsNonRoot,
				RunAsUser:    &runAsUser,
				RunAsGroup:   &runAsGroup,
			}
		}
	}

	// Values from pool.ContainerSecurityContext if provided
	if pool != nil && pool.ContainerSecurityContext != nil {
		containerSecurityContext = *pool.ContainerSecurityContext
	}
	return &containerSecurityContext
}

[cniackz]

For now if we don't change our code, below lines will be always needed:

oc apply -k ~/operator/examples/kustomization/tenant-lite
oc create serviceaccount minio-operator -n tenant-lite
oc adm policy add-scc-to-user privileged -n tenant-lite -z minio-operator
oc adm policy add-scc-to-user privileged -n tenant-lite -z builder
oc adm policy add-scc-to-user privileged -n tenant-lite -z deployer
oc adm policy add-scc-to-user privileged -n tenant-lite -z default
oc adm policy add-scc-to-user privileged -n tenant-lite -z storage-lite-sa

[pjuarezd]

@cniackz just be aware that if we remove the default runAsUser: 1000, runAsGroup: 1000 to none it will work on Openshift because it will assign the proper values, but will break all exisintg setups, cause it will change the runAsUser and runAsGroup to the defaul, on redhat/ubi8 Docker image that user seems to be root.

It is not feasible ask users to chown all existing files in the mounts.

[cniackz]

Ok, thank you Pedro, I will test for an existing setup

Change done

[cniackz]

@pjuarezd this change is not affecting existing setups, already tested. If tenant has been deployed with previous code and is using previous hardcoded values, and then we update operator with this code, it will remain to use those same values as they were already set and hence this will only affect new deployments or if specifically we set securityContext as empty values.

• Images below from previous setup after updating operator with this current branch, Pod keeps using previous user and can be executed with no issue on user change:

[cniackz]

VulnCheck issue in unrelated to this change.

[allanrogerr]

Im looking for your branch under https://github.com/cniackz/operator/branches. I cant see it. Did you delete it? @cniackz

[cniackz]

@allanrogerr New PR is: #1462