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(bigtable): Backup Level IAM #3222

Merged
merged 17 commits into from Dec 29, 2020
Merged

Conversation

AlisskaPie
Copy link
Contributor

Implementing Backup Level IAM methods.

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Nov 17, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 17, 2020
@AlisskaPie AlisskaPie marked this pull request as ready for review November 17, 2020 22:34
@AlisskaPie AlisskaPie requested review from tritone and a team as code owners November 17, 2020 22:34
@AlisskaPie
Copy link
Contributor Author

bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Thanks!

bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
@@ -1243,6 +1248,88 @@ func TestIntegration_TableIam(t *testing.T) {
}
}

func TestIntegration_BackupIAM(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is an integration test file, it feels like we shouldn't be mocking out the interactions. It's probably enough to follow the pattern of TestIntegration_TableIam (which should be IAM, but that's another story 🙃).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. @kolea2 do you know how other languages are handling this testing, and if we will face any quota-related issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's strange, but I got an error while comparing policies without mocking. I had an Etag mismatch, even in the read-modify-write cycle.
From documentation:

It is strongly suggested that systems make use of the etag in the read-modify-write cycle to perform policy updates in order to avoid race conditions: An etag is returned in the response to getIamPolicy, and systems are expected to put that etag in the request to setIamPolicy to ensure that their change will be applied to the same version of the policy.

image

Will wait for response!

P.S. Anyway, I can also rename TestIntegration_TableIam to TestIntegration_TableIAM here :)
It's related to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Etag should only change when there is a modification. So, something else must have modified the value between the initial read and the subsequent attempted write. Mind uploading the updated test without the mock, even if it isn't passing yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure!
Uploaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tritone for Java we take the existing IAM policy and reset it. Probably could be improved: https://github.com/googleapis/java-bigtable/blob/master/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/it/BigtableTableAdminClientIT.java#L194-L217. I haven't seen any quota related issues so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tbpg @tritone any additional thoughts here?

AlisskaPie and others added 2 commits November 20, 2020 00:11
Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
Copy link
Contributor

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -1243,6 +1248,88 @@ func TestIntegration_TableIam(t *testing.T) {
}
}

func TestIntegration_BackupIAM(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Etag should only change when there is a modification. So, something else must have modified the value between the initial read and the subsequent attempted write. Mind uploading the updated test without the mock, even if it isn't passing yet?

func (ac *AdminClient) TableIAM(tableID string) *iam.Handle {
return iam.InternalNewHandleGRPCClient(ac.tClient,
"projects/"+ac.project+"/instances/"+ac.instance+"/tables/"+tableID)
}

// BackupIAM creates an IAM Handle specific to a given Cluster and Backup.
func (ac *AdminClient) BackupIAM(ctx context.Context, cluster, backup string) *iam.Handle {
backupPath := ac.instancePrefix() + "/clusters/" + cluster + "/backups/" + backup
Copy link
Contributor

Choose a reason for hiding this comment

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

is this anywhere else in the code? If not, it seems useful as a common utility.

Copy link
Contributor Author

@AlisskaPie AlisskaPie Dec 3, 2020

Choose a reason for hiding this comment

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

I've met this code in few places. In UpdateBackup, BackupInfo, RestoreTable, DeleteBackup methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool - I think it would be worth it to refactor this as a utility helper then in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay here, I had an emergency.

Done, thanks for advice!

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, lgtm thank you!

@tritone tritone merged commit c77c822 into googleapis:master Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants