Skip to content

Commit ca38991

Browse files
wj-chenwj-chen
authored
Update google_bigquery_table schema change detection to take into account presence of row access policy (#15028)
1 parent d74edc1 commit ca38991

File tree

3 files changed

+232
-19
lines changed

3 files changed

+232
-19
lines changed

‎mmv1/third_party/terraform/services/bigquery/resource_bigquery_table.go.tmpl‎

Lines changed: 55 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,38 @@ func bigQueryTableNormalizePolicyTags(val interface{}) interface{} {
271271
return val
272272
}
273273

274+
func bigQueryTableHasRowAccessPolicy(config *transport_tpg.Config, project, datasetId, tableId string) (bool, error) {
275+
url := fmt.Sprintf("https://bigquery.googleapis.com/bigquery/v2/projects/%s/datasets/%s/tables/%s/rowAccessPolicies", project, datasetId, tableId)
276+
res, err := transport_tpg.SendRequest(transport_tpg.SendRequestOptions{
277+
Config: config,
278+
Method: "GET",
279+
RawURL: url,
280+
UserAgent: config.UserAgent,
281+
})
282+
283+
if err != nil {
284+
return false, err
285+
}
286+
287+
if policies, ok := res["rowAccessPolicies"]; ok {
288+
if policiesList, ok := policies.([]interface{}); ok && len(policiesList) > 0 {
289+
log.Printf("[INFO] Table has row access policies, schema change detected dropped columns and will force the table to recreate.")
290+
return true, nil
291+
}
292+
}
293+
294+
return false, nil
295+
}
296+
297+
func bigQueryTableHasRowAccessPolicyFunc(config *transport_tpg.Config, project, datasetId, tableId string) func() (bool, error) {
298+
return func() (bool, error) {
299+
return bigQueryTableHasRowAccessPolicy(config, project, datasetId, tableId)
300+
}
301+
}
302+
274303
// Compares two existing schema implementations and decides if
275304
// it is changeable.. pairs with a force new on not changeable
276-
func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, isExternalTable bool, topLevel bool) (bool, error) {
305+
func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, isExternalTable bool, topLevel bool, hasRowAccessPolicyFunc func() (bool, error)) (bool, error) {
277306
switch old.(type) {
278307
case []interface{}:
279308
arrayOld := old.([]interface{})
@@ -314,7 +343,7 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, isExternalTab
314343
continue
315344
}
316345

317-
isChangable, err := resourceBigQueryTableSchemaIsChangeable(mapOld[key], mapNew[key], isExternalTable, false)
346+
isChangable, err := resourceBigQueryTableSchemaIsChangeable(mapOld[key], mapNew[key], isExternalTable, false, hasRowAccessPolicyFunc)
318347
if err != nil || !isChangable {
319348
return false, err
320349
} else if isChangable && topLevel {
@@ -325,7 +354,18 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, isExternalTab
325354
// in-place column dropping alongside column additions is not allowed
326355
// as of now because user intention can be ambiguous (e.g. column renaming)
327356
newColumns := len(arrayNew) - sameNameColumns
328-
return (droppedColumns == 0) || (newColumns == 0), nil
357+
isSchemaChangeable := (droppedColumns == 0) || (newColumns == 0)
358+
if isSchemaChangeable && topLevel {
359+
hasRowAccessPolicy, err := hasRowAccessPolicyFunc()
360+
if err != nil {
361+
// Default behavior when we can't get row access policies data.
362+
return isSchemaChangeable, nil
363+
}
364+
if hasRowAccessPolicy {
365+
isSchemaChangeable = false
366+
}
367+
}
368+
return isSchemaChangeable, nil
329369
case map[string]interface{}:
330370
objectOld := old.(map[string]interface{})
331371
objectNew, ok := new.(map[string]interface{})
@@ -371,7 +411,7 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, isExternalTab
371411
return false, nil
372412
}
373413
case "fields":
374-
return resourceBigQueryTableSchemaIsChangeable(valOld, valNew, isExternalTable, false)
414+
return resourceBigQueryTableSchemaIsChangeable(valOld, valNew, isExternalTable, false, hasRowAccessPolicyFunc)
375415

376416
// other parameters: description, policyTags and
377417
// policyTags.names[] are changeable
@@ -388,7 +428,7 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}, isExternalTab
388428
}
389429
}
390430

391-
func resourceBigQueryTableSchemaCustomizeDiffFunc(d tpgresource.TerraformResourceDiff) error {
431+
func resourceBigQueryTableSchemaCustomizeDiffFunc(d tpgresource.TerraformResourceDiff, hasRowAccessPolicyFunc func() (bool, error)) error {
392432
if _, hasSchema := d.GetOk("schema"); hasSchema {
393433
oldSchema, newSchema := d.GetChange("schema")
394434
oldSchemaText := oldSchema.(string)
@@ -411,7 +451,7 @@ func resourceBigQueryTableSchemaCustomizeDiffFunc(d tpgresource.TerraformResourc
411451
log.Printf("[DEBUG] unable to unmarshal json customized diff - %v", err)
412452
}
413453
_, isExternalTable := d.GetOk("external_data_configuration")
414-
isChangeable, err := resourceBigQueryTableSchemaIsChangeable(old, new, isExternalTable, true)
454+
isChangeable, err := resourceBigQueryTableSchemaIsChangeable(old, new, isExternalTable, true, hasRowAccessPolicyFunc)
415455
if err != nil {
416456
return err
417457
}
@@ -426,7 +466,15 @@ func resourceBigQueryTableSchemaCustomizeDiffFunc(d tpgresource.TerraformResourc
426466
}
427467

428468
func resourceBigQueryTableSchemaCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error {
429-
return resourceBigQueryTableSchemaCustomizeDiffFunc(d)
469+
config := meta.(*transport_tpg.Config)
470+
project, err := tpgresource.GetProjectFromDiff(d, config)
471+
if err != nil {
472+
return err
473+
}
474+
datasetId := d.Get("dataset_id").(string)
475+
tableId := d.Get("table_id").(string)
476+
hasRowAccessPolicyFunc := bigQueryTableHasRowAccessPolicyFunc(config, project, datasetId, tableId)
477+
return resourceBigQueryTableSchemaCustomizeDiffFunc(d, hasRowAccessPolicyFunc)
430478
}
431479

432480
func validateBigQueryTableSchema(v interface{}, k string) (warnings []string, errs []error) {

‎mmv1/third_party/terraform/services/bigquery/resource_bigquery_table_internal_test.go.tmpl‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -411,13 +411,6 @@ func (testcase *testUnitBigQueryDataTableJSONChangeableTestCase) check(t *testin
411411
if err := json.Unmarshal([]byte(testcase.jsonNew), &new); err != nil {
412412
t.Fatalf("unable to unmarshal json - %v", err)
413413
}
414-
changeable, err := resourceBigQueryTableSchemaIsChangeable(old, new, testcase.isExternalTable, true)
415-
if err != nil {
416-
t.Errorf("%s failed unexpectedly: %s", testcase.name, err)
417-
}
418-
if changeable != testcase.changeable {
419-
t.Errorf("expected changeable result of %v but got %v for testcase %s", testcase.changeable, changeable, testcase.name)
420-
}
421414

422415
d := &tpgresource.ResourceDiffMock{
423416
Before: map[string]interface{}{},
@@ -432,7 +425,19 @@ func (testcase *testUnitBigQueryDataTableJSONChangeableTestCase) check(t *testin
432425
d.After["external_data_configuration"] = ""
433426
}
434427

435-
err = resourceBigQueryTableSchemaCustomizeDiffFunc(d)
428+
hasRowAccessPolicyFunc := func() (bool, error) {
429+
return false, nil
430+
}
431+
432+
changeable, err := resourceBigQueryTableSchemaIsChangeable(old, new, testcase.isExternalTable, true, hasRowAccessPolicyFunc)
433+
if err != nil {
434+
t.Errorf("%s failed unexpectedly: %s", testcase.name, err)
435+
}
436+
if changeable != testcase.changeable {
437+
t.Errorf("expected changeable result of %v but got %v for testcase %s", testcase.changeable, changeable, testcase.name)
438+
}
439+
440+
err = resourceBigQueryTableSchemaCustomizeDiffFunc(d, hasRowAccessPolicyFunc)
436441
if err != nil {
437442
t.Errorf("error on testcase %s - %v", testcase.name, err)
438443
}

‎mmv1/third_party/terraform/services/bigquery/resource_bigquery_table_test.go‎

Lines changed: 164 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,13 @@ package bigquery_test
22

33
import (
44
"fmt"
5-
"regexp"
6-
"strings"
7-
"testing"
8-
95
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
106
"github.com/hashicorp/terraform-plugin-testing/terraform"
117
"github.com/hashicorp/terraform-provider-google/google/acctest"
128
"github.com/hashicorp/terraform-provider-google/google/envvar"
9+
"regexp"
10+
"strings"
11+
"testing"
1312
)
1413

1514
func TestAccBigQueryTable_Basic(t *testing.T) {
@@ -1803,7 +1802,74 @@ func TestAccBigQueryTable_invalidSchemas(t *testing.T) {
18031802
})
18041803
}
18051804

1805+
func TestAccBigQueryTable_schemaColumnDropWithRowAccessPolicy(t *testing.T) {
1806+
t.Parallel()
1807+
1808+
context := map[string]interface{}{
1809+
"project_id": envvar.GetTestProjectFromEnv(),
1810+
"dataset_id": fmt.Sprintf("tf_test_dataset_%s", acctest.RandString(t, 10)),
1811+
"table_id": fmt.Sprintf("tf_test_table_%s", acctest.RandString(t, 10)),
1812+
"policy_id": fmt.Sprintf("tf_test_policy_%s", acctest.RandString(t, 10)),
1813+
}
1814+
1815+
var tableCreationTime string
1816+
1817+
acctest.VcrTest(t, resource.TestCase{
1818+
PreCheck: func() { acctest.AccTestPreCheck(t) },
1819+
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
1820+
CheckDestroy: testAccCheckBigQueryTableDestroyProducer(t),
1821+
Steps: []resource.TestStep{
1822+
{
1823+
Config: testAccBigQueryTableWithSchemaAndRowAccessPolicy(context),
1824+
Check: resource.ComposeTestCheckFunc(
1825+
// Store the creationTime of the original table
1826+
func(s *terraform.State) error {
1827+
rs, ok := s.RootModule().Resources["google_bigquery_table.test"]
1828+
if !ok {
1829+
return fmt.Errorf("Not found: google_bigquery_table.test")
1830+
}
1831+
tableCreationTime = rs.Primary.Attributes["creation_time"]
1832+
return nil
1833+
},
1834+
),
1835+
},
1836+
{
1837+
ResourceName: "google_bigquery_table.test",
1838+
ImportState: true,
1839+
ImportStateVerify: true,
1840+
ImportStateVerifyIgnore: []string{"deletion_protection", "ignore_auto_generated_schema", "generated_schema_columns"},
1841+
},
1842+
{
1843+
Config: testAccBigQueryTableWithSchemaColumnDroppedAndRowAccessPolicy(context), // Change column to trigger ForceNew
1844+
Check: resource.ComposeTestCheckFunc(
1845+
// Verify that creationTime has changed, implying that the table was recreated.
1846+
func(s *terraform.State) error {
1847+
rs, ok := s.RootModule().Resources["google_bigquery_table.test"]
1848+
if !ok {
1849+
return fmt.Errorf("Not found: google_bigquery_table.test")
1850+
}
1851+
newTimeCreated := rs.Primary.Attributes["creation_time"]
1852+
if newTimeCreated == tableCreationTime {
1853+
return fmt.Errorf("creationTime should have changed on recreation, but it's still %s", newTimeCreated)
1854+
}
1855+
return nil
1856+
},
1857+
),
1858+
ExpectNonEmptyPlan: true,
1859+
},
1860+
{
1861+
ResourceName: "google_bigquery_table.test",
1862+
ImportState: true,
1863+
ImportStateVerify: true,
1864+
ImportStateVerifyIgnore: []string{"deletion_protection", "ignore_auto_generated_schema", "generated_schema_columns"},
1865+
},
1866+
},
1867+
})
1868+
}
1869+
18061870
func TestAccBigQueryTable_schemaWithRequiredFieldAndView(t *testing.T) {
1871+
t.Parallel()
1872+
18071873
datasetID := fmt.Sprintf("tf_test_%s", acctest.RandString(t, 10))
18081874
tableID := fmt.Sprintf("tf_test_%s", acctest.RandString(t, 10))
18091875

@@ -4672,6 +4738,100 @@ resource "google_bigquery_table" "test" {
46724738
`, datasetID, tableID, schema)
46734739
}
46744740

4741+
func testAccBigQueryTableWithSchemaAndRowAccessPolicy(context map[string]interface{}) string {
4742+
return acctest.Nprintf(`
4743+
resource "google_bigquery_dataset" "test" {
4744+
dataset_id = "%{dataset_id}"
4745+
}
4746+
4747+
resource "google_bigquery_table" "test" {
4748+
deletion_protection = false
4749+
dataset_id = google_bigquery_dataset.test.dataset_id
4750+
table_id = "%{table_id}"
4751+
4752+
schema = <<EOF
4753+
[
4754+
{
4755+
"name": "user_id",
4756+
"type": "STRING",
4757+
"mode": "REQUIRED",
4758+
"description": "Unique identifier for the user"
4759+
},
4760+
{
4761+
"name": "email",
4762+
"type": "STRING",
4763+
"mode": "NULLABLE",
4764+
"description": "User's email address"
4765+
},
4766+
{
4767+
"name": "region",
4768+
"type": "STRING",
4769+
"mode": "NULLABLE",
4770+
"description": "User's assigned region"
4771+
}
4772+
]
4773+
EOF
4774+
}
4775+
4776+
resource "google_bigquery_row_access_policy" "test" {
4777+
dataset_id = google_bigquery_table.test.dataset_id
4778+
table_id = google_bigquery_table.test.table_id
4779+
policy_id = "%{policy_id}"
4780+
4781+
grantees = [
4782+
"group:[email protected]"
4783+
]
4784+
filter_predicate = "email = SESSION_USER()"
4785+
4786+
depends_on = [google_bigquery_table.test]
4787+
}
4788+
`, context)
4789+
}
4790+
4791+
func testAccBigQueryTableWithSchemaColumnDroppedAndRowAccessPolicy(context map[string]interface{}) string {
4792+
return acctest.Nprintf(`
4793+
resource "google_bigquery_dataset" "test" {
4794+
dataset_id = "%{dataset_id}"
4795+
}
4796+
4797+
resource "google_bigquery_table" "test" {
4798+
deletion_protection = false
4799+
dataset_id = google_bigquery_dataset.test.dataset_id
4800+
table_id = "%{table_id}"
4801+
4802+
schema = <<EOF
4803+
[
4804+
{
4805+
"name": "user_id",
4806+
"type": "STRING",
4807+
"mode": "REQUIRED",
4808+
"description": "Unique identifier for the user"
4809+
},
4810+
{
4811+
"name": "email",
4812+
"type": "STRING",
4813+
"mode": "NULLABLE",
4814+
"description": "User's email address"
4815+
}
4816+
]
4817+
EOF
4818+
}
4819+
4820+
resource "google_bigquery_row_access_policy" "test" {
4821+
dataset_id = google_bigquery_table.test.dataset_id
4822+
table_id = google_bigquery_table.test.table_id
4823+
policy_id = "%{policy_id}"
4824+
4825+
grantees = [
4826+
"group:[email protected]"
4827+
]
4828+
filter_predicate = "email = SESSION_USER()"
4829+
4830+
depends_on = [google_bigquery_table.test]
4831+
}
4832+
`, context)
4833+
}
4834+
46754835
func testAccBigQueryTableWithReplicationInfoAndView(datasetID, tableID string) string {
46764836
return fmt.Sprintf(`
46774837
resource "google_bigquery_dataset" "test" {

0 commit comments

Comments
 (0)