-
Notifications
You must be signed in to change notification settings - Fork 895
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
fix(docker): missing HOST_ROOT env in "no-driver" docker image #1492
Conversation
Please note that the `HOME` env has been added for consistency purposes with the main docker image. Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
We didn't do the /host env variable thing in that image because with --device you have no way to remap falco devices to /host/dev and thus we were advising to do /var/run/docker.sock on the no-driver image. If we just merge this PR but don't adjust the wording around that too this will just create more confusion. |
@@ -3,7 +3,7 @@ FROM falcosecurity/falco:${FALCO_IMAGE_TAG} | |||
|
|||
LABEL maintainer="cncf-falco-dev@lists.cncf.io" | |||
|
|||
LABEL usage="docker run -i -t -v /dev:/host/dev -v /proc:/host/proc:ro -v /boot:/host/boot:ro -v /lib/modules:/host/lib/modules:ro -v /usr:/host/usr:ro --name NAME IMAGE" | |||
LABEL usage="docker run -i -t --privileged -v /root/.falco:/root/.falco -v /proc:/host/proc:ro -v /boot:/host/boot:ro -v /lib/modules:/host/lib/modules:ro -v /usr:/host/usr:ro -v /etc:/host/etc:ro --name NAME IMAGE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at what we do here https://falco.org/docs/running/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that this ☝️ is the falco-driver-loader
image and I have used the documentation as source, indeed it matches:
docker pull falcosecurity/falco-driver-loader:latest
docker run --rm -i -t \
--privileged \
-v /root/.falco:/root/.falco \
-v /proc:/host/proc:ro \
-v /boot:/host/boot:ro \
-v /lib/modules:/host/lib/modules:ro \
-v /usr:/host/usr:ro \
-v /etc:/host/etc:ro \
falcosecurity/falco-driver-loader:latest
Am I missing something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment
Good point, I have not considered that. Anyway I believe is still convenient
WDYT? I had already prepared a PR to update the doc 👉 falcosecurity/falco-website#313, so if you agree, I can update it again accordingly. |
I agree, if we want to stick with the least privileged wording. |
👍 Yes, I prefer this option, since having different defaults across images can be more confusing. I will update the doc PR ASAP. |
Hey @fntlnz done, PTAL 👉 https://github.com/falcosecurity/falco-website/pull/313/files And thank you for the suggestion! 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked at the #313 PR on the falco-website, and the edits you did to this one.
/lgtm
LGTM label has been added. Git tree hash: 505c4ac1b892636f9f9a12afd2047077f0379088
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fntlnz, leodido The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
/kind cleanup
Any specific area of the project related to this PR?
/area build
What this PR does / why we need it:
This PR solves the problem described in #1491 and makes env vars consistent between the "falco" and the "no-driver" docker images.
It also updates the container labels with the correct docker usage command line.
Which issue(s) this PR fixes:
Fixes #1491
Special notes for your reviewer:
/milestone 0.27.0
Does this PR introduce a user-facing change?: