Skip to content

Commit a348666

Browse files
RedbackThomsonRedbackThomson
authored
Fix grants being applied for default ACLs (#65)
Issue #, if available: aws-controllers-k8s/community#1021 Description of changes: A new bucket will have the `private` canned ACL by default. When adopting this bucket, the `ReadOne` in the controller will adopt it correctly, but will not be able to decide whether the ACL should be `private`, `bucket-owner-read` or `bucket-owner-full-control` (all have the same grants). The controller will set `Spec.ACL` to be a bar delimited list of these values, as well as list of grants that match these canned ACL policies. For the first reconciliation after adoption, the custom hook code was seeing a list of ACLs (rather than a single value) and did not know how to diff this against the described ACL possibilities. This was producing a faulty `PutBucketACLPayload` with values of both the canned ACL and the header grants - causing the error as described in the issue above. This pull request ensures that the diff from the header grants will always be `nil` (by setting all header grants on `a` and `b` in the delta to `nil`) if a canned ACL is supplied. It also preemptively splits the `Spec.ACL` from the desired by the bar delimiter in case there have been multiple values joined in that field previously (most users won't see this functionality, but it needs to be there for adoption to work properly). Lastly, it never sets the grant header in the `PutBucketACLPayload` object if a canned ACL has already been specified. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent bfcb0cb commit a348666

File tree

4 files changed

+116
-23
lines changed

4 files changed

+116
-23
lines changed

‎go.local.mod‎

Lines changed: 61 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,70 @@
11
module github.com/aws-controllers-k8s/s3-controller
22

3-
go 1.14
3+
go 1.17
44

55
replace github.com/aws-controllers-k8s/runtime => ../runtime
66

77
require (
8-
github.com/aws-controllers-k8s/runtime v0.8.0
8+
github.com/aws-controllers-k8s/runtime v0.16.3
99
github.com/aws/aws-sdk-go v1.37.10
10-
github.com/go-logr/logr v0.1.0
10+
github.com/go-logr/logr v1.2.0
1111
github.com/spf13/pflag v1.0.5
12-
k8s.io/api v0.18.2
13-
k8s.io/apimachinery v0.18.6
14-
k8s.io/client-go v0.18.2
15-
sigs.k8s.io/controller-runtime v0.6.0
12+
github.com/stretchr/testify v1.7.0
13+
k8s.io/api v0.23.0
14+
k8s.io/apimachinery v0.23.0
15+
k8s.io/client-go v0.23.0
16+
sigs.k8s.io/controller-runtime v0.11.0
17+
)
18+
19+
require (
20+
github.com/beorn7/perks v1.0.1 // indirect
21+
github.com/cespare/xxhash/v2 v2.1.1 // indirect
22+
github.com/davecgh/go-spew v1.1.1 // indirect
23+
github.com/evanphx/json-patch v4.12.0+incompatible // indirect
24+
github.com/fsnotify/fsnotify v1.5.1 // indirect
25+
github.com/go-logr/zapr v1.2.0 // indirect
26+
github.com/gogo/protobuf v1.3.2 // indirect
27+
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
28+
github.com/golang/protobuf v1.5.2 // indirect
29+
github.com/google/go-cmp v0.5.5 // indirect
30+
github.com/google/gofuzz v1.1.0 // indirect
31+
github.com/google/uuid v1.1.2 // indirect
32+
github.com/googleapis/gnostic v0.5.5 // indirect
33+
github.com/imdario/mergo v0.3.12 // indirect
34+
github.com/jaypipes/envutil v1.0.0 // indirect
35+
github.com/jmespath/go-jmespath v0.4.0 // indirect
36+
github.com/json-iterator/go v1.1.12 // indirect
37+
github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
38+
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
39+
github.com/modern-go/reflect2 v1.0.2 // indirect
40+
github.com/pkg/errors v0.9.1 // indirect
41+
github.com/pmezard/go-difflib v1.0.0 // indirect
42+
github.com/prometheus/client_golang v1.11.0 // indirect
43+
github.com/prometheus/client_model v0.2.0 // indirect
44+
github.com/prometheus/common v0.28.0 // indirect
45+
github.com/prometheus/procfs v0.6.0 // indirect
46+
github.com/stretchr/objx v0.2.0 // indirect
47+
go.uber.org/atomic v1.7.0 // indirect
48+
go.uber.org/multierr v1.6.0 // indirect
49+
go.uber.org/zap v1.19.1 // indirect
50+
golang.org/x/net v0.0.0-20210825183410-e898025ed96a // indirect
51+
golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f // indirect
52+
golang.org/x/sys v0.0.0-20211029165221-6e7872819dc8 // indirect
53+
golang.org/x/term v0.0.0-20210615171337-6886f2dfbf5b // indirect
54+
golang.org/x/text v0.3.7 // indirect
55+
golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac // indirect
56+
gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
57+
google.golang.org/appengine v1.6.7 // indirect
58+
google.golang.org/protobuf v1.27.1 // indirect
59+
gopkg.in/inf.v0 v0.9.1 // indirect
60+
gopkg.in/yaml.v2 v2.4.0 // indirect
61+
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
62+
k8s.io/apiextensions-apiserver v0.23.0 // indirect
63+
k8s.io/component-base v0.23.0 // indirect
64+
k8s.io/klog/v2 v2.30.0 // indirect
65+
k8s.io/kube-openapi v0.0.0-20211115234752-e816edb12b65 // indirect
66+
k8s.io/utils v0.0.0-20210930125809-cb0fa318a74b // indirect
67+
sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 // indirect
68+
sigs.k8s.io/structured-merge-diff/v4 v4.2.0 // indirect
69+
sigs.k8s.io/yaml v1.3.0 // indirect
1670
)

‎pkg/resource/bucket/hook.go‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -490,14 +490,21 @@ func customPreCompare(
490490
}
491491
if a.ko.Spec.ACL != nil {
492492
// Don't diff grant headers if a canned ACL has been used
493+
a.ko.Spec.GrantFullControl = nil
493494
b.ko.Spec.GrantFullControl = nil
495+
a.ko.Spec.GrantRead = nil
494496
b.ko.Spec.GrantRead = nil
497+
a.ko.Spec.GrantReadACP = nil
495498
b.ko.Spec.GrantReadACP = nil
499+
a.ko.Spec.GrantWrite = nil
496500
b.ko.Spec.GrantWrite = nil
501+
a.ko.Spec.GrantWriteACP = nil
497502
b.ko.Spec.GrantWriteACP = nil
498503

499504
// Find the canned ACL from the joined possibility string
500505
if b.ko.Spec.ACL != nil {
506+
// Take the first ACL in case they are defined as delimited list
507+
a.ko.Spec.ACL = &strings.Split(*a.ko.Spec.ACL, CannedACLJoinDelimiter)[0]
501508
b.ko.Spec.ACL = matchPossibleCannedACL(*a.ko.Spec.ACL, *b.ko.Spec.ACL)
502509
}
503510
} else {
@@ -687,22 +694,24 @@ func (rm *resourceManager) newPutBucketACLPayload(
687694
res.SetBucket(*r.ko.Spec.Name)
688695
if r.ko.Spec.ACL != nil {
689696
res.SetACL(*r.ko.Spec.ACL)
690-
}
697+
} else {
698+
// Only put grants on bucket if there is no canned ACL to match
691699

692-
if r.ko.Spec.GrantFullControl != nil {
693-
res.SetGrantFullControl(*r.ko.Spec.GrantFullControl)
694-
}
695-
if r.ko.Spec.GrantRead != nil {
696-
res.SetGrantRead(*r.ko.Spec.GrantRead)
697-
}
698-
if r.ko.Spec.GrantReadACP != nil {
699-
res.SetGrantReadACP(*r.ko.Spec.GrantReadACP)
700-
}
701-
if r.ko.Spec.GrantWrite != nil {
702-
res.SetGrantWrite(*r.ko.Spec.GrantWrite)
703-
}
704-
if r.ko.Spec.GrantWriteACP != nil {
705-
res.SetGrantWriteACP(*r.ko.Spec.GrantWriteACP)
700+
if r.ko.Spec.GrantFullControl != nil {
701+
res.SetGrantFullControl(*r.ko.Spec.GrantFullControl)
702+
}
703+
if r.ko.Spec.GrantRead != nil {
704+
res.SetGrantRead(*r.ko.Spec.GrantRead)
705+
}
706+
if r.ko.Spec.GrantReadACP != nil {
707+
res.SetGrantReadACP(*r.ko.Spec.GrantReadACP)
708+
}
709+
if r.ko.Spec.GrantWrite != nil {
710+
res.SetGrantWrite(*r.ko.Spec.GrantWrite)
711+
}
712+
if r.ko.Spec.GrantWriteACP != nil {
713+
res.SetGrantWriteACP(*r.ko.Spec.GrantWriteACP)
714+
}
706715
}
707716

708717
// Check that there is at least some ACL on the bucket

‎test/e2e/requirements.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
acktest @ git+https://github.com/aws-controllers-k8s/test-infra.git@5220331d003e3a23de4470e68d02c99b16c81989
1+
acktest @ git+https://github.com/aws-controllers-k8s/test-infra.git@cfcae865f4e199977de9957f33e3a6c24bf4d1a9

‎test/e2e/tests/test_bucket.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,19 @@
1818
import time
1919
import logging
2020
import re
21+
import boto3
2122
from typing import Generator
2223
from dataclasses import dataclass
2324

2425
from acktest.resources import random_suffix_name
2526
from acktest.k8s import resource as k8s
27+
from acktest.aws.identity import get_region
28+
from acktest import adoption as adoption
2629
from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_s3_resource
2730
from e2e.replacement_values import REPLACEMENT_VALUES
2831
from e2e.bootstrap_resources import BootstrapResources, get_bootstrap_resources
2932

33+
RESOURCE_KIND = "Bucket"
3034
RESOURCE_PLURAL = "buckets"
3135

3236
CREATE_WAIT_AFTER_SECONDS = 10
@@ -121,6 +125,32 @@ def basic_bucket(s3_client) -> Generator[Bucket, None, None]:
121125
exists = bucket_exists(s3_client, bucket)
122126
assert not exists
123127

128+
class TestAdoptBucket(adoption.AbstractAdoptionTest):
129+
RESOURCE_PLURAL: str = RESOURCE_PLURAL
130+
RESOURCE_VERSION: str = CRD_VERSION
131+
132+
_bucket_name: str = random_suffix_name("ack-adopted-bucket", 63)
133+
134+
def bootstrap_resource(self):
135+
client = boto3.client('s3')
136+
region = get_region()
137+
if region == "us-east-1":
138+
client.create_bucket(Bucket=self._bucket_name)
139+
else:
140+
client.create_bucket(
141+
Bucket=self._bucket_name, CreateBucketConfiguration={"LocationConstraint": region}
142+
)
143+
144+
def cleanup_resource(self):
145+
client = boto3.client('s3')
146+
client.delete_bucket(Bucket=self._bucket_name)
147+
148+
def get_resource_spec(self) -> adoption.AdoptedResourceSpec:
149+
return adoption.AdoptedResourceSpec(
150+
aws=adoption.AdoptedResourceNameOrIDIdentifier(additionalKeys={}, nameOrID=self._bucket_name),
151+
kubernetes=adoption.AdoptedResourceKubernetesIdentifiers(CRD_GROUP, RESOURCE_KIND),
152+
)
153+
124154
@service_marker
125155
class TestBucket:
126156
def test_basic(self, basic_bucket):

0 commit comments

Comments
 (0)