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

Use Distroless as base image #13556

Merged
merged 2 commits into from Jan 22, 2022
Merged

Conversation

yank1
Copy link

@yank1 yank1 commented Dec 23, 2021

Use Distroless as base image to reduce attack surface and image size.

Metioned by #13459 , #10804 and #10805 .

Signed-off-by: yankay kay.yan@daocloud.io

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Signed-off-by: yankay <kay.yan@daocloud.io>
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

I haven't followed previous discussions about Etcd base image, however I'm fully supporting for switching to distroless image for security reasons.

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

SGTM.

Could you please add it to changelog, as it might be important that some debugging tools will not be available within the image.

@yank1 yank1 force-pushed the use-distroless-as-base-image branch from 45e8346 to 94c426f Compare January 18, 2022 12:26
@yank1
Copy link
Author

yank1 commented Jan 18, 2022

Thanks for comments, the message have been add to changelog

SGTM.

Could you please add it to changelog, as it might be important that some debugging tools will not be available within the image.

Signed-off-by: yankay <kay.yan@daocloud.io>
Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Thank you. Will merge when tests are green.

@joestringer
Copy link

Hi @yank1 , I see that this changed the base image to gcr.io/distroless/base-debian11. However, it seems like etcd is compiled statically so it may be possible to further reduce the attack surface by switching to gcr.io/distroless/static-debian11. Did you consider this option?

@serathius
Copy link
Member

It struck me when reading #14792. Busybox is a GPL licensed https://hub.docker.com/_/busybox https://www.busybox.net/license.html

Not a expert, but I think there is a risk of etcd breaking busybox license by distributing docker image that busybox. My recommendation would be to remove it.

Problem is that we backported this PR and released v3.4.23. I would recommend to remove/replace docker images.
cc @ahrtr @ptabor @spzala.

@ptabor
Copy link
Contributor

ptabor commented Dec 21, 2022

Thank you Marek, for spotting this.

My thoughts are following:

  1. I think it didn't introduced any regression. Full debian (k8s.gcr.io/build-image/debian-base:bullseye-v1.1.0) contained also e.g. bash, that is GPL licensed software: https://www.gnu.org/licenses/gpl-3.0.html
  2. Thus I wouldn't perform any replacement in 3.4.23.
  3. Having said that, I think we might not need the busybox at all:

@ahrtr
Copy link
Member

ahrtr commented Dec 22, 2022

It struck me when reading #14792.

I assume you were talking about #15017 or #15016

Not a expert, but I think there is a risk of etcd breaking busybox license by distributing docker image that busybox

Shouldn't it be OK as long as etcd's an open source project?

3. Having said that, I think we might not need the busybox at all:

The only impact would be that users can't log into a running container firstly and then execute command (e.g etcdctl). Instead, they can do it in just one command.

@iavael
Copy link
Contributor

iavael commented Jan 20, 2023

@serathius if etcd doesn't link to busybox, then it's fine

@ahrtr
Copy link
Member

ahrtr commented Jan 20, 2023

FYI. #15037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants