runc
runc copied to clipboard
opencontainers
Disable runc-dmz by default
There are several issues that we caught at the last minute. Several maintainers said it seems safer to disable it by default, so let's do that.
runc-dmz is marked as experimental and besides the build tag you need to opt-in with RUNC_DMZ=true to use it.
Future PRs can go further and maybe even remove the buildtag, simplifying the CI (and the time it takes to run).
cc @cyphar @kolyshkin @lifubang
Grr, there are some github actions that compile with no_dmz, that of course are of no use now. I'll be on PTO starting tomorrow EOD, can someone please pick this up and carry it to the merge line? :)
See dac417174 for what was added by DMZ.
I was about to write a similar PR 😅. I'll close this once I send that one. I'll also be on PTO from next week.
@cyphar as you didn't open a PR, I've changed this one to do that. Let me know what you think, and feel free to open a PR that replaces this.
Suse mirrors are breaking the github actions, but it was passing all the tests before
@kolyshkin for some reason I can't answer to your comment, but I've fixed it. PTAL :)
@rata @cyphar To me, using both build tag and an env var seems to be an overkill. I think we can drop the build tag entirely (and maybe also as well as selinux compat feature).
@kolyshkin I agree, that is why I mentioned it in the PR description. But I wouldn't do it in 1.2, as we are introducing the machinery to cross-compile C, just for runc-dmz. I would have the build-tag in case there si an issue (cross-compiling C is not as easy as cross-compiling go) and we haven't ever released with this machinery.
I would keep it, if it fails people can easily avoid it and build anyways (with the same behavior as runc-dmz is disabled by default), we can see the bug reports and fix it without a huge impact as they have other trivial alternatives.
In 1.3 we can remove the build-tag (even if we don't remove runc-dmz) and that is all.
What do you think?
we are introducing the machinery to cross-compile C, just for runc-dmz
Maybe I do not understand, what do you mean by "cross-compile"? We compile runc-dmz for the same architecture as the main runc (and we already have some C code in runc, see libcontainer/nsenter). IOW, we do not cross-compile runc-dmz (unless we also cross-compile runc).
Perhaps we can leave runc_nodmz as is, meaning dmz is compiled in and the resulting binary is is dmz-capable by default. If someone is having trouble with compilation because of runc-dmz, they can add runc_nodmz build tag.
The next step is what to do about enabling dmz during runtime. We have agreed that we should have it disabled by default. I'm not sure if using an environment variable is the best way to do that -- I remember @crosbymichael being very against using environment to affect how a command works (but I don't remember why exactly, maybe Michael himself or @thaJeztah can shed some light). But for now, let's say, we want RUNC_DMZ=yes or somesuch to be present in the environment for runc to invoke dmz.
Maybe I do not understand, what do you mean by "cross-compile"? We compile runc-dmz for the same architecture as the main runc (and we already have some C code in runc, see libcontainer/nsenter). IOW, we do not cross-compile runc-dmz (unless we also cross-compile runc).
Having said that, yes, we do cross-compile runc ourselves (that includes C stuff in runc-dmz and libct/nsenter) as we want to provide release binaries for multiple architectures. That doesn't mean that most users do it -- I suspect they all compile natively (that's what I'd do).
But for now, let's say, we want
RUNC_DMZ=yesor somesuch to be present in the environment for runc to invoke dmz
@kolyshkin I've updated it to do just that.
This PR changes the buildtag from
runc_nodmztorunc_dmz, the build-tag is enabled by default but runc-dmz is only used whenRUNC_DMZ=trueis set.
I see this sentence in your PR description, but I think this is not implemented in your PR. @rata
And there is another small issue in the main branch: https://github.com/opencontainers/runc/blob/main/Makefile#L82 , could you please fix it in this PR as well? It should be:
rm -f runc runc-* libcontainer/dmz/binary/runc-dmz
I see this sentence in your PR description, but I think this is not implemented in your PR. @rata
Yes, I changed it as @kolyshkin asked. I've updated the PR description to reflect the current state.
And there is another small issue in the main branch: https://github.com/opencontainers/runc/blob/main/Makefile#L82 , could you please fix it in this PR as well? It should be:
Fixed, thanks!
Since RUNC_DMZ is now opt-in, IMHO we can remove the logic in libcontainer/dmz/selinux*.go. We must keep isDmzBinarySafe because we don't want an environment variable to make containers unsafe, but the compatibility logic isn't necessary if the user is explicitly opting in to the feature IMHO (for the same reason we are not going to add all of the capability-related compatibility code once we make runc-dmz opt-in).
WDYT @AkihiroSuda @kolyshkin?
Aside from that, looks good.
Can we just merge this, release v1.2.0 RC, and work on refactoring in a follow-up PR?