[pve-devel] [PATCH pve-manager v2 1/2] fix #4963: firewall: fix editing firewall rules using ips / cidrs

[pve-devel] [PATCH pve-manager v2 1/2] fix #4963: firewall: fix editing firewall rules using ips / cidrs

[Stefan Hanreich]

fall back to using v.ref as value when we do not have an alias or ipset
since scope and name are not set for ips / cidrs

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---

Changes from v1:
* Added fix for an additional bug causing changes to IPs not being
  picked up by the input panel

 www/manager6/form/IPRefSelector.js | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
index b50ac1e10..7e5eea63a 100644
--- a/www/manager6/form/IPRefSelector.js
+++ b/www/manager6/form/IPRefSelector.js
@@ -37,8 +37,10 @@ Ext.define('PVE.form.IPRefSelector', {
 		    calculate: function(v) {
 			if (v.type === 'alias') {
 			    return `${v.scope}/${v.name}`;
-			} else {
+			} else if (v.type === 'ipset') {
 			    return `+${v.scope}/${v.name}`;
+			} else {
+			    return v.ref;
 			}
 		    },
 		},
-- 
2.39.2

[pve-devel] [PATCH pve-manager v2 2/2] firewall: properly detect changes when ip / cidr is used in rule

[Stefan Hanreich]

With the current implementation using queryDelay, this means that the
change event for the input never completes. This in turn leads to
the input panel never changing its dirty status. By using the
beforequery event we can simply cancel the query without resorting to
the queryDelay hack.

Reported-By: Mira Limbeck <m.limbeck@proxmox.com>
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 www/manager6/form/IPRefSelector.js | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
index 7e5eea63a..d41cde5f5 100644
--- a/www/manager6/form/IPRefSelector.js
+++ b/www/manager6/form/IPRefSelector.js
@@ -56,15 +56,6 @@ Ext.define('PVE.form.IPRefSelector', {
 	    },
 	});
 
-	var disable_query_for_ips = function(f, value) {
-	    if (value === null ||
-		value.match(/^\d/)) { // IP address starts with \d
-		f.queryDelay = 9999999999; // hack: disable with long delay
-	    } else {
-		f.queryDelay = 10;
-	    }
-	};
-
 	var columns = [];
 
 	if (!me.ref_type) {
@@ -109,7 +100,9 @@ Ext.define('PVE.form.IPRefSelector', {
 	    },
 	});
 
-	me.on('change', disable_query_for_ips);
+	me.on('beforequery', function(queryPlan) {
+	    return !(queryPlan.query === null || queryPlan.query.match(/^\d/));
+	});
 
         me.callParent();
     },
-- 
2.39.2

Re: [pve-devel] [PATCH pve-manager v2 1/2] fix #4963: firewall: fix editing firewall rules using ips / cidrs

[Mira Limbeck]

On 1/16/24 15:30, Stefan Hanreich wrote:
> fall back to using v.ref as value when we do not have an alias or ipset
> since scope and name are not set for ips / cidrs
> 
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> 
> Changes from v1:
> * Added fix for an additional bug causing changes to IPs not being
>   picked up by the input panel
> 
>  www/manager6/form/IPRefSelector.js | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
> index b50ac1e10..7e5eea63a 100644
> --- a/www/manager6/form/IPRefSelector.js
> +++ b/www/manager6/form/IPRefSelector.js
> @@ -37,8 +37,10 @@ Ext.define('PVE.form.IPRefSelector', {
>  		    calculate: function(v) {
>  			if (v.type === 'alias') {
>  			    return `${v.scope}/${v.name}`;
> -			} else {
> +			} else if (v.type === 'ipset') {
>  			    return `+${v.scope}/${v.name}`;
> +			} else {
> +			    return v.ref;
>  			}
>  		    },
>  		},

lgtm, consider this:

Reviewed-by: Mira Limbeck <m.limbeck@proxmox.com>
Tested-by: Mira Limbeck <m.limbeck@proxmox.com>

Re: [pve-devel] [PATCH pve-manager v2 2/2] firewall: properly detect changes when ip / cidr is used in rule

[Mira Limbeck]

On 1/16/24 15:30, Stefan Hanreich wrote:
> With the current implementation using queryDelay, this means that the
> change event for the input never completes. This in turn leads to
> the input panel never changing its dirty status. By using the
> beforequery event we can simply cancel the query without resorting to
> the queryDelay hack.
> 
> Reported-By: Mira Limbeck <m.limbeck@proxmox.com>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  www/manager6/form/IPRefSelector.js | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
> index 7e5eea63a..d41cde5f5 100644
> --- a/www/manager6/form/IPRefSelector.js
> +++ b/www/manager6/form/IPRefSelector.js
> @@ -56,15 +56,6 @@ Ext.define('PVE.form.IPRefSelector', {
>  	    },
>  	});
>  
> -	var disable_query_for_ips = function(f, value) {
> -	    if (value === null ||
> -		value.match(/^\d/)) { // IP address starts with \d
> -		f.queryDelay = 9999999999; // hack: disable with long delay
> -	    } else {
> -		f.queryDelay = 10;
> -	    }
> -	};
> -
>  	var columns = [];
>  
>  	if (!me.ref_type) {
> @@ -109,7 +100,9 @@ Ext.define('PVE.form.IPRefSelector', {
>  	    },
>  	});
>  
> -	me.on('change', disable_query_for_ips);
> +	me.on('beforequery', function(queryPlan) {
> +	    return !(queryPlan.query === null || queryPlan.query.match(/^\d/));
> +	});
>  
>          me.callParent();
>      },

Tested this with ~900 ipsets and ~1000 aliases.
Did not notice any worse delay or issues with this patch applied.

Tested-by: Mira Limbeck <m.limbeck@proxmox.com>
Reviewed-by: Mira Limbeck <m.limbeck@proxmox.com>

Re: [pve-devel] [PATCH pve-manager v2 1/2] fix #4963: firewall: fix editing firewall rules using ips / cidrs

[Filip Schauer]

Tested with ipset, alias and direct IPv4 addresses as source and
destination. It all works for me.

Tested-by: Filip Schauer <f.schauer@proxmox.com>

On 16/01/2024 15:30, Stefan Hanreich wrote:
> fall back to using v.ref as value when we do not have an alias or ipset
> since scope and name are not set for ips / cidrs
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>
> Changes from v1:
> * Added fix for an additional bug causing changes to IPs not being
>    picked up by the input panel
>
>   www/manager6/form/IPRefSelector.js | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
> index b50ac1e10..7e5eea63a 100644
> --- a/www/manager6/form/IPRefSelector.js
> +++ b/www/manager6/form/IPRefSelector.js
> @@ -37,8 +37,10 @@ Ext.define('PVE.form.IPRefSelector', {
>   		    calculate: function(v) {
>   			if (v.type === 'alias') {
>   			    return `${v.scope}/${v.name}`;
> -			} else {
> +			} else if (v.type === 'ipset') {
>   			    return `+${v.scope}/${v.name}`;
> +			} else {
> +			    return v.ref;
>   			}
>   		    },
>   		},

Re: [pve-devel] [PATCH pve-manager v2 1/2] fix #4963: firewall: fix editing firewall rules using ips / cidrs

[Stefan Hanreich]

ping!

would be nice to have this included, since currently editing FW rules is
a bit painful.

On 1/16/24 15:30, Stefan Hanreich wrote:
> fall back to using v.ref as value when we do not have an alias or ipset
> since scope and name are not set for ips / cidrs
> 
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> 
> Changes from v1:
> * Added fix for an additional bug causing changes to IPs not being
>   picked up by the input panel
> 
>  www/manager6/form/IPRefSelector.js | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
> index b50ac1e10..7e5eea63a 100644
> --- a/www/manager6/form/IPRefSelector.js
> +++ b/www/manager6/form/IPRefSelector.js
> @@ -37,8 +37,10 @@ Ext.define('PVE.form.IPRefSelector', {
>  		    calculate: function(v) {
>  			if (v.type === 'alias') {
>  			    return `${v.scope}/${v.name}`;
> -			} else {
> +			} else if (v.type === 'ipset') {
>  			    return `+${v.scope}/${v.name}`;
> +			} else {
> +			    return v.ref;
>  			}
>  		    },
>  		},

[pve-devel] applied-series: [PATCH pve-manager v2 1/2] fix #4963: firewall: fix editing firewall rules using ips / cidrs

[Fabian Grünbichler]

with T-B/R-B as provided, thanks!

On January 16, 2024 3:30 pm, Stefan Hanreich wrote:
> fall back to using v.ref as value when we do not have an alias or ipset
> since scope and name are not set for ips / cidrs
> 
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> 
> Changes from v1:
> * Added fix for an additional bug causing changes to IPs not being
>   picked up by the input panel
> 
>  www/manager6/form/IPRefSelector.js | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/form/IPRefSelector.js b/www/manager6/form/IPRefSelector.js
> index b50ac1e10..7e5eea63a 100644
> --- a/www/manager6/form/IPRefSelector.js
> +++ b/www/manager6/form/IPRefSelector.js
> @@ -37,8 +37,10 @@ Ext.define('PVE.form.IPRefSelector', {
>  		    calculate: function(v) {
>  			if (v.type === 'alias') {
>  			    return `${v.scope}/${v.name}`;
> -			} else {
> +			} else if (v.type === 'ipset') {
>  			    return `+${v.scope}/${v.name}`;
> +			} else {
> +			    return v.ref;
>  			}
>  		    },
>  		},
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>