Add tests for distroless image #7

g-gaston · 2023-04-03T15:48:54Z

Follow up on #6

In order to run the tests in a distroless image, I rewrote them in Go so we can compile them to a static binary and just copy to the image. They work pretty much like the bash version. In addition they also check that the ip6tables binary resolves correctly. I also added a couple of tests for rules added for ip6.

The distroless image is pretty much a copy of what's used in https://github.com/kubernetes/release/blob/master/images/build/distroless-iptables/distroless, but with a few tweaks to drop the extra dependencies like dash. We could maybe simplify it if we didn't use the installer script.

g-gaston · 2023-06-20T19:16:50Z

@danwinship any chance you can take a look? 🙏🏻

danwinship · 2023-10-20T16:50:44Z

test/Dockerfile.test-distroless

+    ebtables    \
+    ipset       \
+    iptables    \
+    kmod		&& \


You don't need any of those except iptables.

Also, I would just install iptables normally, without trying to do any weird optimizations. We don't need to optimize the size of the test image, and we don't want to be making any sorts of guarantees to users that "this is a supported way of optimizing the size of your image". They can figure that out on their own. (Or copy kubernetes on their own.)

Changed, it only installs iptables now.

I kept the installations scripts I copied because that was faster than re-rewriting the dpkg stuff myself, so the weird optimization is still there (I guess you were referring to filtering everything except libraries and copyrights?). Please, take a look to the new version and if this still feels too complicated I'll rewrite the staging script.

danwinship · 2023-10-20T16:51:14Z

test/Dockerfile.test-distroless

+    iptables    \
+    kmod		&& \
+	`# below binaries and dash are used by iptables-wrapper-installer.sh` \
+    /stage-binary-and-deps.sh "${STAGE_DIR}" /bin/dash \


This is just the build image; none of these files should be needed in the final image, so there's no reason to do anything tricky with them, right? Just install the packages we need, run iptables-wrapper-installer.sh, and then copy stuff to the final image.

Hm... I guess it's tricky to define exactly what "stuff" you need to copy to the final image though. (You want to copy all the iptables-package-related stuff, but nothing else.) Maybe we should have a binary installer for the distroless case. We don't even need that much. Just say that you copy iptables-wrapper to the same directory as iptables (presumably /sbin) and then run iptables-wrapper --install, and it will make the 6 necessary symlinks. (You already have most of the code to do that in the binary anyway...) Everything else in iptables-wrapper-installer.sh is either superfluous (detecting fedora-vs-debian-vs-other) or optional (sanity-checking the iptables version).

Ah that's a good idea, I added this.

Now the wrapper can be run like iptables-wrapper install <path> and it will create symlink pointing to itself in the given path.

I went with the path being an explicit argument to avoid having to auto detect where iptables is. Depending on how you build the image, iptables might not exist neither in /sbin or /usr/sbin. In that case, I would then need to start looking for iptables-nft or iptables-legacy, which adds a bit more complexity. Or create a fake iptables executable somwhere, which is equivalent to passing the path explicitly. Let me know what you think.

test/Dockerfile.test-distroless

danwinship · 2023-10-20T19:19:30Z

test/Dockerfile.test-distroless

+
+FROM scratch
+
+COPY --from=intermediate / /


If you copy everything from intermediate into the final image, then why not just use the "intermediate" image as the final result?

Anyway, I think Dockerfile.test-distroless should look something like:

FROM debian:bullseye-slim as build # FIXME install iptables # FIXME use dpkg to figure out all the packages that iptables depends on # FIXME use dpkg to figure out all of the files in all of the packages from the previous list # FIXME copy all of those files to ${STAGE_DIR} FROM gcr.io/distroless/static COPY --from=build "${STAGE_DIR}" / COPY bin/iptables-wrapper /sbin RUN /sbin/iptables-wrapper --install COPY bin/tests /

Yeah this was great feedback. This is more or less what the dockerfile does now. But as I wrote in a previous comment, I kept the staging scripts.

test/build/stage-binaries-from-package.sh

danwinship · 2023-10-20T19:32:29Z

test/run-test.sh

+fi
+if ! docker run --privileged "iptables-wrapper-test-${tag}" /tests -test.v -test.run "^TestIPTablesWrapperNFT$" ; then
+    FAIL "failed nft iptables"
+fi


Is there some reason for running them separately like this rather than just doing a single docker run for the whole test suite?

Yeah, running one test modifies the container (the symlinks are altered to point to a particular mode).

So instead of building some kind of cleanup in the tests that try to undo all the changes and restore the initial state, I just run them in different containers. It seemed simpler.

danwinship · 2023-10-20T19:46:10Z

test/wrapper_test.go

+	if err != nil {
+		tb.Fatal(err.Error())
+	}
+}


so... yeah, I just think this is far too complicated. The existing shell-based test is short and simple. This is a lot more complex, and the only reason for doing it this way is that you can test that iptables-wrapper works correctly in an image that doesn't contain /bin/sh, which is something that we have no reason to believe would actually be a problem, given that iptables-wrapper never execs anything except iptables.

If we really want to test that case, a simpler way to do it would be to have a normal debian or alpine or whatever install at the top level, and have the "install" image still be in /opt/stage or whatever, and change test.sh so that instead of calling iptables ..., it calls chroot /opt/stage iptables .... Then test.sh can depend on whatever it wants, but iptables-wrapper is forced to only use the things in its chroot.

That's fair. Although this also adds tests for IPv6, which I think it represents a different codepath and wasn't tested before IIRC, so some of the complexity comes from there.

That said, I would prefer to keep this unless you feel extremely against it. I understand what you are saying about not needing to validate that this works in an image without shell. And for it is worth, I agree with you, we have no reason to think it wouldn't work. However, isn't that what the distroless test is trying to verify? That the iptables-wrapper works in a distroless image.

k8s-triage-robot · 2024-01-22T16:32:52Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

g-gaston · 2024-01-22T16:34:00Z

/remove-lifecycle stale
apologies for the delay, I will get back to this

k8s-triage-robot · 2024-04-21T17:22:55Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-05-21T17:29:24Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

g-gaston · 2024-05-22T17:30:55Z

/remove-lifecycle rotten

k8s-triage-robot · 2024-08-20T17:48:21Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-09-19T18:43:19Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

BenTheElder

re: test image: Let's just take the upstream distroless-iptables image as a base image and substitute in the new wrapper script/binary for testing?

that seems less fragile than maintaining a near-copy of it here.

danwinship · 2024-10-08T18:18:43Z

We don't want to test "it works with the current distroless-iptables that has all sorts of stuff to support the old iptables-wrappers" though, we want to test "it works in a distroless image".

The image that gets built by the current version of this PR is much more complicated than it needs to be, and I suggested some changes in review comments.

g-gaston · 2024-10-08T21:13:50Z

Hey folks! Sorry I completely forgot about this.
Let me try to simplify the distroless test image.

k8s-ci-robot · 2024-10-09T21:09:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: g-gaston
Once this PR has been reviewed and has the lgtm label, please assign dcbw for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

g-gaston · 2024-10-09T21:11:16Z

iptables-wrapper-installer.sh

-            --slave /usr/sbin/ip6tables iptables /usr/sbin/iptables-wrapper \
-            --slave /usr/sbin/ip6tables-restore iptables-restore /usr/sbin/iptables-wrapper \
-            --slave /usr/sbin/ip6tables-save iptables-save /usr/sbin/iptables-wrapper
+            --slave /usr/sbin/ip6tables ip6tables /usr/sbin/iptables-wrapper \


I could be wrong here bu I think before this change it wasn't creating symlinks for ip6tables-* because the name overlapped with the previous slaves. Or at least that's what I found during testing.

g-gaston · 2024-10-09T21:12:07Z

test/Dockerfile.test-debian

@@ -19,11 +19,11 @@ FROM debian:buster
 ARG INSTALL_ARGS=
 ARG REPO=buster

-RUN echo deb http://deb.debian.org/debian buster-backports main >> /etc/apt/sources.list; \
+RUN echo deb http://archive.debian.org/debian buster-backports main >> /etc/apt/sources.list; \


they changed this url for buster and that broke the test

g-gaston · 2024-10-09T21:40:14Z

/remove-lifecycle rotten

g-gaston · 2024-10-09T21:40:47Z

@danwinship addressed comments, pls take a look when you have some time
And sorry again for missing this

Add tests for distroless

093425d

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 3, 2023

k8s-ci-robot requested review from danwinship and dcbw April 3, 2023 15:49

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 3, 2023

danwinship reviewed Oct 20, 2023

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2024

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 21, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 21, 2024

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 22, 2024

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 20, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 19, 2024

BenTheElder reviewed Oct 8, 2024

View reviewed changes

BenTheElder mentioned this pull request Oct 8, 2024

migrate distroless-iptables image to new golang iptables-wrapper kubernetes/release#2992

Open

Simplify distroless image by moving installation to binary

d2556dc

g-gaston commented Oct 9, 2024

View reviewed changes

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for distroless image #7

Add tests for distroless image #7

g-gaston commented Apr 3, 2023

g-gaston commented Jun 20, 2023

danwinship Oct 20, 2023

g-gaston Oct 9, 2024

danwinship Oct 20, 2023 •

edited

Loading

g-gaston Oct 9, 2024

danwinship Oct 20, 2023

g-gaston Oct 9, 2024

danwinship Oct 20, 2023

g-gaston Oct 9, 2024

danwinship Oct 20, 2023

g-gaston Oct 9, 2024

k8s-triage-robot commented Jan 22, 2024

g-gaston commented Jan 22, 2024

k8s-triage-robot commented Apr 21, 2024

k8s-triage-robot commented May 21, 2024

g-gaston commented May 22, 2024

k8s-triage-robot commented Aug 20, 2024

k8s-triage-robot commented Sep 19, 2024

BenTheElder left a comment

danwinship commented Oct 8, 2024

g-gaston commented Oct 8, 2024

k8s-ci-robot commented Oct 9, 2024

g-gaston Oct 9, 2024

g-gaston Oct 9, 2024

g-gaston commented Oct 9, 2024

g-gaston commented Oct 9, 2024

Add tests for distroless image #7

Are you sure you want to change the base?

Add tests for distroless image #7

Conversation

g-gaston commented Apr 3, 2023

g-gaston commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship Oct 20, 2023 • edited Loading

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

k8s-triage-robot commented Jan 22, 2024

g-gaston commented Jan 22, 2024

k8s-triage-robot commented Apr 21, 2024

k8s-triage-robot commented May 21, 2024

g-gaston commented May 22, 2024

k8s-triage-robot commented Aug 20, 2024

k8s-triage-robot commented Sep 19, 2024

BenTheElder left a comment

Choose a reason for hiding this comment

danwinship commented Oct 8, 2024

g-gaston commented Oct 8, 2024

k8s-ci-robot commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-gaston commented Oct 9, 2024

g-gaston commented Oct 9, 2024

danwinship Oct 20, 2023 •

edited

Loading