libcontainer: dmz: fix "go get" builds#4101
libcontainer: dmz: fix "go get" builds#4101AkihiroSuda merged 1 commit intoopencontainers:mainfrom cyphar:runc-dmz-go-get-build
Conversation
|
FWIW, I tested this with my cAdvisor PR using and without |
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
LGTM
I've also checked if it makes sense to get rid of debug/elf dependency in order to reduce runc binary size, as we only use a single constant from it.
Apparently not, probably because github.com/cilium/ebpf also uses it.
libcontainer/dmz/dmz_linux.go
Outdated
| // There is an empty file called runc-dmz-dummy-file.txt in libcontainer/dmz in | ||
| // order to work around the restriction that go:embed requires at least one | ||
| // file to match the pattern. |
There was a problem hiding this comment.
A nit. AFAIK we don't need runc-dmz-dummy-file.txt in libcontainer/dmz anymore since we have .gitignore in there (which, by the way, is also being embedded).
Or, we need to do
-//go:embed binary
+//go:embed binary/runc-*so that .gitignore is not included.
Either way is fine.
There was a problem hiding this comment.
Actually .gitignore is not embedded because files starting with _ and . are not included by go:embed when embedding a directory unless you use the all: prefix:
If a pattern names a directory, all files in the subtree rooted at that directory are embedded (recursively), except that files with names beginning with ‘.’ or ‘_’ are excluded. [...] If a pattern begins with the prefix ‘all:’, then the rule for walking directories is changed to include those files beginning with ‘.’ or ‘_’. For example, ‘all:image’ embeds both ‘image/.tempfile’ and ‘image/dir/.tempfile’.
If you remove runc-dmz and dummy-file.txt as well as the go:generate line in libcontainer/dmz/dmz_linux.go (to emulate using go get on runc), you'll find that the build fails with the same error as before.
|
|
Because runc-dmz is not checked into Git, go get will end up creating a copy of libcontainer/dmz with no runc-dmz binary, which causes external libcontainer users to have compilation errors. Unfortunately, we cannot get go:embed to just ignore that there are no files matching the provided pattern, so instead we need to create a dummy file that matches the go:embed (which we check into Git and so go get _will_ copy) and switch to embed.FS. This is a little bit uglier, but at least it will fix external libcontainer users. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
| // Verify that our embedded binary has a standard ELF header. | ||
| if !bytes.HasPrefix(runcDmzBinary, []byte(elf.ELFMAG)) { | ||
| if len(runcDmzBinary) != 0 { | ||
| logrus.Infof("misconfigured build: embedded runc-dmz binary is non-empty but is missing a proper ELF header") |
There was a problem hiding this comment.
The problem is that we cannot tell if the embedded binary is broken at build-time, so making this an error would mean someone with an incredibly broken build system could result in a runc binary that fails to start any containers (when the alternative is we safely fallback to binary cloning).
I guess you could argue that at some point, we can't protect people from their own carelessness, but given that this can only be checked at runtime and falling back to /proc/self/exe cloning is completely safe, I feel like this is the right balance.
4 participants
Because runc-dmz is not checked into Git, go get will end up creating a copy of libcontainer/dmz with no runc-dmz binary, which causes external libcontainer users to have compilation errors.
Unfortunately, we cannot get go:embed to just ignore that there are no files matching the provided pattern, so instead we need to create a dummy file that matches the go:embed (which we check into Git and so go get will copy) and switch to embed.FS.
This is a little bit uglier, but at least it will fix external libcontainer users.
Fixes #4096
Signed-off-by: Aleksa Sarai cyphar@cyphar.com