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.

Sign up for GitHub

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

chore(storage): add UpdateBucketACL implementation #5974

Merged
merged 7 commits into from
May 9, 2022
Merged

chore(storage): add UpdateBucketACL implementation #5974

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
14624d3
chore(storage): add UpdateBucketACL implementation
cojenco May 2, 2022
8ab2cf4
implement gRPC
cojenco May 2, 2022
b66b5e1
revise UpdateBucket by adding unexported fields to uattrs
cojenco May 3, 2022
933cf81
fix comments
cojenco May 5, 2022
0c81e39
address comments
cojenco May 9, 2022
d672a26
Merge branch 'main' into acl-update
cojenco May 9, 2022
ba181e1
Merge branch 'main' into acl-update
cojenco May 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions storage/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,11 +986,17 @@ func (ua *BucketAttrsToUpdate) toProtoBucket() *storagepb.Bucket {
lifecycle = *ua.Lifecycle
}
var bktACL []*storagepb.BucketAccessControl
if ua.acl != nil {
bktACL = toProtoBucketACL(ua.acl)
}
if ua.PredefinedACL != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very confused about what is going on with PredefinedACL and PredefinedDefaultObjectACL here... it doesn't seem like they are actually being set on the returned bucket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, one cannot provide both a predefinedAcl and an acl while patching/updating a bucket. I tried out buckets.patch and buckets.update with the JSON API, and it results in a 409 conflict error.

So in the library code here,, PredefinedACL and PredefinedDefaultObjectACL take precedence and therefore clears ACLs.

That being said, I believe I need to handle the updateMask with more care under the update semantics. Let me know if this clarifies your question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, gotcha. Yeah I think ForceSendFields plays the same role for JSON that the updateMask plays for gRPC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appreciate the review, Chris! Added some inline comments. Hopefully that's clearer for readers and our future selves.

// Clear ACL or the call will fail.
bktACL = nil
}
var bktDefaultObjectACL []*storagepb.ObjectAccessControl
if ua.defaultObjectACL != nil {
bktDefaultObjectACL = toProtoObjectACL(ua.defaultObjectACL)
}
if ua.PredefinedDefaultObjectACL != "" {
// Clear ACLs or the call will fail.
bktDefaultObjectACL = nil
Expand Down Expand Up @@ -1126,6 +1132,17 @@ type BucketAttrsToUpdate struct {
// more information.
RPO RPO

// acl is the list of access control rules on the bucket.
// It is unexported and only used internally by the gRPC client.
// Library users should use ACLHandle methods directly.
acl []ACLRule

// defaultObjectACL is the list of access controls to
// apply to new objects when no object ACL is provided.
// It is unexported and only used internally by the gRPC client.
// Library users should use ACLHandle methods directly.
defaultObjectACL []ACLRule

setLabels map[string]string
deleteLabels map[string]bool
}
Expand Down
43 changes: 43 additions & 0 deletions storage/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,49 @@ func TestListDefaultObjectACLsEmulated(t *testing.T) {
})
}

func TestUpdateBucketACLEmulated(t *testing.T) {
transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) {
ctx := context.Background()
attrs := &BucketAttrs{
Name: bucket,
PredefinedACL: "authenticatedRead",
}
// Create the bucket that will be retrieved.
if _, err := client.CreateBucket(ctx, project, attrs); err != nil {
t.Fatalf("client.CreateBucket: %v", err)
}
var listAcls []ACLRule
var err error
// Assert bucket has two BucketACL entities, including project owner and predefinedACL.
if listAcls, err = client.ListBucketACLs(ctx, bucket); err != nil {
t.Fatalf("client.ListBucketACLs: %v", err)
}
if got, want := len(listAcls), 2; got != want {
t.Errorf("ListBucketACLs: got %v, want %v items", listAcls, want)
}
entity := AllUsers
role := RoleReader
want := &ACLRule{Entity: entity, Role: role}
got, err := client.UpdateBucketACL(ctx, bucket, entity, role)
if err != nil {
t.Fatalf("client.UpdateBucketACL: %v", err)
}
if diff := cmp.Diff(got.Entity, want.Entity); diff != "" {
t.Errorf("got(-),want(+):\n%s", diff)
}
if diff := cmp.Diff(got.Role, want.Role); diff != "" {
t.Errorf("got(-),want(+):\n%s", diff)
}
// Assert bucket now has three BucketACL entities, including existing ACLs.
if listAcls, err = client.ListBucketACLs(ctx, bucket); err != nil {
t.Fatalf("client.ListBucketACLs: %v", err)
}
if got, want := len(listAcls), 3; got != want {
t.Errorf("ListBucketACLs: got %v, want %v items", listAcls, want)
}
})
}

func initEmulatorClients() func() error {
noopCloser := func() error { return nil }
if !isEmulatorEnvironmentSet() {
Expand Down
24 changes: 23 additions & 1 deletion storage/grpc_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,12 @@ func (c *grpcStorageClient) UpdateBucket(ctx context.Context, bucket string, uat
if uattrs.PredefinedDefaultObjectACL != "" {
fieldMask.Paths = append(fieldMask.Paths, "default_object_acl")
}
if uattrs.acl != nil {
fieldMask.Paths = append(fieldMask.Paths, "acl")
}
if uattrs.defaultObjectACL != nil {
fieldMask.Paths = append(fieldMask.Paths, "default_object_acl")
}
if uattrs.StorageClass != "" {
fieldMask.Paths = append(fieldMask.Paths, "storage_class")
}
Expand Down Expand Up @@ -425,8 +431,24 @@ func (c *grpcStorageClient) ListBucketACLs(ctx context.Context, bucket string, o
}
return attrs.ACL, nil
}

func (c *grpcStorageClient) UpdateBucketACL(ctx context.Context, bucket string, entity ACLEntity, role ACLRole, opts ...storageOption) (*ACLRule, error) {
return nil, errMethodNotSupported
// There is no separate API for PATCH in gRPC.
// Make a GET call first to retrieve BucketAttrs.
attrs, err := c.GetBucket(ctx, bucket, nil, opts...)
if err != nil {
return nil, err
}
var acl []ACLRule
acl = append(attrs.ACL, ACLRule{Entity: entity, Role: role})
uattrs := &BucketAttrsToUpdate{acl: acl}
// Call UpdateBucket with a MetagenerationMatch precondition set.
b, err := c.UpdateBucket(ctx, bucket, uattrs, &BucketConditions{MetagenerationMatch: attrs.MetaGeneration}, opts...)
if err != nil {
return nil, err
}
aclRule := b.ACL[len(b.ACL)-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just return the ACLRule we sent rather than picking the last one off the list (I'm not sure if the ordering is guaranteed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return &aclRule, err
}

// Object ACL methods.
Expand Down
26 changes: 22 additions & 4 deletions storage/http_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,28 @@ func (c *httpStorageClient) ListBucketACLs(ctx context.Context, bucket string, o
return toBucketACLRules(acls.Items), nil
}

func (c *httpStorageClient) UpdateBucketACL(ctx context.Context, bucket string, entity ACLEntity, role ACLRole, opts ...storageOption) (*ACLRule, error) {
s := callSettings(c.settings, opts...)
acl := &raw.BucketAccessControl{
Bucket: bucket,
Entity: string(entity),
Role: string(role),
}
var aclRule ACLRule
var err error
err = run(ctx, func() error {
req := c.raw.BucketAccessControls.Update(bucket, string(entity), acl)
configureACLCall(ctx, s.userProject, req)
acl, err = req.Do()
aclRule = toBucketACLRule(acl)
return err
}, s.retry, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, use s.idempotent so we can set it above the interface

if err != nil {
return nil, err
}
return &aclRule, nil
}

// configureACLCall sets the context, user project and headers on the apiary library call.
// This will panic if the call does not have the correct methods.
func configureACLCall(ctx context.Context, userProject string, call interface{ Header() http.Header }) {
Expand All @@ -434,10 +456,6 @@ func configureACLCall(ctx context.Context, userProject string, call interface{ H
setClientHeader(call.Header())
}

func (c *httpStorageClient) UpdateBucketACL(ctx context.Context, bucket string, entity ACLEntity, role ACLRole, opts ...storageOption) (*ACLRule, error) {
return nil, errMethodNotSupported
}

// Object ACL methods.

func (c *httpStorageClient) DeleteObjectACL(ctx context.Context, bucket, object string, entity ACLEntity, opts ...storageOption) error {
Expand Down