#644 Add Support For BigQuery Access Control #1931

chrismorgan-hb · 2018-08-22T23:52:27Z

I went ahead and added separate tests, leaving the "basic" tests to prove that the existing way (no explicit access block) continues to work.

Fixes #644

This is my first non-toy Go code and my first contribution to this project, so feedback is most welcome.

danawillow

Awesome, I'm super excited about this! I have a bunch of comments but overall structure looks good.

danawillow · 2018-08-24T18:33:48Z

google/resource_bigquery_dataset.go

+						"domain": &schema.Schema{
+							Type:          schema.TypeString,
+							Optional:      true,
+							ConflictsWith: []string{"access.group_by_email", "access.special_group", "access.user_by_email", "access.view"},


Sadly this won't work. ConflictsWith needs to point to an exact address of a field, and since this is a list, there's no way to wildcard all indexes. If you want a check like this, you'll just have to do it at apply-time (inside the create fn)

The API error message is pretty good in this case, so I just dropped this validation on our side.

danawillow · 2018-08-24T18:35:48Z

google/resource_bigquery_dataset.go

+			da := bigquery.DatasetAccess{}
+			msi := m.(map[string]interface{})
+			da.Role = msi["role"].(string)
+			if val, ok := msi["domain"]; ok {


I would need to confirm 100% since I don't remember which cases this applies to and which it doesn't, but I think the map will actually have all values in it, even if they weren't set, which would mean you could do:

da := bigquery.DatasetAccess{ Role: msi["role"].(string), Domain: msi["domain}.(string), [etc] }

Changing it that way unfortunately breaks existing tests (where access isn't set at all)

Really? You're already inside a block that checks whether access is set once you've gotten to this point.

danawillow · 2018-08-24T18:36:13Z

google/resource_bigquery_dataset.go

+		access := []*bigquery.DatasetAccess{}
+		for _, m := range v.([]interface{}) {
+			da := bigquery.DatasetAccess{}
+			msi := m.(map[string]interface{})


does msi just stand for map[string]interface? if so maybe a sliiiightly more descriptive var name could be used

danawillow · 2018-08-24T18:38:03Z

google/resource_bigquery_dataset.go

+						"role": &schema.Schema{
+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: validateRole,


We already have a validateFn that you can use for enums: validation.StringInSlice([]string{[values here]}, false),

Nice! Done.

danawillow · 2018-08-24T18:39:09Z

google/resource_bigquery_dataset.go

+	access := make([]map[string]interface{}, 0, len(a))
+	for _, da := range a {
+		var ai map[string]interface{}
+		ai = make(map[string]interface{})


you shouldn't need to do this in two lines, you can just do

ai := make(map[string]interface{})

danawillow · 2018-08-24T18:41:55Z

google/resource_bigquery_dataset_test.go

+  location                    = "EU"
+  default_table_expiration_ms = 3600000
+
+  access = [


this can just be

access { role = [...] user_by_email = [...] }

danawillow · 2018-08-24T18:42:54Z

google/resource_bigquery_dataset_test.go

+resource "google_bigquery_dataset" "access_test" {
+  dataset_id                  = "%s"
+  friendly_name               = "foo"
+  description                 = "This is a foo description"


I'm fine with keeping these here, but I would also be fine with the test only setting required fields + access-related stuff

Makes sense. Removed optional fields.

danawillow · 2018-08-24T18:43:16Z

google/resource_bigquery_dataset_test.go

+  location                    = "EU"
+  default_table_expiration_ms = 3600000
+
+  access = [


likewise, this would just be

access { ... } access { ... }

danawillow · 2018-08-24T18:44:45Z

google/resource_bigquery_dataset_test.go

+    },
+    {
+	view = {
+		project_id = "%s"


could this be project_id = "${google_bigquery_dataset.other_dataset.project}"? would that make sense semantically?

Thanks, that's more consistent.

danawillow · 2018-08-24T18:45:23Z

website/docs/r/bigquery_dataset.html.markdown

@@ -73,6 +84,9 @@ The following arguments are supported:

 * `labels` - (Optional) A mapping of labels to assign to the resource.

+* `access` - (Optional) An array of objects that define dataset access for


Can you add a "structure is documented below" statement and then docs for each of the attributes? search for that string in our existing docs and you'll find a bunch of examples.

chrismorgan-hb

Thanks for the comments!

chrismorgan-hb · 2018-08-27T20:17:15Z

google/resource_bigquery_dataset.go

+						"role": &schema.Schema{
+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: validateRole,


Nice! Done.

chrismorgan-hb · 2018-08-27T20:19:01Z

google/resource_bigquery_dataset.go

+						"domain": &schema.Schema{
+							Type:          schema.TypeString,
+							Optional:      true,
+							ConflictsWith: []string{"access.group_by_email", "access.special_group", "access.user_by_email", "access.view"},


The API error message is pretty good in this case, so I just dropped this validation on our side.

chrismorgan-hb · 2018-08-27T20:36:16Z

google/resource_bigquery_dataset.go

+			da := bigquery.DatasetAccess{}
+			msi := m.(map[string]interface{})
+			da.Role = msi["role"].(string)
+			if val, ok := msi["domain"]; ok {


Changing it that way unfortunately breaks existing tests (where access isn't set at all)

chrismorgan-hb · 2018-08-27T20:38:08Z

google/resource_bigquery_dataset.go

+		access := []*bigquery.DatasetAccess{}
+		for _, m := range v.([]interface{}) {
+			da := bigquery.DatasetAccess{}
+			msi := m.(map[string]interface{})


chrismorgan-hb · 2018-08-27T20:39:17Z

google/resource_bigquery_dataset.go

+	access := make([]map[string]interface{}, 0, len(a))
+	for _, da := range a {
+		var ai map[string]interface{}
+		ai = make(map[string]interface{})


chrismorgan-hb · 2018-08-27T20:42:25Z

google/resource_bigquery_dataset.go

+		}
+		if da.View != nil {
+			view := make(map[string]string)
+			view["project_id"] = da.View.ProjectId


Neat! Done.

chrismorgan-hb · 2018-08-27T20:45:23Z

google/resource_bigquery_dataset_test.go

+resource "google_bigquery_dataset" "access_test" {
+  dataset_id                  = "%s"
+  friendly_name               = "foo"
+  description                 = "This is a foo description"


Makes sense. Removed optional fields.

chrismorgan-hb · 2018-08-27T20:45:35Z

google/resource_bigquery_dataset_test.go

+  location                    = "EU"
+  default_table_expiration_ms = 3600000
+
+  access = [


chrismorgan-hb · 2018-08-27T20:47:36Z

google/resource_bigquery_dataset_test.go

+    },
+    {
+	view = {
+		project_id = "%s"


Thanks, that's more consistent.

chrismorgan-hb · 2018-08-27T20:57:32Z

website/docs/r/bigquery_dataset.html.markdown

@@ -73,6 +84,9 @@ The following arguments are supported:

 * `labels` - (Optional) A mapping of labels to assign to the resource.

+* `access` - (Optional) An array of objects that define dataset access for


chrismorgan-hb

Also fixed markdown formatting with the nested list of special groups

chrismorgan-hb · 2018-09-05T17:40:06Z

Friendly ping @danawillow (now that you're back from vacation and I'm sure your inbox is swamped).

danawillow · 2018-09-05T21:01:51Z

google/resource_bigquery_dataset.go

+			da := bigquery.DatasetAccess{}
+			msi := m.(map[string]interface{})
+			da.Role = msi["role"].(string)
+			if val, ok := msi["domain"]; ok {


Really? You're already inside a block that checks whether access is set once you've gotten to this point.

danawillow · 2018-09-05T21:03:41Z

google/resource_bigquery_dataset.go

+func flattenAccess(a []*bigquery.DatasetAccess) []map[string]interface{} {
+	access := make([]map[string]interface{}, 0, len(a))
+	for _, da := range a {
+		ai := make(map[string]interface{})


nit: this section can also be done in a single block:

ai := map[string]interface{}{ "role": da.Role, ..etc... }

danawillow · 2018-09-05T21:05:22Z

google/resource_bigquery_dataset.go

@@ -228,6 +308,7 @@ func resourceBigQueryDatasetRead(d *schema.ResourceData, meta interface{}) error
 	d.Set("project", id.Project)
 	d.Set("etag", res.Etag)
 	d.Set("labels", res.Labels)
+	d.Set("access", flattenAccess(res.Access))


for setting values that aren't primitives, we're trying to get in the habit of checking the error returned from d.Set (we've had some confusing bugs in the past that would have shown up much sooner if we had error-checked). something like:

if err := d.Set("access", flattenAccess(res.Access)); err != nil { return err }

danawillow · 2018-09-05T21:06:03Z

google/resource_bigquery_dataset_test.go

+func testAccBigQueryDatasetWithOneAccess(datasetID string) string {
+	return fmt.Sprintf(`
+resource "google_bigquery_dataset" "access_test" {
+  dataset_id                  = "%s"


nit: since this isn't trying to align with anything else, you can move the equals sign way back over to the left

danawillow · 2018-09-05T21:06:28Z

google/resource_bigquery_dataset_test.go

+  }
+  access {
+    role	      = "READER"
+    domain	      = "example.com"


nit: looks like a tab snuck in here

danawillow · 2018-09-05T21:07:55Z

google/resource_bigquery_dataset_test.go

+}`, datasetID)
+}
+
+func getBigQueryTableWithView(datasetID, tableID string) string {


Is this func only used in the one place? If so, may as well just inline it

danawillow · 2018-09-05T21:09:00Z

google/resource_bigquery_dataset_test.go

+    {
+	role          = "OWNER"
+	user_by_email = "[email protected]"
+    },


Can you look at this section? The formatting seems to have gotten a bit wonky.

danawillow · 2018-09-05T21:10:06Z

website/docs/r/bigquery_dataset.html.markdown

@@ -26,6 +26,17 @@ resource "google_bigquery_dataset" "default" {
  labels {
    env = "default"
  }
+
+  access = [


this should be:

access { role = "READER" domain = "example.com" } access { role = "WRITER" group_by_email = "[email protected]" }

There's nothing wrong with the other way, this is just more canonical.

danawillow · 2018-09-05T21:10:53Z

website/docs/r/bigquery_dataset.html.markdown

+    the user specified by the other member of the access object. The following
+    string values are supported: `READER`, `WRITER`, `OWNER`.
+
+* `domain` - (Pick one) A domain to grant access to.


I'm not quite sure what "Pick one" means in this context, do you mean Optional?

I was mimic-ing the BQ API documentation, it basically means you have to pick exactly one of domain, group_by_email, special_group, user_by_email, or view. If there's a way to be more clear, I'm happy to change it.

Ah, got it. I'd just try to match the rest of the Terraform documentation, where we just say Required or Optional there. I think that Pick One itself is pretty redundant, right? There isn't a way to specify multiple of the same field, so you don't really have a choice but to pick one (or zero, but that's covered in Required vs Optional).

It's a little more nuanced because these are keys in a map, not values that can be taken by a single field. I updated the docs to reflect that (while marking these optional). Let me know what you think.

danawillow · 2018-09-05T21:13:27Z

google/resource_bigquery_dataset.go

+							Optional: true,
+						},
+						"view": &schema.Schema{
+							Type:     schema.TypeMap,


This has specific keys that have to be in the map, correct? If so, then this should be another nested object (list with MaxSize: 1 and a defined schema) so we don't allow users to set keys in the map that aren't valid options.

danawillow · 2018-09-05T21:16:10Z

Oh also! Fun GH trick: if you write fixes #644 in the body of your PR, it'll close the issue automatically: https://help.github.com/articles/closing-issues-using-keywords/

danawillow · 2018-09-11T19:27:03Z

Yup that looks good to me, thanks! I'll go ahead and merge this.

* Updates based on PR comments * Fix markdown nested list * Second round of feedback * Clarify pick one docs

ghost · 2018-11-16T14:18:13Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

088c4e3

ghost added the size/l label Aug 22, 2018

danawillow suggested changes Aug 24, 2018

View reviewed changes

Updates based on PR comments

328574e

chrismorgan-hb commented Aug 27, 2018

View reviewed changes

Fix markdown nested list

9b845a9

chrismorgan-hb commented Aug 27, 2018

View reviewed changes

danawillow suggested changes Sep 5, 2018

View reviewed changes

Second round of feedback

7698695

ghost added the size/l label Sep 10, 2018

Clarify pick one docs

8b9a2cf

danawillow approved these changes Sep 11, 2018

View reviewed changes

danawillow merged commit 5599183 into hashicorp:master Sep 11, 2018

tmshn mentioned this pull request Sep 21, 2018

Order of google_bigquery_dataset.access should be ignored #2093

Closed

ghost locked and limited conversation to collaborators Nov 16, 2018

		@@ -73,6 +84,9 @@ The following arguments are supported:

		* `labels` - (Optional) A mapping of labels to assign to the resource.

		* `access` - (Optional) An array of objects that define dataset access for

#644 Add Support For BigQuery Access Control #1931

#644 Add Support For BigQuery Access Control #1931

Conversation

chrismorgan-hb commented Aug 22, 2018 • edited Loading

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismorgan-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismorgan-hb left a comment

Choose a reason for hiding this comment

chrismorgan-hb commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow commented Sep 5, 2018

danawillow commented Sep 11, 2018

ghost commented Nov 16, 2018

chrismorgan-hb commented Aug 22, 2018 •

edited

Loading