Merge pull request crossplane-contrib#2214 from swisscom/fix/s3-bucket-logging-config-and-object-owner

fix(s3): bucket loggingConfig and objectOwnership

Author: MisterMX

pkg/clients/s3/bucket.go

@@ -232,7 +232,11 @@ func UpdateBucketACL(ctx context.Context, client BucketClient, bucket *v1beta1.B
 
 // BucketHasACLsDisabled returns true if ACLs are disabled for the bucket, i.e., if ObjectOwnership is set to BucketOwnerEnforced
 func BucketHasACLsDisabled(bucket *v1beta1.Bucket) bool {
-	return s3types.ObjectOwnership(aws.ToString(bucket.Spec.ForProvider.ObjectOwnership)) == s3types.ObjectOwnershipBucketOwnerEnforced
+	if s3types.ObjectOwnership(aws.ToString(bucket.Spec.ForProvider.ObjectOwnership)) == s3types.ObjectOwnershipBucketOwnerEnforced ||
+		bucket.Spec.ForProvider.ObjectOwnership == nil {
+		return true
+	}
+	return false
 }
 
 // UpdateBucketOwnershipControls creates the OwnershipContolsInput, sends the request to put an ObjectOwnership based on the bucket

pkg/controller/s3/bucket/bucket_test.go

@@ -127,10 +127,11 @@ func TestObserve(t *testing.T) {
 				s3: s3Testing.Client(s3Testing.WithPutACL(func(ctx context.Context, input *awss3.PutBucketAclInput, opts []func(*awss3.Options)) (*awss3.PutBucketAclOutput, error) {
 					return nil, errBoom
 				})),
-				cr: s3Testing.Bucket(),
+				cr: s3Testing.Bucket(s3Testing.WithObjectOwnership("BucketOwnerPreferred")),
 			},
 			want: want{
 				cr: s3Testing.Bucket(
+					s3Testing.WithObjectOwnership("BucketOwnerPreferred"),
 					s3Testing.WithArn(fmt.Sprintf("arn:aws:s3:::%s", s3Testing.BucketName)),
 				),
 				err:    errBoom,

pkg/controller/s3/bucket/loggingConfig.go

@@ -55,17 +55,14 @@ func (in *LoggingConfigurationClient) Observe(ctx context.Context, bucket *v1bet
 		return NeedsUpdate, errorutils.Wrap(err, loggingGetFailed)
 	}
 	if !cmp.Equal(GenerateAWSLogging(bucket.Spec.ForProvider.LoggingConfiguration), external.LoggingEnabled,
-		cmpopts.IgnoreTypes(&xpv1.Reference{}, &xpv1.Selector{}), cmpopts.IgnoreTypes(document.NoSerde{})) {
+		cmpopts.EquateEmpty(), cmpopts.IgnoreTypes(&xpv1.Reference{}, &xpv1.Selector{}), cmpopts.IgnoreTypes(document.NoSerde{})) {
 		return NeedsUpdate, nil
 	}
 	return Updated, nil
 }
 
 // CreateOrUpdate sends a request to have resource created on AWS
 func (in *LoggingConfigurationClient) CreateOrUpdate(ctx context.Context, bucket *v1beta1.Bucket) error {
-	if bucket.Spec.ForProvider.LoggingConfiguration == nil {
-		return nil
-	}
 	input := GeneratePutBucketLoggingInput(meta.GetExternalName(bucket), bucket.Spec.ForProvider.LoggingConfiguration)
 	_, err := in.client.PutBucketLogging(ctx, input)
 	return errorutils.Wrap(err, loggingPutFailed)
@@ -76,44 +73,9 @@ func (*LoggingConfigurationClient) Delete(_ context.Context, _ *v1beta1.Bucket)
 	return nil
 }
 
-// LateInitialize is responsible for initializing the resource based on the external value
+// LateInitialize is not needed because loggingConfiguration is not something which is created be default
+// it means if it is not set in the desired state, but it exists on aws side it should be deleted(by CreateOrUpdate), not late initialized
 func (in *LoggingConfigurationClient) LateInitialize(ctx context.Context, bucket *v1beta1.Bucket) error {
-	external, err := in.client.GetBucketLogging(ctx, &awss3.GetBucketLoggingInput{Bucket: pointer.ToOrNilIfZeroValue(meta.GetExternalName(bucket))})
-	if err != nil {
-		return errorutils.Wrap(err, loggingGetFailed)
-	}
-
-	if external == nil || external.LoggingEnabled == nil {
-		// There is no value send by AWS to initialize
-		return nil
-	}
-
-	if bucket.Spec.ForProvider.LoggingConfiguration == nil {
-		// We need the configuration to exist so we can initialize
-		bucket.Spec.ForProvider.LoggingConfiguration = &v1beta1.LoggingConfiguration{}
-	}
-
-	config := bucket.Spec.ForProvider.LoggingConfiguration
-	// Late initialize the target Bucket and target prefix
-	config.TargetBucket = pointer.LateInitialize(config.TargetBucket, external.LoggingEnabled.TargetBucket)
-	config.TargetPrefix = pointer.LateInitializeValueFromPtr(config.TargetPrefix, external.LoggingEnabled.TargetPrefix)
-	// If the there is an external target grant list, and the local one does not exist
-	// we create the target grant list
-	if len(external.LoggingEnabled.TargetGrants) != 0 && config.TargetGrants == nil {
-		config.TargetGrants = make([]v1beta1.TargetGrant, len(external.LoggingEnabled.TargetGrants))
-		for i, v := range external.LoggingEnabled.TargetGrants {
-			config.TargetGrants[i] = v1beta1.TargetGrant{
-				Grantee: v1beta1.TargetGrantee{
-					DisplayName:  v.Grantee.DisplayName,
-					EmailAddress: v.Grantee.EmailAddress,
-					ID:           v.Grantee.ID,
-					Type:         string(v.Grantee.Type),
-					URI:          v.Grantee.URI,
-				},
-				Permission: string(v.Permission),
-			}
-		}
-	}
 	return nil
 }
 
@@ -125,24 +87,30 @@ func (in *LoggingConfigurationClient) SubresourceExists(bucket *v1beta1.Bucket)
 // GeneratePutBucketLoggingInput creates the input for the PutBucketLogging request for the S3 Client
 func GeneratePutBucketLoggingInput(name string, config *v1beta1.LoggingConfiguration) *awss3.PutBucketLoggingInput {
 	bci := &awss3.PutBucketLoggingInput{
-		Bucket: pointer.ToOrNilIfZeroValue(name),
-		BucketLoggingStatus: &types.BucketLoggingStatus{LoggingEnabled: &types.LoggingEnabled{
-			TargetBucket: config.TargetBucket,
-			TargetGrants: make([]types.TargetGrant, 0),
-			TargetPrefix: pointer.ToOrNilIfZeroValue(config.TargetPrefix),
-		}},
+		Bucket:              pointer.ToOrNilIfZeroValue(name),
+		BucketLoggingStatus: &types.BucketLoggingStatus{},
 	}
-	for _, grant := range config.TargetGrants {
-		bci.BucketLoggingStatus.LoggingEnabled.TargetGrants = append(bci.BucketLoggingStatus.LoggingEnabled.TargetGrants, types.TargetGrant{
-			Grantee: &types.Grantee{
-				DisplayName:  grant.Grantee.DisplayName,
-				EmailAddress: grant.Grantee.EmailAddress,
-				ID:           grant.Grantee.ID,
-				Type:         types.Type(grant.Grantee.Type),
-				URI:          grant.Grantee.URI,
-			},
-			Permission: types.BucketLogsPermission(grant.Permission),
-		})
+	if config != nil {
+		bci = &awss3.PutBucketLoggingInput{
+			Bucket: pointer.ToOrNilIfZeroValue(name),
+			BucketLoggingStatus: &types.BucketLoggingStatus{LoggingEnabled: &types.LoggingEnabled{
+				TargetBucket: config.TargetBucket,
+				TargetGrants: make([]types.TargetGrant, 0),
+				TargetPrefix: pointer.ToOrNilIfZeroValue(config.TargetPrefix),
+			}},
+		}
+		for _, grant := range config.TargetGrants {
+			bci.BucketLoggingStatus.LoggingEnabled.TargetGrants = append(bci.BucketLoggingStatus.LoggingEnabled.TargetGrants, types.TargetGrant{
+				Grantee: &types.Grantee{
+					DisplayName:  grant.Grantee.DisplayName,
+					EmailAddress: grant.Grantee.EmailAddress,
+					ID:           grant.Grantee.ID,
+					Type:         types.Type(grant.Grantee.Type),
+					URI:          grant.Grantee.URI,
+				},
+				Permission: types.BucketLogsPermission(grant.Permission),
+			})
+		}
 	}
 	return bci
 }
@@ -155,9 +123,7 @@ func GenerateAWSLogging(local *v1beta1.LoggingConfiguration) *types.LoggingEnabl
 	output := types.LoggingEnabled{
 		TargetBucket: local.TargetBucket,
 		TargetPrefix: pointer.ToOrNilIfZeroValue(local.TargetPrefix),
-	}
-	if local.TargetGrants != nil {
-		output.TargetGrants = make([]types.TargetGrant, len(local.TargetGrants))
+		TargetGrants: []types.TargetGrant{},
 	}
 	for i := range local.TargetGrants {
 		target := types.TargetGrant{
@@ -171,7 +137,7 @@ func GenerateAWSLogging(local *v1beta1.LoggingConfiguration) *types.LoggingEnabl
 			Permission: types.BucketLogsPermission(local.TargetGrants[i].Permission),
 		}
 
-		output.TargetGrants[i] = target
+		output.TargetGrants = append(output.TargetGrants, target)
 	}
 	return &output
 }

pkg/controller/s3/bucket/loggingConfig_test.go

@@ -58,6 +58,13 @@ func generateLoggingConfig() *v1beta1.LoggingConfiguration {
 	}
 }
 
+func generateLoggingConfigNoGrants() *v1beta1.LoggingConfiguration {
+	return &v1beta1.LoggingConfiguration{
+		TargetBucket: &bucketName,
+		TargetPrefix: prefix,
+	}
+}
+
 func generateAWSLogging() *s3types.LoggingEnabled {
 	return &s3types.LoggingEnabled{
 		TargetBucket: &bucketName,
@@ -75,6 +82,14 @@ func generateAWSLogging() *s3types.LoggingEnabled {
 	}
 }
 
+func generateAWSLoggingNoGrants() *s3types.LoggingEnabled {
+	return &s3types.LoggingEnabled{
+		TargetBucket: &bucketName,
+		TargetGrants: []s3types.TargetGrant{},
+		TargetPrefix: &prefix,
+	}
+}
+
 func TestLoggingObserve(t *testing.T) {
 	type args struct {
 		cl *LoggingConfigurationClient
@@ -146,6 +161,20 @@ func TestLoggingObserve(t *testing.T) {
 				err:    nil,
 			},
 		},
+		"NoUpdateExistsWithoutGrants": {
+			args: args{
+				b: s3testing.Bucket(s3testing.WithLoggingConfig(generateLoggingConfigNoGrants())),
+				cl: NewLoggingConfigurationClient(fake.MockBucketClient{
+					MockGetBucketLogging: func(ctx context.Context, input *s3.GetBucketLoggingInput, opts []func(*s3.Options)) (*s3.GetBucketLoggingOutput, error) {
+						return &s3.GetBucketLoggingOutput{LoggingEnabled: generateAWSLoggingNoGrants()}, nil
+					},
+				}),
+			},
+			want: want{
+				status: Updated,
+				err:    nil,
+			},
+		},
 	}
 
 	for name, tc := range cases {
@@ -225,89 +254,3 @@ func TestLoggingCreateOrUpdate(t *testing.T) {
 		})
 	}
 }
-
-func TestLoggingLateInit(t *testing.T) {
-	type args struct {
-		cl SubresourceClient
-		b  *v1beta1.Bucket
-	}
-
-	type want struct {
-		err error
-		cr  *v1beta1.Bucket
-	}
-
-	cases := map[string]struct {
-		args
-		want
-	}{
-		"Error": {
-			args: args{
-				b: s3testing.Bucket(),
-				cl: NewLoggingConfigurationClient(fake.MockBucketClient{
-					MockGetBucketLogging: func(ctx context.Context, input *s3.GetBucketLoggingInput, opts []func(*s3.Options)) (*s3.GetBucketLoggingOutput, error) {
-						return &s3.GetBucketLoggingOutput{}, errBoom
-					},
-				}),
-			},
-			want: want{
-				err: errorutils.Wrap(errBoom, loggingGetFailed),
-				cr:  s3testing.Bucket(),
-			},
-		},
-		"NoLateInitEmpty": {
-			args: args{
-				b: s3testing.Bucket(),
-				cl: NewLoggingConfigurationClient(fake.MockBucketClient{
-					MockGetBucketLogging: func(ctx context.Context, input *s3.GetBucketLoggingInput, opts []func(*s3.Options)) (*s3.GetBucketLoggingOutput, error) {
-						return &s3.GetBucketLoggingOutput{}, nil
-					},
-				}),
-			},
-			want: want{
-				err: nil,
-				cr:  s3testing.Bucket(),
-			},
-		},
-		"SuccessfulLateInit": {
-			args: args{
-				b: s3testing.Bucket(s3testing.WithLoggingConfig(nil)),
-				cl: NewLoggingConfigurationClient(fake.MockBucketClient{
-					MockGetBucketLogging: func(ctx context.Context, input *s3.GetBucketLoggingInput, opts []func(*s3.Options)) (*s3.GetBucketLoggingOutput, error) {
-						return &s3.GetBucketLoggingOutput{LoggingEnabled: generateAWSLogging()}, nil
-					},
-				}),
-			},
-			want: want{
-				err: nil,
-				cr:  s3testing.Bucket(s3testing.WithLoggingConfig(generateLoggingConfig())),
-			},
-		},
-		"NoOpLateInit": {
-			args: args{
-				b: s3testing.Bucket(s3testing.WithLoggingConfig(generateLoggingConfig())),
-				cl: NewLoggingConfigurationClient(fake.MockBucketClient{
-					MockGetBucketLogging: func(ctx context.Context, input *s3.GetBucketLoggingInput, opts []func(*s3.Options)) (*s3.GetBucketLoggingOutput, error) {
-						return &s3.GetBucketLoggingOutput{LoggingEnabled: &s3types.LoggingEnabled{}}, nil
-					},
-				}),
-			},
-			want: want{
-				err: nil,
-				cr:  s3testing.Bucket(s3testing.WithLoggingConfig(generateLoggingConfig())),
-			},
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			err := tc.args.cl.LateInitialize(context.Background(), tc.args.b)
-			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
-				t.Errorf("r: -want, +got:\n%s", diff)
-			}
-			if diff := cmp.Diff(tc.want.cr, tc.args.b, test.EquateConditions()); diff != "" {
-				t.Errorf("r: -want, +got:\n%s", diff)
-			}
-		})
-	}
-}

pkg/controller/s3/testing/testHelper.go

@@ -42,6 +42,10 @@ var (
 // BucketModifier is a function which modifies the Bucket for testing
 type BucketModifier func(bucket *v1beta1.Bucket)
 
+func WithObjectOwnership(s string) BucketModifier {
+	return func(r *v1beta1.Bucket) { r.Spec.ForProvider.ObjectOwnership = &s }
+}
+
 // WithArn sets the ARN for an S3 Bucket
 func WithArn(arn string) BucketModifier {
 	return func(bucket *v1beta1.Bucket) {