Fix grant import #118

gallois · 2024-03-04T14:55:30Z

This fixes the issue described in
#112 where it's
not possible to import some grants. In short, it:

Adds type for parsing the grant resource from data,
desiredProcedureGrant, separating it from desiredGrant, which now
becomes desiredTableGrant. This is used when trying to find a
conflict and can probably be made a bit more generic, but it's likely
outside the scope of this change (see below)
Refactors the old formatDatabaseName into produceGrantId which, as
the name says, produced the grant id that is going to be used by the
provider.
Sets some additional required fields for importing the grants, the
most important one being the id which was missing and causing the
errors that were seen in the issue above.

It also fixes a typo in the docs that I introduced earlier.

There are a few things worth noting:

Role grants are not covered. Unfortunately, I don't currently have a use case for
that, so there's no good way of testing it properly.
As mentioned before, there are parts of the code that could be
refactored to be more generic. That being said, it would also require
implementing the fixes for role grants to write it in such a
way that it would be properly generic.

This fixes the issue described in petoju#112 where it's not possible to import some grants. In short, it: - Adds an additional type for parsing the grant resource from data, `desiredProcedureGrant`, separating it from `desiredGrant`, which now becomes `desiredTableGrant`. This is used when trying to find a conflict and can probably be made a bit more generic, but it's likely outside the scope of this change (see below) - Refactors the old `formatDatabaseName` into `produceGrantId` which, as the name says, produced the grant id that is going to be used by the provider. - Sets some additional required fields for importing the grants, the most important one being the `id` which was missing and causing the errors that were seen in the issue above. It also fixes a typo in the docs that I introduced earlier. There are a few things worth noting: - Role grants are not covered. I don't currently have a use case for that, hence no good way of testing it properly. - As mentioned before, there are parts of the code that could be refactored to be more generic. That being said, it would require implementing the fixes for role grants as well to write it in such a way that it would be properly generic.

mysql/resource_grant.go

petoju · 2024-03-04T17:13:32Z

mysql/resource_grant_test.go

@@ -3,14 +3,15 @@ package mysql
 import (


Could you please create a test to catch this case / issue?

I've fixed the existing ones. Anything else you think should be added?

Generally when you want to fix a regression, there should be a new test catching the very issue you are trying to fix.

So that means you should test importing (I know we don't test it anywhere, but still). And ideally, you should leave all previous tests in place. Or fix them, if there is any error there - but there should be some rationale when removing tests.

I don't think it was a regression in the sense that a feature existed and it stopped working. It was, as I understand, a new issue that was implemented that never really worked. I think the current test is reasonable, ti was just built in a way that worked with the implementation, but that was not working in the first place. I think the current test covers the implementation and I'm not sure what sort of regression, specifically, we'd need to cover.

In the bug #112 , you described

terraform import mysql_grant.foo_grant_local 'foo_user@localhost@test_db@*'

stopped working. Cannot you test import exactly like that here?

I'd do it based on
https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/import#resource-acceptance-testing-implementation

I'm not completely sure how it works - so if that's too difficult and once I find some time, I can also try looking at that.

mysql/resource_grant.go

petoju · 2024-03-05T20:55:29Z

mysql/resource_grant_test.go

@@ -888,21 +889,6 @@ func TestAccGrantOnProcedure(t *testing.T) {
 					prepareProcedure(dbName, procedureName),
 				),
 			},
-			{


What's wrong with this test? I believe it should stay and possibly be fixed if there's something wrong.

I tried running with it and it shows some issues you'd see, if you ran it manually with config.

My understanding—but keep in mind I'm not an expert on the matter—is that semantically it makes no sense to provide procedure grants with tables, so the test was not meaningful. I couldn't find any references online to such a scenario either, so please do correct me if I'm missing something. Again, I'm not an expert on this so there's a good chance I'm wrong :)

This test needs to remain. Although it does make no sense semantically, this is something that was supported by the deprecated Hashicorp mysql provider, and has made its way into other modules:

#99 (comment)

I think the fix for this (as described in that issue), is that we need to introduce a v2 of the mysql_grant resource, ideally specific to each type (eg. mysql_table_grant, mysql_procedure_grant, etc).

Got it. I couldn't find a way to identify how the object is defined in terraform in the code so that I can build the resource accordingly. Are there any downsides to normalising it? I mean, if the object is parsed correctly and the representation is not crucial, can we just pick one and go with it?

From what I tested, regardless of how the terraform object is defined, we can use either of the formats and it works the same way

I mean, if the object is parsed correctly and the representation is not crucial, can we just pick one and go with it?

We cannot do it easily like this.
We could do it, if we:

Preserve the backwards compatibility with whatever was there.

Write custom DiffSuppressFunc so that this change won't cause a persistent diff.

That's one way to solve the issue. That said, this test needs to remain as it reveals and issue that you introduced and it would pass if there was no diff to the user.

As Derek has pointed out, the best way to solve it would be to introduce mysql_table_grant / mysql_procedure_grant - but that also removes backwards compatibility.

petoju · 2024-03-05T20:57:15Z

mysql/resource_grant_test.go

@@ -3,14 +3,15 @@ package mysql
 import (


Generally when you want to fix a regression, there should be a new test catching the very issue you are trying to fix.

So that means you should test importing (I know we don't test it anywhere, but still). And ideally, you should leave all previous tests in place. Or fix them, if there is any error there - but there should be some rationale when removing tests.

DerekTBrown · 2024-03-06T19:52:55Z

mysql/resource_grant.go

-	desiredGrant := &TablePrivilegeGrant{
+	desiredTableGrant := &TablePrivilegeGrant{
 		Database:   database,
-		Table:      table,
+		Table:      tableOrCallable,
 		Grant:      grantOption,
 		UserOrRole: userOrRole,
 	}

+	desiredProcedureGrant := &ProcedurePrivilegeGrant{
+		Database:     database,
+		CallableName: tableOrCallable,
+		Grant:        grantOption,
+		UserOrRole:   userOrRole,
+		ObjectT:      kProcedure,
+	}


I am wondering if we could make this a bit simpler by parsing the tableOrCallable directly:

var desiredGrant *MySQLGrant = nil if kReProcedureWithoutDatabase.MatchString(database) { desiredGrant = &ProcedurePrivilegeGrant{ ... } } else { desiredGrant = &TablePrivilegeGrant{ ... } }

My understanding is that we don't have this in formation at this point—database is derived from the id string being imported, and the way it's constructed now does not expose this information. My initial approach was to do something like this but I couldn't find a clean way.

DerekTBrown · 2024-03-06T19:55:02Z

mysql/resource_grant.go

@@ -688,20 +697,42 @@ func ImportGrant(ctx context.Context, d *schema.ResourceData, meta interface{})
 	return results, nil
 }

+func produceGrantId(database string, username string, host string) string {


This seems potentially redundant with the GetId interface of MySQLGrant. Why do we need both?

That's right! I don't think we do! Thanks for pointing that out.

DerekTBrown · 2024-03-06T20:00:15Z

mysql/resource_grant.go

+		database := procedureGrant.Database
+		id := produceGrantId(database, userOrRole.Name, userOrRole.Host)
+		d.SetId(id)
+		d.Set("database", fmt.Sprintf("PROCEDURE %s.%s", database, procedureGrant.CallableName))


This is not a safe operation, because the user can specify the database as in Example 2:

#99 (comment)

I see, I think I might be able to set table here instead of the string interpolation

DerekTBrown · 2024-03-06T20:02:01Z

mysql/resource_grant_test.go

@@ -888,21 +889,6 @@ func TestAccGrantOnProcedure(t *testing.T) {
 					prepareProcedure(dbName, procedureName),
 				),
 			},
-			{


This test needs to remain. Although it does make no sense semantically, this is something that was supported by the deprecated Hashicorp mysql provider, and has made its way into other modules:

#99 (comment)

I think the fix for this (as described in that issue), is that we need to introduce a v2 of the mysql_grant resource, ideally specific to each type (eg. mysql_table_grant, mysql_procedure_grant, etc).

gallois · 2024-03-08T08:37:24Z

Got it. I'm afraid I'm a bit time-constrained right now to be able to implement additional changes, mostly because "it works on my machine"™

I'll close this PR for now and keep following #112 to ensure I jump back to upstream when it's fixed.

Thanks a lot for the effort in maintaining it :)

gallois added 2 commits March 4, 2024 09:02

Fix acceptance tests

06f37f0

petoju requested changes Mar 4, 2024

View reviewed changes

Address comments

58b18e3

petoju reviewed Mar 5, 2024

View reviewed changes

DerekTBrown suggested changes Mar 6, 2024

View reviewed changes

gallois closed this Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix grant import #118

Fix grant import #118

gallois commented Mar 4, 2024

petoju Mar 4, 2024

gallois Mar 5, 2024

petoju Mar 5, 2024

gallois Mar 6, 2024

petoju Mar 7, 2024

petoju Mar 5, 2024

gallois Mar 6, 2024

DerekTBrown Mar 6, 2024

gallois Mar 6, 2024

petoju Mar 7, 2024

petoju Mar 5, 2024

DerekTBrown Mar 6, 2024

gallois Mar 6, 2024

DerekTBrown Mar 6, 2024

gallois Mar 6, 2024

DerekTBrown Mar 6, 2024

gallois Mar 6, 2024

DerekTBrown Mar 6, 2024

gallois commented Mar 8, 2024

Fix grant import #118

Fix grant import #118

Conversation

gallois commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gallois commented Mar 8, 2024