images: base the default image on distroless/base

[marquiz]

Make distroless/base as the base image for the default image, effectively making the minimal image as the default. Add a new "full" image variant that corresponds the previous default image. The "*-minimal" container image tag is provided for backwards compatibility.

The practical user impact of this change is that hook support is limited to statically linked ELF binaries. Bash or Perl scripts are not supported by the default image, anymore, but the new "full" image variant can be used for backwards compatibility.

Refs #855
Fixes #963

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	cd62f65
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/63d8dfd5c3714000099be02f
😎 Deploy Preview	https://deploy-preview-1027--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[marquiz]

Let's get some feedback
/hold

[fmuyassarov]

/assign

[fmuyassarov]

This looks good to me @marquiz and sorry for such a late review.
I had some nits below, but +1 for the change.

[fmuyassarov]

Suggested change

	-t $(IMAGE_TAG) \
	-t $(IMAGE_TAG)-minimal \
	$(foreach tag,$(IMAGE_EXTRA_TAGS),-t $(tag) -t $(tag)-minimal) \
	$(IMAGE_BUILD_EXTRA_OPTS) ./
	-t $(IMAGE_TAG)-minimal \
	$(foreach tag,$(IMAGE_EXTRA_TAGS),-t $(tag)-minimal) \
	$(IMAGE_BUILD_EXTRA_OPTS) ./

[marquiz]

We still definitely want to keep the "default" image tag (i.e. just v0.13.0 without any -minimal or -full suffix). As explained in the commit message this PR keeps the -minimal tag for backwards compatibility. Makes sense?

[fmuyassarov]

ah right.

[fmuyassarov]

One comment I forgot to mention is, I think we should add a note in the sources.local.hooksEnabled section of the document that the user need to use full image if they are using hooks, like we have notes about hooks deprecation, etc.

[marquiz]

One comment I forgot to mention is, I think we should add a note in the sources.local.hooksEnabled section of the document that the user need to use full image if they are using hooks, like we have notes about hooks deprecation, etc.

That's not entirely true as the "non-full" image (i.e. default or minimal) is able to run hooks but they must be statically linked binaries. Anyway, I added a not about that here, too

[fmuyassarov]

One comment I forgot to mention is, I think we should add a note in the sources.local.hooksEnabled section of the document that the user need to use full image if they are using hooks, like we have notes about hooks deprecation, etc.

That's not entirely true as the "non-full" image (i.e. default or minimal) is able to run hooks but they must be statically linked binaries. Anyway, I added a not about that here, too

if I hook in a shell script, will I be able to run it on distroless image based container?

[fmuyassarov]

my understanding was such that if I inject a shell script then NFD tries to run it (runHook) on a container where there is no runtime available but I guess NFD doesn't require any shell to be available to do it is job but it is rather users who might want to hack in or debug.
/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 8e3d88655f420d684be5a649091abc4a04c9102d

[marquiz]

my understanding was such that if I inject a shell script then NFD tries to run it (runHook) on a container where there is no runtime available

This is correct. If you have a shell script hook and try to run that in the minimal container it will just fail because there is no shell executable inside the container. But you can use a statically linked ELF binary, it doesn't require a shell or virtually any libraries

but I guess NFD doesn't require any shell to be available to do it is job but it is rather users who might want to hack in or debug.

Yes, NFD itself does not require any shell

[marquiz]

@zvonkok @mythi @ArangoGutierrez @PiotrProkop et. al any thoughts on the change of swapping to the minimal image?

[PiotrProkop]

@zvonkok @mythi @ArangoGutierrez @PiotrProkop et. al any thoughts on the change of swapping to the minimal image?

+1 for me. We are using distroless/scratch containers where we can for security reasons.

[mythi]

@zvonkok @mythi @ArangoGutierrez @PiotrProkop et. al any thoughts on the change of swapping to the minimal image?

no objections. our source hooks are currently static Go binaries but we're also planning on moving away from them to feature hooks at some point.

[ArangoGutierrez]

that's a good point, maybe we can hold a bit on this, until we fully deprecate Hooks?

[ArangoGutierrez]

On issue : #856 (comment) @marquiz presents a road map to move to minimal as default, maybe we can add this to that?

[marquiz]

that's a good point, maybe we can hold a bit on this, until we fully deprecate Hooks?

What point do you mean? What do you mean by "full deprecation"? My thinking with this PR was to limit the usage of hooks before disabling them (by default). I.e. default to minimal image one step earlier than I outlined in #856. That would give the benefits of minimal image with still supporting existing users that have statically linked hooks. Others would still be backed up by the full image.

But I'm open to discussion. The question basically being are there many bash or perl hook users.

[ArangoGutierrez]

that's a good point, maybe we can hold a bit on this, until we fully deprecate Hooks?

What point do you mean? What do you mean by "full deprecation"? My thinking with this PR was to limit the usage of hooks before disabling them (by default). I.e. default to minimal image one step earlier than I outlined in #856. That would give the benefits of minimal image with still supporting existing users that have statically linked hooks. Others would still be backed up by the full image.

But I'm open to discussion. The question basically being are there many bash or perl hook users.

Bash probably, perl............... ...... .... ..

[marquiz]

So what do you think @ArangoGutierrez about merging this? Comments from others were favorable

[ArangoGutierrez]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, 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:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[marquiz]

/unhold