[pve-devel] [PATCH v5 manager] 1145 Warn if datacenter firewall is disabled

[Thomas Lamprecht]

On 3/4/19 12:55 PM, Christian Ebner wrote:
> This warns the user that the datacenter firewall is disabled when editing the
> host or the VM/CT firewall status.
> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> 
> Version 5:
>     * Removed unneeded fieldDefaults
>     * Removed unneeded fwtype
>     * Put warning text into gettext()

sorry, only got to take a closer look now, else you may have saved a revision...

> 
>  www/manager6/Makefile                   |  1 +
>  www/manager6/grid/FirewallEnableEdit.js | 49 +++++++++++++++++++++++++++++++++
>  www/manager6/grid/FirewallOptions.js    | 23 ++++++++++++++--
>  3 files changed, 70 insertions(+), 3 deletions(-)
>  create mode 100644 www/manager6/grid/FirewallEnableEdit.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index e75f0de6..951242d4 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -89,6 +89,7 @@ JSSRC= 				                 	\
>  	grid/FirewallRules.js				\
>  	grid/FirewallAliases.js				\
>  	grid/FirewallOptions.js				\
> +	grid/FirewallEnableEdit.js		    	\
>  	tree/ResourceTree.js				\
>  	panel/IPSet.js					\
>  	panel/ConfigPanel.js				\
> diff --git a/www/manager6/grid/FirewallEnableEdit.js b/www/manager6/grid/FirewallEnableEdit.js
> new file mode 100644
> index 00000000..bbe60e43
> --- /dev/null
> +++ b/www/manager6/grid/FirewallEnableEdit.js
> @@ -0,0 +1,49 @@
> +Ext.define('PVE.FirewallEnableEdit', {
> +    extend: 'Proxmox.window.Edit',

this is a window, not a grid, so why are you placing it in the grid folder?
May fit more in window/ as we do not have a firewall folder.

> +    alias: ['widget.pveFirewallEnableEdit'],
> +
> +    subject: gettext('Firewall'),
> +
> +    items: [
> +	{
> +	    xtype: 'proxmoxcheckbox',
> +	    name: 'enable',
> +	    itemId: 'enablecheckbox',
> +	    uncheckedValue: 0,
> +	    defaultValue: 0,
> +	    checked: false,
> +	    deleteDefaultValue: false,
> +	    labelWidth: 120,

If you want to ensure the hint below looks OK and is not line-breaked often I'd
just set a width to the window? e.g., 350 seems to work well here.

> +	    fieldLabel: gettext('Firewall')
> +	},
> +	{
> +	    xtype: 'displayfield',
> +	    name: 'warning',
> +	    itemId: 'warning',
> +	    userCls: 'pve-hint',
> +	    value: gettext('Warning! Firewall disabled at datacenter level!'),

maybe "Warning: Firewall still disabled at datacenter level!"

To many exclamation marks desensitize users ;-)

> +	    hidden: true
> +	}
> +    ],
> +
> +    beforeShow: function() {
> +	var me = this;
> +
> +	var checkbox = me.down('#enablecheckbox');
> +	checkbox.defaultValue = me.defaultValue;
> +	checkbox.checked = me.defaultValue ? true : false;

this could be done with our cbind mixin, but no hard feelings here...

> +
> +	Proxmox.Utils.API2Request({
> +	    url: '/api2/extjs/cluster/firewall/options',
> +	    method: 'GET',
> +	    failure: function(response, opts) {
> +		Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> +	    },
> +	    success: function(response, opts) {
> +		if (!response.result.data.enable) {
> +		    me.down('#warning').setVisible(true);

You could also use 'displayfield[name=warning]' as selector to save the id.
Such general IDs - which should be unique for the whole UI - should be avoided.
Similar above for enable, at least if you decide against the cbind as there it
isn't required at all.

> +		}
> +	    }
> +	});
> +    }
> +});
> diff --git a/www/manager6/grid/FirewallOptions.js b/www/manager6/grid/FirewallOptions.js
> index cddbdbbf..1d56ecc0 100644
> --- a/www/manager6/grid/FirewallOptions.js
> +++ b/www/manager6/grid/FirewallOptions.js
> @@ -64,9 +64,17 @@ Ext.define('PVE.FirewallOptions', {
>  	    };
>  	};
>  
> -
>  	if (me.fwtype === 'node') {
> -	    add_boolean_row('enable', gettext('Firewall'), 1);
> +	    me.rows.enable = {
> +		required: true,
> +		defaultValue: 1,
> +		header: gettext('Firewall'),
> +		renderer: Proxmox.Utils.format_boolean,
> +		editor: {
> +		    xtype: 'pveFirewallEnableEdit',
> +		    defaultValue: 1
> +		}
> +	    };
>  	    add_boolean_row('nosmurfs', gettext('SMURFS filter'), 1);
>  	    add_boolean_row('tcpflags', gettext('TCP flags filter'), 0);
>  	    add_boolean_row('ndp', 'NDP', 1);
> @@ -78,7 +86,16 @@ Ext.define('PVE.FirewallOptions', {
>  	    add_log_row('tcp_flags_log_level', 120);
>  	    add_log_row('smurf_log_level');
>  	} else if (me.fwtype === 'vm') {
> -	    add_boolean_row('enable', gettext('Firewall'), 0);
> +	    me.rows.enable = {
> +		required: true,
> +		defaultValue: 0,
> +		header: gettext('Firewall'),
> +		renderer: Proxmox.Utils.format_boolean,
> +		editor: {
> +		    xtype: 'pveFirewallEnableEdit',
> +		    defaultValue: 0
> +		}
> +	    };
>  	    add_boolean_row('dhcp', 'DHCP', 1);
>  	    add_boolean_row('ndp', 'NDP', 1);
>  	    add_boolean_row('radv', gettext('Router Advertisement'), 0);
>