Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix grant import #118

Closed
wants to merge 3 commits into from
Closed

Fix grant import #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a564271
Fix grant import for table and procedure
gallois Mar 4, 2024
06f37f0
Fix acceptance tests
gallois Mar 4, 2024
58b18e3
Address comments
gallois Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 40 additions & 12 deletions mysql/resource_grant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,16 @@ import (
"context"
"database/sql"
"fmt"
"github.com/hashicorp/go-version"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"log"
"reflect"
"regexp"
"sort"
"strings"
"unicode"

"github.com/hashicorp/go-version"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"

"github.com/go-sql-driver/mysql"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
)
Expand Down Expand Up @@ -393,7 +394,6 @@ var kReProcedureWithoutDatabase = regexp.MustCompile(`(?i)^(function|procedure)
var kReProcedureWithDatabase = regexp.MustCompile(`(?i)^(function|procedure) ([^.]*)\.([^.]*)$`)

func parseResourceFromData(d *schema.ResourceData) (MySQLGrant, diag.Diagnostics) {

// Step 1: Parse the user/role
var userOrRole UserOrRole
userAttr, userOk := d.GetOk("user")
Expand Down Expand Up @@ -652,20 +652,28 @@ func ImportGrant(ctx context.Context, d *schema.ResourceData, meta interface{})
user := userHostDatabaseTable[0]
host := userHostDatabaseTable[1]
database := userHostDatabaseTable[2]
table := userHostDatabaseTable[3]
tableOrCallable := userHostDatabaseTable[3]
grantOption := len(userHostDatabaseTable) == 5
userOrRole := UserOrRole{
Name: user,
Host: host,
}

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,
}
Comment on lines -662 to +675
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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{ ... }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


db, err := getDatabaseFromMeta(ctx, meta)
if err != nil {
return nil, fmt.Errorf("Got error while getting database from meta: %w", err)
Expand All @@ -676,7 +684,8 @@ func ImportGrant(ctx context.Context, d *schema.ResourceData, meta interface{})
return nil, fmt.Errorf("Failed to showUserGrants in import: %w", err)
}
for _, foundGrant := range grants {
if grantsConflict(desiredGrant, foundGrant) {
if grantsConflict(desiredTableGrant, foundGrant) ||
grantsConflict(desiredProcedureGrant, foundGrant) {
res := resourceGrant().Data(nil)
setDataFromGrant(foundGrant, res)
return []*schema.ResourceData{res}, nil
Expand All @@ -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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if database != "*" && !strings.HasSuffix(database, "`") {
reProcedure := regexp.MustCompile(`(?i)^(function|procedure) (.*)$`)
if reProcedure.MatchString(database) {
// This is only a hack - user can specify function / procedure as database.
database = reProcedure.ReplaceAllString(database, "$1 `${2}`")
} else {
database = fmt.Sprintf("`%s`", database)
}
}

return fmt.Sprintf("%s@%s:%s", database, username, host)
}

// setDataFromGrant copies the values from MySQLGrant to the schema.ResourceData
// This function is used when importing a new Grant, or when syncing remote state to Terraform state
// It is responsible for pulling any non-identifying properties (e.g. grant, tls_option) into the Terraform state
// Identifying properties (database, table) are already set either as part of the import id or required properties
// of the Terraform resource.
func setDataFromGrant(grant MySQLGrant, d *schema.ResourceData) *schema.ResourceData {
userOrRole := grant.GetUserOrRole()
if tableGrant, ok := grant.(*TablePrivilegeGrant); ok {
d.Set("grant", grant.GrantOption())
d.Set("tls_option", tableGrant.TLSOption)

database := tableGrant.Database
id := produceGrantId(database, userOrRole.Name, userOrRole.Host)
d.SetId(id)
d.Set("database", database)
d.Set("table", tableGrant.Table)
petoju marked this conversation as resolved.
Show resolved Hide resolved
} else if procedureGrant, ok := grant.(*ProcedurePrivilegeGrant); ok {
d.Set("grant", grant.GrantOption())
d.Set("tls_option", procedureGrant.TLSOption)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#99 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if roleGrant, ok := grant.(*RoleGrant); ok {
d.Set("grant", grant.GrantOption())
d.Set("roles", roleGrant.Roles)
Expand All @@ -726,7 +757,6 @@ func setDataFromGrant(grant MySQLGrant, d *schema.ResourceData) *schema.Resource

// This is a bit of a hack, since we don't have a way to distingush between users and roles
// from the grant itself. We can only infer it from the schema.
userOrRole := grant.GetUserOrRole()
if d.Get("role") != "" {
d.Set("role", userOrRole.Name)
} else {
Expand Down Expand Up @@ -770,7 +800,6 @@ func getMatchingGrant(ctx context.Context, db *sql.DB, desiredGrant MySQLGrant)
return nil, fmt.Errorf("showGrant - getting all grants failed: %w", err)
}
for _, dbGrant := range allGrants {

// Check if the grants cover the same user, table, database
// If not, continue
if !grantsConflict(desiredGrant, dbGrant) {
Expand Down Expand Up @@ -834,7 +863,6 @@ var (
)

func parseGrantFromRow(grantStr string) (MySQLGrant, error) {

// Ignore REVOKE.*
if strings.HasPrefix(grantStr, "REVOKE") {
log.Printf("[WARN] Partial revokes are not fully supported and lead to unexpected behavior. Consult documentation https://dev.mysql.com/doc/refman/8.0/en/partial-revokes.html on how to disable them for safe and reliable terraform. Relevant partial revoke: %s\n", grantStr)
Expand Down
28 changes: 7 additions & 21 deletions mysql/resource_grant_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package mysql
import (
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"context"
"fmt"
_ "github.com/go-sql-driver/mysql"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
"log"
"math/rand"
"regexp"
"strings"
"testing"

_ "github.com/go-sql-driver/mysql"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

func TestAccGrant(t *testing.T) {
Expand Down Expand Up @@ -423,11 +424,11 @@ func testAccPrivilege(rn string, privilege string, expectExists bool, expectGran
return err
}

id := strings.Split(rs.Primary.ID, ":")
id := strings.Split(rs.Primary.ID, "@")

var userOrRole UserOrRole
if strings.Contains(id[0], "@") {
parts := strings.Split(id[0], "@")
if strings.Contains(id[1], ":") {
parts := strings.Split(id[1], ":")
userOrRole = UserOrRole{
Name: parts[0],
Host: parts[1],
Expand Down Expand Up @@ -888,21 +889,6 @@ func TestAccGrantOnProcedure(t *testing.T) {
prepareProcedure(dbName, procedureName),
),
},
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Config: testAccGrantConfigProcedureWithTable(procedureName, dbName, hostName),
Check: resource.ComposeTestCheckFunc(
testAccCheckProcedureGrant("mysql_grant.test_procedure", userName, hostName, procedureName, true),
resource.TestCheckResourceAttr("mysql_grant.test_procedure", "user", userName),
resource.TestCheckResourceAttr("mysql_grant.test_procedure", "host", hostName),
// Note: The database and table name do not change. This is to preserve legacy functionality.
resource.TestCheckResourceAttr("mysql_grant.test_procedure", "database", fmt.Sprintf("PROCEDURE %s", dbName)),
resource.TestCheckResourceAttr("mysql_grant.test_procedure", "table", procedureName),
),
},
{
// Remove the grant
Config: testAccGrantConfigNoGrant(dbName),
},
{
Config: testAccGrantConfigProcedureWithDatabase(procedureName, dbName, hostName),
Check: resource.ComposeTestCheckFunc(
Expand Down
2 changes: 1 addition & 1 deletion website/docs/index.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ The following arguments are supported:
* `password` - (Optional) Password for the given user, if that user has a password, can also be sourced from the `MYSQL_PASSWORD` environment variable.
* `proxy` - (Optional) Proxy socks url, can also be sourced from `ALL_PROXY` or `all_proxy` environment variables.
* `tls` - (Optional) The TLS configuration. One of `false`, `true`, or `skip-verify`. Defaults to `false`. Can also be sourced from the `MYSQL_TLS_CONFIG` environment variable.
* `custom_tls` - (Optional) Sets custom tls options for the connection. Documentation for encrypted connections can be found [here](https://dev.mysql.com/doc/refman/8.0/en/using-encrypted-connections.html). Consider setting shorter `connect_retry_timeout_sec` for debugging, as the default is 10 minutes .This is a block containing an optional `config_key`, which value is discarded but might be useful when troubleshooting, and the following required arguments:
* `custom_tls` - (Optional) Sets custom tls options for the connection. Documentation for encrypted connections can be found [here](https://dev.mysql.com/doc/refman/8.0/en/using-encrypted-connections.html). Consider setting shorter `connect_retry_timeout_sec` for debugging, as the default is 10 minutes. This is a block containing an optional `config_key`, which value is discarded but might be useful when troubleshooting, and the following required arguments:
* `ca_cert`
* `client_cert`
* `client_key`
Expand Down
Loading