Skip to content
This repository has been archived by the owner on Nov 14, 2020. It is now read-only.

Implement grants on database #123

Merged
merged 2 commits into from
Apr 12, 2020
Merged

Implement grants on database #123

merged 2 commits into from
Apr 12, 2020

Conversation

jeromepin
Copy link
Contributor

I'm following @tgermain's comment (we work together) regarding the proposal to handle GRANT on databases (in addition to sequences and tables).

I took the liberty to move the query string creation (for both grant and revoke queries) into two dedicated functions and write unit test for them (although I'm a test newbie). Feel free to discard that if you find it irrelevant.

@ghost ghost added the size/L label Mar 9, 2020
Copy link
Contributor

@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 a lot for your work 👍
Nice to have created the dedicated function for the queries.

Could you quickly add a test like TestAccPostgresqlGrant for database.

You can make it simple with just one privilege and create a function which check that the role has the privilege (like testCheckTablesPrivileges but simpler, like just checking that the role can create a table if you give him this right)

postgresql/resource_postgresql_grant_test.go Outdated Show resolved Hide resolved
"role": roleName,
}),
privileges: []string{"SELECT"},
expected: fmt.Sprintf("GRANT SELECT ON ALL TABLES IN SCHEMA %s TO %s", pq.QuoteIdentifier(databaseName), pq.QuoteIdentifier(roleName)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have even set the generated string as expected value, but as you prefer.

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 mean not using fmt.Sprintf and pq.QuoteIdentifier, and hardcoding the string ?

postgresql/resource_postgresql_grant_test.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_grant_test.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_grant_test.go Show resolved Hide resolved
postgresql/helpers.go Outdated Show resolved Hide resolved
@joaosa
Copy link

joaosa commented Mar 13, 2020

I was just looking for this. Great work :)!

@jeromepin
Copy link
Contributor Author

@cyrilgdn I tried to add an acceptance test, however I'm even less fluent with that kind of tests than with Golang. Take a deep breath before reading :)

@jeromepin jeromepin requested a review from cyrilgdn March 17, 2020 11:14
@ghost ghost added size/XL and removed size/L labels Apr 4, 2020
Copy link
Contributor

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

@jeromepin Sorry for the response delay.

Your acceptance test was actually not testing anything 😄
(and should have failed but you didn't check the error of the query you executed)

As I understand that these tests are really not easy to understand and to dive in, I allowed myself to write it for you and to push on your branch.
Basically I check that if we add CREATE privilege for a role on a database, this role will be able to create a schema in this database (and will not be able to do it if this privilege has not been set).

When writing this test (and testing manually too), I find out a couple a problems that I fixed too.

Mainly:

  • the readRolePrivileges was not working for database, it needs a different query
  • schema parameter needs to be optional now as it doesn't make sense for database.

Would you mind reviewing my code (and if someone else sees this message and want to review, feel free :) ), so we can approve and merge ?

Thank you!

@jeromepin
Copy link
Contributor Author

Your acceptance test was actually not testing anything 😄
(and should have failed but you didn't check the error of the query you executed)

Oh... I knew testing wasn't my strong suit, but I didn't knew I was that bad 😄 🤦‍♂ Sorry for that !

As I understand that these tests are really not easy to understand and to dive in, I allowed myself to write it for you and to push on your branch.
Basically I check that if we add CREATE privilege for a role on a database, this role will be able to create a schema in this database (and will not be able to do it if this privilege has not been set).

When writing this test (and testing manually too), I find out a couple a problems that I fixed too.

Mainly:

  • the readRolePrivileges was not working for database, it needs a different query
  • schema parameter needs to be optional now as it doesn't make sense for database.

Thank you very much for taking the time ! 👍

Would you mind reviewing my code (and if someone else sees this message and want to review, feel free :) ), so we can approve and merge ?

No problem, I'll review it. But since I wasn't able to write a proper acceptance test, I'm not sure I'm the most qualified to review your work 😂

@jeromepin
Copy link
Contributor Author

@cyrilgdn Looks good to me ! 👍

@jeromepin jeromepin requested a review from cyrilgdn April 9, 2020 08:00
@cyrilgdn cyrilgdn merged commit 3e2ad5f into hashicorp:master Apr 12, 2020
@cyrilgdn
Copy link
Contributor

I'll let you known here when it's released.

@jeromepin jeromepin deleted the implement_grants_for_database branch April 12, 2020 14:16
@stefan2718
Copy link

stefan2718 commented May 13, 2020

@cyrilgdn Any ETA on the release date for this? Not being able to allow a user to connect to a database is a huge hole in functionality for this provider, and I would really love for this to be released. Thanks!

Edit:
For those looking for a workaround until release, this is what worked for me:

# Install latest plugin from master
go get -u github.com/terraform-providers/terraform-provider-postgresql

# Add as "custom" plugin with obviously fake version
mkdir -p ~/.terraform.d/plugins
ln -s $GOPATH/bin/terraform-provider-postgresql ~/.terraform.d/plugins/terraform-provider-postgresql_v999.0.0

Then specify the fake version in your provider block

provider "postgresql" {
  version = "999.0.0
  ...
}

Reinitializing with terraform init should then pick up this custom version.

@cyrilgdn
Copy link
Contributor

@stefan2718 Sorry for the delay,

As I explained here #138 , v1.6.0 will be released during the week (next Monday at the latest).

@cyrilgdn
Copy link
Contributor

Hi,

This feature has been released in v1.6.0 last Friday.

@cparmar
Copy link

cparmar commented Jun 30, 2020

I've created this merge request to update the provider documentation to reflect the changes made in this MR.

#151

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

Successfully merging this pull request may close these issues.

5 participants