Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Factory cleanup #3354

Merged
merged 6 commits into from
Feb 11, 2022
Merged

Factory cleanup #3354

merged 6 commits into from
Feb 11, 2022

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jan 27, 2022 •
edited
Loading

This used to be part of #3316.

This set cleanups LinuxFactory, removing some interfaces, option functions etc. Lots of changes, but mostly removal.

This removes about 6K of code and 5K of data from the binary, reducing its overall size by 0.1% (not counting debuginfo, i.e. for a stripped binary) without any change in functionality.

More importantly, this improves readability a notch by simplifying some code and data structures, and removing about 200 lines of code and tests.

@kolyshkin kolyshkin mentioned this pull request Jan 27, 2022
Closed
7 tasks
@kolyshkin kolyshkin marked this pull request as ready for review February 1, 2022 01:33
@kolyshkin
Copy link
Contributor Author

Release job failed because of a network glitch:

E: Failed to fetch https://provo-mirror.opensuse.org/repositories/devel:/tools:/criu/Debian_11/amd64/criu_3.16.1-3_amd64.deb > Could not connect to provo-mirror.opensuse.org:443 (91.193.113.70). - connect (111: Connection refused)

Restarted.

@kolyshkin
Copy link
Contributor Author

@opencontainers/runc-maintainers PTAL (I have more changes pending that depend on these commits)

@kolyshkin
libct: Container, Factory: rm InitPath, InitArgs
630c0d7 
Those are *always* /proc/self/exe init, and it does not make sense
to ever change these. More to say, if InitArgs option func (removed
by this commit) is used to change these parameters, it will break
things, since "init" is hardcoded elsewhere.

Remove this.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
libct: remove Validator interface
0d21515 
We only have one implementation of config validator, which is always
used. It makes no sense to have Validator interface.

Having validate.Validator field in Factory does not make sense for all
the same reasons.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
libct: Container, Factory: rm newuidmap/newgidmap
39bd7b7 
These were introduced in commit d8b6694 back in 2017, with a TODO
of "make binary names configurable". Apparently, everyone is happy with
the hardcoded names. In fact, they *are* configurable (by prepending the
PATH with a directory containing own version of newuidmap/newgidmap).

Now, these binaries are only needed in a few specific cases (when
rootless is set etc.), so let's look them up only when needed.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
thaJeztah
thaJeztah reviewed Feb 3, 2022
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

One comment, but otherwise looks good

libcontainer/container_linux.go Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Contributor Author

Should we use golang.org/x/sys/execabs for these? (https://github.com/golang/sys/tree/master/execabs)

Interesting.

From my assessment, it seems we are fine. If the PATH contains . and newuidmap/newgidmap are present in the current directory, they will be used. Since they are executed from the same context as the user (we cd $BUNDLE before doing the lookup, and we're still in the same directory when we call newuidmap/newgidmap, so there is no issue; it works as intended.

Now, theoretically, if the code does cd after calling ExecLookup and before calling newuidmap/newgidmap, a different binary might be called, which is probably still OK, since the binary is still the host binary (i.e. it is not part of the container image downloaded from a random place).

Nevertheless, out of abundance of caution it might make sense to not allow a non-absolute path to newuidmap.

@kolyshkin
Copy link
Contributor Author

Should we use golang.org/x/sys/execabs for these? (https://github.com/golang/sys/tree/master/execabs)

Added. I also took a look at other uses of exec.LookPath and exec.Command, and it seems that these should be left as is.

@kolyshkin
libct/start: use execabs for newuidmap lookup
9258eac 
Since we are looking up the path to newuidmap/newgidmap in one context,
and executing those in another (libct/nsenter), it might make sense to
use a stricter rules for looking up path to those binaries.

Practically it means that if someone wants to use custom newuidmap and
newgidmap binaries from $PATH, it would be impossible to use these from
the current directory by means of PATH=.:$PATH; instead one would have
to do something like PATH=$(pwd):$PATH.

See https://go.dev/blog/path-security for background.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
libct: rm TestGetContainerStats, mockIntelRdtManager
8593285 
TestGetContainerStats test a function that is smaller than the test
itself, and only calls a couple of other functions (which are
represented by mocks). It does not make sense to have it.

mockIntelRdtManager is only needed for TestGetContainerStats
and TestGetContainerState, which basically tests that Path
is called. Also, it does not make much sense to have it.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
libct: rm intelrtd.Manager interface, NewIntelRdtManager
dbd990d 
Remove intelrtd.Manager interface, since we only have a single
implementation, and do not expect another one.

Rename intelRdtManager to Manager, and modify its users accordingly.

Remove NewIntelRdtManager from factory.

Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the
feature is not available.

Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew.

Add internal function newManager to be used for tests (to make sure
some testing is done even when the feature is not available in
kernel/hardware).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@thaJeztah
Copy link
Member

Thanks for looking at that; yes I was thinking along the same lines; in normal situations, runc and newgidmap would likely be living side-by-side already (which would be valid), but runc could be executed from some non-standard path with a bad binary next to it. Looks like the extra caution doesn't hurt us, so 👍

thaJeztah
thaJeztah approved these changes Feb 4, 2022
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, nice, thanks!

@kolyshkin
Copy link
Contributor Author

@opencontainers/runc-maintainers PTAL

@kolyshkin
Copy link
Contributor Author

@opencontainers/runc-maintainers PTAL 🙏🏻 (I have a few more cleanups on top of this)

@hqhq hqhq merged commit 657ed0d into opencontainers:main Feb 11, 2022
@hqhq
Copy link
Contributor

hqhq commented Feb 11, 2022

Some of the changes might make code less scalable, but we can add them back when we really want to scale someday, it does make code cleaner, 👍

This was referenced Feb 11, 2022
Merged
Merged
@kolyshkin kolyshkin mentioned this pull request Feb 19, 2022
Merged
@kolyshkin kolyshkin mentioned this pull request Aug 16, 2022
Closed
@kolyshkin kolyshkin mentioned this pull request Aug 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hqhq hqhq hqhq approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kolyshkin @thaJeztah @hqhq