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

Dockerfile: add minimal image #469

Merged
merged 4 commits into from
Mar 10, 2021

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Mar 9, 2021

Build a "minimal" variant of the nfd image based on
gcr.io/distroless/base. The motivations behind the minimal image are
image hardening (security) and reducing the image footprint (from ca.
108MB down to about 40MB).

The practical effect of deploying the minimal image is that no runtimes
for running worker hooks are present, not even a shell. This means that
only statically linked linked hook binaries are supported. Also, because
of the image hardening live debugging of the minimal image by attaching
to the container is not possible, and, the "full" image needs to be used
for that purpose.

This doesn't change the default usage/deployment of NFD in any way. Just adds a new image variant that can be alternatively deployed. The minimal variant might become the default in the future, though

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 9, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Mar 9, 2021

Pondering that should the minimal image be based on vgcr.io/distroless/base, instead. That would add a bit more flexibility to the hooks, supporting statically linked C binaries, for example.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Mar 9, 2021

/cc mythi kad

@k8s-ci-robot
Copy link
Contributor

@marquiz: GitHub didn't allow me to request PR reviews from the following users: mythi.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc mythi kad

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested a review from kad March 9, 2021 11:11
@zvonkok
Copy link
Contributor

zvonkok commented Mar 9, 2021

Makes sense. Moving forward if it becomes "the" default we should explicitly mention it in the documentation. We had #247 for explaining the runtime options. When already minimal we could also add readOnlyFilesystem: true to the security context.

@marquiz
Copy link
Contributor Author

marquiz commented Mar 9, 2021

Moving forward if it becomes "the" default we should explicitly mention it in the documentation.

Yes, sure. This PR adds a note about lack of runtimes in the minimal image .

@ArangoGutierrez
Copy link
Contributor

Debugging minimal images is...fun, but this PR looks good
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Mar 9, 2021

@zvonkok @mythi @kad @adrianchiris any thoughts on distroless/static vs. distroless/base?

@mythi
Copy link
Contributor

mythi commented Mar 10, 2021

@zvonkok @mythi @kad @adrianchiris any thoughts on distroless/static vs. distroless/base

Would distroless/static set too tight restrictions to existing non-static source hooks, i.e., users cannot move to minimal without also updating their own hooks?

@adrianchiris
Copy link
Contributor

adrianchiris commented Mar 10, 2021

@zvonkok @mythi @kad @adrianchiris any thoughts on distroless/static vs. distroless/base

So, going through this PR, i did not find the motivation to provide such an image (also no related issue on it).
e.g smaller footprint, security (is that obvious and should not be mentioned ?)

My main concern with this (and in the future moving to this image by default) are:

  1. Possibly breaking existing hooks which may not run or may run and fail due to the change to minimal image. These would need to be recompiled statically.
  2. Feature sources cannot rely on OS tooling (i know they dont today, but this kinda shuts the door down the road)
  3. Debugging

In regards to static vs base : i would go with base as to give hooks that are not written in go a chance.

@marquiz
Copy link
Contributor Author

marquiz commented Mar 10, 2021

So, going through this PR, i did not find the motivation to provide such an image (also no related issue on it).

Yeah, this was kind of quick spin-off from #221.

e.g smaller footprint, security (is that obvious and should not be mentioned ?)

Yes, these are the motivations behind this. I need to explain that in the commit message.

My main concern with this (and in the future moving to this image by default) are:

  1. Possibly breaking existing hooks which may not run or may run and fail due to the change to minimal image. These would need to be recompiled statically.

Yes. This might be too strict and base would be a better choice

  1. Feature sources cannot rely on OS tooling (i know they dont today, but this kinda shuts the door down the road)

True. We have been deliberately avoiding this dependency. I think it's good as we (at least so far) have detected low-level features that are mainly available from kernel interfaces.

This doesn't shut the door for using such tools in the future, though (if we choose the base image). We just need to install the tool and the libraries it depends on. I actually think that this is also good as it really makes you consider what and how to contain in the image.

  1. Debugging

The full image can be used for debugging. I think it's actually desirable from the security point of view to not be able to attach to running production containers. It vastly reduces the attack surface.

In regards to static vs base : i would go with base as to give hooks that are not written in go a chance.

Yes, after sleeping over this PR I'm inclined to agree with this.

@marquiz
Copy link
Contributor Author

marquiz commented Mar 10, 2021

Would distroless/static set too tight restrictions to existing non-static source hooks, i.e., users cannot move to minimal without also updating their own hooks?

Yes, I think so. Only supporting "fully static" binaries is probably unnecessarily strict, limiting hooks practically only to statically linked Go binaries. With base we will have gllibc which improves the support considerably

@marquiz
Copy link
Contributor Author

marquiz commented Mar 10, 2021

My main concern with this (and in the future moving to this image by default)

Moving to minimal image by default must be carefully considered and I don't see it happening anytime soon. We're kind of held captive by the hooks, which I don't have any idea how widespread their usage is 🤔 Might be that nobody uses them and would even notice the change

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2021
Build a "minimal" variant of the nfd image based on
gcr.io/distroless/base. The motivations behind the minimal image are
image hardening (security) and reducing the image footprint (from ca.
108MB down to about 40MB).

The practical effect of deploying the minimal image is that no runtimes
for running worker hooks are present, not even a shell. This means that
only statically linked linked hook binaries are supported. Also, because
of the image hardening live debugging of the minimal image by attaching
to the container is not possible, and, the "full" image needs to be used
for that purpose.
Poll all images that are supposed to be tagged. Both the "full" and
"minimal" variant, including tags created from IMAGE_EXTRA_TAG_NAMES.
Test both the full and minimal image. This might be paranoid but better
be sure(r) than sorry. The end-to-end tests are not that expensive.
@marquiz
Copy link
Contributor Author

marquiz commented Mar 10, 2021

Changed the base image to be base on distroless/base. Improved commit message of the first patch.

@adrianchiris
Copy link
Contributor

Thanks for addressing my comments @marquiz.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Mar 10, 2021

Looks like there's a sort of consensus on this. And, the minimal image is optional, anyway.
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit be2a051 into kubernetes-sigs:master Mar 10, 2021
@marquiz marquiz mentioned this pull request Mar 16, 2021
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants