fix: make ACLs optional for AWS S3 buckets (open-telemetry#39354)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Fixes a problem where ACLs have become required on configuration of the
AWS S3 exporter.

AWS explicitly recommends to disable ACLs on buckets
https://docs.aws.amazon.com/AmazonS3/latest/userguide/ensure-object-ownership.html

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes open-telemetry#39346

<!--Describe what testing was performed and which tests were added.-->
#### Testing

Added test to ensure configuration of the exporter worked as expected
when ACL values were set.

Updated existing config tests to ensure no ACL is set by default.

<!--Describe the documentation added.-->
#### Documentation

Updated README.md to show ACLs are optional and off by default. 

Additionally added myself as a codeowner as I'm willing to take on
partial ownership here.

<!--Please delete paragraphs that you did not use before submitting.-->

Author: Erog38

.chloggen/fix_optional-acls.yaml

@@ -0,0 +1,27 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
+component: awss3exporter
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "Fixes an issue where the AWS S3 Exporter was forcing an ACL to be set, leading to unexpected behavior in S3 bucket permissions"
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [39346]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: "Current behavior of the AWS S3 Exporter is to set the ACL to 'private' by default, this removes that behavior and sets no ACL if not specified."
+
+# If your change doesn't affect end users or the exported elements of any package,
+# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: ["user"]

.github/CODEOWNERS

@@ -42,7 +42,7 @@ exporter/alibabacloudlogserviceexporter/                         @open-telemetry
 exporter/awscloudwatchlogsexporter/                              @open-telemetry/collector-contrib-approvers @boostchicken @rapphil
 exporter/awsemfexporter/                                         @open-telemetry/collector-contrib-approvers @Aneurysm9 @mxiamxia
 exporter/awskinesisexporter/                                     @open-telemetry/collector-contrib-approvers @Aneurysm9 @MovieStoreGuy
-exporter/awss3exporter/                                          @open-telemetry/collector-contrib-approvers @atoulme @pdelewski
+exporter/awss3exporter/                                          @open-telemetry/collector-contrib-approvers @atoulme @pdelewski @Erog38
 exporter/awsxrayexporter/                                        @open-telemetry/collector-contrib-approvers @wangzlei @srprash
 exporter/azureblobexporter/                                      @open-telemetry/collector-contrib-approvers @hgaol @MovieStoreGuy
 exporter/azuredataexplorerexporter/                              @open-telemetry/collector-contrib-approvers @ag-ramachandran

exporter/awss3exporter/README.md

@@ -6,7 +6,7 @@
 | Stability     | [alpha]: traces, metrics, logs   |
 | Distributions | [contrib] |
 | Issues        | [![Open issues](https://img.shields.io/github/issues-search/open-telemetry/opentelemetry-collector-contrib?query=is%3Aissue%20is%3Aopen%20label%3Aexporter%2Fawss3%20&label=open&color=orange&logo=opentelemetry)](https://github.com/open-telemetry/opentelemetry-collector-contrib/issues?q=is%3Aopen+is%3Aissue+label%3Aexporter%2Fawss3) [![Closed issues](https://img.shields.io/github/issues-search/open-telemetry/opentelemetry-collector-contrib?query=is%3Aissue%20is%3Aclosed%20label%3Aexporter%2Fawss3%20&label=closed&color=blue&logo=opentelemetry)](https://github.com/open-telemetry/opentelemetry-collector-contrib/issues?q=is%3Aclosed+is%3Aissue+label%3Aexporter%2Fawss3) |
-| [Code Owners](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#becoming-a-code-owner)    | [@atoulme](https://www.github.com/atoulme), [@pdelewski](https://www.github.com/pdelewski) |
+| [Code Owners](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#becoming-a-code-owner)    | [@atoulme](https://www.github.com/atoulme), [@pdelewski](https://www.github.com/pdelewski), [@Erog38](https://www.github.com/Erog38) |
 
 [alpha]: https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/component-stability.md#alpha
 [contrib]: https://github.com/open-telemetry/opentelemetry-collector-releases/tree/main/distributions/otelcol-contrib
@@ -19,25 +19,25 @@ This exporter targets to support proto/json format.
 
 The following exporter configuration parameters are supported.
 
-| Name                      | Description                                                                                                                                | Default     |
-|:--------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------|-------------|
-| `region`                  | AWS region.                                                                                                                                | "us-east-1" |
-| `s3_bucket`               | S3 bucket                                                                                                                                  |             |
-| `s3_prefix`               | prefix for the S3 key (root directory inside bucket).                                                                                      |             |
+| Name                      | Description                                                                                                                                | Default                                     |
+|:--------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------|
+| `region`                  | AWS region.                                                                                                                                | "us-east-1"                                 |
+| `s3_bucket`               | S3 bucket                                                                                                                                  |                                             |
+| `s3_prefix`               | prefix for the S3 key (root directory inside bucket).                                                                                      |                                             |
 | `s3_partition_format`     | filepath formatting for the partition; See [strftime](https://www.man7.org/linux/man-pages/man3/strftime.3.html) for format specification. | "year=%Y/month=%m/day=%d/hour=%H/minute=%M" |
-| `role_arn`                | the Role ARN to be assumed                                                                                                                 |             |
-| `file_prefix`             | file prefix defined by user                                                                                                                |             |
-| `marshaler`               | marshaler used to produce output data                                                                                                      | `otlp_json` |
-| `encoding`                | Encoding extension to use to marshal data. Overrides the `marshaler` configuration option if set.                                          |             |
-| `encoding_file_extension` | file format extension suffix when using the `encoding` configuration option. May be left empty for no suffix to be appended.               |             |
-| `endpoint`                | (REST API endpoint) overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket`                   |             |
-| `storage_class`           | [S3 storageclass](https://docs.aws.amazon.com/AmazonS3/latest/userguide/storage-class-intro.html)                                          | STANDARD    |
-| `acl`                     | [S3 Object Canned ACL](https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html#canned-acl)                                 | private     |
-| `s3_force_path_style`     | [set this to `true` to force the request to use path-style addressing](http://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html) | false       |
-| `disable_ssl`             | set this to `true` to disable SSL when sending requests                                                                                    | false       |
-| `compression`             | should the file be compressed                                                                                                              | none        |
-| `sending_queue`           | [exporters common queuing](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/README.md)          | disabled    |
-| `timeout`                 | [exporters common timeout](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/README.md)          | 5s          |
+| `role_arn`                | the Role ARN to be assumed                                                                                                                 |                                             |
+| `file_prefix`             | file prefix defined by user                                                                                                                |                                             |
+| `marshaler`               | marshaler used to produce output data                                                                                                      | `otlp_json`                                 |
+| `encoding`                | Encoding extension to use to marshal data. Overrides the `marshaler` configuration option if set.                                          |                                             |
+| `encoding_file_extension` | file format extension suffix when using the `encoding` configuration option. May be left empty for no suffix to be appended.               |                                             |
+| `endpoint`                | (REST API endpoint) overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket`                   |                                             |
+| `storage_class`           | [S3 storageclass](https://docs.aws.amazon.com/AmazonS3/latest/userguide/storage-class-intro.html)                                          | STANDARD                                    |
+| `acl`                     | [S3 Object Canned ACL](https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html#canned-acl)                                 | none (does not set by default)              |
+| `s3_force_path_style`     | [set this to `true` to force the request to use path-style addressing](http://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html) | false                                       |
+| `disable_ssl`             | set this to `true` to disable SSL when sending requests                                                                                    | false                                       |
+| `compression`             | should the file be compressed                                                                                                              | none                                        |
+| `sending_queue`           | [exporters common queuing](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/README.md)          | disabled                                    |
+| `timeout`                 | [exporters common timeout](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/README.md)          | 5s                                          |
 
 
 ### Marshaler

exporter/awss3exporter/config.go

@@ -95,7 +95,7 @@ func (c *Config) Validate() error {
 		errs = multierr.Append(errs, errors.New("invalid StorageClass"))
 	}
 
-	if !validACLs[c.S3Uploader.ACL] {
+	if c.S3Uploader.ACL != "" && !validACLs[c.S3Uploader.ACL] {
 		errs = multierr.Append(errs, errors.New("invalid ACL"))
 	}
 

exporter/awss3exporter/config_test.go

@@ -46,7 +46,6 @@ func TestLoadConfig(t *testing.T) {
 			S3Bucket:          "foo",
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "otlp_json",
 	}, e,
@@ -88,7 +87,6 @@ func TestConfig(t *testing.T) {
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			Endpoint:          "http://endpoint.com",
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "otlp_json",
 	}, e,
@@ -121,7 +119,6 @@ func TestConfigS3StorageClass(t *testing.T) {
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			Endpoint:          "http://endpoint.com",
 			StorageClass:      "STANDARD_IA",
-			ACL:               "private",
 		},
 		QueueSettings:   queueCfg,
 		TimeoutSettings: timeoutCfg,
@@ -156,7 +153,41 @@ func TestConfigS3ACL(t *testing.T) {
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			Endpoint:          "http://endpoint.com",
 			StorageClass:      "STANDARD_IA",
-			ACL:               "private",
+		},
+		QueueSettings:   queueCfg,
+		TimeoutSettings: timeoutCfg,
+		MarshalerName:   "otlp_json",
+	}, e,
+	)
+}
+
+func TestConfigS3ACLDefined(t *testing.T) {
+	factories, err := otelcoltest.NopFactories()
+	assert.NoError(t, err)
+
+	factory := NewFactory()
+	factories.Exporters[factory.Type()] = factory
+	// https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33594
+	cfg, err := otelcoltest.LoadConfigAndValidate(
+		filepath.Join("testdata", "config-s3_canned-acl.yaml"), factories)
+
+	require.NoError(t, err)
+	require.NotNil(t, cfg)
+
+	e := cfg.Exporters[component.MustNewID("awss3")].(*Config)
+	queueCfg := exporterhelper.NewDefaultQueueConfig()
+	queueCfg.Enabled = false
+	timeoutCfg := exporterhelper.NewDefaultTimeoutConfig()
+
+	assert.Equal(t, &Config{
+		S3Uploader: S3UploaderConfig{
+			Region:            "us-east-1",
+			S3Bucket:          "foo",
+			S3Prefix:          "bar",
+			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
+			Endpoint:          "http://endpoint.com",
+			StorageClass:      "STANDARD",
+			ACL:               "bucket-owner-full-control",
 		},
 		QueueSettings:   queueCfg,
 		TimeoutSettings: timeoutCfg,
@@ -195,7 +226,6 @@ func TestConfigForS3CompatibleSystems(t *testing.T) {
 			S3ForcePathStyle:  true,
 			DisableSSL:        true,
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "otlp_json",
 	}, e,
@@ -309,7 +339,6 @@ func TestMarshallerName(t *testing.T) {
 			S3Bucket:          "foo",
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "sumo_ic",
 	}, e,
@@ -325,7 +354,6 @@ func TestMarshallerName(t *testing.T) {
 			S3Bucket:          "bar",
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "otlp_proto",
 	}, e,
@@ -359,7 +387,6 @@ func TestCompressionName(t *testing.T) {
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			Compression:       "gzip",
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "otlp_json",
 	}, e,
@@ -376,7 +403,6 @@ func TestCompressionName(t *testing.T) {
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			Compression:       "none",
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "otlp_proto",
 	}, e,

exporter/awss3exporter/factory.go

@@ -37,7 +37,6 @@ func createDefaultConfig() component.Config {
 			Region:            "us-east-1",
 			S3PartitionFormat: "year=%Y/month=%m/day=%d/hour=%H/minute=%M",
 			StorageClass:      "STANDARD",
-			ACL:               "private",
 		},
 		MarshalerName: "otlp_json",
 	}

exporter/awss3exporter/internal/upload/writer.go

@@ -20,6 +20,8 @@ type Manager interface {
 	Upload(ctx context.Context, data []byte) error
 }
 
+type ManagerOpt func(Manager)
+
 type s3manager struct {
 	bucket       string
 	builder      *PartitionKeyBuilder
@@ -30,14 +32,20 @@ type s3manager struct {
 
 var _ Manager = (*s3manager)(nil)
 
-func NewS3Manager(bucket string, builder *PartitionKeyBuilder, service *s3.Client, storageClass s3types.StorageClass, acl s3types.ObjectCannedACL) Manager {
-	return &s3manager{
+func NewS3Manager(bucket string, builder *PartitionKeyBuilder, service *s3.Client, storageClass s3types.StorageClass, opts ...ManagerOpt) Manager {
+	manager := &s3manager{
 		bucket:       bucket,
 		builder:      builder,
 		uploader:     manager.NewUploader(service),
 		storageClass: storageClass,
-		acl:          acl,
 	}
+	for _, opt := range opts {
+		if opt != nil {
+			opt(manager)
+		}
+	}
+
+	return manager
 }
 
 func (sw *s3manager) Upload(ctx context.Context, data []byte) error {
@@ -87,3 +95,13 @@ func (sw *s3manager) contentBuffer(raw []byte) (*bytes.Buffer, error) {
 		return bytes.NewBuffer(raw), nil
 	}
 }
+
+func WithACL(acl s3types.ObjectCannedACL) func(Manager) {
+	return func(m Manager) {
+		s3m, ok := m.(*s3manager)
+		if !ok {
+			return
+		}
+		s3m.acl = acl
+	}
+}

exporter/awss3exporter/internal/upload/writer_test.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/s3"
+	s3types "github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/stretchr/testify/assert"
 	"github.com/tilinna/clock"
 	"go.opentelemetry.io/collector/config/configcompression"
@@ -27,7 +28,7 @@ func TestNewS3Manager(t *testing.T) {
 		&PartitionKeyBuilder{},
 		s3.New(s3.Options{}),
 		"STANDARD",
-		"private",
+		WithACL(s3types.ObjectCannedACLPrivate),
 	)
 
 	assert.NotNil(t, sm, "Must have a valid client returned")
@@ -155,7 +156,7 @@ func TestS3ManagerUpload(t *testing.T) {
 					Region:       "local",
 				}),
 				"STANDARD_IA",
-				"private",
+				WithACL(s3types.ObjectCannedACLPrivate),
 			)
 
 			// Using a mocked virtual clock to fix the timestamp used

exporter/awss3exporter/metadata.yaml

@@ -6,7 +6,7 @@ status:
     alpha: [traces, metrics, logs]
   distributions: [contrib]
   codeowners:
-    active: [atoulme, pdelewski]
+    active: [atoulme, pdelewski, Erog38]
 
 tests:
   expect_consumer_error: true

exporter/awss3exporter/s3_writer.go

@@ -60,6 +60,12 @@ func newUploadManager(
 		})
 	}
 
+	var managerOpts []upload.ManagerOpt
+	if conf.S3Uploader.ACL != "" {
+		managerOpts = append(managerOpts,
+			upload.WithACL(s3types.ObjectCannedACL(conf.S3Uploader.ACL)))
+	}
+
 	return upload.NewS3Manager(
 		conf.S3Uploader.S3Bucket,
 		&upload.PartitionKeyBuilder{
@@ -72,6 +78,6 @@ func newUploadManager(
 		},
 		s3.NewFromConfig(cfg, s3Opts...),
 		s3types.StorageClass(conf.S3Uploader.StorageClass),
-		s3types.ObjectCannedACL(conf.S3Uploader.ACL),
+		managerOpts...,
 	), nil
 }