Re: [pve-devel] [PATCH manager 2/2] ui: fw: allow selecting network interface for rules using combogrid

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

just some thoughts from my side:

On Thu, May 11, 2023 at 11:46:20AM +0200, Christoph Heiss wrote:
> For nodes, VMs and CTs we can show the user a list of available network
> interfaces (as that information is available) when creating a new
> firewall rule, much like it is already done in similar places.
> Adds a lot of convenience when creating new firewall rules if they are
> interface-specific, as you get a nice summary of the available ones and
> can simply select it instead of typing it out each time.
> 
> Nodes can use the new `NetworkInterfaceSelector`, for VMs and CTs a new
> component is needed, as the VM/CT config needs to be parsed
> appropriately. It's mostly modeled after the `NetworkInterfaceSelector`
> component and pretty straight-forward.
> For datacenter rules, the simple textbox is kept.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Note: iptables(8) allows two wildcards for the interface, `!` and `+`.
> For VMs and CTs this cannot be specified currently anyway, as the API
> only allows /^net\d+$/. For nodes, since they accept any arbritrary
> string as interface name, this possibility to specify a wildcard for the
> interface gets essentially lost.
> 
> I guess we could still allow users to input any strings if they want -
> is that something that should be possible (using the GUI)? IOW, do we
> want to allow that?



> 
>  www/manager6/Makefile                         |  1 +
>  .../form/VMNetworkInterfaceSelector.js        | 79 +++++++++++++++++++
>  www/manager6/grid/FirewallRules.js            | 37 ++++++++-
>  www/manager6/lxc/Config.js                    |  1 +
>  www/manager6/qemu/Config.js                   |  1 +
>  5 files changed, 115 insertions(+), 4 deletions(-)
>  create mode 100644 www/manager6/form/VMNetworkInterfaceSelector.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index a2f5116c..57ba331b 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -71,6 +71,7 @@ JSSRC= 							\
>  	form/UserSelector.js				\
>  	form/VLanField.js				\
>  	form/VMCPUFlagSelector.js			\
> +	form/VMNetworkInterfaceSelector.js		\
>  	form/VMSelector.js				\
>  	form/VNCKeyboardSelector.js			\
>  	form/ViewSelector.js				\
> diff --git a/www/manager6/form/VMNetworkInterfaceSelector.js b/www/manager6/form/VMNetworkInterfaceSelector.js
> new file mode 100644
> index 00000000..fbe631ba
> --- /dev/null
> +++ b/www/manager6/form/VMNetworkInterfaceSelector.js
> @@ -0,0 +1,79 @@
> +Ext.define('PVE.form.VMNetworkInterfaceSelector', {
> +    extend: 'Proxmox.form.ComboGrid',
> +    alias: 'widget.PVE.form.VMNetworkInterfaceSelector',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    cbindData: (initialConfig) => ({
> +	isQemu: initialConfig.pveSelNode.data.type === 'qemu',
> +    }),
> +
> +    displayField: 'id',
> +
> +    store: {
> +	fields: ['id', 'name', 'bridge', 'ip'],

Not a fan of only including the 'ip' field without also including the
'ip6' field. And not sure about the formatting with both included :-)
(They can also be "manual" and "dhcp", and ip6 can additionally be
"auto", so it might look weird, but 🤷)

In patch 1 the NetworkInterfaceSelector has different fields
('iface', 'active', 'type')

> +	filterOnLoad: true,
> +	sorters: {
> +	    property: 'id',
> +	    direction: 'ASC',
> +	},
> +    },
> +
> +    listConfig: {
> +	cbind: {},
> +	columns: [
> +	    {
> +		header: 'ID',
> +		dataIndex: 'id',
> +		hideable: false,
> +		width: 80,
> +	    },
> +	    {
> +		header: gettext('Name'),
> +		dataIndex: 'name',
> +		flex: 1,
> +		cbind: {
> +		    hidden: '{isQemu}',
> +		},
> +	    },
> +	    {
> +		header: gettext('Bridge'),
> +		dataIndex: 'bridge',
> +		flex: 1,
> +	    },
> +	    {
> +		header: gettext('IP address'),
> +		dataIndex: 'ip',
> +		flex: 1,
> +		cbind: {
> +		    hidden: '{isQemu}',
> +		},
> +	    },
> +	],
> +    },
> +
> +    initComponent: function() {
> +	const { node: nodename, type, vmid } = this.pveSelNode.data;
> +
> +	Proxmox.Utils.API2Request({
> +	    url: `/nodes/${nodename}/${type}/${vmid}/config`,
> +	    method: 'GET',
> +	    success: ({ result: { data } }) => {
> +		let networks = [];
> +		for (const [id, value] of Object.entries(data)) {
> +		    if (id.match(/^net\d+/)) {
> +			const parsed = type === 'lxc'
> +			    ? PVE.Parser.parseLxcNetwork(value)
> +			    : PVE.Parser.parseQemuNetwork(id, value);
> +
> +			networks.push({ ...parsed, id });
> +		    }
> +		}
> +
> +		this.store.loadData(networks);
> +	    },
> +	});
> +
> +	this.callParent();
> +    },
> +});
> +
> diff --git a/www/manager6/grid/FirewallRules.js b/www/manager6/grid/FirewallRules.js
> index 5777c7f4..9085bd64 100644
> --- a/www/manager6/grid/FirewallRules.js
> +++ b/www/manager6/grid/FirewallRules.js
> @@ -153,6 +153,7 @@ Ext.define('PVE.FirewallRulePanel', {
>      allow_iface: false,
> 
>      list_refs_url: undefined,
> +    pveSelNode: undefined,
> 
>      onGetValues: function(values) {
>  	var me = this;
> @@ -206,13 +207,35 @@ Ext.define('PVE.FirewallRulePanel', {
>          ];
> 
>  	if (me.allow_iface) {
> -	    me.column1.push({
> -		xtype: 'proxmoxtextfield',
> +	    const commonFields = {
>  		name: 'iface',
>  		deleteEmpty: !me.isCreate,
> -		value: '',
>  		fieldLabel: gettext('Interface'),
> -	    });
> +		allowBlank: true,
> +		autoSelect: false,
> +	    };
> +
> +	    if (me.pveSelNode?.data.type === 'node') {
> +		me.column1.push({
> +		    ...commonFields,
> +		    xtype: 'PVE.form.NetworkInterfaceSelector',
> +		    nodename: me.pveSelNode.data.node,
> +		});
> +	    } else if (me.pveSelNode?.data.type) {
> +		// qemu and lxc
> +		me.column1.push({
> +		    ...commonFields,
> +		    xtype: 'PVE.form.VMNetworkInterfaceSelector',

^ The store here AFAICT has different fields.
Non-VM one has 'iface', 'active', type', the VM one has 'id', 'name',
'bridge', 'ip'.
It may just show how little I deal with our extjs code, but that seems
like it shouldn't quite fit here 🤷