Skip to content
This repository has been archived by the owner on Oct 3, 2020. It is now read-only.

Refactor code to support running on secure environments #233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geoffmore
Copy link

I did the following to support running in secure/unprivileged environments (namely OpenShift)

  • Explicitly define the namespace for all objects. In the event where one wants to change the namespace of the deployment, there needs to be a change to the ClusterRoleBinding of the kube-ops-view service account. This change makes it easier to understand and change the namespace of everything systematically.
  • Add a service account for the redis pod. This is a bit more of an opinionated change, but I think explicitly defining a service account to consume allows more flexibility if permissions need to be changed at a later date.
  • Change serviceAccount to serviceAccountName in pod spec. Kubernetes 1.15 docs use serviceAccountName (https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account) so I'm updating this field to conform to that. I've noticed that when I deploy on OpenShift 3.11 using serviceAccountName instead of serviceAccount, the object actually created contains both fields and behaves as intended.
  • Update resource requests and limits for kube-ops-view deployment. I frequently ran into issues with the kube-ops-view pod getting OOMKilled due to lack of resources in an environment with over 15 nodes. After bumping these resources, I no longer noticed any issues.
  • Removed uid 1000 from security context in kube-ops-view pod. I removed this line and I don't see any error messages being thrown, so I think it's safe to remove.
  • Refactored redis deployment. This is the big one for this commit. Running privileged containers is strongly discouraged in my environments, so I think I found a good solution to this. The container image is switched to bitnami/redis to support a rootless redis. Volumes are explicitly defined to support read-only containers. An init container is added to the bitnami image to support having the conf directory of the redis pod being read-write without mounting over its contents.

I'm looking for feedback and suggestions before this is merged. As I mentioned, I've tested this on OpenShift 3.11. This is a large PR, so if you need data from other Kubernetes environments (GKE, AKS) let me know I can test against those environments as well. Finally, if there is any specific data you need from these environments, let me know and I can get started on adding the data to the PR.

@hjacobs
Copy link
Owner

hjacobs commented Jul 18, 2019

Thanks for your PR!

  • I don't agree with adding namespace to all manifests as this is redundant (kubectl apply ... -n myns is enough), IMHO it also has nothing to do with "security".
  • serviceAccountName: makes sense
  • resource limits: OK
  • I will check out the proposed Redis changes, I guess they make sense 😄

@geoffmore
Copy link
Author

Jacob,

One of the main reasons why I explicitly define the namespace for all objects is that the roleBinding for the kube-ops-view serviceaccount binds to a subject in an explicitly defined namespace, so if a user wanted to deploy this in another namespace, there would still need to be modifications to a file before successful deployment. I agree that my approach is a bit heavy-handed though... Should I instead add documentation that the rolebinding needs to be modified before deployment into other namespaces?

@gregswift
Copy link

I tried applying this to a hardened k8s environment and i guess the image need to be updated to go along with these changes?

  Normal   Pulled     21s (x8 over 97s)  kubelet  Successfully pulled image "hjacobs/kube-ops-view:0.12"
  Warning  Failed     21s (x8 over 97s)  kubelet  Error: container has runAsNonRoot and image will run as root
  Normal   Pulling    8s (x9 over 101s)  kubelet  pulling image "hjacobs/kube-ops-view:0.12"

apiVersion: v1
kind: ServiceAccount
metadata:
name: kube-ops-view
namespace: default

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not namespace: {{ .Values.Namespace }}?

@megabreit
Copy link

Basically I did the same in my Openshift deployment.
I removed runAsNonRoot and runAsUser so the container will start with a dynamically created non-root uid.
But I kept the original redis container and simply added an emptyDir for /data:

    volumeMounts:
    - name: redis-data
      mountPath: /data
  volumes:
  - name: redis-data
    emptyDir: {}

Any dynamicaly created user is able to write to /data now.
I don't know if there are other reasons to change the container. But this deployment looks simpler to me... and works fine.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants