Allow seldon manager to run as non-root

[mo-saeed]

Describe the bug

the helm installation of seldon-core-operator failed coz seldon-controller-manager fails to start with this error

container has runAsNonRoot and image will run as root

So it seems that the image tries to run as a root and our pod security policy dissallow that.

Please have a look into this thread https://seldondev.slack.com/archives/CSVSD5S75/p1605193934193300

To reproduce

• you need to have a pod security policy in place where it restricts running images as root
• run the helm installation (for seldon-core-operator)

Expected behaviour

Installation completes successfully and all pods should run not with root user.

Environment

K8s cluster- AKS v1.18
seldon-core-operator version 1.4.0

[ryandawsonuk]

Makes sense, we need to add a section like https://github.com/helm/charts/blob/master/stable/ambassador/templates/deployment.yaml#L49

Not quite as simple as it sounds as our helm chart is generated from kustomize using python code

[mo-saeed]

@ryandawsonuk Thank you for your update

As this is a blocker for us to proceed with seldon deployment, can the fix be a part of the next release?

[mo-saeed]

@ryandawsonuk Any updates?

[adriangonz]

Hey @mo-saeed, we've already started to cut the 1.5 release. However, we could include this for 1.6.

Given that you've already started to look into this locally, would you be willing to contribute a PR with the required changes?

[mo-saeed]

Hi @adriangonz

Thanks for the updates, I hoped to have such an important fix in 1.5.

Can you please provide documentation on how to use this script so I can create the pull request, the change for me sounds to be easy which is adding this

                # SecurityContext
                res["spec"]["template"]["spec"]["securityContext"]["runAsUser"] = helm_value(
                    "defaultUserID"
                )

after this line https://github.com/SeldonIO/seldon-core/blob/master/operator/helm/split_resources.py#L156

But I dunno how to test this properly and I can't find any documentation.

Thanks

[ukclivecox]

• The source of truth for the yaml for the manager is the kustomize in https://github.com/SeldonIO/seldon-core/blob/master/operator/config/manager/manager.yaml
  So we should change this file.
• Then you can add your change as above to split_resources.
• After which run make create in that folder to generate the Helm chart templates.
• You should then update the seldon-operator helm values file to contain the new variable with a matching name e.g. defaultUserID in https://github.com/SeldonIO/seldon-core/blob/master/helm-charts/seldon-core-operator/values.yaml

I agree we should document these steps in a developer section.

[mo-saeed]

@cliveseldon @adriangonz Pull request created, please have a look.

There were also some other changes caused by the build process, I am not sure if it can be avoided or it's ok to have it.

[ukclivecox]

closed by #2709
@mo-saeed Have you tested master in your context and this solves your issue?

[ukclivecox]

The PR that fixed this was incorrect

1. as manager needs to bind to port 443 by default for web hook
2. it was adding runAsUser as string while it needs to be an int

[ukclivecox]

reverted in #2752

[mo-saeed]

And why was it reverted, instead of only fixing the issue?

I don't get the first point, coz nth was changed regarding the webhook port.

for the second point, how can that be configured properly in this python script?

[ukclivecox]

The install is broken in master so have reverted. We can put the fix back in with corrections. Will need to look what we do for openshift where we change the webhook port to a non-root value such as 4443 and also ensure the value is correctly set.

[mo-saeed]

when I look here , I see the webhook port is defined the same way using helm_value method. so not clear for me how to set it as an integer not string. even here sounds for me the same...

I hope we can have an easy and documented way of updating helm values so users can contribute and test properly.

[ukclivecox]

Yes agreed. The issue was it was setting it as a string when it needs to be an int.