Skip to content
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 support for access #48

Merged
merged 2 commits into from
Feb 14, 2020
Merged

Add support for access #48

merged 2 commits into from
Feb 14, 2020

Conversation

umairidris
Copy link
Contributor

@umairidris umairidris commented Feb 14, 2020

Also set a better default (a single owner which is projectOwners). The defaults of BQ are not very good which is why I made this PR to begin with as we would like to use this module for our e2e solutions. The default BQ adds the calling user as an owner on the dataset which is a big no-no for us (access should only be granted via explicit groups/service accounts or inheritance). This default is much more secure and in line with how other GCP resources work.

I thought it would be ok to improve the default for CFT as better defaults was one of the goals. But if you prefer to avoid diff I can just set this to null as default which shouldn't need a new release.

Fix #12

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, that now makes more sense and I'm okay with including it. @jaketf just to confirm this is a best practice.

If you get a chance, it'd be nice to add:

  1. Tests for the access param
  2. A note in the README about how to use access and the default we set.

# special_group: A special group to grant access to.
variable "access" {
description = "An array of objects that define dataset access for one or more entities."
type = any
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: once hashicorp/terraform#19898 is resolved we should switch to type = list(object)

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

Successfully merging this pull request may close these issues.

Control dataset access
2 participants