Add "view, edit, and add Video devices" to detailspage #1795
Conversation
There was a problem hiding this comment.
Thanks for the PR!
Without diving to deep into the PR and testing it out. We have discussed improving the VNC/Spice UI in the past here making it configurable. I need to look if I can find more recent design proposals.
Is adding more devices a hard requirement or is just changing the default port/host/password good enough?
|
@Shotaro-Kawaguchi : Thanks! Some initial notes:
I suggest to start with a single feature addition (like set/change VNC password), cover it with proper tests, and then add more things as follow-up PRs if there's any use case. |
|
Thanks for this PR! We have had a design for this for a few years already, as @jelly pointed out. Hopefully this PR is enough to push the feature forward. If that's the case, perhaps I can find some time to revisit the design and adjust it. It's been in a "holding pattern", waiting for someone to start working on it, so I can allocate time to it again. Having fewer options (in other words: removing SPICE) should help simplify the UI further too. Are there any use cases for having more than one VNC display per VM? All I can think about are:
We shouldn't add something because you can do it, but because it's actually useful to do. I don't think any of the above are really typical uses of running VMs on a server using Cockpit. (Please prove me wrong though, and/or provide other realistic cases.) We had an issue suggesting adding VNC like in this PR before, but it doesn't make sense, as we have a dedicated place for graphics and remote graphics is quite different from other devices for several reasons. (It's integrated in-page, it really probably should be just 1 connection, it has graphical and text modes, it's not really a standard virtualize device provided to the VM like disks and network cards, and we already have a place for graphics at the top so we shouldn't have two different places for it.) Also, I need to point out again that this feature is mostly useless as-is standalone, as it requires having access to the ports, which means either poking holes in the firewall or turning off a firewall. To really make this useful, we'd want to add in firewall integration. This doesn't mean we shouldn't make it better (we definitely should), but that we shouldn't stop just here. In other words, we need to make sure this works on a remote server (without jumping through hoops), not just from localhost. Again, thank you for "getting the ball rolling again" with this PR! It's a good first step to getting remote graphics support improved overall! |
|
@jelly Thank you for your response.
|
|
@martinpitt Thank you for your response.
I was not aware that the SPICE type is deprecated. Thank you for pointing it out.
I was not aware of the number of devices that can be added. Thank you for pointing it out.
I will try to create test code.
In this pull request, I will make it possible to view, delete, and change the password of VNC devices. |
Would it be good to move the button as shown in the attached image?
|
|
@Shotaro-Kawaguchi: Can you reply to my comment above please? Have you looked at the mockups? (The current mockups are @ #553 (comment).) I'll have to adapt the mockups for the SPICE removal (and I've started doing that). But I won't merge this PR if it's about adding VNC in a different place. We need to have the VNC console be an integrated experience, not scattered across the page. Since you're interested, I did dust off the old designs and started to update them and would be happy to work with you to get the features integrated... as long as the design of it is considered. Thanks! (I think we always have VNC in all VMs Cockpit creates. In VMs where it's not enabled, we could have edit implicitly create it. In the edit dialog, we could also have a remove action. I could try to illustrate this in a mockup tomorrow, as it's getting late here today.) |
I apologize for the delayed reply.
I was not aware that assigning multiple VNC displays to a VM would prevent the VM from starting due to the following error. So, I think one VNC display is sufficient. |
I've spent a little bit of time to start to update the mockups a bit and even designed an "empty state" pattern that would make it easy to add VNC if it hadn't been added to a machine already. By default, machines with VNC and serial text consoles would look like this:
Here's what clicking on "Text" would look like:
If a machine does not have VNC support, then it should probably default to text, and if the VNC tab is active, then we could display a message saying that the support has not been enabled and that it can be added. That would look like this, which would have a button that would add VNC support with defaults set up in the same way VNC support is added to VMs created by Cockpit:
|
|
Thank you for sending the mockups. |
We still support SPICE on Fedora as only RHEL dropped it but Fedora and other distros still have it around. So it should be possible add or remove it. |
|
I plan to proceed with the implementation according to the mockup created by Garrett. |
|
@Shotaro-Kawaguchi There's no "source code" for the mockup, as it's a mockup (and not a prototype). I do have a Penpot file of the current state: (You'd have to extract the zip file, then import the penpot file in Penpot.) I've already exported the interesting parts as PNGs and attached them above. |
|
@garrett Thank you for sharing the mockup. |
|
There are some differences from the mockup, but I have implemented the functional parts of addition and editing. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@garrett @martinpitt |
|
Thanks! We sure make you implement a lot of stuff just to be able to change the VNC password, no? :-) I'll be your host for the next couple of rounds of review, and I hope we can get this in soon. Let's start with how it looks:
|
| @@ -61,6 +74,10 @@ class Consoles extends React.Component { | |||
| if (newSerial.length !== oldSerial.length || oldSerial.some((pty, index) => pty.alias !== newSerial[index].alias)) | |||
| return { serial: newSerial }; | |||
|
|
|||
| if (nextProps.selectedConsole !== prevState.selectedConsole) { | |||
There was a problem hiding this comment.
This looks problematic. The getDerivedStateFromProps function might have already returned a new state before even reaching this. I think both serial and selectedConsole need to be processed on every call.
There was a problem hiding this comment.
But: selectedConsole should not be in state in the first place. See below.
| const { vm, onAddErrorNotification, isExpanded } = this.props; | ||
| const { serial } = this.state; | ||
| const { serial, selectedConsole } = this.state; |
There was a problem hiding this comment.
selectedConsole should not be in state, IMO. It's already in props, and we can just use it from there.
| </AccessConsoles> | ||
| <> | ||
| {selectedConsole === 'SerialConsole' && serial.map((pty, idx) => ( | ||
| <SerialConsole type={serial.length == 1 ? "SerialConsole" : cockpit.format(_("Serial console ($0)"), pty.alias || idx)} |
There was a problem hiding this comment.
When there is more than one serial console, this will just show them all, no? I think there needs to be dropdown here to select one of them when there are multiple.
| icon={<ExpandIcon />} | ||
| iconPosition="right">{_("Expand")}</Button> | ||
| : null, | ||
| actions: ( |
There was a problem hiding this comment.
This all also needs to work on the "expanded consoles" variant of the details page, see above around line 80. Right now it just crashes and doesn't show any console. We probably need a mockup from @garrett for where the toggle buttons should be in the expanded console layout. (Probably in the place where the dropdown selector used to be.)
src/components/vm/vmDetailsPage.scss
Outdated
| @@ -44,7 +44,7 @@ | |||
| } | |||
| } | |||
|
|
|||
| .networks-card, .disks-card, .snapshots-card, .hostdevs-card, .filesystems-card { | |||
| .networks-card, .disks-card, .snapshots-card, .hostdevs-card, .filesystems-card, .videodev-card { | |||
There was a problem hiding this comment.
.videodev-card can be removed, it's no longer used.
| constructor (props) { | ||
| super(props); | ||
|
|
||
| this.state = { | ||
| serial: props.vm.displays && props.vm.displays.filter(display => display.type == 'pty'), | ||
| selectedConsole: this.getDefaultConsole(props.vm), | ||
| addVncInProgress: false, | ||
| vncAddress: "", |
There was a problem hiding this comment.
Also, these three can be removed when add() is removed.
src/components/vm/consoles/vnc.jsx
Outdated
| this.state = { | ||
| path: undefined, | ||
| isActionOpen: false, | ||
| vncAddress: props.consoleDetail.address || '', |
src/components/vm/consoles/vnc.jsx
Outdated
| @@ -114,7 +126,17 @@ class Vnc extends React.Component { | |||
| this.setState({ isActionOpen: false }); | |||
| } | |||
|
|
|||
| onValueChanged(key, value) { | |||
src/components/vm/consoles/vnc.jsx
Outdated
| this.setState(stateDelta); | ||
| } | ||
|
|
||
| dialogErrorSet(text, detail) { |
| @@ -107,6 +108,23 @@ export const VmDetailsPage = ({ | |||
| ); | |||
| } | |||
|
|
|||
| const [selectedConsole, setSelectedConsole] = React.useState(() => { | |||
| if (vm.displays) { | |||
There was a problem hiding this comment.
This is different from the old getDefaultConsole. We used to default to "vnc", then "spice", then "serial", in that order. Now we prefer "serial". Is that intentional?
|
Big things:
This is all due to replacing the |
|
Ok, my proposal is to reduce this work to the bare minimum. See #1973 for what I think that entails. |
@Shotaro-Kawaguchi, please check #1973. Do you think we should continue there? I have reserved some time to work on this myself. #1973 has your dialogs, plus some of the improvements. I will also start working on implementing the mockups from #1795 (comment). Also see https://issues.redhat.com/browse/COCKPIT-868, which has many more items that I want to work on eventually. |
|
@Shotaro-Kawaguchi: Hi! Thanks again for sharing your work here. So we talked about this PR for a while in our face to face meeting last week. We want to make sure we're solving tasks you and your team need, while also avoiding any unintended side effects, like breaking features for others or setting incorrect expectations for other people using Cockpit Machines. I feel like there's a bunch of things we would like to know more about, to be able to step back and look at the bigger picture... basically, to better understand what you're trying to do and why. And then we can use this information to help with the redesign and implementation that solves your needs as well as others. So, here are a few questions that we were wondering about that would help us to understand where you're coming from and how to best address your needs:
Thanks in advance for helping us out in answering these questions! For what it's worth, we did talk about solving some of the issues with something other than ports (which are unencrypted and require firewall configuration as well as a UI to set) and are thinking about SSH tunneling instead, which can be done like But answers to the above questions would help us to know if that would work for you or what else we'd need to consider. Thanks again! |
|
@garrett |
|
@garrett : Thank you for considering this PR.
The goal is to securely manage VNC passwords for our virtual machines, enabling add, edit, and set passwords directly within the Cockpit interface.
We are currently using virt-manager in production.
We are not currently using Cockpit Machines in production.
Since we are not using the in-page VNC connection, we haven't encountered specific bugs or limitations.
Thanks for pointing that out.
I appreciate the security warning. Thank you. |
How do you connect to libvirt with virt-manager? Do you use the default "QEMU/KVM" connection, or do you use the "Add connection" feature to access the KVM host over SSH?
(The VNC password is not actually send in cleartext, but everything else is, so this technicality does not change the fact that not using encryption is a bad idea.) So for #1973 this means that we drop all configurability and just keep the "Add VNC" functionality? |
|
@Shotaro-Kawaguchi: Thanks for your replies to all the questions! It really helps clarify a lot of points for us!
I think it's probably obvious enough from the rest of your replies and also from @mvollmer's reply, but I'll state it plainly here to be absolutely clear: That "securely" means the code and UI in this PR is (sadly) not suitable as-is. From my understanding, this means we need follow up with extra work to:
The results of looking into this will need to inform the UX which will inform the UI which will then need to be coded in such a way where it works well to solve the secure connection issue. Considering the above, to make a TODO list of what's needed, I think we'll want the following, which can be split into separate steps (some of which is already being worked on, and not all are completely interrelated):
|
I don't see anything insecure here. It's not at all obvious to me, can you please explain? Adding a localhost-only VNC to a VM is no different than what we do by default for newly created VMs. |
The in-page VNC uses the same type of connection that Cockpit uses to reach the KVM host. This is typically https and might also involve SSH along the way. So we can assume it is secure enough. |
The "qemu+ssh://" connection scheme (aka SSH tunneling) is not supported on Windows, but it is on MacOS. (I assume the sentence "Once started, you can use virsh as you would on Linux." implies that.) |
I just yesterday figured out that, for a VM that has a VNC password configured, virt-viewer and virt-manager prompt for the VNC password, while Cockpit's in-page VNC viewer does not. It's obvious that remote-viewer and other non-libvirt-aware applications like GNOME Connections will prompt for a password: They need it to actually open the VNC connection. But libvirt-aware applications like virt-manager, virt-viewer, and Cockpit do not strictly need to prompt. They can retrieve the password from the VM configuration (just like they can actually change the password). Cockpit does that, while virt-manager and virt-viewer do not. So, there might be some access policy options where a user can get enough access to libvirt to figure out the port to connect to, but not the password. Is there maybe also a read-only access mode? I will have to investigate more, I really don't know much here. (Sure would be nice to have libvirt expert working with us on this. :-) In any case, we might want to consider changing Cockpit to always prompt when a password is needed (instead of digging it out of the VM config, which might be a privileged operation). |
The insecure part we're talking about is opening up a port and connecting to it via VNC/SPICE/RDP without using SSH or TLS. In-page is secure, going through the same protocol as Cockpit. I just wanted someone to chime in on that to confirm that is indeed the case, so it's not just me saying that. This is why I think that for most people, the easiest way to make sure there's a good remote interaction with the VM is to use the in-browser version. But then we should also work on TLS and/or SSH secured whatever remote protocol(s) (VNC/RDP/SPICE) for the few cases where the in-page one is not enough. |
|
@garrett, I think you have everything now to start working on the designs and interaction flows. Here is the order in which I think they would be most useful to me:
Thanks! I'll concentrate on plumbing stuff while waiting for these. |
Added a feature to display video devices in virtual machine information.
With this commit, it will be possible to add, edit, and delete video devices.
This feature was implemented by a use case that sets a VNC password.
Screenshot viewing video device.
Click the "Add video device" button to display the following screen.
Click the "Edit" button to display the following screen.
Click the "..." button to display the following screen.
Click the "Remove" button to display the following screen.
