[pmg-devel] [PATCH pmg-api 1/2] fix #4392: keep empty user bl/wl in database

[pmg-devel] [PATCH pmg-api 1/2] fix #4392: keep empty user bl/wl in database

[Dominik Csapak]

since our cluster sync does currently not handle vanishing rows.
So by keeping the empty entries, they get properly synced to the
other nodes.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
This should be seen as a workaround for now. The "real" fix would
probably be to use a better db schema for the user bl/wl. With that
we have to do a db migration anyway so we can then cleanup the empty
entries too.

 src/PMG/Quarantine.pm | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/PMG/Quarantine.pm b/src/PMG/Quarantine.pm
index d3d0640..bd231e4 100644
--- a/src/PMG/Quarantine.pm
+++ b/src/PMG/Quarantine.pm
@@ -71,16 +71,15 @@ sub add_to_blackwhite {
 	}
 
 	my $queries = "DELETE FROM UserPrefs WHERE pmail = $qu AND (Name = 'WL' OR Name = 'BL');";
-	if (scalar(keys %{$list->{WL}})) {
-	    $queries .=
-	    "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
-	    "VALUES ($qu, 'WL', $wlist, EXTRACT (EPOCH FROM now())::INTEGER);";
-	}
-	if (scalar(keys %{$list->{BL}})) {
-	    $queries .=
-	    "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
-	    "VALUES ($qu, 'BL', $blist, EXTRACT (EPOCH FROM now())::INTEGER);";
-	}
+
+	$queries .=
+	"INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
+	"VALUES ($qu, 'WL', $wlist, EXTRACT (EPOCH FROM now())::INTEGER);";
+
+	$queries .=
+	"INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
+	"VALUES ($qu, 'BL', $blist, EXTRACT (EPOCH FROM now())::INTEGER);";
+
 	$dbh->do($queries);
     }
 
-- 
2.30.2

[pmg-devel] [RFC PATCH pmg-api 2/2] api: proxy user bl/wl calls to master

[Dominik Csapak]

if we don't do that, the non-master nodes would get updated (when logged
in there), but that change would never be synced back to the master.

So to prevent diverging configs, proxy them all to master.

The GET calls are also proxied, because if we don't do that, the user
might wonder why his change is not visible on the GUI right away.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
Sending only as RFC, because we now loose the ability to see what the
non-master nodes actually have in their db...

 src/PMG/API2/Quarantine.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
index a100e11..4eec64e 100644
--- a/src/PMG/API2/Quarantine.pm
+++ b/src/PMG/API2/Quarantine.pm
@@ -229,6 +229,7 @@ __PACKAGE__->register_method ({
     name => 'whitelist',
     path => 'whitelist',
     method => 'GET',
+    proxyto => 'master',
     permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
     description => "Show user whitelist.",
     parameters => {
@@ -258,6 +259,7 @@ __PACKAGE__->register_method ({
     name => 'whitelist_add',
     path => 'whitelist',
     method => 'POST',
+    proxyto => 'master',
     description => "Add user whitelist entries.",
     permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
     protected => 1,
@@ -284,6 +286,7 @@ __PACKAGE__->register_method ({
     name => 'whitelist_delete_base',
     path => 'whitelist',
     method => 'DELETE',
+    proxyto => 'master',
     description => "Delete user whitelist entries.",
     permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
     protected => 1,
@@ -311,6 +314,7 @@ __PACKAGE__->register_method ({
     name => 'blacklist',
     path => 'blacklist',
     method => 'GET',
+    proxyto => 'master',
     permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
     description => "Show user blacklist.",
     parameters => {
@@ -340,6 +344,7 @@ __PACKAGE__->register_method ({
     name => 'blacklist_add',
     path => 'blacklist',
     method => 'POST',
+    proxyto => 'master',
     description => "Add user blacklist entries.",
     permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
     protected => 1,
@@ -366,6 +371,7 @@ __PACKAGE__->register_method ({
     name => 'blacklist_delete_base',
     path => 'blacklist',
     method => 'DELETE',
+    proxyto => 'master',
     description => "Delete user blacklist entries.",
     permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
     protected => 1,
-- 
2.30.2

Re: [pmg-devel] [RFC PATCH pmg-api 2/2] api: proxy user bl/wl calls to master

[Stefan Sterz]

On Thu Feb 15, 2024 at 12:58 PM CET, Dominik Csapak wrote:
> if we don't do that, the non-master nodes would get updated (when logged
> in there), but that change would never be synced back to the master.
>
> So to prevent diverging configs, proxy them all to master.
>
> The GET calls are also proxied, because if we don't do that, the user
> might wonder why his change is not visible on the GUI right away.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> Sending only as RFC, because we now loose the ability to see what the
> non-master nodes actually have in their db...
>

this does improve the ux considerably as changes appear immediatelly.

consider this:

Tested-by: Stefan Sterz <s.sterz@proxmox.com>

>  src/PMG/API2/Quarantine.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> index a100e11..4eec64e 100644
> --- a/src/PMG/API2/Quarantine.pm
> +++ b/src/PMG/API2/Quarantine.pm
> @@ -229,6 +229,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist',
>      path => 'whitelist',
>      method => 'GET',
> +    proxyto => 'master',
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      description => "Show user whitelist.",
>      parameters => {
> @@ -258,6 +259,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist_add',
>      path => 'whitelist',
>      method => 'POST',
> +    proxyto => 'master',
>      description => "Add user whitelist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -284,6 +286,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist_delete_base',
>      path => 'whitelist',
>      method => 'DELETE',
> +    proxyto => 'master',
>      description => "Delete user whitelist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -311,6 +314,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist',
>      path => 'blacklist',
>      method => 'GET',
> +    proxyto => 'master',
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      description => "Show user blacklist.",
>      parameters => {
> @@ -340,6 +344,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist_add',
>      path => 'blacklist',
>      method => 'POST',
> +    proxyto => 'master',
>      description => "Add user blacklist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -366,6 +371,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist_delete_base',
>      path => 'blacklist',
>      method => 'DELETE',
> +    proxyto => 'master',
>      description => "Delete user blacklist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,

Re: [pmg-devel] [RFC PATCH pmg-api 2/2] api: proxy user bl/wl calls to master

[Stoiko Ivanov]

Thanks for the patch!

did not apply it for now, as the previous patch in this series fixes the
introduction of new inconsistencies to the userpref-table (where the user
wl/bl are stored) - so I'd like to keep the option for users who run into
this to fix it manually on the node for a while)

Recorded the patch on our internal docs for breaking changes to not forget
it!


On Thu, 15 Feb 2024 12:58:59 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> if we don't do that, the non-master nodes would get updated (when logged
> in there), but that change would never be synced back to the master.
> 
> So to prevent diverging configs, proxy them all to master.
> 
> The GET calls are also proxied, because if we don't do that, the user
> might wonder why his change is not visible on the GUI right away.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> Sending only as RFC, because we now loose the ability to see what the
> non-master nodes actually have in their db...
> 
>  src/PMG/API2/Quarantine.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> index a100e11..4eec64e 100644
> --- a/src/PMG/API2/Quarantine.pm
> +++ b/src/PMG/API2/Quarantine.pm
> @@ -229,6 +229,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist',
>      path => 'whitelist',
>      method => 'GET',
> +    proxyto => 'master',
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      description => "Show user whitelist.",
>      parameters => {
> @@ -258,6 +259,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist_add',
>      path => 'whitelist',
>      method => 'POST',
> +    proxyto => 'master',
>      description => "Add user whitelist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -284,6 +286,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist_delete_base',
>      path => 'whitelist',
>      method => 'DELETE',
> +    proxyto => 'master',
>      description => "Delete user whitelist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -311,6 +314,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist',
>      path => 'blacklist',
>      method => 'GET',
> +    proxyto => 'master',
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      description => "Show user blacklist.",
>      parameters => {
> @@ -340,6 +344,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist_add',
>      path => 'blacklist',
>      method => 'POST',
> +    proxyto => 'master',
>      description => "Add user blacklist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -366,6 +371,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist_delete_base',
>      path => 'blacklist',
>      method => 'DELETE',
> +    proxyto => 'master',
>      description => "Delete user blacklist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,

Re: [pmg-devel] [RFC PATCH pmg-api 2/2] api: proxy user bl/wl calls to master

[Stoiko Ivanov]

On Thu, 15 Feb 2024 12:58:59 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> if we don't do that, the non-master nodes would get updated (when logged
> in there), but that change would never be synced back to the master.
forgot to mention - this is not the case - the change from the nodes do
get synced back to master (had the opportunity to look through our
cluster-sync code again)

still - proxying to master for something relevant to the rulesystem (the
userprefs), and thus also not getting updated too oftern (it needs
user-input) seems like a sensible way forward!
> 
> So to prevent diverging configs, proxy them all to master.
> 
> The GET calls are also proxied, because if we don't do that, the user
> might wonder why his change is not visible on the GUI right away.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> Sending only as RFC, because we now loose the ability to see what the
> non-master nodes actually have in their db...
> 
>  src/PMG/API2/Quarantine.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> index a100e11..4eec64e 100644
> --- a/src/PMG/API2/Quarantine.pm
> +++ b/src/PMG/API2/Quarantine.pm
> @@ -229,6 +229,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist',
>      path => 'whitelist',
>      method => 'GET',
> +    proxyto => 'master',
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      description => "Show user whitelist.",
>      parameters => {
> @@ -258,6 +259,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist_add',
>      path => 'whitelist',
>      method => 'POST',
> +    proxyto => 'master',
>      description => "Add user whitelist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -284,6 +286,7 @@ __PACKAGE__->register_method ({
>      name => 'whitelist_delete_base',
>      path => 'whitelist',
>      method => 'DELETE',
> +    proxyto => 'master',
>      description => "Delete user whitelist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -311,6 +314,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist',
>      path => 'blacklist',
>      method => 'GET',
> +    proxyto => 'master',
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      description => "Show user blacklist.",
>      parameters => {
> @@ -340,6 +344,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist_add',
>      path => 'blacklist',
>      method => 'POST',
> +    proxyto => 'master',
>      description => "Add user blacklist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,
> @@ -366,6 +371,7 @@ __PACKAGE__->register_method ({
>      name => 'blacklist_delete_base',
>      path => 'blacklist',
>      method => 'DELETE',
> +    proxyto => 'master',
>      description => "Delete user blacklist entries.",
>      permissions => { check => [ 'admin', 'qmanager', 'audit', 'quser'] },
>      protected => 1,

Re: [pmg-devel] [PATCH pmg-api 1/2] fix #4392: keep empty user bl/wl in database

[Stefan Sterz]

On Thu Feb 15, 2024 at 12:58 PM CET, Dominik Csapak wrote:
> since our cluster sync does currently not handle vanishing rows.
> So by keeping the empty entries, they get properly synced to the
> other nodes.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> This should be seen as a workaround for now. The "real" fix would
> probably be to use a better db schema for the user bl/wl. With that
> we have to do a db migration anyway so we can then cleanup the empty
> entries too.
>

this does seem to work as advertised. one small side-effect of keeping
empty entries is that they will still be suggested in the front-ends
combobox.

syncing worked in both direction for me.

>  src/PMG/Quarantine.pm | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/src/PMG/Quarantine.pm b/src/PMG/Quarantine.pm
> index d3d0640..bd231e4 100644
> --- a/src/PMG/Quarantine.pm
> +++ b/src/PMG/Quarantine.pm
> @@ -71,16 +71,15 @@ sub add_to_blackwhite {
>  	}
>
>  	my $queries = "DELETE FROM UserPrefs WHERE pmail = $qu AND (Name = 'WL' OR Name = 'BL');";
> -	if (scalar(keys %{$list->{WL}})) {
> -	    $queries .=
> -	    "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> -	    "VALUES ($qu, 'WL', $wlist, EXTRACT (EPOCH FROM now())::INTEGER);";
> -	}
> -	if (scalar(keys %{$list->{BL}})) {
> -	    $queries .=
> -	    "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> -	    "VALUES ($qu, 'BL', $blist, EXTRACT (EPOCH FROM now())::INTEGER);";
> -	}
> +
> +	$queries .=
> +	"INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> +	"VALUES ($qu, 'WL', $wlist, EXTRACT (EPOCH FROM now())::INTEGER);";
> +
> +	$queries .=
> +	"INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> +	"VALUES ($qu, 'BL', $blist, EXTRACT (EPOCH FROM now())::INTEGER);";
> +

one small nit (that really isn't this patches fault): imo this should be
wrapped like this:

```
$queries .= "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
    "VALUES ($qu, 'WL', $wlist, EXTRACT (EPOCH FROM now())::INTEGER);";

$queries .= "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
    "VALUES ($qu, 'BL', $blist, EXTRACT (EPOCH FROM now())::INTEGER);";
```

so consider this:

Tested-by: Stefan Sterz <s.sterz@proxmox.com>

>  	$dbh->do($queries);
>      }
>

[pmg-devel] applied: [PATCH pmg-api 1/2] fix #4392: keep empty user bl/wl in database

[Stoiko Ivanov]

applied this patch, with Stefan's T-b - Thanks big-time!

followed-up with a patch to address the indentation issue
(tested my reproducer with this second patch ;)


On Thu, 15 Feb 2024 12:58:58 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> since our cluster sync does currently not handle vanishing rows.
> So by keeping the empty entries, they get properly synced to the
> other nodes.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> This should be seen as a workaround for now. The "real" fix would
> probably be to use a better db schema for the user bl/wl. With that
> we have to do a db migration anyway so we can then cleanup the empty
> entries too.
> 
>  src/PMG/Quarantine.pm | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PMG/Quarantine.pm b/src/PMG/Quarantine.pm
> index d3d0640..bd231e4 100644
> --- a/src/PMG/Quarantine.pm
> +++ b/src/PMG/Quarantine.pm
> @@ -71,16 +71,15 @@ sub add_to_blackwhite {
>  	}
>  
>  	my $queries = "DELETE FROM UserPrefs WHERE pmail = $qu AND (Name = 'WL' OR Name = 'BL');";
> -	if (scalar(keys %{$list->{WL}})) {
> -	    $queries .=
> -	    "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> -	    "VALUES ($qu, 'WL', $wlist, EXTRACT (EPOCH FROM now())::INTEGER);";
> -	}
> -	if (scalar(keys %{$list->{BL}})) {
> -	    $queries .=
> -	    "INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> -	    "VALUES ($qu, 'BL', $blist, EXTRACT (EPOCH FROM now())::INTEGER);";
> -	}
> +
> +	$queries .=
> +	"INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> +	"VALUES ($qu, 'WL', $wlist, EXTRACT (EPOCH FROM now())::INTEGER);";
> +
> +	$queries .=
> +	"INSERT INTO UserPrefs (PMail, Name, Data, MTime) " .
> +	"VALUES ($qu, 'BL', $blist, EXTRACT (EPOCH FROM now())::INTEGER);";
> +
>  	$dbh->do($queries);
>      }
>