-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add AlloyDB user support #8431
Add AlloyDB user support #8431
Conversation
Hello! I am a robot. It looks like you are a community contributor. @slevenick, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 787 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules|TestAccAlloydbUser_alloydbUserExample |
Rerun these tests in REPLAYING mode to catch issues
|
Added tests to increase test coverage for the missing fields, can we run the tests again? |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 827 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbUser_alloydbUserIamUserExample|TestAccAlloydbUser_alloydbUserExample|TestAccContainerAwsNodePool_BetaBasicHandWritten |
Rerun these tests in REPLAYING mode to catch issues
|
Anyway of getting the log where the test I tested them on my own account on which it passed.
|
I realised that I never ran the tests at the same time, by doing this I realised that the
|
} | ||
|
||
resource "google_compute_network" "<%= ctx[:primary_resource_id] %>" { | ||
name = "alloydb-cluster" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should change to a parameterized value so it gets a random suffix attached. Right now this is causing the failure because it collides with another network named the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slevenick I am skipping the generation of tests because this particular resource does not work well with the autogenerated tests.
If we destroy the cluster or instance during the CheckDestroy phase that will result in Internal 500 errors when trying to verify if an user still exists, therefore I need to check it while the parent resource still exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's.... frightening. What do we expect the user experience to be when they created a user within Terraform and then they destroyed it via another tool? They would hit the 500 and be confused about what is happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share either the error message in logs or a use case that triggers it? I can reach out to the internal team because returning a 500 on an attempted GET is pretty bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slevenick I did not realise the impact in certain scenario's on users and didn't report it, for future contributions I will keep this in mind.
There are a few scenario's where this can happens but usually boils down to not having an instance in it's cluster.
I tested the following 2 scenario's:
You create a cluster and try to add an user without creating an instance.
2023-07-26T20:57:40.102+0200 [DEBUG] provider.terraform-provider-google: ---[ REQUEST ]---------------------------------------
2023-07-26T20:57:40.102+0200 [DEBUG] provider.terraform-provider-google: POST /v1/projects/playground-x/locations/europe-west4/clusters/tf-test-alloydb-cluster/users?alt=json&userId=test HTTP/1.1
2023-07-26T20:57:40.102+0200 [DEBUG] provider.terraform-provider-google: Host: alloydb.googleapis.com
2023-07-26T20:57:40.102+0200 [DEBUG] provider.terraform-provider-google: User-Agent: Terraform/1.5.0 (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/dev
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: Content-Length: 67
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: Content-Type: application/json
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: Accept-Encoding: gzip
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google:
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: {
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: "databaseRoles": [
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: "alloydbiamuser"
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: ],
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: "userType": "ALLOYDB_IAM_USER"
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: }
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google:
2023-07-26T20:57:40.103+0200 [DEBUG] provider.terraform-provider-google: -----------------------------------------------------
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: 2023/07/26 20:57:40 [DEBUG] Google API Response Details:
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: ---[ RESPONSE ]--------------------------------------
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: HTTP/2.0 500 Internal Server Error
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Cache-Control: private
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Content-Type: application/json; charset=UTF-8
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Date: Wed, 26 Jul 2023 18:57:40 GMT
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Server: ESF
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Vary: Origin
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Vary: X-Origin
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: Vary: Referer
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: X-Content-Type-Options: nosniff
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: X-Frame-Options: SAMEORIGIN
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: X-Xss-Protection: 0
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google:
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: {
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: "error": {
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: "code": 500,
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: "message": "An internal error has occurred (4e2d52b2-e97b-466a-9d22-a1a2c9b499bc)",
2023-07-26T20:57:40.263+0200 [DEBUG] provider.terraform-provider-google: "status": "INTERNAL"
2023-07-26T20:57:40.264+0200 [DEBUG] provider.terraform-provider-google: }
2023-07-26T20:57:40.264+0200 [DEBUG] provider.terraform-provider-google: }
│ Error: Error creating User: timeout while waiting for state to become 'success' (timeout: 1m0s)
│
│ with google_alloydb_user.default,
│ on main.tf line 61, in resource "google_alloydb_user" "user":
│ 34: resource "google_alloydb_user" "user" {
You create a cluster, instance & user through Terraform and we delete the instance and try to pull the state of the user resource
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google: ---[ REQUEST ]---------------------------------------
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google: GET /v1/projects/playground-x/locations/europe-west4/clusters/tf-test-alloydb-cluster/users/test?alt=json HTTP/1.1
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google: Host: alloydb.googleapis.com
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google: User-Agent: Terraform/1.5.0 (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/dev
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google: Content-Type: application/json
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google: Accept-Encoding: gzip
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google:
2023-07-26T20:23:52.799+0200 [DEBUG] provider.terraform-provider-google:
2023-07-26T20:23:52.800+0200 [DEBUG] provider.terraform-provider-google: -----------------------------------------------------
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: 2023/07/26 20:23:52 [DEBUG] Google API Response Details:
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: ---[ RESPONSE ]--------------------------------------
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: HTTP/2.0 500 Internal Server Error
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Cache-Control: private
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Content-Type: application/json; charset=UTF-8
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Date: Wed, 26 Jul 2023 18:23:52 GMT
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Server: ESF
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Vary: Origin
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Vary: X-Origin
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: Vary: Referer
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: X-Content-Type-Options: nosniff
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: X-Frame-Options: SAMEORIGIN
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: X-Xss-Protection: 0
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google:
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: {
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: "error": {
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: "code": 500,
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: "message": "An internal error has occurred (044b08c2-93bd-4981-a397-05c5c1b32f2b)",
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: "status": "INTERNAL"
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: }
2023-07-26T20:23:52.837+0200 [DEBUG] provider.terraform-provider-google: }
│ Error: Error when reading or editing AlloydbUser "projects/playground-x/locations/europe-west4/clusters/tf-test-alloydb-cluster/users/test": googleapi: Error 500: An internal error has occurred (570d762a-8316-4854-b15c-a40116fb4e97)
│
│ with google_alloydb_user.user,
│ on main.tf line 61, in resource "google_alloydb_user" "user":
│ 61: resource "google_alloydb_user" "user" {
While I was at it I tested what happens with an invalid cluster name and it got rejected as a 404.
Given these results I am unsure if this should be merged at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GauravJain21 please take a look at this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error is: global/networks/alloydb-cluster' already exists, alreadyExists
Please change the name to be parameterized so it gets randomized
This was changed in the "handwritten" tests and should work. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 827 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccAlloydbUser_alloydbUserExample|TestAccAlloydbUser_alloydbUserIamUserExample |
Rerun these tests in REPLAYING mode to catch issues
|
@GauravJain21 can you take a look at the error message here? This looks to be a bug on the service side, we shouldn't be getting a 500 response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some changes before merging, but the main questions come around the stability of this API for integration, especially around the 500 error returned in some cases.
At this point we should wait for @GauravJain21 to review
description: | | ||
The ID of the alloydb user. | ||
- !ruby/object:Api::Type::ResourceRef | ||
name: 'cluster' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it preferable to have this set up as a single parent resource rather than a series of project/location/cluster?
Generally we have each broken out into an individual field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would mean that the user has to set it's location and I'd argue this resource isn't a regional object on it's own but rather part of a regional object.
As the cluster dictates what region the user resides in I wouldn't give the user the option to specify location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. On the other hand existing resources like google_sql_user
work this way, where the project and instance are both specified independently: https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_user#project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest difference here is that google_sql_user
doesn't rely on a region in his end-point nor is it a field that can be set.
It is implied that it lives in the region where the instance resides in.
POST https://sqladmin.googleapis.com/v1/projects/{project}/instances/{instance}/users
I am curious what others believe the right approach is here.
} | ||
|
||
resource "google_compute_network" "<%= ctx[:primary_resource_id] %>" { | ||
name = "alloydb-cluster" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GauravJain21 please take a look at this
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 887 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccAlloydbUser_alloydbUserIamUser|TestAccAlloydbUser_alloydbUser |
Rerun these tests in REPLAYING mode to catch issues
|
Anyway we can re-run the tests? |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 945 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccIapWebTypeAppEngineIamBindingGenerated|TestAccAlloydbUser_alloydbUser|TestAccAlloydbUser_alloydbUserIamUser |
Rerun these tests in REPLAYING mode to catch issues
|
Tests look good, I'm going to hold off on more review until @GauravJain21 has a chance to look at this next week. |
Hi @GauravJain21, did you have an opportunity yet to take a look at this? |
Hey Daniel,
I am currently working on something else. I intend to review this next week
or the week after.
Best,
Gaurav Jain
…On Thu, Aug 10, 2023 at 5:37 PM Daniel Rieske ***@***.***> wrote:
Hi @GauravJain21 <https://github.com/GauravJain21>, did you have an
opportunity yet to take a look at this?
—
Reply to this email directly, view it on GitHub
<#8431 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACTUA3NRCNBFM5JFUY2TOELXUTFIPANCNFSM6AAAAAA2USJU7U>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi @GauravJain21, anything I can do to help at this point? |
Hey Daniel, |
@GauravJain21 Thank you for your response and sad to hear that major features created by the external community won't be accepted anymore. Will there be an official statement on what type of contributions will or will not be considered? I truly enjoy working on this product and have thoroughly enjoyed contributing to it. But if certain contributions won't be accepted anymore then I'd like to know to what extent so I won’t spend time on contributions that might not get accepted. Thanks a lot! |
@GauravJain21 any update on this? |
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)
Closes: hashicorp/terraform-provider-google#14272