-
Notifications
You must be signed in to change notification settings - Fork 75
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
Show helper to set up SSH tunnel for Desktop Viewer #1084
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,9 +17,12 @@ | |
* along with Cockpit; If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
import React from "react"; | ||
import { CodeBlock, CodeBlockCode } from "@patternfly/react-core/dist/esm/components/CodeBlock"; | ||
import { DesktopViewer } from '@patternfly/react-console'; | ||
|
||
import { getServerAddress, needsTunnel } from "./utils.js"; | ||
import cockpit from "cockpit"; | ||
import store from './../../../store.js'; | ||
|
||
const _ = cockpit.gettext; | ||
|
||
|
@@ -37,6 +40,11 @@ function fmt_to_fragments(fmt) { | |
} | ||
|
||
const DesktopConsoleDownload = ({ vnc, spice, onDesktopConsole }) => { | ||
// DesktopViewer prefers spice over vnc | ||
const address = (spice && spice.address) || (vnc && vnc.address); | ||
const serverAddress = getServerAddress(); | ||
const loggedUser = store.getState().systemInfo.loggedUser; | ||
|
||
return ( | ||
<DesktopViewer spice={spice} | ||
vnc={vnc} | ||
|
@@ -57,6 +65,12 @@ const DesktopConsoleDownload = ({ vnc, spice, onDesktopConsole }) => { | |
<p> | ||
{fmt_to_fragments(_("Clicking \"Launch remote viewer\" will download a .vv file and launch $0."), <i>Remote Viewer</i>)} | ||
</p> | ||
{needsTunnel(address, serverAddress) && <p> | ||
{_("SSH tunnel needs to be set up on client:")} | ||
<CodeBlock> | ||
<CodeBlockCode>{`ssh -L 5900:localhost:5900 -N ${loggedUser.name}@${serverAddress}`}</CodeBlockCode> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually really nice! Even if the serverAddress is slightly incorrect (due to reverse proxy and such), it will at least give admins an idea what needs to happen. Same for the local port -- it might be busy, and this doesn't work if you run this for more than one VM (you need to pick a different local port then), but this is meant as a guideline. However, is it always 5900 on the remote side? Are VNC and spice using the same port? Can we ask libvirt about the port? |
||
</CodeBlock> | ||
</p>} | ||
<p> | ||
{fmt_to_fragments(_("$0 is available for most operating systems. To install it, search for it in GNOME Software or run the following:"), <i>Remote Viewer</i>)} | ||
</p> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* This file is part of Cockpit. | ||
* | ||
* Copyright (C) 2023 Red Hat, Inc. | ||
* | ||
* Cockpit is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU Lesser General Public License as published by | ||
* the Free Software Foundation; either version 2.1 of the License, or | ||
* (at your option) any later version. | ||
* | ||
* Cockpit is distributed in the hope that it will be useful, but | ||
* WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
* Lesser General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Lesser General Public License | ||
* along with Cockpit; If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
export function getServerAddress() { | ||
return window.location.hostname; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @martinpitt Is there any way for me to mock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not directly -- if you write to that attribute, you will actually change the page. But you can add a destructive test with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you just said in the chat, this won't help, as the browser runs outside the VMs and the cockpit port always gets tunneled to 127.0.0.2. We can't run a browser inside the VM. So this is actually exactly the unfixable cockpit behind a reverse proxy scenario below. But there's a workaround: You can restrict the "localhost" check to 127.0.0.1, treat any other 127.* address as "remote", and then have one test set up a |
||
} | ||
|
||
export function isLocalhost(address) { | ||
return address === "localhost" || address.startsWith("127"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to be concerned about IPv6? |
||
} | ||
|
||
export function isAmbigious(address) { | ||
skobyda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return isLocalhost(address) || ["0", "0.0.0.0"].includes(address); | ||
} | ||
|
||
// Get address where VNC or SPICE server is located | ||
export function getConsoleAddress(consoleDetails) { | ||
let address = consoleDetails.address; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a server listening address then, judging by the test that you run on it. Would this ever be an actually real address? Even if it's interface specific, it's usually a netmask then, such as 10.1.2.0/24 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that is an address at which a VNC server is listening to. |
||
|
||
if (!address || isAmbigious(address)) | ||
address = getServerAddress(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why the UI wouldn't just always show window.location.hostname -- in what situations would that be wrong? It could be if Cockpit is running behind a reverse proxy -- but then you have no chance of figuring out the real server name/IP anyway, and the UI will be wrong. But I don't see much that you can do about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Server can have multiple IP addresses, and VNC server can be configured to listen only to one specific IP address. So I think I'll drop this logic here, and see if somebody asks for it in the future |
||
|
||
return address; | ||
} | ||
|
||
export function needsTunnel(consoleAddress, serverAddress) { | ||
return isLocalhost(consoleAddress) && !isLocalhost(serverAddress); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,8 @@ class TestMachinesConsoles(VirtualMachinesCase): | |
b.wait_in_text('.pf-c-expandable-section__content', | ||
'Clicking "Launch remote viewer" will download') | ||
|
||
b.wait_not_in_text('.pf-c-expandable-section__content', 'SSH tunnel needs to be set up on client') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs a positive test case, too. You already asked about this, just opening a thread to track it. |
||
|
||
b.assert_pixels("#vm-subVmTest1-consoles-page", "vm-details-console-external", skip_layouts=["rtl"]) | ||
|
||
def testInlineConsole(self, urlroot=""): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spice?.address || vnc?.address