Skip to content

Conversation

@rbtr
Copy link
Collaborator

@rbtr rbtr commented Apr 18, 2022

Reason for Change:

Adds a CLI tool ("dropgz") for reliably deploying arbitrary files via a single self-contained binary/image.
Initial use case: to install the CNI binaries at runtime via an init container in the CNS daemonset.

Issue Fixed:

Requirements:

Notes:
dropgz is effectively a CLI wrapper around Go's embed.FS, allowing us to pack the contents of a directory in to a binary and deploy the contents back out to the filesystem on-demand. The contents of the binary are determined at build-time, and SHA checksums are computed and stored with the files in the embed FS to allow us to verify integrity of the deployed files (bonus, we can skip re-deploying if the existing file-on-disk has the correct SHA).

Q: Why not just do this with by copying the bins to deploy in to a busybox container (or other base image with a shell that can cp)?
A: Packing the files in to a Go bin lets us build in a scratch container with no shell, reducing the potential attack surface for a privileged container that is writing files out to the host disk (in our use-case, writing the CNI bins which are invoked by CRI on all Pod create/delete events on a K8s Node).

@rbtr rbtr self-assigned this Apr 18, 2022
@rbtr rbtr added enhancement cni Related to CNI. ci Infra or tooling. labels Apr 18, 2022
@rbtr rbtr changed the title Add gz dropper module for CNI installer Add dropgz module for CNI installer Apr 19, 2022
@rbtr rbtr closed this Apr 19, 2022
@rbtr rbtr reopened this Apr 19, 2022
@rbtr rbtr force-pushed the dropgz branch 2 times, most recently from ea28ed1 to a8824d2 Compare April 19, 2022 21:34
@rbtr rbtr marked this pull request as ready for review April 19, 2022 21:38
@rbtr rbtr requested review from matmerr and vakalapa as code owners April 19, 2022 21:38
@rbtr rbtr requested a review from thatmattlong April 21, 2022 21:52
@rbtr rbtr force-pushed the dropgz branch 3 times, most recently from 7672187 to 91f6fb5 Compare April 26, 2022 22:44
@rbtr rbtr force-pushed the dropgz branch 4 times, most recently from 2f70b51 to 9a85288 Compare June 4, 2022 00:05
Copy link

@wedaly wedaly left a comment

Choose a reason for hiding this comment

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

overall looks good! had a few small comments/questions

One other thing to think about is testing. I'd suggest validating that all the expected files have been embedded, can be written out, and have the correct checksums (better to catch problems before this gets deployed to a cluster)

Copy link
Member

@timraymond timraymond left a comment

Choose a reason for hiding this comment

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

Looks pretty good--no showstoppers that I can see. I have a couple of suggestions and questions though.

Comment on lines +58 to +59
skipVerify bool
outs []string
Copy link
Member

Choose a reason for hiding this comment

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

Might be helpful to document that these are flags... it took me a second or two to figure out why they were globals

rbtr added 3 commits June 27, 2022 22:21
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
wedaly
wedaly previously approved these changes Jun 27, 2022
Copy link
Member

@tamilmani1989 tamilmani1989 left a comment

Choose a reason for hiding this comment

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

unit tests are missing for this new package.

ARG VERSION
WORKDIR /azure-container-networking
COPY . .
RUN CGO_ENABLED=0 go build -a -o bin/azure-vnet -trimpath -ldflags "-X main.version="$VERSION"" -gcflags="-dwarflocationlists=true" cni/network/plugin/main.go
Copy link
Member

Choose a reason for hiding this comment

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

build with aimetadata?

tamilmani1989
tamilmani1989 previously approved these changes Jun 28, 2022
Copy link
Member

@tamilmani1989 tamilmani1989 left a comment

Choose a reason for hiding this comment

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

lgtm.. please add work item to cover unit tests in separate PR as discussed offline

@rbtr rbtr enabled auto-merge (squash) June 28, 2022 18:26
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@rbtr rbtr dismissed stale reviews from tamilmani1989 and wedaly via 7d4f324 June 28, 2022 19:09
@rbtr rbtr merged commit c391f67 into Azure:master Jul 15, 2022
@rbtr rbtr deleted the dropgz branch July 15, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Infra or tooling. cni Related to CNI. enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants