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: ALL implicit privileges equality check #339

Merged
merged 16 commits into from
Oct 24, 2024
Merged

fix: ALL implicit privileges equality check #339

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
09a9f30
feat: improved privilege matching in case of ALL
talbx Aug 26, 2023
edacad1
feat: added implicit privilege check for all object types except colu…
talbx Aug 27, 2023
e3b92a0
test: provided unit test for arePriviligesEqual function
talbx Aug 27, 2023
add228c
Merge remote-tracking branch 'origin/master' into all-privileges
talbx Nov 15, 2023
f58fbb3
chore: merge master
talbx Feb 26, 2024
8dfcf90
fix: apply review findings - removed redundant wanted param of privil…
talbx Feb 26, 2024
db54bfc
refactor: inlined all equal checks
talbx Feb 26, 2024
54fc593
fix: use the granted priviliges set from tf instead of db, added acce…
talbx Mar 5, 2024
c64e956
cleanup: fixed function name typos & removed useless testing helper fn
talbx Mar 5, 2024
a091370
provided a more reasonable name for testCheck function in implicit gr…
talbx Mar 5, 2024
30d4b45
resolve merge conflicts
talbx Mar 5, 2024
d288aaf
Merge branch 'cyrilgdn:main' into all-privileges
talbx Oct 2, 2024
55875e9
Merge branch 'main' into all-privileges
cyrilgdn Oct 23, 2024
0393157
simplified privilege set creation
talbx Oct 23, 2024
c35441d
fix: always set id, no matter if privileges equal or not
talbx Oct 23, 2024
4f65139
fmt
Oct 24, 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
28 changes: 28 additions & 0 deletions postgresql/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,34 @@ func validatePrivileges(d *schema.ResourceData) error {
return nil
}

func arePrivilegesEqual(granted *schema.Set, wanted *schema.Set, d *schema.ResourceData) bool {
objectType := d.Get("object_type").(string)

if granted.Equal(wanted) {
return true
}

if !wanted.Contains("ALL") {
return false
}

// implicit check: e.g. for object_type schema -> ALL == ["CREATE", "USAGE"]
log.Printf("The wanted privilege is 'ALL'. therefore, we will check if the current privileges are ALL implicitely")
implicits := make([]string, 0)
for _, p := range allowedPrivileges[objectType] {
if p != "ALL" {
implicits = append(implicits, p)
}
}

s := make([]interface{}, len(implicits))
for i, privilege := range implicits {
s[i] = privilege
}
wantedSet := schema.NewSet(schema.HashString, s)
return granted.Equal(wantedSet)
}

func pgArrayToSet(arr pq.ByteaArray) *schema.Set {
s := make([]interface{}, len(arr))
for i, v := range arr {
Expand Down
72 changes: 72 additions & 0 deletions postgresql/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package postgresql

import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -17,3 +18,74 @@ func TestFindStringSubmatchMap(t *testing.T) {
},
)
}

func TestArePrivilegesEqual(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

Cool 👍


type PrivilegesTestObject struct {
d *schema.ResourceData
granted *schema.Set
wanted *schema.Set
assertion bool
}

tt := []PrivilegesTestObject{
{
buildResourceData("database", t),
buildPrivilegesSet("CONNECT", "CREATE", "TEMPORARY"),
all(),
true,
},
{
buildResourceData("database", t),
buildPrivilegesSet("CREATE", "USAGE"),
buildPrivilegesSet("USAGE"),
false,
},
{
buildResourceData("table", t),
buildPrivilegesSet("SELECT", "INSERT", "UPDATE", "DELETE", "TRUNCATE", "REFERENCES", "TRIGGER"),
all(),
true,
},
{
buildResourceData("table", t),
buildPrivilegesSet("SELECT"),
buildPrivilegesSet("SELECT, INSERT"),
false,
},
{
buildResourceData("schema", t),
buildPrivilegesSet("CREATE", "USAGE"),
all(),
true,
},
{
buildResourceData("schema", t),
buildPrivilegesSet("CREATE"),
all(),
false,
},
}

for _, configuration := range tt {
equal := arePrivilegesEqual(configuration.granted, configuration.wanted, configuration.d)
assert.Equal(t, configuration.assertion, equal)
}
}

func buildPrivilegesSet(grants ...interface{}) *schema.Set {
return schema.NewSet(schema.HashString, grants)
}

func all() *schema.Set {
return buildPrivilegesSet("ALL")
}

func buildResourceData(objectType string, t *testing.T) *schema.ResourceData {
var testSchema = map[string]*schema.Schema{
"object_type": {Type: schema.TypeString},
}
m := make(map[string]interface{})
m["object_type"] = objectType
return schema.TestResourceDataRaw(t, testSchema, m)
}
8 changes: 6 additions & 2 deletions postgresql/resource_postgresql_default_privileges.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,12 @@ func readRoleDefaultPrivileges(txn *sql.Tx, d *schema.ResourceData) error {
}

privilegesSet := pgArrayToSet(privileges)
d.Set("privileges", privilegesSet)
d.SetId(generateDefaultPrivilegesID(d))
privilegesEqual := arePrivilegesEqual(privilegesSet, d.Get("privileges").(*schema.Set), d)

if !privilegesEqual {
d.Set("privileges", privilegesSet)
d.SetId(generateDefaultPrivilegesID(d))
Copy link
Owner

Choose a reason for hiding this comment

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

Why the SetId is in the if? It's not the case for postgresql_grant and I think we want to set it in any cases.

Currently this works anyway because the ID is based on fields that recreate the resource if they change so it will never change here, but this is still totally unrelated to privileges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, this was probably just a slip.
changed this with c35441d

}

return nil
}
Expand Down
40 changes: 32 additions & 8 deletions postgresql/resource_postgresql_grant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ func resourcePostgreSQLGrantRead(db *DBConnection, d *schema.ResourceData) error
if err := validateFeatureSupport(db, d); err != nil {
return fmt.Errorf("feature is not supported: %v", err)
}

if true {
//return fmt.Errorf("hello world! %v", "you")
}
exists, err := checkRoleDBSchemaExists(db.client, d)
if err != nil {
return err
Expand Down Expand Up @@ -269,8 +271,12 @@ WHERE grantee = $2
if err := txn.QueryRow(query, dbName, roleOID).Scan(&privileges); err != nil {
return fmt.Errorf("could not read privileges for database %s: %w", dbName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
wanted := d.Get("privileges").(*schema.Set)
equal := arePrivilegesEqual(granted, wanted, d)
if !equal {
return d.Set("privileges", wanted)
Copy link
Owner

Choose a reason for hiding this comment

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

granted no ? as this function read the state of the database. Otherwise there will never be any diff as you set in the state what's in the state already.

Suggested change
return d.Set("privileges", wanted)
return d.Set("privileges", granted)

Copy link
Contributor Author

@talbx talbx Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not quite sure about that. We check whats on the DB...

if the state on the DB does not equal whats requested via terraform, we want to set the privileges that are requested. granted is what we already have. this would then probably also apply to your other comment.

Also, that would be strange because that would be a fundamental bug in the "feature". Since i tested the changes with a database locally and had great success with the former implementation, I would be very much surprised if this was wrong all the way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only thing that is weird to me right now is the fact, that I fetch the required privileges from the resourcedata and then put it back there again. this call is quite superflous but still won't hurt.

Copy link
Owner

Choose a reason for hiding this comment

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

we want to set the privileges that are requested. granted is what we already have.

Indeed, but that's not exactly how this function works.

The goal of a resource read function (in this case resourcePostgreSQLGrantRead) is to refresh the Terraform state with the current status of what's in Postgres.

Then Terraform will compare the resource as it's defined in your code with how it is in the state (so in Postgres).
So the goal of this line is to set the privileges as it is in the dabase so it will be compared with what's defined in you resource definition.

Since i tested the changes with a database locally and had great success with the former implementation

It's because you haven't tested all the use cases (if I dare 😅 )

You think you fully fix that because there's no more diff in terraform plan compared to before, but actually there will now never be any diff because you say that the current state in the database is what's defined in your code, so Terraform will always say that everything is already correct.

Easy to demonstrate with this simple code:

resource "postgresql_database" "test" { 
  name = "test_db"
}

resource "postgresql_role" "test" {
  name = "test"
}

resource "postgresql_grant" "db_all" {
  database    = postgresql_database.test.name
  role        = postgresql_role.test.name
  object_type = "database"
  privileges  = ["ALL"]
}

that you can apply:

terraform apply

Then, you manually remove a permission in Postgres:

$ psql -c "revoke connect ON database test_db from test "
REVOKE

And next terraform plan will not show any diff when it should say that it needs to fix the permissions.

If I rebuild my provider with the suggestion I did above (set granted instead of wanted), if I manually remove a permission from the database, I will have this plan:

$ terraform plan
[...]
Terraform will perform the following actions:

  # postgresql_grant.db_all must be replaced
-/+ resource "postgresql_grant" "db_all" {
      ~ id                = "test_test_db_database" -> (known after apply)
      ~ privileges        = [ # forces replacement
          - "CREATE",
          - "TEMPORARY",
          + "ALL",
        ]
        # (4 unchanged attributes hidden)
    }

And once applied, thanks to your fix, there will not be any diff in the next plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. that sounds reasonable 😅 i adjusted that with 54fc593 and provided an acceptance test for it. would be cool if you could check again @cyrilgdn

thanks in advance

}
return nil
}

Expand All @@ -289,7 +295,12 @@ WHERE grantee = $2
return fmt.Errorf("could not read privileges for schema %s: %w", dbName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
wanted := d.Get("privileges").(*schema.Set)
equal := arePrivilegesEqual(granted, wanted, d)
if !equal {
return d.Set("privileges", wanted)
}
return nil
}

Expand All @@ -309,7 +320,12 @@ WHERE grantee = $2
return fmt.Errorf("could not read privileges for foreign data wrapper %s: %w", fdwName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
wanted := d.Get("privileges").(*schema.Set)
equal := arePrivilegesEqual(granted, wanted, d)
if !equal {
return d.Set("privileges", wanted)
}
return nil
}

Expand All @@ -329,7 +345,12 @@ WHERE grantee = $2
return fmt.Errorf("could not read privileges for foreign server %s: %w", srvName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
wanted := d.Get("privileges").(*schema.Set)
equal := arePrivilegesEqual(granted, wanted, d)
if !equal {
return d.Set("privileges", wanted)
}
return nil
}

Expand Down Expand Up @@ -415,6 +436,9 @@ func readRolePrivileges(txn *sql.Tx, d *schema.ResourceData) error {
role := d.Get("role").(string)
objectType := d.Get("object_type").(string)
objects := d.Get("objects").(*schema.Set)
for _, entry := range objects.List() {
log.Printf("this is a entry: %v", entry)
}

roleOID, err := getRoleOID(txn, role)
if err != nil {
Expand Down Expand Up @@ -502,8 +526,8 @@ GROUP BY pg_class.relname
}

privilegesSet := pgArrayToSet(privileges)

if !privilegesSet.Equal(d.Get("privileges").(*schema.Set)) {
privilegesEqual := arePrivilegesEqual(privilegesSet, d.Get("privileges").(*schema.Set), d)
if !privilegesEqual {
// If any object doesn't have the same privileges as saved in the state,
// we return its privileges to force an update.
log.Printf(
Expand Down