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.

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

Fix #2131 Add preflight check for WSL2 #2323

Merged
merged 1 commit into from
May 11, 2021

Conversation

gbraad
Copy link
Contributor

@gbraad gbraad commented May 10, 2021

Fixes: Issue #2131

Solution/Idea

This checks the kernel version for the pattern Microsoft as is the case when the WSL2 provided kernel is used. Note: any custom compiled kernel is ignored,. but in that case we can assume a power-user is at work.

Proposed changes

List main as well as consequential changes you introduced or had to introduce.

  1. The start command will fail when this is called from inside a WSL2 hosted distribution

Testing

Use Windows 10 with WSL2

  1. start fails with an error message

On a regular Linux setup (or VM)

  1. start will check but succeed the test.

Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

This is to prevent a setup that uses Nested Virtualization.

I thought the main issue with wsl2 was that we can't configure networking, or some other crc requirements?

func checkRunningInsideWSL2() error {
version, err := ioutil.ReadFile("/proc/version")
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

hopefully there are no 'hardened' setups where reading this fail will fail. Wondering if we should ignore this error instead of failing, but probably not.

Copy link
Contributor Author

@gbraad gbraad May 10, 2021

Choose a reason for hiding this comment

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

hopefully there are no 'hardened' setups where reading this fail will fail.

You can not run with selinux or apparmor on WSL2. Not sure about other setups... haven't tested. But reading this is a known hardened entry?

Note: There are a lot of limitations, like no systemd, no cgroupsv2 exclusive use, etc

Copy link
Contributor

Choose a reason for hiding this comment

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

You can not run with selinux or apparmor on WSL2.

But this code will run on all linux systems, not just WSL2. I have no idea if it's really blocked in some places, just wondered while reading this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about other setups... haven't tested. But reading this is a known hardened entry?

^^

@cfergeau
Copy link
Contributor

And actually I'm not sure what this means exactly

Note: any custom compiled kernel is ignored,. but in that case we can assume a power-user is at work.

Is this about a custom compiled kernel being run under WSL2, where /proc/version won't contain Microsoft? Or is this about something different?

@gbraad
Copy link
Contributor Author

gbraad commented May 10, 2021

I thought the main issue with wsl2 was that we can't configure networking, or some other crc requirements?

When WSL2 is used, they already have access to Hyper-V. The issue is a multitude of things, like no way to configure networking, but also no way to properly check the memory assignment (as this is dynamic), performance hit, inability of the kernel to run nested setup (no virt enabled in public kernels).

Is this about a custom compiled kernel being run under WSL2, where /proc/version won't contain Microsoft?

Bingo

@cfergeau
Copy link
Contributor

The issue is a multitude of things, like no way to configure networking, but also no way to properly check the memory assignment (as this is dynamic), performance hit, inability of the kernel to run nested setup (no virt enabled in public kernels).

Seems like nested virt is not the main worry here then, maybe add that list to the commit log?

Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Actually CI's broken, taking back my approval

pkg/crc/preflight/preflight_checks_linux.go:46: File is not `gofmt`-ed with `-s` (gofmt)

@openshift-ci openshift-ci bot removed the lgtm label May 10, 2021
@openshift-ci
Copy link

openshift-ci bot commented May 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gbraad

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gbraad gbraad force-pushed the issue2131 branch 2 times, most recently from 3cfa040 to b08c4c1 Compare May 10, 2021 12:37
@@ -220,6 +229,7 @@ func removeVsockCrcSettings() error {
func getAllPreflightChecks() []Check {
usingSystemdResolved := checkSystemdResolvedIsRunning()
checks := getPreflightChecksForDistro(distro(), network.SystemNetworkingMode, usingSystemdResolved == nil)
checks = append(checks, wsl2PreflightChecks)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add this line in individuals getPreflightChecks.

getAllPreflightChecks is used for registering configuration and cleanup. It must includes your check but it won't be run in setup unless you add it in `getPreflightChecks

Copy link
Contributor Author

@gbraad gbraad May 11, 2021

Choose a reason for hiding this comment

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

Adding a preflight has become very messy and cumbersome. Too much complexity and variables with no clear meaning have been introduced.

This check looks at the kernel version string and determines if a
Microsoft compiled kernel is used, as is the case with WSL2. This is to
prevent a setup that uses Nested Virtualization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add check to the Linux pre-flights to prevent installation on WSL2
4 participants