Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Harden Docker container #2153

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Harden Docker container #2153

wants to merge 5 commits into from

Conversation

hazcod
Copy link

@hazcod hazcod commented Nov 13, 2019

  • produce go static builds
  • remove unused libraries
  • remove shell

@CLAassistant
Copy link

CLAassistant commented Nov 13, 2019

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@zwass zwass left a comment

Choose a reason for hiding this comment

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

Very cool! Thank you for your contribution.

Dockerfile Outdated Show resolved Hide resolved
@@ -107,10 +107,10 @@ endif
build: fleet fleetctl

fleet: .prefix .pre-build .pre-fleet
go build -i -o build/${OUTPUT} -ldflags ${KIT_VERSION} ./cmd/fleet
go build -i -o build/${OUTPUT} -ldflags ${KIT_VERSION} -ldflags "-w -s -extldflags '-static'" ./cmd/fleet
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary when using base-debian10? https://github.com/GoogleContainerTools/distroless/blob/master/base/README.md seems to suggest that this may not be needed.

Copy link
Author

Choose a reason for hiding this comment

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

Exactly right, I've changed it to static since it now receives glibc from the original go image used to built the code from .circleci/. Seems clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry it wasn't clear, but my preferred strategy would be to leave the build the same while using the base-debian10 image. Do you see a compelling reason to change the build flags for all binaries vs. using the base image?

Copy link
Author

Choose a reason for hiding this comment

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

@zwass care to share your relationale behind that? Since distroless does not have an update policy, there is no guarantee when they pull in new glibc versions so it would make more sense to me to package that in during compilation. GoogleContainerTools/distroless#326

Copy link
Contributor

Choose a reason for hiding this comment

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

My rationale is that we are changing the build flags for all the binaries produced by this Makefile, not just those being placed in the Docker container. Maybe we can refactor the Makefile in such a way to enable the fully static build for the binary used in the container while leaving other binary builds unchanged?

Copy link
Author

@hazcod hazcod Nov 20, 2019

Choose a reason for hiding this comment

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

I think it would make more sense to build fleet in a multi-stage container, what do you think? So we can move GOOS=linux go build -i -o build/linux/${OUTPUT} -ldflags ${KIT_VERSION} ./cmd/fleet to the build container.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am on board with that strategy. It will likely require running make deps-js && make generate-js in a Node container, then make deps-go && make generate-go && make in a Go container.

@hazcod hazcod requested a review from zwass November 13, 2019 19:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants