Repository: mariadb-operator/mariadb-operator

Issue:
[Bug] Revoking privileges on a user with specific database and GRANT OPTION can produce invalid SQL statement
<!--
Bugs should be filed for issues encountered whilst operating mariadb-operator.
Please provide as much detail as possible. 
-->

**Documentation**
- [x] I acknowledge that I have read the relevant [documentation](https://github.com/mariadb-operator/mariadb-operator/tree/main/docs).

**Describe the bug**

Revoking grants can produce an invalid SQL statement when attempting to revoke a user with both a specific database assignment and the user also has `GRANT OPTION`.

The SQL produced by revoke has a subtle problem which can be described in an old mysql bug report I found: https://bugs.mysql.com/bug.php?id=69313

Basically, you can't issue a:
```REVOKE ALL PRIVILEGES, GRANT OPTION ON `nova_cell0`.* FROM 'nova'@'%';``` as it produces an error:

```
MariaDB [(none)]> REVOKE ALL PRIVILEGES, GRANT OPTION ON `nova_cell0`.* FROM 'nova'@'%';
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'ON `nova_cell0`.* FROM 'nova'@'%'' at line 1
```


You can only do `REVOKE all privileges, grant option FROM user` - it can't be combined with the specific database nova_cell0 with ```ON `nova_cell0`.*```

Maybe there should be two separate REVOKEs - 1 for revoking on a specific database, and a second revoke for the grant options?

MariaDB revoke reference docs showing the `REVOKE ... ON` versus `REVOKE GRANT OPTION FROM`: https://mariadb.com/kb/en/revoke/

**Expected behaviour**

Expecting revoke to work without error.

**Steps to reproduce the bug**

Using manifest:

```
---
apiVersion: k8s.mariadb.com/v1alpha1
kind: Grant
metadata:
  name: nova-cell0-grant
  namespace: openstack
spec:
  mariaDbRef:
    name: mariadb
    waitForIt: true
  privileges:
    - "ALL"
  database: "nova_cell0"
  table: "*"
  username: nova
  grantOption: true
  host: "%"
  retryInterval: 5s
```

Then issuing a revoke. We added logging to see the actual SQL statement being executed, which is invalid:

```
REVOKE ALL,GRANT OPTION ON `nova_cell0`.* FROM 'nova'@'%';
```

```
MariaDB [(none)]> REVOKE ALL PRIVILEGES, GRANT OPTION ON `nova_cell0`.* FROM 'nova'@'%';
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'ON `nova_cell0`.* FROM 'nova'@'%'' at line 1
```

And this error in the operator logs:

```
{"level":"error","ts":1740421634.2657945,"msg":"Reconciler error","controller":"grant","controllerGroup":"k8s.mariadb.com","controllerKind":"Grant","Grant":{"name":"nova-cell0-grant","namespace":"openstack"},"namespace":"openstack","name":"nova-cell0-grant","reconcileID":"a690ec22-3c8d-47f5-ba31-e8c51b3b414e","error":"error reconciling in TemplateReconciler: error reconciling in TemplateFinalizer: error revoking grant in MariaDB: 1 error occurred:\n\t* Error 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'ON `nova_cell0`.* FROM 'nova'@'%'' at line 1\n\n","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.0/pkg/internal/controller/controller.go:316\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.0/pkg/internal/controller/controller.go:263\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.0/pkg/internal/controller/controller.go:224"}
```

**Environment details**:
- MariaDB Operator version: 0.31.0
- MariaDB Server version: 11.0.3-MariaDB-1:11.0.3+maria~ubu2204
- Install method: argocd + helm


Discussion:
@IvanPiatnishin: hi, can i work on it?
@github-actions: This issue is stale because it has been open 60 days with no activity.@github-actions: This issue was closed because it has been stalled for 30 days with no activity.@mmontes11: Hey @IvanPiatnishin ! Yes, please, go ahead@mmontes11: Please take into account the new changes that have been introduced here, which handles the `REVOKE` differently as stated in the issue:
https://github.com/mariadb-operator/mariadb-operator/pull/1326@github-actions: This issue is stale because it has been open 60 days with no activity.@github-actions: This issue was closed because it has been stalled for 30 days with no activity.@krische: So is there any solution to this? I think I'm seeing this on my operator and it seems like it is crashing the operator pod after so many failed attempts

Solution patch:
diff --git a/api/v1alpha1/grant_types.go b/api/v1alpha1/grant_types.go
index 71c9ec04..472e7b16 100644
--- a/api/v1alpha1/grant_types.go
+++ b/api/v1alpha1/grant_types.go
@@ -21,7 +21,7 @@ type GrantSpec struct {
 	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:MinItems=1
 	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	Privileges []string `json:"privileges" webhook:"inmutable"`
+	Privileges []string `json:"privileges"`
 	// Database to use in the Grant.
 	// +optional
 	// +kubebuilder:default=*
@@ -54,6 +54,11 @@ type GrantStatus struct {
 	// +optional
 	// +operator-sdk:csv:customresourcedefinitions:type=status,xDescriptors={"urn:alm:descriptor:io.kubernetes.conditions"}
 	Conditions []metav1.Condition `json:"conditions,omitempty"`
+	// CurrentPrivileges is the list of current privileges used in the Grant.
+	// It allows to detect the divergence from the desired privileges.
+	// +optional
+	// +operator-sdk:csv:customresourcedefinitions:type=status
+	CurrentPrivileges []string `json:"currentPrivileges"`
 }
 
 func (g *GrantStatus) SetCondition(condition metav1.Condition) {
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
index 688efd55..d29529d4 100644
--- a/api/v1alpha1/zz_generated.deepcopy.go
+++ b/api/v1alpha1/zz_generated.deepcopy.go
@@ -1476,6 +1476,11 @@ func (in *GrantStatus) DeepCopyInto(out *GrantStatus) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
+	if in.CurrentPrivileges != nil {
+		in, out := &in.CurrentPrivileges, &out.CurrentPrivileges
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GrantStatus.
diff --git a/config/crd/bases/k8s.mariadb.com_grants.yaml b/config/crd/bases/k8s.mariadb.com_grants.yaml
index 4be3b443..f02379bb 100644
--- a/config/crd/bases/k8s.mariadb.com_grants.yaml
+++ b/config/crd/bases/k8s.mariadb.com_grants.yaml
@@ -183,6 +183,13 @@ spec:
                   - type
                   type: object
                 type: array
+              currentPrivileges:
+                description: |-
+                  CurrentPrivileges is the list of current privileges used in the Grant.
+                  It allows to detect the divergence from the desired privileges.
+                items:
+                  type: string
+                type: array
             type: object
         type: object
     served: true
diff --git a/deploy/charts/mariadb-operator-crds/templates/crds.yaml b/deploy/charts/mariadb-operator-crds/templates/crds.yaml
index de790ffa..6e3a68d9 100644
--- a/deploy/charts/mariadb-operator-crds/templates/crds.yaml
+++ b/deploy/charts/mariadb-operator-crds/templates/crds.yaml
@@ -1720,6 +1720,13 @@ spec:
                   - type
                   type: object
                 type: array
+              currentPrivileges:
+                description: |-
+                  CurrentPrivileges is the list of current privileges used in the Grant.
+                  It allows to detect the divergence from the desired privileges.
+                items:
+                  type: string
+                type: array
             type: object
         type: object
     served: true
diff --git a/internal/controller/grant_controller.go b/internal/controller/grant_controller.go
index 9f3ed1d7..96102ab8 100644
--- a/internal/controller/grant_controller.go
+++ b/internal/controller/grant_controller.go
@@ -3,6 +3,7 @@ package controller
 import (
 	"context"
 	"fmt"
+	"slices"
 
 	mariadbv1alpha1 "github.com/mariadb-operator/mariadb-operator/api/v1alpha1"
 	condition "github.com/mariadb-operator/mariadb-operator/pkg/condition"
@@ -85,6 +86,22 @@ func newWrappedGrantReconciler(client client.Client, refResolver refresolver.Ref
 
 func (wr *wrappedGrantReconciler) Reconcile(ctx context.Context, mdbClient *sqlClient.Client) error {
 	var opts []sqlClient.GrantOption
+
+	if wr.grant.Status.CurrentPrivileges != nil {
+		if revokePrivileges := wr.privilegesToRevoke(); len(revokePrivileges) > 0 {
+			if err := mdbClient.Revoke(
+				ctx,
+				revokePrivileges,
+				wr.grant.Spec.Database,
+				wr.grant.Spec.Table,
+				wr.grant.AccountName(),
+				opts...,
+			); err != nil {
+				return fmt.Errorf("error revoking privileges in MariaDB: %v", err)
+			}
+		}
+	}
+
 	if wr.grant.Spec.GrantOption {
 		opts = append(opts, sqlClient.WithGrantOption())
 	}
@@ -98,12 +115,24 @@ func (wr *wrappedGrantReconciler) Reconcile(ctx context.Context, mdbClient *sqlC
 	); err != nil {
 		return fmt.Errorf("error granting privileges in MariaDB: %v", err)
 	}
+
+	if err := wr.patchStatusWithFunc(ctx, func(status *mariadbv1alpha1.GrantStatus) {
+		wr.grant.Status.CurrentPrivileges = wr.grant.Spec.Privileges
+	}); err != nil {
+		return fmt.Errorf("error patching current privileges: %v", err)
+	}
 	return nil
 }
 
 func (wr *wrappedGrantReconciler) PatchStatus(ctx context.Context, patcher condition.Patcher) error {
+	return wr.patchStatusWithFunc(ctx, func(status *mariadbv1alpha1.GrantStatus) {
+		patcher(status)
+	})
+}
+
+func (wr *wrappedGrantReconciler) patchStatusWithFunc(ctx context.Context, patchFn func(status *mariadbv1alpha1.GrantStatus)) error {
 	patch := client.MergeFrom(wr.grant.DeepCopy())
-	patcher(&wr.grant.Status)
+	patchFn(&wr.grant.Status)
 
 	if err := wr.Client.Status().Patch(ctx, wr.grant, patch); err != nil {
 		return fmt.Errorf("error patching Grant status: %v", err)
@@ -111,6 +140,19 @@ func (wr *wrappedGrantReconciler) PatchStatus(ctx context.Context, patcher condi
 	return nil
 }
 
+func (wr *wrappedGrantReconciler) privilegesToRevoke() []string {
+	var revokePrivileges []string
+	if slices.Equal(wr.grant.Status.CurrentPrivileges, wr.grant.Spec.Privileges) {
+		return revokePrivileges
+	}
+	for _, privilege := range wr.grant.Status.CurrentPrivileges {
+		if !slices.Contains(wr.grant.Spec.Privileges, privilege) {
+			revokePrivileges = append(revokePrivileges, privilege)
+		}
+	}
+	return revokePrivileges
+}
+
 func userKey(grant *mariadbv1alpha1.Grant) types.NamespacedName {
 	return types.NamespacedName{
 		Name:      grant.Spec.Username,



Test patch:
diff --git a/internal/webhook/v1alpha1/grant_webhook_test.go b/internal/webhook/v1alpha1/grant_webhook_test.go
index d9b30a39..6d9b62c1 100644
--- a/internal/webhook/v1alpha1/grant_webhook_test.go
+++ b/internal/webhook/v1alpha1/grant_webhook_test.go
@@ -146,7 +146,7 @@ var _ = Describe("v1alpha1.Grant webhook", func() {
 						"UPDATE",
 					}
 				},
-				true,
+				false,
 			),
 			Entry(
 				"Updating Database",