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.

Author: RedbackThomson

go.local.mod

@@ -1,16 +1,70 @@
 module github.com/aws-controllers-k8s/s3-controller
 
-go 1.14
+go 1.17
 
 replace github.com/aws-controllers-k8s/runtime => ../runtime
 
 require (
-	github.com/aws-controllers-k8s/runtime v0.8.0
+	github.com/aws-controllers-k8s/runtime v0.16.3
 	github.com/aws/aws-sdk-go v1.37.10
-	github.com/go-logr/logr v0.1.0
+	github.com/go-logr/logr v1.2.0
 	github.com/spf13/pflag v1.0.5
-	k8s.io/api v0.18.2
-	k8s.io/apimachinery v0.18.6
-	k8s.io/client-go v0.18.2
-	sigs.k8s.io/controller-runtime v0.6.0
+	github.com/stretchr/testify v1.7.0
+	k8s.io/api v0.23.0
+	k8s.io/apimachinery v0.23.0
+	k8s.io/client-go v0.23.0
+	sigs.k8s.io/controller-runtime v0.11.0
+)
+
+require (
+	github.com/beorn7/perks v1.0.1 // indirect
+	github.com/cespare/xxhash/v2 v2.1.1 // indirect
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/evanphx/json-patch v4.12.0+incompatible // indirect
+	github.com/fsnotify/fsnotify v1.5.1 // indirect
+	github.com/go-logr/zapr v1.2.0 // indirect
+	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
+	github.com/golang/protobuf v1.5.2 // indirect
+	github.com/google/go-cmp v0.5.5 // indirect
+	github.com/google/gofuzz v1.1.0 // indirect
+	github.com/google/uuid v1.1.2 // indirect
+	github.com/googleapis/gnostic v0.5.5 // indirect
+	github.com/imdario/mergo v0.3.12 // indirect
+	github.com/jaypipes/envutil v1.0.0 // indirect
+	github.com/jmespath/go-jmespath v0.4.0 // indirect
+	github.com/json-iterator/go v1.1.12 // indirect
+	github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
+	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
+	github.com/modern-go/reflect2 v1.0.2 // indirect
+	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
+	github.com/prometheus/client_golang v1.11.0 // indirect
+	github.com/prometheus/client_model v0.2.0 // indirect
+	github.com/prometheus/common v0.28.0 // indirect
+	github.com/prometheus/procfs v0.6.0 // indirect
+	github.com/stretchr/objx v0.2.0 // indirect
+	go.uber.org/atomic v1.7.0 // indirect
+	go.uber.org/multierr v1.6.0 // indirect
+	go.uber.org/zap v1.19.1 // indirect
+	golang.org/x/net v0.0.0-20210825183410-e898025ed96a // indirect
+	golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f // indirect
+	golang.org/x/sys v0.0.0-20211029165221-6e7872819dc8 // indirect
+	golang.org/x/term v0.0.0-20210615171337-6886f2dfbf5b // indirect
+	golang.org/x/text v0.3.7 // indirect
+	golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac // indirect
+	gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
+	google.golang.org/appengine v1.6.7 // indirect
+	google.golang.org/protobuf v1.27.1 // indirect
+	gopkg.in/inf.v0 v0.9.1 // indirect
+	gopkg.in/yaml.v2 v2.4.0 // indirect
+	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
+	k8s.io/apiextensions-apiserver v0.23.0 // indirect
+	k8s.io/component-base v0.23.0 // indirect
+	k8s.io/klog/v2 v2.30.0 // indirect
+	k8s.io/kube-openapi v0.0.0-20211115234752-e816edb12b65 // indirect
+	k8s.io/utils v0.0.0-20210930125809-cb0fa318a74b // indirect
+	sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 // indirect
+	sigs.k8s.io/structured-merge-diff/v4 v4.2.0 // indirect
+	sigs.k8s.io/yaml v1.3.0 // indirect
 )

pkg/resource/bucket/hook.go

@@ -490,14 +490,21 @@ func customPreCompare(
 	}
 	if a.ko.Spec.ACL != nil {
 		// Don't diff grant headers if a canned ACL has been used
+		a.ko.Spec.GrantFullControl = nil
 		b.ko.Spec.GrantFullControl = nil
+		a.ko.Spec.GrantRead = nil
 		b.ko.Spec.GrantRead = nil
+		a.ko.Spec.GrantReadACP = nil
 		b.ko.Spec.GrantReadACP = nil
+		a.ko.Spec.GrantWrite = nil
 		b.ko.Spec.GrantWrite = nil
+		a.ko.Spec.GrantWriteACP = nil
 		b.ko.Spec.GrantWriteACP = nil
 
 		// Find the canned ACL from the joined possibility string
 		if b.ko.Spec.ACL != nil {
+			// Take the first ACL in case they are defined as delimited list
+			a.ko.Spec.ACL = &strings.Split(*a.ko.Spec.ACL, CannedACLJoinDelimiter)[0]
 			b.ko.Spec.ACL = matchPossibleCannedACL(*a.ko.Spec.ACL, *b.ko.Spec.ACL)
 		}
 	} else {
@@ -687,22 +694,24 @@ func (rm *resourceManager) newPutBucketACLPayload(
 	res.SetBucket(*r.ko.Spec.Name)
 	if r.ko.Spec.ACL != nil {
 		res.SetACL(*r.ko.Spec.ACL)
-	}
+	} else {
+		// Only put grants on bucket if there is no canned ACL to match
 
-	if r.ko.Spec.GrantFullControl != nil {
-		res.SetGrantFullControl(*r.ko.Spec.GrantFullControl)
-	}
-	if r.ko.Spec.GrantRead != nil {
-		res.SetGrantRead(*r.ko.Spec.GrantRead)
-	}
-	if r.ko.Spec.GrantReadACP != nil {
-		res.SetGrantReadACP(*r.ko.Spec.GrantReadACP)
-	}
-	if r.ko.Spec.GrantWrite != nil {
-		res.SetGrantWrite(*r.ko.Spec.GrantWrite)
-	}
-	if r.ko.Spec.GrantWriteACP != nil {
-		res.SetGrantWriteACP(*r.ko.Spec.GrantWriteACP)
+		if r.ko.Spec.GrantFullControl != nil {
+			res.SetGrantFullControl(*r.ko.Spec.GrantFullControl)
+		}
+		if r.ko.Spec.GrantRead != nil {
+			res.SetGrantRead(*r.ko.Spec.GrantRead)
+		}
+		if r.ko.Spec.GrantReadACP != nil {
+			res.SetGrantReadACP(*r.ko.Spec.GrantReadACP)
+		}
+		if r.ko.Spec.GrantWrite != nil {
+			res.SetGrantWrite(*r.ko.Spec.GrantWrite)
+		}
+		if r.ko.Spec.GrantWriteACP != nil {
+			res.SetGrantWriteACP(*r.ko.Spec.GrantWriteACP)
+		}
 	}
 
 	// Check that there is at least some ACL on the bucket

test/e2e/requirements.txt

@@ -1 +1 @@
-acktest @ git+https://github.com/aws-controllers-k8s/test-infra.git@5220331d003e3a23de4470e68d02c99b16c81989
+acktest @ git+https://github.com/aws-controllers-k8s/test-infra.git@cfcae865f4e199977de9957f33e3a6c24bf4d1a9

test/e2e/tests/test_bucket.py

@@ -18,15 +18,19 @@
 import time
 import logging
 import re
+import boto3
 from typing import Generator
 from dataclasses import dataclass
 
 from acktest.resources import random_suffix_name
 from acktest.k8s import resource as k8s
+from acktest.aws.identity import get_region
+from acktest import adoption as adoption
 from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_s3_resource
 from e2e.replacement_values import REPLACEMENT_VALUES
 from e2e.bootstrap_resources import BootstrapResources, get_bootstrap_resources
 
+RESOURCE_KIND = "Bucket"
 RESOURCE_PLURAL = "buckets"
 
 CREATE_WAIT_AFTER_SECONDS = 10
@@ -121,6 +125,32 @@ def basic_bucket(s3_client) -> Generator[Bucket, None, None]:
     exists = bucket_exists(s3_client, bucket)
     assert not exists
 
+class TestAdoptBucket(adoption.AbstractAdoptionTest):
+    RESOURCE_PLURAL: str = RESOURCE_PLURAL
+    RESOURCE_VERSION: str = CRD_VERSION
+
+    _bucket_name: str = random_suffix_name("ack-adopted-bucket", 63)
+
+    def bootstrap_resource(self):
+        client = boto3.client('s3')
+        region = get_region()
+        if region == "us-east-1":
+            client.create_bucket(Bucket=self._bucket_name)
+        else:
+            client.create_bucket(
+                Bucket=self._bucket_name, CreateBucketConfiguration={"LocationConstraint": region}
+            )
+
+    def cleanup_resource(self):
+        client = boto3.client('s3')
+        client.delete_bucket(Bucket=self._bucket_name)
+
+    def get_resource_spec(self) -> adoption.AdoptedResourceSpec:
+        return adoption.AdoptedResourceSpec(
+            aws=adoption.AdoptedResourceNameOrIDIdentifier(additionalKeys={}, nameOrID=self._bucket_name),
+            kubernetes=adoption.AdoptedResourceKubernetesIdentifiers(CRD_GROUP, RESOURCE_KIND),
+        )
+
 @service_marker
 class TestBucket:
     def test_basic(self, basic_bucket):