Support BigQuery authorized routines #6680
Conversation
|
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @melinath, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 450 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccBigQueryDataset_bigqueryDatasetAuthorizedRoutineExample|TestAccBigQueryDataset_bigqueryDatasetAuthorizedDatasetExample|TestAccBigQueryDataset_bigqueryDatasetBasicExample|TestAccBigQueryDataset_access|TestAccBigQueryDataset_basic |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
|
@melinath |
There was a problem hiding this comment.
Thanks for checking in & apologies for the delayed review. It looks like one of the tests is currently failing with the following error:
provider_test.go:315: Step 1/2 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
stdout
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# google_bigquery_dataset.private will be updated in-place
~ resource "google_bigquery_dataset" "private" {
id = "projects/ci-project-id/datasets/privatefcsorz706l"
# (12 unchanged attributes hidden)
- access {
- routine {
- dataset_id = "publicfcsorz706l" -> null
- project_id = "ci-project-id" -> null
- routine_id = "sample_routine" -> null
}
}
- access {
- role = "OWNER" -> null
- user_by_email = "service-account@ci-project-id.iam.gserviceaccount.com" -> null
}
- access {
- role = "OWNER" -> null
- special_group = "projectOwners" -> null
}
- access {
- role = "READER" -> null
- special_group = "projectReaders" -> null
}
- access {
- role = "WRITER" -> null
- special_group = "projectWriters" -> null
}
+ access {
+ routine {
+ dataset_id = "publicfcsorz706l"
+ project_id = "ci-project-id"
+ routine_id = "sample_routine"
}
}
}
Plan: 0 to add, 1 to change, 0 to destroy.
This means there's a permadiff - the change isn't being persisted properly in the API. Looking into the debug logs, I see the following request / response cycle:
---[ REQUEST ]---------------------------------------
POST /bigquery/v2/projects/ci-project-id/datasets?alt=json HTTP/1.1
Host: bigquery.googleapis.com
User-Agent: Terraform/1.1.8 (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google-beta/acc
Content-Length: 233
Content-Type: application/json
Accept-Encoding: gzip
{
"access": [
{
"routine": {
"datasetId": "publicfcsorz706l",
"projectId": "ci-project-id",
"routineId": "sample_routine"
}
}
],
"datasetReference": {
"datasetId": "privatefcsorz706l"
},
"description": "This dataset is private",
"location": "US"
}
-----------------------------------------------------
2022/10/11 11:51:08 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 200 OK
Cache-Control: private
Content-Type: application/json; charset=UTF-8
Date: Tue, 11 Oct 2022 11:51:08 GMT
Etag: pn0YMy4Q9HkSyI6NrE2O0g==
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0
{
"kind": "bigquery#dataset",
"etag": "pn0YMy4Q9HkSyI6NrE2O0g==",
"id": "ci-project-id:privatefcsorz706l",
"selfLink": "https://bigquery.googleapis.com/bigquery/v2/projects/ci-project-id/datasets/privatefcsorz706l",
"datasetReference": {
"datasetId": "privatefcsorz706l",
"projectId": "ci-project-id"
},
"description": "This dataset is private",
"access": [
{
"role": "WRITER",
"specialGroup": "projectWriters"
},
{
"role": "OWNER",
"specialGroup": "projectOwners"
},
{
"role": "OWNER",
"userByEmail": "service-account@ci-project-id.iam.gserviceaccount.com"
},
{
"role": "READER",
"specialGroup": "projectReaders"
},
{
"routine": {
"projectId": "ci-project-id",
"datasetId": "publicfcsorz706l",
"routineId": "sample_routine"
}
}
],
"creationTime": "1665489068549",
"lastModifiedTime": "1665489068549",
"location": "US",
"type": "DEFAULT"
}
It looks like the API is adding a bunch of access entries that are not present in the request object.
This seems to correspond to hashicorp/terraform-provider-google#8370
I think that, as a workaround, you could specify a legacy role on the resource - I've added annotations on how I think you could do this below.
If the workaround doesn't work - one option would be to try to resolve the permadiff ticket in a separate PR first. Otherwise I'll have to think about what you could do.
mmv1/templates/terraform/examples/bigquery_dataset_authorized_routine.tf.erb
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…routine.tf.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
|
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, 452 insertions(+), 7 deletions(-)) |
|
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, 476 insertions(+), 7 deletions(-)) |
|
@melinath I added legacy roles to test case. |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccBigQueryDataset_bigqueryDatasetAuthorizedRoutineExample|TestAccBigQueryDataset_bigqueryDatasetWithMaxTimeTravelHoursExample |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
| description = "This dataset is private" | ||
| access { | ||
| role = "WRITER" | ||
| specialGroup = "projectWriters" |
There was a problem hiding this comment.
this would need to be special_group to match the Terraform field name - but I'd prefer not including these special_group blocks. If we just match what the API is returning we won't be able to tell whether it's acting on what we send it or it's ignoring what we sent but it just happens to match what's in the config right now.
There was a problem hiding this comment.
@melinath
Thank you for your comment.
Sorry for my misunderstanding about your advice.
I agree with your opinion.
I removed them.
On the other hands, I'm worried about the failed test.
|
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, 452 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccBigQueryDataset_bigqueryDatasetAuthorizedRoutineExample |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
| name: "bigquery_dataset_authorized_routine" | ||
| primary_resource_id: "dataset" | ||
| vars: | ||
| private: "private" |
There was a problem hiding this comment.
These need a hyphen so that they automatically get prefixed with tf-test (so that they can be easily cleaned up after the tests run, if something goes wrong.)
| private: "private" | |
| private: "private-dataset" | |
| public: "public-dataset" |
|
|
||
| resource "google_bigquery_routine" "public" { | ||
| dataset_id = google_bigquery_dataset.public.dataset_id | ||
| routine_id = "sample_routine" |
There was a problem hiding this comment.
Is this automatically deleted if the dataset is deleted? And/or is there any issue with quotas for this resource, or name conflicts if parallel test runs happen at the same time? Otherwise it might be worth making this use a var as well.
| skip_test: true # not importable | ||
| primary_resource_id: "access" | ||
| vars: | ||
| private: "private" |
| @@ -41,6 +41,14 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
| private: "private" | |||
| public: "public" | |||
| account_name: "bqowner" | |||
| - !ruby/object:Provider::Terraform::Examples | |||
| name: "bigquery_dataset_authorized_routine" | |||
| primary_resource_id: "dataset" | |||
There was a problem hiding this comment.
This needs to match the second part of a dataset resource's terraform id. For example, for:
resource "google_bigquery_dataset" "public" {
// args
}the resource address is google_bigquery_dataset.public, so primary_resource_id would need to be public.
The "primary resource" will be imported as part of the test. In this case, the important thing is to make sure that imports work correctly for the routine field (which is on the private dataset)
| primary_resource_id: "dataset" | |
| primary_resource_id: "private" |
| - !ruby/object:Provider::Terraform::Examples | ||
| name: "bigquery_dataset_access_authorized_routine" | ||
| skip_test: true # not importable | ||
| primary_resource_id: "access" |
There was a problem hiding this comment.
In this example, the routine is actually set on google_bigquery_dataset_access.authorized_routine, which is not the expected resource type, so you'll need to set that explicitly as well.
| primary_resource_id: "access" | |
| primary_resource_type: "google_bigquery_dataset_access" | |
| primary_resource_id: "authorized_routine" |
|
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, 452 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccBigQueryDataset_bigqueryDatasetAuthorizedRoutineExample |
|
Tests failed during RECORDING mode: Please fix these to complete your PR |
|
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, 452 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccBigQueryDataset_bigqueryDatasetAuthorizedRoutineExample |
There was a problem hiding this comment.
Congrats on getting the test passing! This is looking really great. I'm sorry for just noticing this, but it looks like the dataset access tests are not being generated because dataset access is not importable. This means that you'll need to add some handwritten tests for the routine field on the dataset_access resource.
Existing tests for that resource are at https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tests/resource_bigquery_dataset_access_test.go
The easiest way to do this might be to uncomment the "skip test" line, generate the test, then copy it over to the other file and remove the import lines (since it's not importable.)
|
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 ( 7 files changed, 534 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
| vcrTest(t, resource.TestCase{ | ||
| PreCheck: func() { testAccPreCheck(t) }, | ||
| Providers: testAccProviders, | ||
| CheckDestroy: testAccCheckBigQueryDatasetAccessDestroyProducer(t), |
There was a problem hiding this comment.
It looks like this is causing the tests to fail to compile:
google/resource_bigquery_dataset_access_test.go:126:17: undefined: testAccCheckBigQueryDatasetAccessDestroyProducer
It looks like the other tests skip having a CheckDestroy and handle this in another way.
| CheckDestroy: testAccCheckBigQueryDatasetAccessDestroyProducer(t), |
| Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.private_dataset", expected), | ||
| }, | ||
| { | ||
| Config: testAccBigQueryDatasetAccess_destroy(context["private_dataset"], "private_dataset"), |
There was a problem hiding this comment.
It looks like this is causing the tests to fail to compile:
google/resource_bigquery_dataset_access_test.go:133:50: cannot use context["private_dataset"] (map index expression of type interface{}) as type string in argument to testAccBigQueryDatasetAccess_destroy:
It looks like the simplest fix is to add a type assertion here.
| Config: testAccBigQueryDatasetAccess_destroy(context["private_dataset"], "private_dataset"), | |
| // Destroy step instead of CheckDestroy so we can check the access is removed without deleting the dataset | |
| Config: testAccBigQueryDatasetAccess_destroy(context["private_dataset"].(string), "private"), |
| @@ -100,6 +100,43 @@ func TestAccBigQueryDatasetAccess_authorizedDataset(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestAccBigQueryDatasetAccess_authorizedRoutine(t *testing.T) { | |||
| t.Parallel() | |||
There was a problem hiding this comment.
It looks like these can't run in VCR based on the surrounding tests.
| t.Parallel() | |
| // Multiple fine-grained resources | |
| skipIfVcr(t) | |
| t.Parallel() |
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testAccBigQueryDatasetAccess_authorizedRoutine(context), | ||
| Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.private_dataset", expected), |
There was a problem hiding this comment.
| Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.private_dataset", expected), | |
| Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.private", expected), |
There was a problem hiding this comment.
needs to match what's in the config
| }, | ||
| { | ||
| Config: testAccBigQueryDatasetAccess_destroy(context["private_dataset"], "private_dataset"), | ||
| Check: testAccCheckBigQueryDatasetAccessAbsent(t, "google_bigquery_dataset.private_dataset", expected), |
There was a problem hiding this comment.
| Check: testAccCheckBigQueryDatasetAccessAbsent(t, "google_bigquery_dataset.private_dataset", expected), | |
| Check: testAccCheckBigQueryDatasetAccessAbsent(t, "google_bigquery_dataset.private", expected), |
| } | ||
|
|
||
| expected := map[string]interface{}{ | ||
| "dataset": map[string]interface{}{ |
There was a problem hiding this comment.
Okay I think this plus the above changes should do this. Basically, the nesting inside dataset here and the targetTypes field below seem to be left over from a different test. For this test, the expected structure is:
expected := map[string]interface{}{
"routine": map[string]interface{}{
"projectId": getTestProjectFromEnv(),
"datasetId": context["public_dataset"],
"routineId": context["public_routine"],
},
}
|
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 ( 7 files changed, 533 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease |
|
Manually running tests out of caution / because one is skipIfVcr: |
mmv1/third_party/terraform/tests/resource_bigquery_dataset_access_test.go
Outdated
Show resolved
Hide resolved
…ess_test.go Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
|
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 ( 7 files changed, 533 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: All tests passed in REPLAYING mode |
|
@melinath Thank you for your long term support. 🥇 |
* BigQuery: Added support for authorized routine * BigQuery: Added examples for authorized routine * Update mmv1/products/bigquery/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/templates/terraform/examples/bigquery_dataset_authorized_routine.tf.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * add legacy iam access to test case * remove legacy roles * correct variable names and values * replace - to _ * [wip]added hand written test * fix corrupted test * Update mmv1/third_party/terraform/tests/resource_bigquery_dataset_access_test.go Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Add support for authorized routines on BigQuery.
Fixes hashicorp/terraform-provider-google#11501 .
I just tested to assign some routines as an authorized routines to some datasets using local built provider.
But I can not run acceptance tests.
If this PR is for Terraform, I acknowledge that I have:
make testandmake lintto ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)