[pve-devel] [PATCH manager] 1145 Show datacenter firewall status in firewall options tabs

[Dominik Csapak]

high level comment:

i believe it would be better to show such a message in the
edit window of the enable checkbox

we could even have two warnings, one
for 'dc firewall not enabled' and
'pve-firewall service not running'

some technical comments inline:

On 2/14/19 4:12 PM, Christian Ebner wrote:
> Display the status of the datacenter firewall in the top bar of every firewall
> options tab.
> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
>   www/manager6/grid/FirewallOptions.js | 58 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/grid/FirewallOptions.js b/www/manager6/grid/FirewallOptions.js
> index cddbdbbf..b804ec68 100644
> --- a/www/manager6/grid/FirewallOptions.js
> +++ b/www/manager6/grid/FirewallOptions.js
> @@ -1,3 +1,12 @@
> +Ext.define('dcfirewall-options', {
> +    extend: 'Ext.data.Model',
> +    fields: [ 'ebtables', 'enable', 'policy_in', 'policy_out' ],
> +    proxy: {
> +	type: 'proxmox',
> +	url: '/api2/json/cluster/firewall/options'
> +    }
> +});
> +
>   Ext.define('PVE.FirewallOptions', {
>       extend: 'Proxmox.grid.ObjectGrid',
>       alias: ['widget.pveFirewallOptions'],
> @@ -64,6 +73,30 @@ Ext.define('PVE.FirewallOptions', {
>   	    };
>   	};
>   
> +	me.dcfirewall_store = Ext.create("Proxmox.data.UpdateStore", {
> +	    storeid: 'pve-dcfirewall-options',
> +	    model: 'dcfirewall-options',
> +	    interval: 3000
> +	});
> +
> +	me.dcfirewall_store.on('load', function(s, rec, success) {
> +	    if (!success) { return; }
> +
> +	    var status_comp = me.down('#dcfirewall-status');

if you move the on('load') after the me.callParent()
you can pull out the me.down part

this way you only have to look up the component only once instead
of every time the store is loaded, but should not matter much
performance wise since it only happens once every 3s anyway

> +	    if (rec[0].data.enable) {
> +		status_comp.update({
> +		    dcfirewall_status: 'enabled',
> +		    dcfirewall_state: 'good',
> +		    dcfirewall_icon: 'fa-play-circle'
> +		});
> +	    } else {
> +		status_comp.update({
> +		    dcfirewall_status: 'disabled',
> +		    dcfirewall_state: 'critical',
> +		    dcfirewall_icon: 'fa-stop-circle'
> +		});
> +	    }
> +	});
>   
>   	if (me.fwtype === 'node') {
>   	    add_boolean_row('enable', gettext('Firewall'), 1);
> @@ -145,7 +178,26 @@ Ext.define('PVE.FirewallOptions', {
>   
>   	Ext.apply(me, {
>   	    url: "/api2/json" + me.base_url,
> -	    tbar: [ edit_btn ],
> +	    tbar: [
> +		edit_btn,
> +		'->',
> +		{
> +		    xtype: 'component',
> +		    itemId: 'dcfirewall-status',
> +		    data: {
> +			dcfirewall_status: 'disabled',
> +			dcfirewall_state: 'critical',
> +			dcfirewall_icon: 'fa-stop-circle'
> +		    },
> +		    tpl: [
> +			'<div class="right-align">',
> +			    gettext('Datacenter firewall status: '),
> +			    '<i class="{dcfirewall_state} fa fa-fw {dcfirewall_icon}">&nbsp</i>',
> +			    '{dcfirewall_status}',
> +			'</div>'
> +		    ]

personally, i would structure this differently

i would add a static label with 'Datacenter firewall status'
and then a component with a simple template:

'<i class="{dc_fw_state} fa fa-fw {icon}"></i>{status}'

you do not have to prefix everything with dcfirewall_
it should be clear from the context what the fields are for

> +		}
> +	    ],
>   	    editorConfig: {
>   		url: '/api2/extjs/' + me.base_url
>   	    },
> @@ -160,5 +212,9 @@ Ext.define('PVE.FirewallOptions', {
>   	me.on('activate', me.rstore.startUpdate);
>   	me.on('destroy', me.rstore.stopUpdate);
>   	me.on('deactivate', me.rstore.stopUpdate);
> +
> +	me.on('activate', me.dcfirewall_store.startUpdate);
> +	me.on('destroy', me.dcfirewall_store.stopUpdate);
> +	me.on('deactivate', me.dcfirewall_store.stopUpdate);
>       }
>   });
>