Merge pull request #4667 from thefirstofthe300/reduce-log-spam

🐛 cleanup: eliminate log spam when using S3 secrets

cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go

@@ -284,6 +284,7 @@ func (t Template) ControllersPolicy() *iamv1.PolicyDocument {
 			Action: iamv1.Actions{
 				"s3:CreateBucket",
 				"s3:DeleteBucket",
+				"s3:GetObject",
 				"s3:PutObject",
 				"s3:DeleteObject",
 				"s3:PutBucketPolicy",

cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml

@@ -289,6 +289,7 @@ Resources:
         - Action:
           - s3:CreateBucket
           - s3:DeleteBucket
+          - s3:GetObject
           - s3:PutObject
           - s3:DeleteObject
           - s3:PutBucketPolicy

controllers/awsmachine_controller.go

@@ -850,8 +850,6 @@ func (r *AWSMachineReconciler) deleteIgnitionBootstrapDataFromS3(machineScope *s
 		return nil
 	}
 
-	machineScope.Info("Deleting unneeded entry from AWS S3", "secretPrefix", machineScope.GetSecretPrefix())
-
 	if err := objectStoreSvc.Delete(machineScope); err != nil {
 		return errors.Wrap(err, "deleting bootstrap data object")
 	}

controllers/awsmachine_controller_unit_test.go

@@ -1136,7 +1136,6 @@ func TestAWSMachineReconciler(t *testing.T) {
 
 				_, err := reconciler.reconcileDelete(ms, cs, cs, cs, cs)
 				g.Expect(err).To(BeNil())
-				g.Expect(buf.String()).To(ContainSubstring("Deleting unneeded entry from AWS S3"))
 			})
 		})
 

pkg/cloud/services/s3/s3.go

@@ -173,27 +173,52 @@ func (s *Service) Delete(m *scope.MachineScope) error {
 	bucket := s.bucketName()
 	key := s.bootstrapDataKey(m)
 
-	s.scope.Info("Deleting object", "bucket_name", bucket, "key", key)
-
-	_, err := s.S3Client.DeleteObject(&s3.DeleteObjectInput{
+	_, err := s.S3Client.HeadObject(&s3.HeadObjectInput{
 		Bucket: aws.String(bucket),
 		Key:    aws.String(key),
 	})
-	if err == nil {
-		return nil
-	}
-
-	aerr, ok := err.(awserr.Error)
-	if !ok {
+	if err != nil {
+		if aerr, ok := err.(awserr.Error); ok {
+			switch aerr.Code() {
+			case "Forbidden":
+				// In the case that the IAM policy does not have sufficient
+				// permissions to get the object, we will attempt to delete it
+				// anyway for backwards compatibility reasons.
+				s.scope.Debug("Received 403 forbidden from S3 HeadObject call. If GetObject permission has been granted to the controller but not ListBucket, object is already deleted. Attempting deletion anyway in case GetObject permission hasn't been granted to the controller but DeleteObject has.", "bucket", bucket, "key", key)
+
+				_, err = s.S3Client.DeleteObject(&s3.DeleteObjectInput{
+					Bucket: aws.String(bucket),
+					Key:    aws.String(key),
+				})
+				if err != nil {
+					return errors.Wrap(err, "deleting S3 object")
+				}
+
+				s.scope.Debug("Delete object call succeeded despite missing GetObject permission", "bucket", bucket, "key", key)
+
+				return nil
+			case s3.ErrCodeNoSuchKey:
+				s.scope.Debug("Object already deleted", "bucket", bucket, "key", key)
+				return nil
+			case s3.ErrCodeNoSuchBucket:
+				s.scope.Debug("Bucket does not exist", "bucket", bucket)
+				return nil
+			default:
+				return errors.Wrap(aerr, "deleting S3 object")
+			}
+		}
+	}
+
+	s.scope.Info("Deleting S3 object", "bucket", bucket, "key", key)
+
+	_, err = s.S3Client.DeleteObject(&s3.DeleteObjectInput{
+		Bucket: aws.String(bucket),
+		Key:    aws.String(key),
+	})
+	if err != nil {
 		return errors.Wrap(err, "deleting S3 object")
 	}
 
-	switch aerr.Code() {
-	case s3.ErrCodeNoSuchBucket:
-	default:
-		return errors.Wrap(aerr, "deleting S3 object")
-	}
-
 	return nil
 }
 

pkg/cloud/services/s3/s3_test.go

@@ -631,6 +631,8 @@ func TestDeleteObject(t *testing.T) {
 			},
 		}
 
+		s3Mock.EXPECT().HeadObject(gomock.Any())
+
 		s3Mock.EXPECT().DeleteObject(gomock.Any()).Do(func(deleteObjectInput *s3svc.DeleteObjectInput) {
 			t.Run("use_configured_bucket_name_on_cluster_level", func(t *testing.T) {
 				t.Parallel()
@@ -672,7 +674,7 @@ func TestDeleteObject(t *testing.T) {
 			},
 		}
 
-		s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchBucket, "", nil)).Times(1)
+		s3Mock.EXPECT().HeadObject(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchBucket, "", nil))
 
 		if err := svc.Delete(machineScope); err != nil {
 			t.Fatalf("Unexpected error, got: %v", err)
@@ -696,6 +698,7 @@ func TestDeleteObject(t *testing.T) {
 				},
 			}
 
+			s3Mock.EXPECT().HeadObject(gomock.Any())
 			s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, errors.New("foo")).Times(1)
 
 			if err := svc.Delete(machineScope); err == nil {
@@ -747,6 +750,7 @@ func TestDeleteObject(t *testing.T) {
 			},
 		}
 
+		s3Mock.EXPECT().HeadObject(gomock.Any()).Times(2)
 		s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, nil).Times(2)
 
 		if err := svc.Delete(machineScope); err != nil {