Prepared for review, removed all manual verification, added tests

apis/storage/v1alpha3/bucketpolicy_types.go

@@ -17,8 +17,6 @@ limitations under the License.
 package v1alpha3
 
 import (
-	"encoding/json"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	runtimev1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1"
@@ -36,6 +34,30 @@ type S3BucketPolicyParameters struct {
 	PolicyStatement []S3BucketPolicyStatement `json:"Statement"`
 }
 
+//Serialize is the custom marshaller for the S3BucketPolicyParameters
+func (p *S3BucketPolicyParameters) Serialize() (interface{}, error) {
+	m := make(map[string]interface{})
+	m["Version"] = p.PolicyVersion
+	if p.PolicyID != "" {
+		m["Id"] = p.PolicyID
+	}
+	slc := make([]interface{}, len(p.PolicyStatement))
+	for i, v := range p.PolicyStatement {
+		msg, err := v.Serialize()
+		if err != nil {
+			return nil, err
+		}
+		slc[i] = msg
+	}
+	m["Statement"] = slc
+	//dat, err := json.Marshal(m)
+	//if err != nil {
+	//	return nil, err
+	//}
+	//str := string(dat)
+	return m, nil
+}
+
 // S3BucketPolicyStatement defines an individual statement within the
 // S3BucketPolicyBody
 type S3BucketPolicyStatement struct {
@@ -63,22 +85,22 @@ type S3BucketPolicyStatement struct {
 	ResourcePath []string `json:"Resource"`
 }
 
-//MarshalJSON is the custom marshaller for the S3BucketPolicyStatement
-//func (p *S3BucketPolicyStatement) MarshalJSON() ([]byte, error) {
-//	m := make(map[string]interface{})
-//	if len(p.ResourcePath) == 1 {
-//		m["Resource"] = p.ResourcePath[0]
-//	} else {
-//		m["Resource"] = p.ResourcePath
-//	}
-//	m["Principal"] = p.Principal
-//	m["Action"] = p.PolicyAction
-//	m["Effect"] = p.Effect
-//	if p.StatementID != "" {
-//		m["Sid"] = p.StatementID
-//	}
-//	return json.Marshal(m)
-//}
+//Serialize is the custom marshaller for the S3BucketPolicyStatement
+func (p *S3BucketPolicyStatement) Serialize() (interface{}, error) {
+	m := make(map[string]interface{})
+	principal, err := p.Principal.Serialize()
+	if err != nil {
+		return nil, err
+	}
+	m["Principal"] = principal
+	m["Action"] = tryFirst(p.PolicyAction)
+	m["Resource"] = tryFirst(p.ResourcePath)
+	m["Effect"] = p.Effect
+	if p.StatementID != "" {
+		m["Sid"] = p.StatementID
+	}
+	return m, nil
+}
 
 // S3BucketPrincipal defines the principal users affected by
 // the S3BucketPolicyStatement
@@ -89,21 +111,25 @@ type S3BucketPrincipal struct {
 
 	// This list contains the all of the AWS IAM users which are affected
 	// by the policy statement
-	AWSPrincipal []string `json:"AWS"`
+	AWSPrincipal []string `json:"AWS,omitempty"`
 }
 
-//MarshalJSON is the custom marshaller for the S3BucketPrincipal
-func (p *S3BucketPrincipal) MarshalJSON() ([]byte, error) {
+func tryFirst(slc []string) interface{} {
+	if len(slc) == 1 {
+		return slc[0]
+	}
+	return slc
+}
+
+//Serialize is the custom serializer for the S3BucketPrincipal
+func (p *S3BucketPrincipal) Serialize() (interface{}, error) {
+	all := "*"
 	if p.AllowAnon {
-		return json.Marshal("*")
+		return all, nil
 	}
 	m := make(map[string]interface{})
-	//if len(p.AWSPrincipal) == 1 {
-	//	m["AWS"] = p.AWSPrincipal[0]
-	//} else {
-	m["AWS"] = p.AWSPrincipal
-	//}
-	return json.Marshal(m)
+	m["AWS"] = tryFirst(p.AWSPrincipal)
+	return m, nil
 }
 
 // An S3BucketPolicySpec defines the desired state of an

apis/storage/v1alpha3/referencers.go

@@ -15,6 +15,7 @@ package v1alpha3
 
 import (
 	"context"
+
 	"github.com/crossplane/crossplane-runtime/pkg/reference"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -34,7 +35,6 @@ func S3BucketIAMUser() reference.ExtractValueFn {
 // ResolveReferences of this S3BucketPolicy
 func (mg *S3BucketPolicy) ResolveReferences(ctx context.Context, c client.Reader) error {
 	r := reference.NewAPIResolver(c, mg)
-	println("References")
 	// Resolve spec.BucketName
 	rsp, err := r.Resolve(ctx, reference.ResolutionRequest{
 		CurrentValue: mg.Spec.BucketName,
@@ -63,8 +63,5 @@ func (mg *S3BucketPolicy) ResolveReferences(ctx context.Context, c client.Reader
 	mg.Spec.UserName = rsp.ResolvedValue
 	mg.Spec.UserNameRef = rsp.ResolvedReference
 
-	println(mg.Spec.UserName)
-	println(mg.Spec.BucketName)
-
 	return nil
 }

config/crd/storage.aws.crossplane.io_s3bucketpolicies.yaml

@@ -202,8 +202,6 @@ spec:
                             description: This flag indicates if the policy should
                               be made available to all anonymous users.
                             type: boolean
-                        required:
-                        - AWS
                         type: object
                       Resource:
                         description: The paths on which this resource will apply

pkg/clients/s3/fake/s3bucketpolicy.go

@@ -1,8 +1,12 @@
 package fake
 
 import (
+	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/aws/awserr"
 	"github.com/aws/aws-sdk-go-v2/service/s3"
 
+	"github.com/crossplane/provider-aws/pkg/clients/iam"
+	"github.com/crossplane/provider-aws/pkg/clients/iam/fake"
 	clientset "github.com/crossplane/provider-aws/pkg/clients/s3"
 )
 
@@ -30,3 +34,18 @@ func (m *MockBucketPolicyClient) PutBucketPolicyRequest(input *s3.PutBucketPolic
 func (m *MockBucketPolicyClient) DeleteBucketPolicyRequest(input *s3.DeleteBucketPolicyInput) s3.DeleteBucketPolicyRequest {
 	return m.MockDeleteBucketPolicyRequest(input)
 }
+
+// NewMockBucketPolicyClient returns a new client given an aws config
+func NewMockBucketPolicyClient(conf *aws.Config) (clientset.BucketPolicyClient, iam.Client, error) {
+	s3client := MockBucketPolicyClient{}
+	iamclient := fake.Client{}
+	return &s3client, &iamclient, nil
+}
+
+// IsErrorNotFound returns true if the error code indicates that the item was not found
+func IsErrorNotFound(err error) bool {
+	if s3Err, ok := err.(awserr.Error); ok && s3Err.Code() == "NoSuchBucketPolicy" {
+		return true
+	}
+	return false
+}

pkg/controller/s3/s3bucketpolicy/s3bucketpolicy.go

@@ -20,8 +20,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+
 	"github.com/aws/aws-sdk-go-v2/aws"
 	awss3 "github.com/aws/aws-sdk-go-v2/service/s3"
+	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -108,43 +110,37 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
 		return managed.ExternalObservation{}, errors.New(errUnexpectedObject)
 	}
 
-	_, err := e.client.GetBucketPolicyRequest(&awss3.GetBucketPolicyInput{
+	resp, err := e.client.GetBucketPolicyRequest(&awss3.GetBucketPolicyInput{
 		Bucket: aws.String(cr.Spec.BucketName),
 	}).Send(ctx)
 	if err != nil {
 		return managed.ExternalObservation{}, errors.Wrap(resource.Ignore(s3.IsErrorNotFound, err), errGet)
 	}
 
-	//policyData, err := e.formatBucketPolicy(cr)
-	//
-	//if err != nil {
-	//	return managed.ExternalObservation{}, errors.Wrap(resource.Ignore(s3.IsErrorNotFound, err), errGet)
-	//}
-
-	//utilize string to interface map for unmarshalling response and local formatted json.
-	//localPolicy := map[string]interface{}{}
-	//externalPolicy := map[string]interface{}{}
-	//
-	//intfLocalPolicy := json.Unmarshal(policyData, &localPolicy)
-	//intfExternalPolicy := json.Unmarshal([]byte(*resp.Policy), &externalPolicy)
-	//
-	////Compare both maps, if they are not equal, then our version must have diverged.
-	//if !cmp.Equal(intfLocalPolicy, intfExternalPolicy) {
-	//	return managed.ExternalObservation{
-	//		ResourceExists: true,
-	//		ResourceUpToDate: false,
-	//	}, nil
-	//}
+	policyData, err := e.formatBucketPolicy(cr)
+
+	if err != nil {
+		return managed.ExternalObservation{}, errors.Wrap(resource.Ignore(s3.IsErrorNotFound, err), errGet)
+	}
+
+	cr.SetConditions(runtimev1alpha1.Available())
+
+	if !cmp.Equal(*policyData, *resp.Policy) {
+		return managed.ExternalObservation{
+			ResourceExists:   true,
+			ResourceUpToDate: false,
+		}, nil
+	}
 
 	// If our version and the external version are the same, we return ResourceUpToDate: true
 	return managed.ExternalObservation{
-		ResourceExists: true,
+		ResourceExists:   true,
 		ResourceUpToDate: true,
 	}, nil
 }
 
 //formatBucketPolicy parses and formats the bucket.Spec.BucketPolicy struct
-func (e *external) formatBucketPolicy(original *bucketv1alpha3.S3BucketPolicy) ([]byte, error) {
+func (e *external) formatBucketPolicy(original *bucketv1alpha3.S3BucketPolicy) (*string, error) {
 	c := original.DeepCopy()
 	iamUsername := c.Spec.UserName
 	accountID, err := e.iamclient.GetAccountID()
@@ -167,7 +163,16 @@ func (e *external) formatBucketPolicy(original *bucketv1alpha3.S3BucketPolicy) (
 		newStatements = append(newStatements, statement)
 	}
 	c.Spec.PolicyBody.PolicyStatement = newStatements
-	return json.Marshal(c.Spec.PolicyBody)
+	body, err := c.Spec.PolicyBody.Serialize()
+	if err != nil {
+		return nil, err
+	}
+	byteData, err := json.Marshal(body)
+	if err != nil {
+		return nil, err
+	}
+	str := string(byteData)
+	return &str, nil
 }
 
 func (e *external) Create(ctx context.Context, mgd resource.Managed) (managed.ExternalCreation, error) {
@@ -183,8 +188,7 @@ func (e *external) Create(ctx context.Context, mgd resource.Managed) (managed.Ex
 		return managed.ExternalCreation{}, errors.Wrap(err, errAttach)
 	}
 
-	policyString := string(policyData)
-	println(policyString)
+	policyString := *policyData
 	_, err = e.client.PutBucketPolicyRequest(&awss3.PutBucketPolicyInput{Bucket: aws.String(cr.Spec.BucketName), Policy: aws.String(policyString)}).Send(context.TODO())
 	return managed.ExternalCreation{}, errors.Wrap(err, errAttach)
 }
@@ -196,14 +200,12 @@ func (e *external) Update(ctx context.Context, mgd resource.Managed) (managed.Ex
 		return managed.ExternalUpdate{}, errors.New(errUnexpectedObject)
 	}
 
-	cr.SetConditions(runtimev1alpha1.Creating())
-
 	policyData, err := e.formatBucketPolicy(cr)
 	if err != nil {
 		return managed.ExternalUpdate{}, errors.Wrap(err, errUpdate)
 	}
 
-	policyString := string(policyData)
+	policyString := *policyData
 	_, err = e.client.PutBucketPolicyRequest(&awss3.PutBucketPolicyInput{Bucket: aws.String(cr.Spec.BucketName), Policy: aws.String(policyString)}).Send(context.TODO())
 	return managed.ExternalUpdate{}, errors.Wrap(err, errUpdate)
 }
@@ -214,7 +216,7 @@ func (e *external) Delete(ctx context.Context, mgd resource.Managed) error {
 	if !ok {
 		return errors.New(errUnexpectedObject)
 	}
+	cr.SetConditions(runtimev1alpha1.Deleting())
 	_, err := e.client.DeleteBucketPolicyRequest(&awss3.DeleteBucketPolicyInput{Bucket: aws.String(cr.Spec.BucketName)}).Send(context.TODO())
-
-	return errors.Wrap(err, errDelete)
-}
+	return errors.Wrap(resource.Ignore(s3.IsErrorNotFound, err), errDelete)
+}