Add new resource to support ALTER ROLE

[bmedlock-depop]

This change will add in a new resource to the provider, ALTER_ROLE.

The resource is designed to be used where you need to alter certain attributes on a role. For example, when setting attributes associated with PGAudit, ALTER ROLE testrole SET pgaudit.log to 'all'

Closes #210

[robinjhector]

Sorry to bump this, but is this good to merge and release @cyrilgdn ?

[ALX-TH]

same question to @cyrilgdn , any chance to have such feature ? thanks.

[hunkeelin]

@robinjhector the CI tests are failing.

[jenrik]

@bmedlock-depop The test are failing because the test setup run them both as a superuser role and a less privileged roles. This can be fixed by adding the following line to the top of the test that require superuser access: testSuperuserPreCheck(t)

https://github.com/depop/terraform-provider-postgresql/blob/PLAT-1484-add-alter-role-resource/postgresql/utils_test.go#L32

[bmedlock-depop]

@jenrik Thanks for the help! I should have time this week to fix this.

[bmedlock-depop]

@cyrilgdn Tests should now be passing.

Thanks for your patience, everyone!

[ALX-TH]

@cyrilgdn can you please take a look on this request ? ;)

[bmedlock-depop]

@cyrilgdn I've just fixed the conflicts on the branch. Everything should be good. Please could you take look?

[jbg]

This proposed resource is inconsistent with the declarative model of Terraform. A resource is a thing, not an operation. The resource is the role, and this resource already exists (postgresql_role). ALTER ROLE is something the provider runs when that resource is changed.

The right way to implement this would be to add configuration parameter support to postgresql_role. You would then import the role to the Terraform state, if it's not already there, and then add/modify the configuration parameter, which the provider would apply by running ALTER ROLE.

In the Terraform configuration, that would look something like:

resource "postgresql_role" "foo" {
  name = "foo"

  parameter {
    name = "pgaudit.log"
    value = "all"
  }
}

[rowanmoul]

This proposed resource is inconsistent with the declarative model of Terraform. A resource is a thing, not an operation. The resource is the role, and this resource already exists (postgresql_role). ALTER ROLE is something the provider runs when that resource is changed.

I completely agree, however, sometimes the role was created by a different terraform provider that does not provide the ability to make the necessary changes. See #234 You could of course argue that those other providers should be fixed to give more control over the role that is created, but they are at the mercy of the underlying cloud platforms, and those are unlikely to be responsive to changes in any sort of timely manner. An ALTER ROLE resource (or a flag on the postgresql_role resource that indicates the role already exists) would allow people to use native terraform resources to work around this, rather than using provisioners or manually run scripts.

[sscheible]

This proposed resource is inconsistent with the declarative model of Terraform. A resource is a thing, not an operation. The resource is the role, and this resource already exists (postgresql_role). ALTER ROLE is something the provider runs when that resource is changed.

I completely agree, however, sometimes the role was created by a different terraform provider that does not provide the ability to make the necessary changes. See #234 You could of course argue that those other providers should be fixed to give more control over the role that is created, but they are at the mercy of the underlying cloud platforms, and those are unlikely to be responsive to changes in any sort of timely manner. An ALTER ROLE resource (or a flag on the postgresql_role resource that indicates the role already exists) would allow people to use native terraform resources to work around this, rather than using provisioners or manually run scripts.

Having the same with google_sql_user resources which I need to ALTER. A more declarative resource approach could be to restrict the ALTER ROLE to configuration parameters and call the resource "postgresql_role_configuration_parameter", the thing to be created being the configuration parameter on the role. This essentially leads to #305

Edit in response to @jbg: google_sql_user is explicitly created, but falls in the same 'at mercy' category as mentioned by @rowanmoul

[jbg]

I completely agree, however, sometimes the role was created by a different terraform provider that does not provide the ability to make the necessary changes. See #234 You could of course argue that those other providers should be fixed to give more control over the role that is created, but they are at the mercy of the underlying cloud platforms, and those are unlikely to be responsive to changes in any sort of timely manner. An ALTER ROLE resource (or a flag on the postgresql_role resource that indicates the role already exists) would allow people to use native terraform resources to work around this, rather than using provisioners or manually run scripts.

Generally you'd deal with this by importing the implicitly-created resource after the other provider has created it, after which you can deal with it declaratively like normal. This has been improved recently with the addition of import blocks, though there is still work to do there.

[ktham]

This proposed resource is inconsistent with the declarative model of Terraform. A resource is a thing, not an operation. The resource is the role, and this resource already exists (postgresql_role).

I understand the concern and agree in principle, but I don't see an inconsistency with this proposed PR, perhaps the naming of the resource can be better. In fact, if you look at the parameters for CREATE ROLE, you will not find session config parameters there, it's a separate query to get these added (or removed) using a "SET <session_config_param> = <session_config_value>" statement with an ALTER ROLE query after the PG role has been created to get these session config params added for the role, so I think this can be reasonably managed independently as a separate Terraform resource.

This is very similar to how we have aws_security_group resources and at the same time, we have aws_security_group_rule. Or aws_s3_bucket and s3_bucket_ownership_controls (which used to be contained inside the aws_s3_bucket resource in versions prior to 4.x of the AWS provider, but are now separate Terraform resources).

@bmedlock-depop My suggestion would be to call this resource postgresql_role_configuration_parameter to make this clearer rather than the present name of postgresql_alter_role.

I think this PR is the simpler and more flexible solution relative to the other proposal of adding this in-line to the postgresql_role resource as this would handle the use-case of PG roles that are created and managed outside of our control (e.g. like those created for us by the cloud vendor such as AWS, Azure, etc...), and thus, managed by a Terraform resource of a separate provider. The suggestion to import it seems hacky and it's unclear how reliable this would be, to have two providers manage the same resource, I can see potentially this bringing in more problems than it attempts to solve. The solution here seems much simpler and doesn't conflict with Terraform's declarative model and so I would vote for this PR (after the resource rename to postgresql_role_configuration_parameter)

[bhoriuchi]

For special permissions like CREATEROLE, CREATEDB, etc the following

https://github.com/cyrilgdn/terraform-provider-postgresql/pull/211/files#diff-485fa37a4d8bc2c9c3bf2bb5991a782b2e2135653bf55684d098534742e85cecR170

doesnt work. Would it be possible to get some special case queries for these. i.e.

func createAlterRoleQuery(d *schema.ResourceData) string {
	alterRole, _ := d.Get("role_name").(string)
	alterParameterKey, _ := d.Get("parameter_key").(string)
	alterParameterValue, _ := d.Get("parameter_value").(string)

	query := fmt.Sprintf(
		"ALTER ROLE %s SET %s TO %s",
		pq.QuoteIdentifier(alterRole),
		pq.QuoteIdentifier(alterParameterKey),
		pq.QuoteIdentifier(alterParameterValue),
	)

	switch key := strings.ToUpper(alterParameterKey); key {
	case "SUPERUSER",
		"CREATEDB",
		"CREATEROLE",
		"INHERIT",
		"LOGIN",
		"REPLICATION",
		"BYPASSRLS":
		query = fmt.Sprintf("ALTER ROLE %s %s", pq.QuoteIdentifier(alterRole), key)
	}

	return query
}

func createResetAlterRoleQuery(d *schema.ResourceData) string {
	alterRole, _ := d.Get("role_name").(string)
	alterParameterKey, _ := d.Get("parameter_key").(string)

	query := fmt.Sprintf(
		"ALTER ROLE %s RESET %s",
		pq.QuoteIdentifier(alterRole),
		pq.QuoteIdentifier(alterParameterKey),
	)

	switch key := strings.ToUpper(alterParameterKey); key {
	case "SUPERUSER",
		"CREATEDB",
		"CREATEROLE",
		"INHERIT",
		"LOGIN",
		"REPLICATION",
		"BYPASSRLS":
		query = fmt.Sprintf("ALTER ROLE %s NO%s", pq.QuoteIdentifier(alterRole), key)
	}

	return query
}