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

Update Kubernetes configs - will fix a few things #95

Closed
incertum opened this issue Dec 18, 2023 · 12 comments · Fixed by falcosecurity/charts#644
Closed

Update Kubernetes configs - will fix a few things #95

incertum opened this issue Dec 18, 2023 · 12 comments · Fixed by falcosecurity/charts#644
Assignees
Labels
kind/feature New feature or request

Comments

@incertum
Copy link
Contributor

incertum commented Dec 18, 2023

  1. Missing HOST_ROOT env in initContainers falco-driver-loader
env:
- name: HOST_ROOT
  value: /host

For completeness suggest explicitly adding HOST_ROOT env to the falco container, plus add FALCO_HOSTNAME env as we now use it in Falco's metrics option and expose it as filter in evt.hostname, so it would be a great example template.

env:
- name: FALCO_HOSTNAME
  valueFrom:
    fieldRef:
      fieldPath: spec.nodeName
- name: HOST_ROOT
  value: /host
  1. DISREGARD

I notice we mount the entire /usr which is not needed, suggest changing to the following in order to adopt best production practices (least access):

volumes:
 - name: usr-src-kernels-fs
  hostPath:
    path: /usr/src/kernels/
...

volumeMounts:
- mountPath: /host/usr/src/kernels/
  name: usr-src-kernels-fs
  readOnly: true
...

The docs here are not sufficient, happy to update them!

@incertum incertum added the kind/feature New feature or request label Dec 18, 2023
@incertum
Copy link
Contributor Author

@leogr @FedeDP @Issif

@leogr
Copy link
Member

leogr commented Dec 19, 2023

The first 3 points should be addressed in the Helm Chart eventually, I guess. cc @alacuku

Anyway:

  • re HOST_ROOT=/host: it should be the default value configured via the Dockerfile in all container images, IIRC.
  • re /usr: I vague recall there're other reasons to mount the full /usr, as stated by our documentation. This needs to be double-checked.

@incertum
Copy link
Contributor Author

Sounds good, I wold still add HOST_ROOT=/host just so we serve up a better and clearer template.
ACK re double-checking /usr

@maxgio92
Copy link
Member

Thanks @incertum! I agree with all points except for /usr mount as stated by @leogr if it's really needed, otherwise good point for me to selectively mount only /usr/src/kernels one.

@incertum
Copy link
Contributor Author

incertum commented Jan 9, 2024

We clarified that we indeed need entire /usr/src because of ubuntu distros etc, I updated the original post indicating to disregard that comment. Thanks.

@incertum
Copy link
Contributor Author

Quick question since it came up in a different issue. Aren't we missing the host /etc mount for the falco container in order to resolve user uids to their names and such?

https://github.com/falcosecurity/deploy-kubernetes/blob/main/kubernetes/falco/templates/daemonset.yaml#L80

Add the following:

- mountPath: /host/etc
  name: etc-fs

@leogr
Copy link
Member

leogr commented Jan 11, 2024

Quick question since it came up in a different issue. Aren't we missing the host /etc mount for the falco container in order to resolve user uids to their names and such?

https://github.com/falcosecurity/deploy-kubernetes/blob/main/kubernetes/falco/templates/daemonset.yaml#L80

Add the following:

- mountPath: /host/etc
  name: etc-fs

I can't recall if we looked up from /etc for that purpose 🤔 @FedeDP ?

@leogr
Copy link
Member

leogr commented Jan 11, 2024

/assign

@FedeDP
Copy link

FedeDP commented Jan 11, 2024

Yes, we actually need it to resolve user and group related info: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/user.cpp#L60

I think we completely forgot to update our charts back when we implemented the feat.
Great catch @incertum !

@incertum
Copy link
Contributor Author

Hello, one more thing 🙃

See falcosecurity/cncf-green-review-testing#9 we also totally forgot to include /run/k3s/containerd/containerd.sock as container runtime socket and k3s appears to be more frequently used now.

@alacuku
Copy link
Member

alacuku commented Jan 15, 2024

Yes, we actually need it to resolve user and group related info: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/user.cpp#L60

I think we completely forgot to update our charts back when we implemented the feat. Great catch @incertum !

Hi @incertum, @FedeDP I fixed it here: falcosecurity/charts@3cf8bdf. Will be released once falco 0.37.0 is out.
Is it ok, or do we need the fix before that?

@FedeDP
Copy link

FedeDP commented Jan 15, 2024

Thanks @alacuku ! Great job, as always :)
For me, it's ok to release the fix with the Falco 0.37.0 chart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants