cirruslabs / tart

macOS and Linux VMs on Apple Silicon to use in CI and other automations
https://tart.run
Other
3.91k stars 116 forks source link

Add flag to allow automatically reconfigure display for linux VM #951

Closed Fred78290 closed 1 week ago

Fred78290 commented 1 week ago

By default when using tart to create linux VM with graphical desktop, the resizing of the window don't resize linux desktop and doesn't permit fullscreen.

This pull request add a flag --allows-reconfigure-display to run command permitting machineView.automaticallyReconfiguresDisplay = true

edigaryev commented 1 week ago

Hello Frederic 👋

What do you think about having a --display-auto-reconfigure option in tart set instead?

automaticallyReconfiguresDisplay will be then set to a value that depends on the conditions below:

Platform displayAutoReconfigure in config.json automaticallyReconfiguresDisplay
macOS null1 true1
macOS true true
macOS false false
Linux null1 false1
Linux true true
Linux false false

1: backwards compatibility with VM images created before --display-auto-reconfigure was implemented

I also propose to not populate the displayAutoReconfigure in config.json for now.

Fred78290 commented 1 week ago

1: backwards compatibility with VM images created before --display-auto-reconfigure was implemented

I also propose to not populate the displayAutoReconfigure in config.json for now.

This is the reason why i don't want put it in config.json. The argument remains discrete for the user as nested flag :)

Else every options from run command must be moved to Config.json, such as network options or vnc options.

edigaryev commented 1 week ago

The argument remains discrete for the user as nested flag :)

We cannot expose every possible option in tart run. Our strategy is try to avoid adding any additional options if the same can be achieved by a sane default.

Unfortunately, in this case, due to https://github.com/cirruslabs/tart/issues/682 it's not possible, so here we go.

Else every options from run command must be moved to Config.json, such as network options or vnc options.

It's not black and white 😉

We try to keep security-sensitive options in tart run, so that a freshly cloned VM is secure and doesn't e.g. mount your home directory automatically on a first run.

But from the security point of view, --display-auto-reconfigure seems to be a relatively safe option.

Another point is that we already have --display option in tart set. Why not co-locate them, to have a lower cognitive load for the user?

Fred78290 commented 1 week ago

The argument remains discrete for the user as nested flag :)

We cannot expose every possible option in tart run. Our strategy is try to avoid adding any additional options if the same can be achieved by a sane default.

Unfortunately, in this case, due to #682 it's not possible, so here we go.

Else every options from run command must be moved to Config.json, such as network options or vnc options.

It's not black and white 😉

We try to keep security-sensitive options in tart run, so that a freshly cloned VM is secure and doesn't e.g. mount your home directory automatically on a first run.

But from the security point of view, --display-auto-reconfigure seems to be a relatively safe option.

It was my first approach...

Another point is that we already have --display option in tart set. Why not co-locate them, to have a lower cognitive load for the user?

I don't understand your logic, in my previous pull request I putted the nested option in config.json and you moved it as flag on run command, so now the kind of flag that i request looks like the same behavior and now you say to put it in config.json.

You gimme some trouble :)

edigaryev commented 1 week ago

I don't understand your logic, in my previous pull request I putted the nested option in config.json and you moved it as flag on run command, so now the kind of flag that i request looks like the same behavior and now you say to put it in config.json.

Please take a look at https://github.com/cirruslabs/tart/pull/951#issuecomment-2483821971, I've explained some of the reasoning there.

--nested, for example, has a direct impact on security (just Google nested virtualization CVE), so it makes sense to not enable it by default nor preserve it in the config.json, which will be propagated through the OCI registry.

Fred78290 commented 1 week ago

--nested, for example, has a direct impact on security (just Google nested virtualization CVE), so it makes sense to not enable it by default nor preserve it in the config.json, which will be propagated through the OCI registry.

It's concern AMD with KVM not apple sicilon with VZ....