[pve-devel] [PATCH v3 manager] 1145 Warn if datacenter firewall or host firewall service is disabled
Tim Marx
t.marx at proxmox.com
Thu Feb 28 12:15:38 CET 2019
> Christian Ebner <c.ebner at proxmox.com> hat am 28. Februar 2019 um 10:23 geschrieben:
>
>
> This shows a warning when the user edits the host firewall status or the VM/CT
> firewall status, but the datacenter level firewall is disabled or the
> pve-firewall service is not running on the host.
>
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
>
> Version 3:
> * As discussed offline with Dominik and Thomas, we should keep this as
> simple as possible, only showing the warinings, no checkboxes for NIC ecc.
> * The code was completely refactored as compared to the previous version,
> the main functionality is now contained within FirewallEnableEdit.js
>
> www/manager6/Makefile | 1 +
> www/manager6/grid/FirewallEnableEdit.js | 74 +++++++++++++++++++++++++++++++++
> www/manager6/grid/FirewallOptions.js | 25 +++++++++--
> www/manager6/lxc/Config.js | 3 +-
> www/manager6/node/Config.js | 3 +-
> www/manager6/qemu/Config.js | 3 +-
> 6 files changed, 103 insertions(+), 6 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..b2ee0400
> --- /dev/null
> +++ b/www/manager6/grid/FirewallEnableEdit.js
> @@ -0,0 +1,74 @@
> +Ext.define('PVE.FirewallEnableEdit', {
> + extend: 'Proxmox.window.Edit',
> + alias: ['widget.pveFirewallEnableEdit'],
> +
> + initComponent : function() {
> + var me = this;
> +
> + var dcFirewallDisabledHint = Ext.createWidget({
> + xtype: 'displayfield',
> + userCls: 'pve-hint',
> + value: 'Warning! Firewall disabled at datacenter level!',
> + hidden: true
> + });
> +
> + var fwServiceDisabledHint = Ext.createWidget({
> + xtype: 'displayfield',
> + userCls: 'pve-hint',
> + value: 'Warning! Firewall service not running on node!',
> + hidden: true
> + });
> +
AFAIK createWidget is deprecated and private.
> + 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) {
> + dcFirewallDisabledHint.setVisible(true);
> + }
> + }
> + });
> +
> + Proxmox.Utils.API2Request({
> + url: '/api2/extjs/nodes/' + me.nodename + '/services/pve-firewall/state',
> + method: 'GET',
> + failure: function(response, opts) {
> + Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> + },
> + success: function(response, opts) {
> + var data = response.result.data;
> + if (data.state !== 'running') {
> + fwServiceDisabledHint.setVisible(true);
> + }
> + }
> + });
> +
> + Ext.applyIf(me, {
> + subject: gettext('Firewall'),
> + fieldDefaults: {
> + labelWidth: 100
> + },
> + items: [
> + {
> + xtype: 'proxmoxcheckbox',
> + name: 'enable',
> + uncheckedValue: 0,
> + defaultValue: 0,
> + checked: true,
> + deleteDefaultValue: false,
> + labelWidth: Proxmox.Utils.compute_min_label_width(
> + gettext('Firewall'), 120),
> + fieldLabel: gettext('Firewall')
> + },
> + dcFirewallDisabledHint,
> + fwServiceDisabledHint
> + ]
> + });
> +
> + me.callParent();
> + me.load();
> + }
> +});
This seems to be checked regardless of the actual state in the row. If my VM firewall is off and I edit it, its already checked.
Is this intended?
Most of this is static, you could probably do this in a more declarative way.
Offline discussion:
>From my perspective the check for pve-firewall running is wrongly placed here. If such a check is really needed this should be done with a different concept. This would need a general notification strategy in my opinion and this has to be discussed in more detail. At the end it's ok, because the overhead isn't that much and maybe it helps someone who has disabled the service some time ago.
> diff --git a/www/manager6/grid/FirewallOptions.js b/www/manager6/grid/FirewallOptions.js
> index cddbdbbf..0eb1e02c 100644
> --- a/www/manager6/grid/FirewallOptions.js
> +++ b/www/manager6/grid/FirewallOptions.js
> @@ -64,9 +64,18 @@ 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',
> + nodename: me.nodename,
> + fwtype: me.fwtype
> + }
> + };
> 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 +87,17 @@ 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',
> + nodename: me.nodename,
> + fwtype: me.fwtype
> + }
> + };
> add_boolean_row('dhcp', 'DHCP', 1);
> add_boolean_row('ndp', 'NDP', 1);
> add_boolean_row('radv', gettext('Router Advertisement'), 0);
> diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
> index 51864f1a..2338721c 100644
> --- a/www/manager6/lxc/Config.js
> +++ b/www/manager6/lxc/Config.js
> @@ -269,7 +269,8 @@ Ext.define('PVE.lxc.Config', {
> title: gettext('Options'),
> base_url: base_url + '/firewall/options',
> fwtype: 'vm',
> - itemId: 'firewall-options'
> + itemId: 'firewall-options',
> + nodename: nodename
> },
> {
> xtype: 'pveFirewallAliases',
> diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js
> index f9a62670..9a033521 100644
> --- a/www/manager6/node/Config.js
> +++ b/www/manager6/node/Config.js
> @@ -261,7 +261,8 @@ Ext.define('PVE.node.Config', {
> groups: ['firewall'],
> base_url: '/nodes/' + nodename + '/firewall/options',
> fwtype: 'node',
> - itemId: 'firewall-options'
> + itemId: 'firewall-options',
> + nodename: nodename
> });
> }
>
> diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
> index 38496f4f..f6d36928 100644
> --- a/www/manager6/qemu/Config.js
> +++ b/www/manager6/qemu/Config.js
> @@ -283,7 +283,8 @@ Ext.define('PVE.qemu.Config', {
> title: gettext('Options'),
> base_url: base_url + '/firewall/options',
> fwtype: 'vm',
> - itemId: 'firewall-options'
> + itemId: 'firewall-options',
> + nodename: nodename
> },
> {
> xtype: 'pveFirewallAliases',
> --
> 2.11.0
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
More information about the pve-devel
mailing list