Use distroless image to build controller and speaker containers #2418

minmzzhang · 2024-06-03T13:11:03Z

Inspired by #2117
fixes #2110

A big thank you to @oribon and @georgettica for the detail discussion in #2117 which is the design document I followed to this solution. Also appreciate @oribon 's help during design implementation, that I was able to run local integration and e2e tests!

Is this a BUG FIX or a FEATURE ?:

/kind feature

What this PR does / why we need it:
gcr.io/distroless/static base image is used to replace the original docker.io/alpine image, for a distroless solution that controller and speaker images will be built from scratch. Since distroless base image has no shell entry, there are two major impacts with this solution:

Some of the frr init containers which need to move files around will need a local copy tool.
Some of the e2e tests which need to execute certain commands (e.g. "ifconfig") from inside the speaker will need to have an ephemeral container attached to the pod to complete the task.

In this PR, a local copy tool (cp-tool) was introduced to cope with the first concern, and a podDebugExecutor was introduced under e2etest to attach a debug ephemeral container when there is a need to execute additional commands.

Special notes for your reviewer:

Release note:

The speaker and the controller containers are now based on a distroless image, reducing the attack surface

georgettica · 2024-06-03T14:29:11Z

Also, the cp-tool is a good addition to solve this. Thank you!

One thing I thought of that might happen is if you debug twice on the same pod. It'll fail I reckon.

Not that I think it'll happen now, but just adding here if someone hits this bug and wants to resolve.

Amazing work and I thank you deeply for making the efforts we had come to a reality, I am truly glad

oribon

awesome work! thanks a lot
can you also squash your commits into one with the gist of the change?(https://metallb.io/community/#commit-messages)

oribon · 2024-06-05T07:09:46Z

e2etest/pkg/executor/executor.go

+	if Kubectl == "" {
+		return "", errors.New("the kubectl parameter is not set")
+	}
+	fullargs := append([]string{"debug", "-it", "-n", pd.namespace, pd.name, "-c", "pod-executor-debugger", "--image=busybox", "--", cmd}, args...)


I think we should randomize the container name here pod-executor-debugger-.., the reason being that if you run the same debug container multiple times it only remembers the first output (the first debug command that caused the container to be terminated) which could cause confusion if the test is ran multiple times on the same cluster

A randomized string (length=5) was added to the pod executor debugger.

oribon · 2024-06-05T07:11:20Z

e2etest/pkg/executor/executor.go

+// add ephemeral container to deal with distroless image
+type podDebugExecutor struct {
+	namespace string
+	name      string


wdyt about addibg an image for the container here (instead of defaulting to busybox)? this will make the call for ForPodDebug more resemblant to the usual kubectl debug where the caller specifies the image.

added the randomized string for debugger.

image name is configurable now.

oribon · 2024-06-05T07:14:20Z

frr-tools/cp-tool/cp-tool.go

+	sourceFile = filepath.Clean(sourceFile)
+	destinationFile = filepath.Clean(destinationFile)
+
+	err = cp.Copy(sourceFile, destinationFile)


did you try something that doesn't require importing another dependency? I wonder if we can just do this with some io.Copy (or anything else from the standard library) that wouldn't be too messy since our usecase is pretty simple

Originally there was file permission issue for the local copy tool when using io.Copy, so I switched to a community package. With the new submission, I've added the preserve permission functionality, so it should be clean now.

oribon · 2024-06-05T07:15:22Z

frr-tools/cp-tool/cp-tool.go

+)
+
+func main() {
+	logger, err := logging.Init("error")


we don't really need a logger here, let's just panic / fmt.Printf on the few occasions where needed

logger removed.

oribon

left a few small comments, looks really good!
also, please change the commit message to be more about "what is the change" and not "how we changed" :)
approved, cc @fedepaol to also give this a look

oribon · 2024-06-10T09:22:41Z

frr-tools/cp-tool/cp-tool.go

+	sourceFile = filepath.Clean(sourceFile)
+	destinationFile = filepath.Clean(destinationFile)


small nit: this is probably redundant with the one happening inside the function

removed the redundant part.

oribon · 2024-06-10T09:23:52Z

frr-tools/cp-tool/cp-tool.go

+func CopyFile(srcFile string, destFile string) error {
+	srcFile = filepath.Clean(srcFile)
+	destFile = filepath.Clean(destFile)


nit: rename the input to src,dst (so the new vars are declared with new names)

renamed the inputs

oribon · 2024-06-10T09:24:14Z

frr-tools/cp-tool/cp-tool.go

+	}
+
+	srcPerm := srcStat.Mode().Perm()
+	fmt.Printf("File permission before copying %v \n", srcPerm)


nit: I think we can remove it (unless you think this adds value)

removed after checking the functionality.

oribon · 2024-06-10T09:24:25Z

frr-tools/cp-tool/cp-tool.go

+	dstStat, err := os.Stat(destFile)
+	if err != nil {
+		err = fmt.Errorf("failed to check stats for %s: %w", destFile, err)
+		return err
+	}
+
+	dstPerm := dstStat.Mode().Perm()
+	fmt.Printf("File permission After copying %v \n", dstPerm)


removed after checking the functionality.

fedepaol · 2024-06-11T15:10:01Z

e2etest/pkg/executor/executor.go

+		return "", errors.New("the kubectl parameter is not set")
+	}
+
+	imagearg := "--image=" + pd.image


nit: imageArg, targetArg, debuggerArg

fedepaol · 2024-06-11T15:16:46Z

e2etest/l2tests/interface_selector.go

@@ -170,7 +170,7 @@ var _ = ginkgo.Describe("L2-interface selector", func() {
 			}, 1*time.Minute, 1*time.Second).Should(Not(HaveOccurred()))
 			speakerPod, err := metallb.SpeakerPodInNode(cs, svcNode.Name)
 			Expect(err).NotTo(HaveOccurred())
-			selectorMac, err := mac.GetIfaceMac(NodeNics[0], executor.ForPod(speakerPod.Namespace, speakerPod.Name, "speaker"))
+			selectorMac, err := mac.GetIfaceMac(NodeNics[0], executor.ForPodDebug(speakerPod.Namespace, speakerPod.Name, "speaker", "busybox"))


the only concern I have here is relying on another image (from dockerhub, also), which may result in hitting the pull limits. How about using the agnhost image we are already using here (assuming it already has all we need to debug)?

good idea! changed.

fedepaol · 2024-06-11T15:20:37Z

website/content/troubleshooting/_index.md

@@ -288,3 +288,11 @@ Additionally, the status of the service and of the endpoints must be provided:
 kubectl get endpointslices <my-service> -o yaml
 kubectl get svc <my_service> -o yaml
 ```
+
+### How to debug a container with distroless image


Suggested change

### How to debug a container with distroless image

### How to debug the speaker and the controller containers

fedepaol · 2024-06-11T15:21:38Z

website/content/troubleshooting/_index.md

+
+### How to debug a container with distroless image
+
+With the distroless solution, if someone needs to debug inside speaker container, an ephemeral container should be used: 


Suggested change

With the distroless solution, if someone needs to debug inside speaker container, an ephemeral container should be used:

Due to the fact that both the speaker and the controller containers are based on a distroless image, an ephemeral container should be used to debug inside them:

fedepaol · 2024-06-11T15:23:02Z

Also, I'd reword the release notes section, saying that now the speaker and the controller are based on distroless.

Signed-off-by: Min Zhang <minzhang@redhat.com>

fedepaol · 2024-06-12T18:50:10Z

LGTM

minmzzhang requested review from fedepaol, russellb, gclawes and oribon as code owners June 3, 2024 13:11

github-actions bot added kind/feature and removed kind/feature labels Jun 3, 2024

minmzzhang force-pushed the distroless-try branch 3 times, most recently from d357807 to aa930fa Compare June 3, 2024 14:01

minmzzhang force-pushed the distroless-try branch 9 times, most recently from a04d45e to e7746b0 Compare June 4, 2024 14:24

oribon reviewed Jun 5, 2024

View reviewed changes

minmzzhang force-pushed the distroless-try branch 4 times, most recently from 755b9b4 to 4878b57 Compare June 6, 2024 14:45

oribon approved these changes Jun 10, 2024

View reviewed changes

minmzzhang force-pushed the distroless-try branch 2 times, most recently from 34d7943 to 7370af4 Compare June 10, 2024 14:35

fedepaol reviewed Jun 11, 2024

View reviewed changes

minmzzhang force-pushed the distroless-try branch from cf72ae7 to a998591 Compare June 11, 2024 16:59

github-actions bot added kind/feature and removed kind/feature labels Jun 11, 2024

use distroless static image to build speaker and controller

c3439b7

Signed-off-by: Min Zhang <minzhang@redhat.com>

minmzzhang force-pushed the distroless-try branch from 5248b62 to c3439b7 Compare June 11, 2024 17:33

github-actions bot added kind/feature and removed kind/feature labels Jun 11, 2024

fedepaol enabled auto-merge June 12, 2024 18:50

fedepaol added this pull request to the merge queue Jun 12, 2024

Merged via the queue into metallb:main with commit 66d8395 Jun 12, 2024
36 of 37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use distroless image to build controller and speaker containers #2418

Use distroless image to build controller and speaker containers #2418

minmzzhang commented Jun 3, 2024 •

edited by fedepaol

Loading

georgettica commented Jun 3, 2024

oribon left a comment

oribon Jun 5, 2024

minmzzhang Jun 5, 2024

oribon Jun 5, 2024

minmzzhang Jun 5, 2024

minmzzhang Jun 5, 2024

oribon Jun 5, 2024

minmzzhang Jun 5, 2024

oribon Jun 5, 2024

minmzzhang Jun 5, 2024

oribon left a comment

oribon Jun 10, 2024

minmzzhang Jun 10, 2024

oribon Jun 10, 2024

minmzzhang Jun 10, 2024

oribon Jun 10, 2024

minmzzhang Jun 10, 2024

oribon Jun 10, 2024

minmzzhang Jun 10, 2024

fedepaol Jun 11, 2024

minmzzhang Jun 11, 2024

fedepaol Jun 11, 2024

minmzzhang Jun 11, 2024

fedepaol Jun 11, 2024

minmzzhang Jun 11, 2024

fedepaol Jun 11, 2024

minmzzhang Jun 11, 2024

fedepaol commented Jun 11, 2024

fedepaol commented Jun 12, 2024

		sourceFile = filepath.Clean(sourceFile)
		destinationFile = filepath.Clean(destinationFile)

	### How to debug a container with distroless image
	### How to debug the speaker and the controller containers


		### How to debug a container with distroless image

		With the distroless solution, if someone needs to debug inside speaker container, an ephemeral container should be used:

	With the distroless solution, if someone needs to debug inside speaker container, an ephemeral container should be used:
	Due to the fact that both the speaker and the controller containers are based on a distroless image, an ephemeral container should be used to debug inside them:

Use distroless image to build controller and speaker containers #2418

Use distroless image to build controller and speaker containers #2418

Conversation

minmzzhang commented Jun 3, 2024 • edited by fedepaol Loading

georgettica commented Jun 3, 2024

oribon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oribon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedepaol commented Jun 11, 2024

fedepaol commented Jun 12, 2024

minmzzhang commented Jun 3, 2024 •

edited by fedepaol

Loading