fix: ALL implicit privileges equality check #339

talbx · 2023-08-27T14:42:30Z

When using resources postgresql_default_privileges or postgresql_grant you have the option to define a fine grained set of privileges (for example only CONNECT for object_type database) or all at once, either by setting all privileges (for database: ["CREATE", "CONNECT", "TEMPORARY"] or simply ["ALL"].

The latter one is quite handy, since you do not have to find out which privileges are part of it. When using ALL, the provider will grant the privileges by using the set of the actual grants, so in this scenario by granting "CREATE", "CONNECT", "TEMPORARY" and not ALL. Probably because ALL is not known to postgres. 🤷🏼‍♂️

Anyways, when executing a terraform plan the next time after the apply for ALL, the diff will find out, that not ALL is set as privilege, but "CREATE", "CONNECT", "TEMPORARY". Since these privileges are the same or have the same semantic to it, there shouldn't be really a update each time you use terraform, because the state on the DB is totally fine and according to the resource.

To prevent this unnecessary modification, I implemented some more detailed equality check for all object_types except column.

I tested the changes locally with a dockerized postgres and also wrote some unit tests for it.

Would love to hear some feedback about it, especially since it's my first time working on a terraform provider..

Cheers

fix #32

…mn aswell as for default privilege resource

talbx · 2023-11-15T12:50:18Z

@cyrilgdn any chance you can take a look at that?

cyrilgdn

Hi @talbx,

Thanks for opening this PR and many apologies for the response delay, I didn't have the time to work on this provider.

I like the idea 👍 , it's a simple solution to fix this very old bug. I still need to test what you did though, cf my comment about wanted / granted when you set the value, I'm not sure it works like that.

Thanks for the unittest on the new function 👍
It would be nice to also have acceptance test about that on postgresql_grant resource, there's already many tests in postgresql_grant_test.go
You can have a look at this one for example: https://github.com/cyrilgdn/terraform-provider-postgresql/blob/master/postgresql/resource_postgresql_grant_test.go#L1082-L1140

but let me know if you need help or if you don't have time, I can have a look.

postgresql/helpers.go

postgresql/resource_postgresql_grant.go

cyrilgdn · 2024-02-24T18:43:01Z

postgresql/resource_postgresql_grant.go

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


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)

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

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.

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.

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

postgresql/resource_postgresql_grant.go

…egesEqual fn, removed former logging bits, adapted tests

talbx · 2024-02-26T10:16:21Z

Thanks for opening this PR and many apologies for the response delay, I didn't have the time to work on this provider.

no worries, thanks for checking in again :)

Thanks for the unittest on the new function 👍 It would be nice to also have acceptance test about that on postgresql_grant resource, there's already many tests in postgresql_grant_test.go You can have a look at this one for example: https://github.com/cyrilgdn/terraform-provider-postgresql/blob/master/postgresql/resource_postgresql_grant_test.go#L1082-L1140

I will give it a shot.

cyrilgdn

cf my answer here: #339 (comment)

…ptance test

…ants acceptance test

talbx · 2024-03-05T21:07:55Z

might relate to #303

dzavalkin-scayle · 2024-03-19T11:41:30Z

Any chance this PR will be merged soon @cyrilgdn @talbx 🙏 ? We are tired seeing perpetual diff in all places where we use postgresql_grant resource...

talbx · 2024-03-20T13:37:10Z

Any chance this PR will be merged soon @cyrilgdn @talbx 🙏 ? We are tired seeing perpetual diff in all places where we use postgresql_grant resource...

I am still waiting for further feedback by @cyrilgdn; although i am not sure it will resolve your problems, @dzavalkin-scayle since you haven't specified them in detail and also, this PR solely focusses on the implicit "ALL" diff. I haven't tested if this PR fixes other cases aswell.

dzavalkin-scayle · 2024-03-20T13:46:35Z

i am not sure it will resolve your problems, @dzavalkin-scayle since you haven't specified them in detail and also, this PR solely focusses on the implicit "ALL" diff. I haven't tested, if this PR fixes other cases aswell.

Hmm I was under impression that it will fix diff issue entirely, no matter what are the privileges...
Without this fix we need to add privileges to ignore_changes which isn't nice at all and raises a question if we should look for another postgresql provider then 😞

talbx · 2024-03-20T18:28:11Z

Hmm I was under impression that it will fix diff issue entirely, no matter what are the privileges... Without this fix we need to add privileges to ignore_changes which isn't nice at all and raises a question if we should look for another postgresql provider then 😞

@dzavalkin-scayle can you give me a Terraform snippet to reproduce your problem? i can recheck if its included

dzavalkin-scayle · 2024-03-20T19:58:42Z

@talbx Thanks a lot! Here is the code we have:

locals {
  default_privileges = merge([
    for owner, users in var.rds_postgres_user_default_privileges :
    {
      for user in users : "${owner}-${user}" => {
        owner = owner
        user  = user
      }
    }
  ]...)
}

resource "random_password" "user_password" {
  for_each = { for k, v in var.rds_postgres_users : k => v if !lookup(v, "allow_iam_role_access", false) }

  length = 42

  special = true
  numeric = true
  lower   = true
  upper   = true

  override_special = "!()-_=[]{}"
  min_numeric      = 3
  min_lower        = 3
  min_upper        = 3
}

resource "postgresql_role" "postgres_users" {
  for_each = var.rds_postgres_users

  name     = each.key
  login    = true
  password = lookup(each.value, "allow_iam_role_access", false) ? "" : random_password.user_password[each.key].result
  roles    = lookup(each.value, "allow_iam_role_access", false) ? ["rds_iam"] : []
}

resource "postgresql_default_privileges" "postgresql_user_default_table_privileges" {
  for_each = {
    for key, value in local.default_privileges :
    key => value
    if lookup(var.rds_postgres_users[value.user], "table_privileges", "") != ""
  }

  database    = var.rds_database_name
  object_type = "table"
  owner       = each.value.owner
  privileges  = split(",", var.rds_postgres_users[each.value.user].table_privileges)
  role        = each.value.user
  schema      = "public"

  depends_on = [
    postgresql_role.postgres_users
  ]
}

resource "postgresql_default_privileges" "postgresql_user_default_sequence_privileges" {
  for_each = {
    for key, value in local.default_privileges :
    key => value
    if lookup(var.rds_postgres_users[value.user], "sequence_privileges", "") != ""
  }

  database    = var.rds_database_name
  object_type = "sequence"
  owner       = each.value.owner
  privileges  = split(",", var.rds_postgres_users[each.value.user].sequence_privileges)
  role        = each.value.user
  schema      = "public"

  depends_on = [
    postgresql_role.postgres_users
  ]
}

resource "postgresql_default_privileges" "postgresql_user_default_routine_privileges" {
  for_each = {
    for key, value in local.default_privileges :
    key => value
    if lookup(var.rds_postgres_users[value.user], "routine_privileges", "") != ""
  }

  database    = var.rds_database_name
  object_type = "function"
  owner       = each.value.owner
  privileges  = split(",", var.rds_postgres_users[each.value.user].routine_privileges)
  role        = each.value.user
  schema      = "public"

  depends_on = [
    postgresql_role.postgres_users
  ]
}

resource "postgresql_grant" "postgresql_user_table_privileges" {
  for_each = {
    for username, user_settings in var.rds_postgres_users :
    username => user_settings
    if lookup(user_settings, "table_privileges", "") != ""
  }

  database          = var.rds_database_name
  object_type       = "table"
  privileges        = split(",", each.value.table_privileges)
  role              = each.key
  schema            = "public"
  with_grant_option = lookup(each.value, "grant", false)
  depends_on = [
    postgresql_role.postgres_users
  ]
}

resource "postgresql_grant" "postgresql_user_database_privileges" {
  for_each = {
    for username, user_settings in var.rds_postgres_users :
    username => user_settings
    if lookup(user_settings, "database_privileges", "") != ""
  }

  database          = var.rds_database_name
  object_type       = "database"
  privileges        = split(",", each.value.database_privileges)
  role              = each.key
  with_grant_option = lookup(each.value, "grant", false)
  depends_on = [
    postgresql_role.postgres_users
  ]
}

resource "postgresql_grant" "postgresql_user_routine_privileges" {
  for_each = {
    for username, user_settings in var.rds_postgres_users :
    username => user_settings
    if lookup(user_settings, "routine_privileges", "") != ""
  }

  database          = var.rds_database_name
  role              = each.key
  object_type       = "routine"
  schema            = "public"
  privileges        = split(",", each.value["routine_privileges"])
  with_grant_option = lookup(each.value, "grant", false)
  depends_on = [
    postgresql_role.postgres_users
  ]
}

resource "postgresql_grant" "postgresql_user_schema_privileges" {
  for_each = {
    for username, user_settings in var.rds_postgres_users :
    username => user_settings
    if lookup(user_settings, "schema_privileges", "") != ""
  }

  database          = var.rds_database_name
  object_type       = "schema"
  privileges        = split(",", each.value["schema_privileges"])
  role              = each.key
  schema            = "public"
  with_grant_option = lookup(each.value, "grant", false)
  depends_on = [
    postgresql_role.postgres_users
  ]
}

resource "postgresql_grant" "postgresql_user_sequence_privileges" {
  for_each = {
    for username, user_settings in var.rds_postgres_users :
    username => user_settings
    if lookup(user_settings, "sequence_privileges", "") != ""
  }

  database          = var.rds_database_name
  role              = each.key
  object_type       = "sequence"
  schema            = "public"
  privileges        = split(",", each.value["sequence_privileges"])
  with_grant_option = lookup(each.value, "grant", false)
  depends_on = [
    postgresql_role.postgres_users
  ]
}

and values for input variables are (passed from Terragrunt):

  rds_postgres_users = {
    app_schema = {
      database_privileges = "CONNECT"
      grant               = true
      schema_privileges   = "CREATE,USAGE"
      routine_privileges  = "EXECUTE"
      sequence_privileges = "SELECT,UPDATE"
      table_privileges    = "SELECT,INSERT,UPDATE,DELETE,TRUNCATE,REFERENCES,TRIGGER"
    },
    app_unrestricted = {
      database_privileges = "CONNECT"
      routine_privileges  = "EXECUTE"
      schema_privileges   = "USAGE"
      sequence_privileges = "SELECT,UPDATE"
      table_privileges    = "SELECT,INSERT,UPDATE,DELETE,TRIGGER"
    },
    readonly_user = {
      allow_iam_role_access = true
      schema_privileges     = "USAGE"
      sequence_privileges   = "SELECT"
      table_privileges      = "SELECT"
    },
    rw_user = {
      allow_iam_role_access = true
      database_privileges   = "CONNECT"
      routine_privileges    = "EXECUTE"
      schema_privileges     = "USAGE"
      sequence_privileges   = "SELECT"
      table_privileges      = "SELECT,UPDATE,INSERT,DELETE,TRIGGER"
    }
  }
  rds_postgres_user_default_privileges = {
    app_schema = [
      "app_unrestricted",
      "readonly_user",
      "rw_user",
    ],
  }

Please let me know if you can use this code or you want me to simplify it somehow (though in this case we won't test our exact use cases).

talbx · 2024-03-26T09:16:17Z

@dzavalkin-scayle It would be nice if you could simplify that setup to a minimum so I can test it more easily. Would also appreciate it if you could make it self-contained so I can just copy paste and run. Thanks in advance!

talbx · 2024-03-26T09:16:55Z

@cyrilgdn can you take another look 👀

igor-nikiforov · 2024-05-02T05:02:10Z

@cyrilgdn just a friendly reminder about this very import PR.

talbx · 2024-07-09T11:24:21Z

hey @cyrilgdn, are you alive 😅 ?

talbx · 2024-10-02T12:42:43Z

friendly reminder to take a look again @cyrilgdn.
This PR already celebrated its first birthday 🎂

cyrilgdn

This PR already celebrated its first birthday 🎂

At 1yo I was still peeing on myself, this is quite young actually

Joke apart sorry for this huuuge delay again, with my work and personal life I have not much time for this project and each review takes hours 😅

But I still try to come back from time to time to create new releases with community PRs.

Two small comment otherwise it seems good to me 👍
I promise to merge quickly after your fix

hey @cyrilgdn, are you alive 😅 ?

unless I die meanwhile 🤷‍♂️
I'd be careful with this kind of comment though, you don't know who is behind a nickname and what happen in their personal life 😉

postgresql/helpers.go

cyrilgdn · 2024-10-23T15:10:36Z

postgresql/helpers_test.go

@@ -45,3 +46,78 @@ func TestQuoteTableName(t *testing.T) {
 		})
 	}
 }
+
+func TestArePrivilegesEqual(t *testing.T) {


cyrilgdn · 2024-10-23T15:15:03Z

postgresql/resource_postgresql_default_privileges.go

+
+	if !privilegesEqual {
+		d.Set("privileges", privilegesSet)
+		d.SetId(generateDefaultPrivilegesID(d))


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

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

cyrilgdn · 2024-10-23T15:31:14Z

postgresql/resource_postgresql_grant_test.go

@@ -1139,6 +1139,83 @@ resource "postgresql_grant" "test" {
 	})
 }

+func TestAccPostgresqlImplicitGrants(t *testing.T) {


Co-authored-by: Cyril Gaudin <[email protected]>

cyrilgdn

Thanks for your work 👍 , this will be released in v1.25 in the next days

alamin-floenergy · 2024-11-01T05:33:42Z

@cyrilgdn Can we please get a release soon with this changes?

cyrilgdn · 2024-12-05T22:11:08Z

@talbx @alamin-floenergy This has just been release in https://github.com/cyrilgdn/terraform-provider-postgresql/releases/tag/v1.25.0

talbx added 4 commits August 26, 2023 11:14

feat: improved privilege matching in case of ALL

09a9f30

feat: added implicit privilege check for all object types except colu…

edacad1

…mn aswell as for default privilege resource

test: provided unit test for arePriviligesEqual function

e3b92a0

Merge remote-tracking branch 'origin/master' into all-privileges

add228c

cyrilgdn requested changes Feb 24, 2024

View reviewed changes

talbx added 3 commits February 26, 2024 09:01

chore: merge master

f58fbb3

fix: apply review findings - removed redundant wanted param of privil…

8dfcf90

…egesEqual fn, removed former logging bits, adapted tests

refactor: inlined all equal checks

db54bfc

talbx requested a review from cyrilgdn February 26, 2024 10:13

cyrilgdn requested changes Mar 2, 2024

View reviewed changes

talbx added 3 commits March 5, 2024 21:42

fix: use the granted priviliges set from tf instead of db, added acce…

54fc593

…ptance test

cleanup: fixed function name typos & removed useless testing helper fn

c64e956

provided a more reasonable name for testCheck function in implicit gr…

a091370

…ants acceptance test

talbx mentioned this pull request Mar 5, 2024

Constant configuration drift on postgresql_grant for ALL TABLES #303

Open

resolve merge conflicts

30d4b45

talbx changed the title ~~feat: enhanced privilege equality check~~ fix: ALL implicit privileges equality check Mar 5, 2024

talbx requested a review from cyrilgdn March 6, 2024 15:44

cyrilgdn force-pushed the main branch from e1e781a to 25809b4 Compare September 8, 2024 16:44

cyrilgdn force-pushed the main branch 2 times, most recently from f2c2e47 to dea1401 Compare September 8, 2024 17:06

Merge branch 'cyrilgdn:main' into all-privileges

d288aaf

cyrilgdn reviewed Oct 23, 2024

View reviewed changes

cyrilgdn and others added 3 commits October 23, 2024 17:40

Merge branch 'main' into all-privileges

55875e9

simplified privilege set creation

0393157

Co-authored-by: Cyril Gaudin <[email protected]>

fix: always set id, no matter if privileges equal or not

c35441d

talbx requested a review from cyrilgdn October 23, 2024 19:13

fmt

4f65139

cyrilgdn approved these changes Oct 24, 2024

View reviewed changes

cyrilgdn merged commit fc5c40d into cyrilgdn:main Oct 24, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ALL implicit privileges equality check #339

fix: ALL implicit privileges equality check #339

talbx commented Aug 27, 2023 •

edited by cyrilgdn

Loading

talbx commented Nov 15, 2023

cyrilgdn left a comment

cyrilgdn Feb 24, 2024

talbx Feb 26, 2024 •

edited

Loading

talbx Feb 26, 2024

cyrilgdn Mar 2, 2024

talbx Mar 5, 2024

talbx commented Feb 26, 2024

cyrilgdn left a comment

talbx commented Mar 5, 2024

dzavalkin-scayle commented Mar 19, 2024

talbx commented Mar 20, 2024 •

edited

Loading

dzavalkin-scayle commented Mar 20, 2024

talbx commented Mar 20, 2024 •

edited

Loading

dzavalkin-scayle commented Mar 20, 2024

talbx commented Mar 26, 2024

talbx commented Mar 26, 2024

igor-nikiforov commented May 2, 2024

talbx commented Jul 9, 2024

talbx commented Oct 2, 2024

cyrilgdn left a comment

cyrilgdn Oct 23, 2024

cyrilgdn Oct 23, 2024

talbx Oct 23, 2024

cyrilgdn Oct 23, 2024

cyrilgdn left a comment

alamin-floenergy commented Nov 1, 2024

cyrilgdn commented Dec 5, 2024

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

fix: ALL implicit privileges equality check #339

fix: ALL implicit privileges equality check #339

Conversation

talbx commented Aug 27, 2023 • edited by cyrilgdn Loading

talbx commented Nov 15, 2023

cyrilgdn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talbx Feb 26, 2024 • edited Loading

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

talbx commented Feb 26, 2024

cyrilgdn left a comment

Choose a reason for hiding this comment

talbx commented Mar 5, 2024

dzavalkin-scayle commented Mar 19, 2024

talbx commented Mar 20, 2024 • edited Loading

dzavalkin-scayle commented Mar 20, 2024

talbx commented Mar 20, 2024 • edited Loading

dzavalkin-scayle commented Mar 20, 2024

talbx commented Mar 26, 2024

talbx commented Mar 26, 2024

igor-nikiforov commented May 2, 2024

talbx commented Jul 9, 2024

talbx commented Oct 2, 2024

cyrilgdn left a 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

cyrilgdn left a comment

Choose a reason for hiding this comment

alamin-floenergy commented Nov 1, 2024

cyrilgdn commented Dec 5, 2024

talbx commented Aug 27, 2023 •

edited by cyrilgdn

Loading

talbx Feb 26, 2024 •

edited

Loading

talbx commented Mar 20, 2024 •

edited

Loading

talbx commented Mar 20, 2024 •

edited

Loading