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.

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

Conversation

talbx
Copy link
Contributor

@talbx talbx commented Aug 27, 2023

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

@talbx
Copy link
Contributor Author

talbx commented Nov 15, 2023

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

Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
postgresql/resource_postgresql_grant.go Outdated Show resolved Hide resolved
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

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

postgresql/resource_postgresql_grant.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_grant.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_grant.go Outdated Show resolved Hide resolved
@talbx talbx requested a review from cyrilgdn February 26, 2024 10:13
@talbx
Copy link
Contributor Author

talbx commented Feb 26, 2024

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.

Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

cf my answer here: #339 (comment)

@talbx
Copy link
Contributor Author

talbx commented Mar 5, 2024

might relate to #303

@talbx talbx changed the title feat: enhanced privilege equality check fix: ALL implicit privileges equality check Mar 5, 2024
@talbx talbx requested a review from cyrilgdn March 6, 2024 15:44
@dzavalkin-scayle
Copy link

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
Copy link
Contributor Author

talbx commented Mar 20, 2024

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
Copy link

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
Copy link
Contributor Author

talbx commented Mar 20, 2024

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
Copy link

@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
Copy link
Contributor Author

talbx commented Mar 26, 2024

@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
Copy link
Contributor Author

talbx commented Mar 26, 2024

@cyrilgdn can you take another look 👀

@igor-nikiforov
Copy link

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

@talbx
Copy link
Contributor Author

talbx commented Jul 9, 2024

hey @cyrilgdn, are you alive 😅 ?

@cyrilgdn cyrilgdn force-pushed the main branch 2 times, most recently from f2c2e47 to dea1401 Compare September 8, 2024 17:06
@talbx
Copy link
Contributor Author

talbx commented Oct 2, 2024

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

Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

This PR already celebrated its first birthday 🎂

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

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 Outdated Show resolved Hide resolved
@@ -45,3 +46,78 @@ func TestQuoteTableName(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 👍


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

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

func TestAccPostgresqlImplicitGrants(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.

💪

@talbx talbx requested a review from cyrilgdn October 23, 2024 19:13
Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

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

@cyrilgdn cyrilgdn merged commit fc5c40d into cyrilgdn:main Oct 24, 2024
7 checks passed
@alamin-floenergy
Copy link

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

@cyrilgdn
Copy link
Owner

cyrilgdn commented Dec 5, 2024

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

@rhyek
Copy link

rhyek commented Dec 24, 2024

FWIW, I'm using 1.25.0 and still seeing the issue with this config:

resource "random_password" "web_api_db_password" {
  length  = 32
  special = false
}

resource "postgresql_role" "web_api" {
  name     = "web_api"
  provider = postgresql.main
  login    = true
  password = random_password.web_api_db_password.result

  create_database = false
  create_role     = false
  superuser       = false
}

resource "postgresql_grant" "web_api_public_schema" {
  provider    = postgresql.main
  database    = aws_db_instance.postgresql.db_name
  role        = postgresql_role.web_api.name
  schema      = "public"
  object_type = "schema"
  privileges  = ["USAGE"]
}

resource "postgresql_grant" "web_api_public_tables" {
  provider    = postgresql.main
  database    = aws_db_instance.postgresql.db_name
  role        = postgresql_role.web_api.name
  schema      = "public"
  object_type = "table"
  privileges  = ["ALL"]
}

resource "postgresql_grant" "web_api_public_sequences" {
  provider    = postgresql.main
  database    = aws_db_instance.postgresql.db_name
  role        = postgresql_role.web_api.name
  schema      = "public"
  object_type = "sequence"
  privileges  = ["ALL"]
}

resource "postgresql_default_privileges" "web_api_tables_default" {
  provider    = postgresql.main
  database    = aws_db_instance.postgresql.db_name
  role        = postgresql_role.web_api.name
  schema      = "public"
  owner       = aws_db_instance.postgresql.username
  object_type = "table"
  privileges  = ["ALL"]
}

resource "postgresql_default_privileges" "web_api_sequences_default" {
  provider    = postgresql.main
  database    = aws_db_instance.postgresql.db_name
  role        = postgresql_role.web_api.name
  schema      = "public"
  owner       = aws_db_instance.postgresql.username
  object_type = "sequence"
  privileges  = ["ALL"]
}
Terraform will perform the following actions:

  # module.rds.postgresql_default_privileges.web_api_tables_default will be updated in-place
  ~ resource "postgresql_default_privileges" "web_api_tables_default" {
        id                = "web_api_c_prod_public_postgres_table"
      ~ privileges        = [
          - "DELETE",
          - "INSERT",
          - "MAINTAIN",
          - "REFERENCES",
          - "SELECT",
          - "TRIGGER",
          - "TRUNCATE",
          - "UPDATE",
          + "ALL",
        ]
        # (6 unchanged attributes hidden)
    }

  # module.rds.postgresql_grant.web_api_public_tables will be updated in-place
  ~ resource "postgresql_grant" "web_api_public_tables" {
        id                = "web_api_c_prod_public_table"
      ~ privileges        = [
          - "DELETE",
          - "INSERT",
          - "MAINTAIN",
          - "REFERENCES",
          - "SELECT",
          - "TRIGGER",
          - "TRUNCATE",
          - "UPDATE",
          + "ALL",
        ]
        # (5 unchanged attributes hidden)
    }

Plan: 0 to add, 2 to change, 0 to destroy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ALL privileges for postgresql_grant
6 participants