-
Notifications
You must be signed in to change notification settings - Fork 2.5k
refactor: Use embed package for manifests directory #12670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Emin Umut Gercek <[email protected]>
In what way does this modify the "attack surface" of this project? How does the current method of embedding comprise a vulnerability or avenue of attack? We could be open to switching embedding libraries, but I'd like to see a specific reason to do so. And we would not be open to anything that drops compression, as we are highly sensitive to anything that will increase the size of our release artifact. Also note that there are two k3s binaries - the self-extracting wrapper, and the actual cli that is extracted out, or used standalone in the docker image. Have you confirmed that the size of both is not impacted? |
After using
I also believe this simplifies maintenance, since there's no additional step to build these manifests, which is easier to develop and debug.
Sorry for my lack of k3s building, I did below steps to find binary size changes: # Finding master size
git clone https://github.com/k3s-io/k3s.git
cd k3s
mkdir -p build/data && make download && make generate
make
ls -la dist/artifacts/ > /tmp/master-size
# Find this PR's branch size
git remote add mine https://github.com/eugercek/k3s.git
git fetch mine
git switch -c embed-manifests mine/embed-manifests
make
ls -la dist/artifacts/ > /tmp/pr-size
echo "Printing size differences"
echo "Master:"
cat /tmp/master-size
echo "PR:"
cat /tmp/pr-size And it looks like binary size is smaller in this version, below is the size differences. Let me know if this is not proper test. (Re-run the build steps for proper data, and double checking).
master:
this pr:
|
I appreciate that you took the time to address some tech debt. Embed was not available at the time we started using go-bindata, and our CI and build pipeline ensures that the content does not drift so I don't believe that there is any vulnerability present here. That said, referring to the current state of things as being "obfuscated" (which implies malicious intent) or comprising an "attack surface" is not correct, and sets an unnecessarily contentious tone. If you'd like to update the title and PR template to describe this in terms of addressing tech debt, instead of playing up the current state as somehow insecure, I think it's something we could approve. We have had issues in the past when accepting PRs that advertise things as security fixes - having to clean up externally-issued CVEs with incorrect information, for example. |
A simple way of refactoring is realized: 06689013 |
@muicoder that does appear to be a much more minimal change than what's proposed here. Has the same easily-addressed issues with missing build tags, but lgtm other than that. |
Hi @brandond — My intent wasn’t to be contentious; any sharpness was likely a language issue on my side. My goal was to reduce maintenance overhead, I use k3s daily and wanted to contribute back. With the new PR, embedding all static assets is good idea, will simplify even more. Should I close this PR? |
Thanks for pointing this out as a potential improvement @eugercek - as per the other PR, go-bindata has now been completely removed from k3s. |
Proposed Changes
This PR migrates the manifests in
manifests/
fromgo‑bindata
to Go’s built‑inembed
package.Unlike other externally downloaded assets, the files under
manifests/
are already stored as plain‑text in the repository, which makesgo:embed
the straightforward and more secure choice.go‑bindata
is useful when content is dynamically generated or when files are so large that they shouldn’t live in the repo, but the manifests fit neither case. Keeping them as a gzip‑compressed blob, complicates audits, and resembles the hidden‑payload pattern exploited in the XZ Utils backdoor.Types of Changes
Added
manifests/embed.go
, which contains the//go:embed
directive and two small helper functions that abstract access to the embedded files. Ifgo:embed
gains new options in the future or any other reason to add functionality, only the helper functions need to be updated and callers remain unchanged. Abstraction provides a small and simple interface.Verification
I built from source then run
dist/artifacts/k3s server
and checked manifests directory:Example output
Testing
Not covered.
Linked Issues
Per the contribution guide: "If you’re fixing a small issue, you can simply submit a PR and we will pick it up", I've opened this PR without a separate GitHub issue.
User-Facing Change
Further Comments
Design decisions:
manifests
it may complicate things for other projects etc, just ignoredembed.go
inList
method.fs.Walkdir
, another approach may befs.Glob
code will be simpler but this is more future proof, adding new hierarchies etc are easier.List
to make api more precise. This package should be very simple and straightforward IMO.Listed trade-offs of this PR below:
Cons:
go:embed
, k3s usesgo-bindata
in multiple places.Pros:
02c898d
) and my branch.Example sizes
Other changes comes from
make format
.Thanks for your time.