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

Network improvements #1365

Merged
merged 21 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions rust/agama-server/src/network/nm/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ impl<'a> DeviceFromProxyBuilder<'a> {
.context("Unsupported device type: {device_type}")?;

let state = self.proxy.state().await? as u8;
let (_, state_reason) = self.proxy.state_reason().await?;
let state: DeviceState = state
.try_into()
.context("Unsupported device state: {state}")?;
Expand All @@ -46,6 +47,7 @@ impl<'a> DeviceFromProxyBuilder<'a> {
name: self.proxy.interface().await?,
type_,
state,
state_reason: state_reason as u8,
..Default::default()
};

Expand Down
8 changes: 7 additions & 1 deletion rust/agama-server/src/network/nm/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ impl<'a> NetworkManagerClient<'a> {
/// Returns the list of network devices.
pub async fn devices(&self) -> Result<Vec<Device>, ServiceError> {
let mut devs = vec![];
for path in &self.nm_proxy.get_devices().await? {
for path in &self.nm_proxy.get_all_devices().await? {
let proxy = DeviceProxy::builder(&self.connection)
.path(path.as_str())?
.build()
Expand Down Expand Up @@ -172,6 +172,12 @@ impl<'a> NetworkManagerClient<'a> {
.path(path.as_str())?
.build()
.await?;
let flags = proxy.flags().await?;
if flags > 0 {
log::warn!("Skipped connection because of flags: {}", flags);
continue;
}

let settings = proxy.get_settings().await?;

if let Some(mut connection) = connection_from_dbus(settings.clone()) {
Expand Down
5 changes: 3 additions & 2 deletions rust/agama-server/src/network/nm/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,12 @@ impl TryFrom<NmDeviceType> for DeviceType {

fn try_from(value: NmDeviceType) -> Result<Self, Self::Error> {
match value {
NmDeviceType(32) => Ok(DeviceType::Loopback),
NmDeviceType(1) => Ok(DeviceType::Ethernet),
NmDeviceType(2) => Ok(DeviceType::Wireless),
NmDeviceType(22) => Ok(DeviceType::Dummy),
NmDeviceType(10) => Ok(DeviceType::Bond),
NmDeviceType(13) => Ok(DeviceType::Bridge),
NmDeviceType(22) => Ok(DeviceType::Dummy),
NmDeviceType(32) => Ok(DeviceType::Loopback),
NmDeviceType(_) => Err(NmError::UnsupportedDeviceType(value.into())),
}
}
Expand Down
8 changes: 7 additions & 1 deletion rust/agama-server/src/network/nm/watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,13 @@ impl DeviceChangedStream {

fn handle_changed(message: PropertiesChanged) -> Option<DeviceChange> {
const IP_CONFIG_PROPS: &[&str] = &["AddressData", "Gateway", "NameserverData", "RouteData"];
const DEVICE_PROPS: &[&str] = &["DeviceType", "HwAddress", "Interface", "State"];
const DEVICE_PROPS: &[&str] = &[
"DeviceType",
"HwAddress",
"Interface",
"State",
"StateReason",
];

let path = OwnedObjectPath::from(message.path()?);
let args = message.args().ok()?;
Expand Down
32 changes: 32 additions & 0 deletions web/src/client/network/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,38 @@ class NetworkClient {
return this.client.get(`/network/connections/${connection.id}/disconnect`);
}

async loadNetworks(devices, connections, accessPoints) {
const knownSsids = [];

return accessPoints
.sort((a, b) => b.strength - a.strength)
.reduce((networks, ap) => {
// Do not include networks without SSID
if (!ap.ssid || ap.ssid === "") return networks;
// Do not include "duplicates"
if (knownSsids.includes(ap.ssid)) return networks;

const network = {
...ap,
settings: connections.find(c => c.wireless?.ssid === ap.ssid),
device: devices.find(c => c.connection === ap.ssid)
};

// Group networks
if (network.device) {
networks.connected.push(network);
} else if (network.settings) {
networks.configured.push(network);
} else {
networks.others.push(network);
}

knownSsids.push(network.ssid);

return networks;
}, { connected: [], configured: [], others: [] });
}

/**
* Apply network changes
*/
Expand Down
1 change: 1 addition & 0 deletions web/src/client/network/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const DeviceState = Object.freeze({
UNAVAILABLE: "unavailable",
DISCONNECTED: "disconnected",
CONFIG: "config",
IPCHECK: "ipCheck",
NEEDAUTH: "needAuth",
ACTIVATED: "activated",
DEACTIVATING: "deactivating",
Expand Down
2 changes: 2 additions & 0 deletions web/src/components/layout/Icon.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ import Visibility from "@icons/visibility.svg?component";
import VisibilityOff from "@icons/visibility_off.svg?component";
import Wifi from "@icons/wifi.svg?component";
import WifiFind from "@icons/wifi_find.svg?component";
import WifiOff from "@icons/wifi_off.svg?component";

// Icons from react-simple-icons

Expand Down Expand Up @@ -146,6 +147,7 @@ const icons = {
warning: Warning,
wifi: Wifi,
wifi_find: WifiFind,
wifi_off: WifiOff,
// brand icons
linux_logo: SiLinux,
windows_logo: SiWindows
Expand Down
6 changes: 3 additions & 3 deletions web/src/components/network/IpSettingsForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,12 @@ export default function IpSettingsForm() {
...connection,
addresses: sanitizedAddresses,
method4: method,
gatewa4: gateway,
gateway4: gateway,
nameservers: sanitizedNameservers.map(s => s.address)
};

client.network.updateConnection(updatedConnection)
.then(navigate(".."))
.then(navigate(-1))
// TODO: better error reporting. By now, it sets an error for the whole connection.
.catch(({ message }) => setErrors({ object: message }));
};
Expand Down Expand Up @@ -205,7 +205,7 @@ export default function IpSettingsForm() {
</Page.MainContent>

<Page.NextActions>
<Page.CancelAction />
<Page.CancelAction navigateTo={-1} />
<Page.Action type="submit" form="editConnectionForm">
{_("Accept")}
</Page.Action>
Expand Down
179 changes: 73 additions & 106 deletions web/src/components/network/NetworkPage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,32 +21,17 @@

// @ts-check

import React, { useEffect, useState } from "react";
import React, { useCallback, useEffect, useState } from "react";
import { Button, CardBody, Grid, GridItem, Split, Skeleton, Stack } from "@patternfly/react-core";
import { useLoaderData } from "react-router-dom";
import { Button, CardBody, Flex, Grid, GridItem, Skeleton, Stack } from "@patternfly/react-core";
import { useInstallerClient } from "~/context/installer";
import { CardField, Page } from "~/components/core";
import { ConnectionsTable, WifiSelector } from "~/components/network";
import { ButtonLink, CardField, EmptyState, Page } from "~/components/core";
import { ConnectionsTable } from "~/components/network";
import { NetworkEventTypes } from "~/client/network";
import { useInstallerClient } from "~/context/installer";
import { _ } from "~/i18n";

const WifiSelection = ({ wifiScanSupported }) => {
const [wifiSelectorOpen, setWifiSelectorOpen] = useState(false);

if (!wifiScanSupported) return;

const openWifiSelector = () => setWifiSelectorOpen(true);
const closeWifiSelector = () => setWifiSelectorOpen(false);

return (
<>
<Button variant="secondary" onClick={openWifiSelector}>
{_("Connect to a Wi-Fi network")}
</Button>
<WifiSelector isOpen={wifiSelectorOpen} onClose={closeWifiSelector} />
</>
);
};
import { formatIp } from "~/client/network/utils";
import { sprintf } from "sprintf-js";
import { DeviceState } from "~/client/network/model";

/**
* Internal component for displaying info when none wire connection is found
Expand All @@ -58,135 +43,117 @@ const NoWiredConnections = () => {
);
};

/**
* Internal component for displaying info when none WiFi connection is found
* @component
*
* @param {object} props
* @param {boolean} props.supported - whether the system supports scanning WiFi networks
* @param {boolean} props.openWifiSelector - the function for opening the WiFi selector
*/
const NoWifiConnections = ({ wifiScanSupported }) => {
const message = wifiScanSupported
? _("The system has not been configured for connecting to a WiFi network yet.")
: _("The system does not support WiFi connections, probably because of missing or disabled hardware.");

return (
<Stack hasGutter>
<div>{_("No WiFi connections found.")}</div>
<div>{message}</div>
</Stack>
);
};

/**
* Page component holding Network settings
* @component
*/
export default function NetworkPage() {
const { network: client } = useInstallerClient();
const { connections: initialConnections, settings } = useLoaderData();
const { connections: initialConnections, devices: initialDevices, settings } = useLoaderData();
const [connections, setConnections] = useState(initialConnections);
const [devices, setDevices] = useState(undefined);
const [selectedConnection, setSelectedConnection] = useState(null);
const [devices, setDevices] = useState(initialDevices);
const [updateState, setUpdateState] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is updateState used? Perhaps a better name could help to know the intention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was updateNetworks, as the method was get from WifiSelector and updateDevicesConnections was like too much.. was not very inspired at the moment of writing it :P

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, it is wrong because we are not checking the updateState value.


useEffect(() => {
return client.onNetworkChange(({ type, payload }) => {
switch (type) {
case NetworkEventTypes.DEVICE_ADDED: {
setDevices((devs) => {
const newDevices = devs.filter((d) => d.name !== payload.name);
return [...newDevices, client.fromApiDevice(payload)];
});
break;
}
const fetchState = useCallback(async () => {
const devices = await client.devices();
const connections = await client.connections();
setDevices(devices);
setConnections(connections);
}, [client]);

case NetworkEventTypes.DEVICE_UPDATED: {
const [name, data] = payload;
setDevices(devs => {
const newDevices = devs.filter((d) => d.name !== name);
return [...newDevices, client.fromApiDevice(data)];
});
break;
}
useEffect(() => {
fetchState();
setUpdateState(false);
}, [fetchState, updateState]);

case NetworkEventTypes.DEVICE_REMOVED: {
setDevices(devs => devs.filter((d) => d.name !== payload));
break;
}
useEffect(() => {
return client.onNetworkChange(({ type }) => {
if ([NetworkEventTypes.DEVICE_ADDED, NetworkEventTypes.DEVICE_UPDATED, NetworkEventTypes.DEVICE_REMOVED].includes(type)) {
setUpdateState(true);
}
client.connections().then(setConnections);
});
}, [client, devices]);

useEffect(() => {
if (devices !== undefined) return;
});

client.devices().then(setDevices);
}, [client, devices]);

const selectConnection = ({ id }) => {
client.getConnection(id).then(setSelectedConnection);
};
const connectionDevice = ({ id }) => devices?.find(({ connection }) => id === connection);
const connectionAddresses = (connection) => {
const device = connectionDevice(connection);
const addresses = device ? device.addresses : connection.addresses;

const forgetConnection = async ({ id }) => {
await client.deleteConnection(id);
setConnections(undefined);
};

const updateConnections = async () => {
setConnections(undefined);
setDevices(undefined);
return addresses?.map(formatIp).join(", ");
};

const ready = (connections !== undefined) && (devices !== undefined);

const WifiConnections = () => {
const wifiConnections = connections.filter(c => c.wireless);
const { wireless_enabled: wifiAvailable } = settings;

if (wifiConnections.length === 0) {
if (!wifiAvailable) {
return (
<NoWifiConnections wifiScanSupported={settings.wireless_enabled} />
<CardField>
<CardField.Content>
<EmptyState title={_("No WiFi support")} icon="wifi_off" color="warning-color-200">
teclator marked this conversation as resolved.
Show resolved Hide resolved
{_("The system does not support WiFi connections, probably because of missing or disabled hardware.")}
Copy link
Contributor

Choose a reason for hiding this comment

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

Put in the shoes of an inexperienced user... I do not know if "missing hardware" is good enough.

</EmptyState>
</CardField.Content>
</CardField>
);
}

const wifiConnections = connections.filter(c => c.wireless);
const activeWifiDevice = devices.find(d => d.type === "wireless" && d.state === "activated");
const activeConnection = wifiConnections.find(c => c.id === activeWifiDevice?.connection);

return (
<ConnectionsTable connections={wifiConnections} devices={devices} onEdit={selectConnection} onForget={forgetConnection} />
<CardField
label={_("Wi-Fi")}
actions={
<ButtonLink isPrimary={!activeConnection} to="wifis">
{activeConnection ? _("Change") : _("Connect")}
</ButtonLink>
}
>
<CardField.Content>
{activeConnection
? (
<EmptyState title={sprintf(_("Conected to %s"), activeConnection.id)} icon="wifi" color="success-color-100">
{connectionAddresses(activeConnection)}
</EmptyState>
)
: (
<EmptyState title={_("No connected yet")} icon="wifi_off" color="color-300">
{_("The system has not been configured for connecting to a WiFi network yet.")}
teclator marked this conversation as resolved.
Show resolved Hide resolved
</EmptyState>
)}
</CardField.Content>
</CardField>
);
};

const WiredConnections = () => {
const wiredConnections = connections.filter(c => !c.wireless && (c.id !== "lo"));
const wiredConnections = connections.filter(c => !c.wireless);

if (wiredConnections.length === 0) return <NoWiredConnections />;

return <ConnectionsTable connections={wiredConnections} devices={devices} onEdit={selectConnection} />;
return <ConnectionsTable connections={wiredConnections} devices={devices} />;
};

return (
<>
<Page.Header>
<Flex justifyContent={{ default: "justifyContentSpaceBetween" }}>
<h2>{_("Network")}</h2>
<WifiSelection wifiScanSupported={settings.wireless_enabled} />
</Flex>
<h2>{_("Network")}</h2>
</Page.Header>

<Page.MainContent>
<Grid hasGutter>
<GridItem sm={12}>
<CardField label={_("Wired connections")}>
<GridItem sm={12} xl={6}>
<CardField label={_("Wired")}>
<CardBody>
{ready ? <WiredConnections /> : <Skeleton />}
</CardBody>
</CardField>
</GridItem>
<GridItem sm={12}>
<CardField label={_("WiFi connections")}>
<CardBody>
{ready ? <WifiConnections /> : <Skeleton />}
</CardBody>
</CardField>
<GridItem sm={12} xl={6}>
<WifiConnections />
</GridItem>
</Grid>
</Page.MainContent>
Expand Down
Loading
Loading