Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for read-only root filesystem and enable by default #235

Merged
merged 10 commits into from
Nov 30, 2023

Conversation

eli-gc
Copy link
Contributor

@eli-gc eli-gc commented Nov 29, 2023

resolves kiali/kiali#6888

  1. Mount emptyDir to /tmp instead of /tmp/ansible-operator/runner
  2. Define ANSIBLE_LOCAL_TEMP env variable to set path for Ansible temporary directory to /tmp/ansible/tmp instead of root filesystem.
  3. Create localAnsibleTmpPath in Values so users can set their own path.
  4. Set readOnlyRootFilesystem to true in operator deployment.yaml to enable this feature by default.

@eli-gc eli-gc changed the title Add support for read-only root filesystem and enable by default (#6888) Add support for read-only root filesystem and enable by default #6888 Nov 29, 2023
@eli-gc eli-gc changed the title Add support for read-only root filesystem and enable by default #6888 Add support for read-only root filesystem and enable by default Nov 29, 2023
Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

Comment on lines 92 to 96
# localAnsibleTmpPath is the path of the local Ansible temp directory. This sets the ANSIBLE_LOCAL_TEMP variable which
# in turn sets the DEFAULT_LOCAL_TMP configuration. An emptyDir is mounted to /tmp for the kiali-operator container.
# Ansible needs write access on this directory so modifying it might have implications if read-only root filesystem is enabled.
localAnsibleTmpPath: /tmp/ansible/tmp

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? Why would someone need to change this?

IMO, this should be fixed/hardcoded. I don't see a need for anyone to change this, and it just invites users to misconfigure something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I'll remove it from values.

@jmazzitelli jmazzitelli dismissed their stale review November 29, 2023 22:35

I'll run tests over here but the changes look good

@eli-gc
Copy link
Contributor Author

eli-gc commented Nov 29, 2023

This will also require changes to the OLM metadata for those who install the operator via OLM (as opposed to helm). So this means a PR over in the kiali-operator repo. See the checklist here.

The files where you will want to make similar changes will be here:

* https://github.com/kiali/kiali-operator/blob/master/manifests/kiali-upstream/1.78.0/manifests/kiali.v1.78.0.clusterserviceversion.yaml

* https://github.com/kiali/kiali-operator/blob/master/manifests/kiali-community/1.78.0/manifests/kiali.v1.78.0.clusterserviceversion.yaml

* https://github.com/kiali/kiali-operator/blob/master/manifests/kiali-ossm/manifests/kiali.clusterserviceversion.yaml

Created PR in kiali-operator. Let me know if I'm missing anything. Thanks! kiali/kiali-operator#729

Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

Testing locally ... operator is failing to install Kiali.

Here is how I tested and what I see:

  1. First build the helm charts from this PR:
make build-helm-charts
  1. Use make from kiali repo to push and deploy the operator using the new helm chart:
make -e CLUSTER_TYPE=minikube -e HELM_CHARTS_REPO_PULL=false build build-ui cluster-push operator-create
  1. Confirm securityContext has read-only fs:
$ kubectl get pods -n kiali-operator -l app.kubernetes.io/name=kiali-operator -o jsonpath='{..spec.containers..securityContext}' | jq
{
  "allowPrivilegeEscalation": false,
  "capabilities": {
    "drop": [
      "ALL"
    ]
  },
  "privileged": false,
  "readOnlyRootFilesystem": true,
  "runAsNonRoot": true
}
  1. Check the tmp volume mount exists:
$ kubectl get pods -n kiali-operator -l app.kubernetes.io/name=kiali-operator -o jsonpath='{..spec.containers..volumeMounts}' | jq
[
  {
    "mountPath": "/tmp",
    "name": "tmp"
  },
  {
    "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount",
    "name": "kube-api-access-nt5qq",
    "readOnly": true
  }
]
  1. Check the env var:
$ kubectl get pods -n kiali-operator -l app.kubernetes.io/name=kiali-operator -o jsonpath='{..spec.containers..env}' | jq
...
  {
    "name": "ANSIBLE_LOCAL_TEMP",
    "value": "/tmp/ansible/tmp"
  }
  1. Now install Kiali to make sure the operator still works (this assumes istio is installed in istio-system):
make -e CLUSTER_TYPE=minikube -e HELM_CHARTS_REPO_PULL=false kiali-create

Kiali fails to install because the operator gets this internal error:

$ kubectl logs -n kiali-operator -l app.kubernetes.io/name=kiali-operator

TASK [default/kiali-deploy : Update CR status progress field with any additional status fields] ***
fatal: [localhost]: UNREACHABLE! => {"changed": false, "msg": "Failed to create temporary directory.In some cases, you may have been able to authenticate and did not have permissions on the target directory. Consider changing the remote tmp path in ansible.cfg to a path rooted in \"/tmp\", for more error information use -vvv. Failed command was: ( umask 77 && mkdir -p \"` echo /opt/ansible/.ansible/tmp `\"&& mkdir \"` echo /opt/ansible/.ansible/tmp/ansible-tmp-1701351578.757277-149-226722000580315 `\" && echo ansible-tmp-1701351578.757277-149-226722000580315=\"` echo /opt/ansible/.ansible/tmp/ansible-tmp-1701351578.757277-149-226722000580315 `\" ), exited with result 1", "unreachable": true}

PLAY RECAP *********************************************************************
localhost                  : ok=11   changed=0    unreachable=1    failed

@jmazzitelli
Copy link
Contributor

@eli-gc please provide testing procedures for this - the commands you use to test (compare to what I posted in my review above). It isn't working for me, so I am curious what I did differently than you.

@jmazzitelli
Copy link
Contributor

@eli-gc I think I fixed it. Add another env var... ANSIBLE_REMOTE_TEMP - set its value to the same as what you have for ANSIBLE_LOCAL_TEMP.

@eli-gc
Copy link
Contributor Author

eli-gc commented Nov 30, 2023

PR updated with requested changes. Thanks for finding that.

Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

lgtm - testing shows this working

@jmazzitelli jmazzitelli merged commit a0b4df2 into kiali:master Nov 30, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2 participants