[feature proposal] first class support for procedures and function grants

[DerekTBrown]

Context

The deprecated Hashicorp Terraform provider supported role grants in a very clunky way:

# Example 1
resource "mysql_grant" "test_procedure" {
    user       = "jdoe"
    host       = "%"
    privileges = ["EXECUTE"]
    database   = "PROCEDURE my_db.my_procedure"
}

# Example 2
resource "mysql_grant" "test_procedure" {
    user       = "jdoe"
    host       = "%"
    privileges = ["EXECUTE"]
    database   = "PROCEDURE my_db"
    table = "my_procedure"
}

It appears that some attempts have been made to preserve this functionality:

• https://github.com/petoju/terraform-provider-mysql/blob/master/mysql/resource_grant.go#L534
• https://github.com/petoju/terraform-provider-mysql/blob/master/mysql/resource_grant.go#L536

However, backwards compatibility (particularly Example 1) has been lost. Attempting to migrate directly from the upstream provider to the petoju one results in an error like the following:

Error: Error running SQL (GRANT EXECUTE ON PROCEDURE `my_db.my_procedure`.* TO 'jdoe'@'%'): Error 1144 (42000): Illegal GRANT/REVOKE command; please consult the manual to see which privileges can be used

Moreover, this approach is incredibly hacky and hard to reason about. Here is my ugly attempt to provide backwards compatibility:

https://github.com/petoju/terraform-provider-mysql/pull/98/files

Proposal

To simplify the logic, I think it makes sense to provide a first-class mysql_grant_procedure and mysql_grant_function resource. This would greatly simplify the amount of special-case logic, and provide a clearer interface for consumers.

[petoju]

Oh, I didn't know Example 1 has ever worked. Table is * by default and that whole doesn't make sense.
That said, this is not documented, so people could use it in any way.

There are these things to solve:

• Preserve backwards compatibility at least in the form how it works (Example 2). I believe there should be at least 6 months or 1 year of warnings before removing it (and in the best case, leave it as it is).
• Grants like "EXECUTE" shouldn't be read by mysql_grant resource in case we decide to remove the old solution. I believe this is already like that, but further changes shouldn't break it.

If we have solution, I'd review & merge the fixes.

[DerekTBrown]

@petoju Sorry for the delay. I ran into some additional things that rely on Example 1. Would it be possible to merge #98 , which adds backwards compatibility?

[petoju]

@DerekTBrown I added some comments to find out what you think about some areas, but I don't have any serious issues with merging it.

[DerekTBrown]

@petoju After reviewing your comments, I realized that my prior PR (#98) was going to make things way, way uglier (for instance, we could no have resource grants with no table, but with a procedure in the database field, ick). It is a much more involved change, but I have a PR for schematizing the various types of role grants, and adding support for procedures:

#102

[DerekTBrown]

@petoju thanks for reviewing!

To simplify the logic, I think it makes sense to provide a first-class mysql_grant_procedure and mysql_grant_function resource. This would greatly simplify the amount of special-case logic, and provide a clearer interface for consumers.

Do you think it makes sense to introduce specific resource blocks, now that the "backend" separates between these?

[petoju]

@DerekTBrown I'd merge them if the code was reused where possible, but I wouldn't like to delete the old ones in the near future. Users of this provider got used to them and refactoring is a lot of work.

[DerekTBrown]

So, I just realized that between #98 and #102, I forgot to include tests for both Example 1 and Example 2. It looks like the new implementation has issues with Example 2, because it doesn't like a Procedure grant having a table:

...
          # mysql_grant.test_procedure must be replaced
        -/+ resource "mysql_grant" "test_procedure" {
              ~ database   = "PROCEDURE tf-test-48.test_procedure" -> "PROCEDURE tf-test-48"
              ~ id         = "jdoe-tf-test-48@%:`tf-test-48`" -> (known after apply)
              ~ table      = "*" -> "test_procedure"
                # (5 unchanged attributes hidden)
            }

The simplicity gains of #102 come from the fact that there is a 1:1 mapping between a SHOW GRANTS row and the resource data (we effectively go from ResourceData -> Grant -> ResourceData). Allowing both modes of defining procedures creates a 2:1 mapping, which breaks this model.

[DerekTBrown]

Here is a fix:

#104

[DerekTBrown]

Re-opening: So, we definitely made a step closer to this by having different structs, but we need a different resource to truly make the code cleaner.