Repository: crossplane-contrib/provider-sql
Pull request #134 (2023-03-06 19:59:31)

Fix/grant deletion hang after user is removed error 1141
<!--
Thank you for helping to improve Crossplane!

Please read through https://git.io/fj2m9 if this is your first time opening a
Crossplane pull request. Find us in https://slack.crossplane.io/messages/dev if
you need any help contributing.
-->

### Description of your changes

<!--
Briefly describe what this pull request does. Be sure to direct your reviewers'
attention to anything that needs special consideration.

We love pull requests that resolve an open Crossplane issue. If yours does, you
can uncomment the below line to indicate which issue your PR fixes, for example
"Fixes #500":

-->
Fixes [#47](https://github.com/crossplane-contrib/provider-sql/issues/47)

I have:

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `make reviewable` to ensure this PR is ready for review.

### How has this code been tested

<!--
Before reviewers can be confident in the correctness of this pull request, it
needs to tested and shown to be correct. Briefly describe the testing that has
already been done or which is planned for this change.
-->
Add unit tests for grant controller observe method:
 - SuccessDiffGrantUsage
 
Update unit tests for grant controller observe method:
 - SuccessDiffGrantNoDatabaseNoTable
   - Since this issue does not happen when a database is provided, as the default mysql behavior of granting `USAGE` to `*.*` does not affect grant observe comparison between actual and requested in resource spec privileges

[contribution process]: https://git.io/fj2m9


alereca: @Duologic would you recommend keeping the old file name `examples/mysql/grant.yaml` or updating to `examples/mysql/grant-database.yaml` which I consider to be more descriptive?
Duologic: Rename sounds good. 👍

Patch:
diff --git a/examples/mysql/grant_all_databases.yaml b/examples/mysql/grant_all_databases.yaml
new file mode 100644
index 0000000..0ffad8c
--- /dev/null
+++ b/examples/mysql/grant_all_databases.yaml
@@ -0,0 +1,12 @@
+apiVersion: mysql.sql.crossplane.io/v1alpha1
+kind: Grant
+metadata:
+  name: example-grant-all-databases
+spec:
+  forProvider:
+    privileges:
+      - DROP
+      - CREATE ROUTINE
+      - EVENT
+    userRef:
+      name: example-user
diff --git a/examples/mysql/grant.yaml b/examples/mysql/grant_database.yaml
similarity index 88%
rename from examples/mysql/grant.yaml
rename to examples/mysql/grant_database.yaml
index 37c89b6..3c80bab 100644
--- a/examples/mysql/grant.yaml
+++ b/examples/mysql/grant_database.yaml
@@ -1,7 +1,7 @@
 apiVersion: mysql.sql.crossplane.io/v1alpha1
 kind: Grant
 metadata:
-  name: example-grant
+  name: example-grant-database
 spec:
   forProvider:
     privileges:
diff --git a/pkg/controller/mysql/grant/reconciler.go b/pkg/controller/mysql/grant/reconciler.go
index 1861e66..6fad9ab 100644
--- a/pkg/controller/mysql/grant/reconciler.go
+++ b/pkg/controller/mysql/grant/reconciler.go
@@ -202,10 +202,34 @@ func parseGrant(grant, dbname string, table string) (privileges []string) {
 
 func (c *external) getPrivileges(ctx context.Context, username, dbname string, table string) ([]string, *managed.ExternalObservation, error) {
 	username, host := mysql.SplitUserHost(username)
+
+	privileges, err := c.parseGrantRows(ctx, username, host, dbname, table)
+	if err != nil {
+		var myErr *mysqldriver.MySQLError
+		if errors.As(err, &myErr) && myErr.Number == errCodeNoSuchGrant {
+			// The user doesn't (yet) exist and therefore no grants either
+			return nil, &managed.ExternalObservation{ResourceExists: false}, nil
+		}
+
+		return nil, nil, errors.Wrap(err, errCurrentGrant)
+	}
+
+	// In mysql when all grants are revoked from user, it still grants usage (meaning no privileges) on *.*
+	// So the grant can be considered as non existent, just like when privileges slice is nil/empty
+	// https://dev.mysql.com/doc/refman/8.0/en/privileges-provided.html#priv_usage
+	if privileges == nil || privilegesEqual(privileges, []string{"USAGE"}) {
+		return nil, &managed.ExternalObservation{ResourceExists: false}, nil
+	}
+
+	return privileges, nil, nil
+}
+
+func (c *external) parseGrantRows(ctx context.Context, username string, host string, dbname string, table string) ([]string, error) {
 	query := fmt.Sprintf("SHOW GRANTS FOR %s@%s", mysql.QuoteValue(username), mysql.QuoteValue(host))
 	rows, err := c.db.Query(ctx, xsql.Query{String: query})
+
 	if err != nil {
-		return nil, nil, errors.Wrap(err, errCurrentGrant)
+		return nil, err
 	}
 	defer rows.Close() //nolint:errcheck
 
@@ -213,7 +237,7 @@ func (c *external) getPrivileges(ctx context.Context, username, dbname string, t
 	for rows.Next() {
 		var grant string
 		if err := rows.Scan(&grant); err != nil {
-			return nil, nil, errors.Wrap(err, errCurrentGrant)
+			return nil, err
 		}
 		p := parseGrant(grant, dbname, table)
 
@@ -225,17 +249,10 @@ func (c *external) getPrivileges(ctx context.Context, username, dbname string, t
 	}
 
 	if err := rows.Err(); err != nil {
-		var myErr *mysqldriver.MySQLError
-		if errors.As(err, &myErr) && myErr.Number == errCodeNoSuchGrant {
-			// The user doesn't (yet) exist and therefore no grants either
-			return nil, &managed.ExternalObservation{ResourceExists: false}, nil
-		}
-		return nil, nil, errors.Wrap(err, errCurrentGrant)
+		return nil, err
 	}
-	if privileges == nil {
-		return nil, &managed.ExternalObservation{ResourceExists: false}, nil
-	}
-	return privileges, nil, nil
+
+	return privileges, nil
 }
 
 func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.ExternalCreation, error) {


Test patch:
diff --git a/cluster/local/integration_tests.sh b/cluster/local/integration_tests.sh
index 5737c92..5f21451 100755
--- a/cluster/local/integration_tests.sh
+++ b/cluster/local/integration_tests.sh
@@ -252,15 +252,15 @@ echo_step_completed
 
 echo_step "creating MySQL Grant resource"
 # create grant
-"${KUBECTL}" apply -f ${projectdir}/examples/mysql/grant.yaml
+"${KUBECTL}" apply -f ${projectdir}/examples/mysql/grant_database.yaml
 
 echo_info "check if is ready"
-"${KUBECTL}" wait --timeout 2m --for condition=Ready -f ${projectdir}/examples/mysql/grant.yaml
+"${KUBECTL}" wait --timeout 2m --for condition=Ready -f ${projectdir}/examples/mysql/grant_database.yaml
 echo_step_completed
 
 # uninstall
 echo_step "uninstalling ${PROJECT_NAME}"
-"${KUBECTL}" delete -f ${projectdir}/examples/mysql/grant.yaml
+"${KUBECTL}" delete -f ${projectdir}/examples/mysql/grant_database.yaml
 "${KUBECTL}" delete -f ${projectdir}/examples/mysql/database.yaml
 "${KUBECTL}" delete -f ${projectdir}/examples/mysql/user.yaml
 echo "${PROVIDER_CONFIG_YAML}" | "${KUBECTL}" delete -f -
diff --git a/pkg/controller/mysql/grant/reconciler_test.go b/pkg/controller/mysql/grant/reconciler_test.go
index 9955484..a3f0351 100644
--- a/pkg/controller/mysql/grant/reconciler_test.go
+++ b/pkg/controller/mysql/grant/reconciler_test.go
@@ -215,7 +215,7 @@ func TestObserve(t *testing.T) {
 			},
 		},
 		"SuccessNoGrant": {
-			reason: "We should return ResourceExists: false when no grant is found",
+			reason: "We should return ResourceExists: false when no grant is found, being privileges result empty",
 			fields: fields{
 				db: mockDB{
 					MockQuery: func(ctx context.Context, q xsql.Query) (*sql.Rows, error) {
@@ -319,7 +319,7 @@ func TestObserve(t *testing.T) {
 			},
 		},
 		"SuccessDiffGrants": {
-			reason: "We should return no error if different grants exist",
+			reason: "We should return no error if different grants exist for the provided database",
 			fields: fields{
 				db: mockDB{
 					MockQuery: func(ctx context.Context, q xsql.Query) (*sql.Rows, error) {
@@ -349,6 +349,65 @@ func TestObserve(t *testing.T) {
 				},
 			},
 		},
+		"SuccessDiffGrantNoDatabaseNoTable": {
+			reason: "We should return no error if different grants exist and no database and table are provided",
+			fields: fields{
+				db: mockDB{
+					MockQuery: func(ctx context.Context, q xsql.Query) (*sql.Rows, error) {
+						return mockRowsToSQLRows(
+							sqlmock.NewRows([]string{"Grants"}).
+								AddRow("GRANT INSERT ON *.* TO 'success-user'@%"),
+						), nil
+					},
+				},
+			},
+			args: args{
+				mg: &v1alpha1.Grant{
+					Spec: v1alpha1.GrantSpec{
+						ForProvider: v1alpha1.GrantParameters{
+							User:       pointer.StringPtr("success-user"),
+							Privileges: v1alpha1.GrantPrivileges{"DROP", "CREATE"},
+						},
+					},
+				},
+			},
+			want: want{
+				o: managed.ExternalObservation{
+					ResourceExists:   true,
+					ResourceUpToDate: false,
+				},
+				err: nil,
+			},
+		},
+		"SuccessDiffGrantUsage": {
+			reason: "We should return ResourceExists: false when a USAGE grant is found, since it is equivalent to having no grants",
+			fields: fields{
+				db: mockDB{
+					MockQuery: func(ctx context.Context, q xsql.Query) (*sql.Rows, error) {
+						return mockRowsToSQLRows(
+							sqlmock.NewRows([]string{"Grants"}).
+								AddRow("GRANT USAGE ON *.* TO 'success-user'@%"),
+						), nil
+					},
+				},
+			},
+			args: args{
+				mg: &v1alpha1.Grant{
+					Spec: v1alpha1.GrantSpec{
+						ForProvider: v1alpha1.GrantParameters{
+							User:       pointer.StringPtr("success-user"),
+							Privileges: v1alpha1.GrantPrivileges{"DROP", "CREATE"},
+						},
+					},
+				},
+			},
+			want: want{
+				o: managed.ExternalObservation{
+					ResourceExists: false,
+				},
+				err: nil,
+			},
+		},
 		"SuccessManyGrants": {
 			reason: "We should return no error if there are more than one grant for a user",
 			fields: fields{
@@ -382,7 +441,7 @@ func TestObserve(t *testing.T) {
 			},
 		},
 		"SuccessGrantNoDatabaseNoTable": {
-			reason: "We should return no error if no database and table were provided",
+			reason: "We should return no error if no database and table were provided and grants were equal to the ones in resource spec",
 			fields: fields{
 				db: mockDB{
 					MockQuery: func(ctx context.Context, q xsql.Query) (*sql.Rows, error) {