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

feat(bigquery): support IAM conditions in datasets #11123

Merged
merged 12 commits into from
Dec 3, 2024

Conversation

shollyman
Copy link
Contributor

@shollyman shollyman commented Nov 12, 2024

This PR adds support for IAM conditions via the existing dataset access mechanism. To do so, the following changes are necessary:

  • add the Expr type for expressing conditions, and wire it into the existing DatasetAccessEntry.
  • add an option pattern to the Dataset-related RPC methods
  • Add a new WithAccessPolicyVersion option for setting access policies

To expose the new functionality, this PR adds CreateWithOptions, UpdateWithOptions, MetadataWithOptions methods on Dataset that accept the new option.

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the BigQuery API. label Nov 12, 2024
@shollyman shollyman changed the title !feat(bigquery): support IAM conditions in datasets feat(bigquery)!: support IAM conditions in datasets Nov 14, 2024
@shollyman shollyman changed the title feat(bigquery)!: support IAM conditions in datasets feat(bigquery): support IAM conditions in datasets Nov 15, 2024
@shollyman shollyman marked this pull request as ready for review November 15, 2024 23:01
@shollyman shollyman requested review from a team as code owners November 15, 2024 23:01
Copy link
Contributor

@alvarowolfx alvarowolfx left a comment

Choose a reason for hiding this comment

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

I wonder if we can check inside the Create/Update/Metadata methods if the dataset has any AccessEntry with a non nil Condition and automatically add the AccessPolicyVersion = 3, without requiring to add the new *WithOptions methods and DatasetOption for now.

One thing that I'm concerned is if users starts to use IAM conditions and have a mixed set of Datasets with and without conditions, they have to change all of their code to adopt the new *WithOptions methods and check by themselves if the dataset has conditions or not to add the proper WithAccessPolicyVersion version. Calling the Metadata call might be the most common one that would cause more confusions.

Maybe the Dataset struct has a method like hasConditions where if checks for AccessEntry.Condition and on the Create/Update/Metadata methods we check that and add the version.


// Expr represents the conditional information related to dataset access policies.
type Expr struct {
//Textual representation of an expression in Common Expression Language syntax.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space here after //

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

bigquery/dataset_integration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alvarowolfx alvarowolfx left a comment

Choose a reason for hiding this comment

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

per internal discussion, it's better for users to be explicit about integrating with IAM conditions, so makes sense to have the *WIthOptions methods. When they start adopting this feature, they can use Policy version 3 for all of they dataset access usage.

@shollyman shollyman added the automerge Merge the pull request once unit tests and other checks pass. label Dec 3, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit d93c2d9 into googleapis:main Dec 3, 2024
7 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 3, 2024
@shollyman shollyman deleted the dataset-cond branch December 3, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants