From 138679459d255abab6d965275366bc2c1fde0cff Mon Sep 17 00:00:00 2001 From: daboucha Date: Thu, 2 May 2024 10:10:59 +0200 Subject: [PATCH 01/11] fix(s3exporter/config-validation): draft: endpoint should contain region and S3 bucket --- exporter/awss3exporter/config.go | 7 +++++-- exporter/awss3exporter/config_test.go | 26 ++++++++++++++++++-------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/exporter/awss3exporter/config.go b/exporter/awss3exporter/config.go index 276d73f357b1..06e7c2b8fcac 100644 --- a/exporter/awss3exporter/config.go +++ b/exporter/awss3exporter/config.go @@ -47,12 +47,15 @@ type Config struct { func (c *Config) Validate() error { var errs error - if c.S3Uploader.Region == "" { + if c.S3Uploader.Region == "" && c.S3Uploader.Endpoint == "" { errs = multierr.Append(errs, errors.New("region is required")) } - if c.S3Uploader.S3Bucket == "" { + if c.S3Uploader.S3Bucket == "" && c.S3Uploader.Endpoint == "" { errs = multierr.Append(errs, errors.New("bucket is required")) } + if c.S3Uploader.Endpoint == "" && (c.S3Uploader.S3Bucket == "" && c.S3Uploader.Region == "") { + errs = multierr.Append(errs, errors.New("endpoint is required")) + } compression := c.S3Uploader.Compression if compression.IsCompressed() { if compression != configcompression.TypeGzip { diff --git a/exporter/awss3exporter/config_test.go b/exporter/awss3exporter/config_test.go index 8638aa27076b..41d6ce8ece24 100644 --- a/exporter/awss3exporter/config_test.go +++ b/exporter/awss3exporter/config_test.go @@ -12,7 +12,6 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/otelcol/otelcoltest" - "go.uber.org/multierr" "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awss3exporter/internal/metadata" ) @@ -108,13 +107,24 @@ func TestConfig_Validate(t *testing.T) { config *Config errExpected error }{ + // Valid config with endpoint provided + { + // endpoint should contain region and bucket name + // https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html + name: "valid with endpoint only", + config: func() *Config { + c := createDefaultConfig().(*Config) + c.S3Uploader.Endpoint = "http://example.com" + return c + }(), + errExpected: nil, + }, { name: "valid", config: func() *Config { c := createDefaultConfig().(*Config) c.S3Uploader.Region = "foo" c.S3Uploader.S3Bucket = "bar" - c.S3Uploader.Endpoint = "http://example.com" return c }(), errExpected: nil, @@ -124,26 +134,26 @@ func TestConfig_Validate(t *testing.T) { config: func() *Config { c := createDefaultConfig().(*Config) c.S3Uploader.Region = "" + c.S3Uploader.S3Bucket = "" + c.S3Uploader.Endpoint = "" return c }(), - errExpected: multierr.Append(errors.New("region is required"), - errors.New("bucket is required")), + errExpected: errors.New("endpoint should be provided, or region and bucket"), }, { - name: "endpoint and region", + name: "region only", config: func() *Config { c := createDefaultConfig().(*Config) - c.S3Uploader.Endpoint = "http://example.com" c.S3Uploader.Region = "foo" + c.S3Uploader.S3Bucket = "" return c }(), errExpected: errors.New("bucket is required"), }, { - name: "endpoint and bucket", + name: "bucket only", config: func() *Config { c := createDefaultConfig().(*Config) - c.S3Uploader.Endpoint = "http://example.com" c.S3Uploader.S3Bucket = "foo" c.S3Uploader.Region = "" return c From 853d91611baa3f8190ef00b9ba1f6aec67196db4 Mon Sep 17 00:00:00 2001 From: daboucha Date: Thu, 2 May 2024 14:21:20 +0200 Subject: [PATCH 02/11] fix(region): required, even when endpoint is provided --- exporter/awss3exporter/config.go | 4 ++-- exporter/awss3exporter/config_test.go | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/exporter/awss3exporter/config.go b/exporter/awss3exporter/config.go index 06e7c2b8fcac..c8dc5e8b196b 100644 --- a/exporter/awss3exporter/config.go +++ b/exporter/awss3exporter/config.go @@ -47,14 +47,14 @@ type Config struct { func (c *Config) Validate() error { var errs error - if c.S3Uploader.Region == "" && c.S3Uploader.Endpoint == "" { + if c.S3Uploader.Region == "" { errs = multierr.Append(errs, errors.New("region is required")) } if c.S3Uploader.S3Bucket == "" && c.S3Uploader.Endpoint == "" { errs = multierr.Append(errs, errors.New("bucket is required")) } if c.S3Uploader.Endpoint == "" && (c.S3Uploader.S3Bucket == "" && c.S3Uploader.Region == "") { - errs = multierr.Append(errs, errors.New("endpoint is required")) + errs = multierr.Append(errs, errors.New("endpoint is required, or region and bucket")) } compression := c.S3Uploader.Compression if compression.IsCompressed() { diff --git a/exporter/awss3exporter/config_test.go b/exporter/awss3exporter/config_test.go index 41d6ce8ece24..5d7f057f9c54 100644 --- a/exporter/awss3exporter/config_test.go +++ b/exporter/awss3exporter/config_test.go @@ -111,16 +111,17 @@ func TestConfig_Validate(t *testing.T) { { // endpoint should contain region and bucket name // https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html - name: "valid with endpoint only", + name: "valid with endpoint and region", config: func() *Config { c := createDefaultConfig().(*Config) c.S3Uploader.Endpoint = "http://example.com" + c.S3Uploader.Region = "foo" return c }(), errExpected: nil, }, { - name: "valid", + name: "valid with S3Bucket and region", config: func() *Config { c := createDefaultConfig().(*Config) c.S3Uploader.Region = "foo" @@ -138,7 +139,7 @@ func TestConfig_Validate(t *testing.T) { c.S3Uploader.Endpoint = "" return c }(), - errExpected: errors.New("endpoint should be provided, or region and bucket"), + errExpected: errors.New("endpoint and region should be provided, or bucket amd region"), }, { name: "region only", @@ -160,6 +161,16 @@ func TestConfig_Validate(t *testing.T) { }(), errExpected: errors.New("region is required"), }, + { + name: "endpoint only", + config: func() *Config { + c := createDefaultConfig().(*Config) + c.S3Uploader.Endpoint = "http://example.com" + c.S3Uploader.Region = "" + return c + }(), + errExpected: errors.New("region is required"), + }, } for _, tt := range tests { From b29567ca38c68e26b14f95d0bc2c766c57ac04a0 Mon Sep 17 00:00:00 2001 From: daboucha Date: Thu, 2 May 2024 14:24:06 +0200 Subject: [PATCH 03/11] fix(message): edit error message --- exporter/awss3exporter/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/awss3exporter/config.go b/exporter/awss3exporter/config.go index c8dc5e8b196b..51214453ed2c 100644 --- a/exporter/awss3exporter/config.go +++ b/exporter/awss3exporter/config.go @@ -54,7 +54,7 @@ func (c *Config) Validate() error { errs = multierr.Append(errs, errors.New("bucket is required")) } if c.S3Uploader.Endpoint == "" && (c.S3Uploader.S3Bucket == "" && c.S3Uploader.Region == "") { - errs = multierr.Append(errs, errors.New("endpoint is required, or region and bucket")) + errs = multierr.Append(errs, errors.New("endpoint and region are required, or bucket and region")) } compression := c.S3Uploader.Compression if compression.IsCompressed() { From c79f3750ad9639a56b8f77df2fa1ec57b16c2370 Mon Sep 17 00:00:00 2001 From: daboucha Date: Thu, 2 May 2024 14:26:05 +0200 Subject: [PATCH 04/11] fix(condition) --- exporter/awss3exporter/config.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/exporter/awss3exporter/config.go b/exporter/awss3exporter/config.go index 51214453ed2c..87ef597bab52 100644 --- a/exporter/awss3exporter/config.go +++ b/exporter/awss3exporter/config.go @@ -53,9 +53,6 @@ func (c *Config) Validate() error { if c.S3Uploader.S3Bucket == "" && c.S3Uploader.Endpoint == "" { errs = multierr.Append(errs, errors.New("bucket is required")) } - if c.S3Uploader.Endpoint == "" && (c.S3Uploader.S3Bucket == "" && c.S3Uploader.Region == "") { - errs = multierr.Append(errs, errors.New("endpoint and region are required, or bucket and region")) - } compression := c.S3Uploader.Compression if compression.IsCompressed() { if compression != configcompression.TypeGzip { From 5a23120ccee51aaa96f025c667d2d2bb7c8498fd Mon Sep 17 00:00:00 2001 From: daboucha Date: Fri, 3 May 2024 10:55:09 +0200 Subject: [PATCH 05/11] fix(tests): review feedback --- exporter/awss3exporter/config_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/exporter/awss3exporter/config_test.go b/exporter/awss3exporter/config_test.go index 5d7f057f9c54..63a9a2d13894 100644 --- a/exporter/awss3exporter/config_test.go +++ b/exporter/awss3exporter/config_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/otelcol/otelcoltest" + "go.uber.org/multierr" "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awss3exporter/internal/metadata" ) @@ -107,10 +108,8 @@ func TestConfig_Validate(t *testing.T) { config *Config errExpected error }{ - // Valid config with endpoint provided { - // endpoint should contain region and bucket name - // https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html + // endpoint overrides bucket name. name: "valid with endpoint and region", config: func() *Config { c := createDefaultConfig().(*Config) @@ -121,6 +120,8 @@ func TestConfig_Validate(t *testing.T) { errExpected: nil, }, { + // Endpoint will be built from bucket and region. + // https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html name: "valid with S3Bucket and region", config: func() *Config { c := createDefaultConfig().(*Config) @@ -139,7 +140,8 @@ func TestConfig_Validate(t *testing.T) { c.S3Uploader.Endpoint = "" return c }(), - errExpected: errors.New("endpoint and region should be provided, or bucket amd region"), + errExpected: multierr.Append(errors.New("region is required"), + errors.New("bucket or endpoint is required")), }, { name: "region only", From 3d95e5a162a2732f766923a6d977a6ba5c3f621e Mon Sep 17 00:00:00 2001 From: daboucha Date: Fri, 3 May 2024 13:42:58 +0200 Subject: [PATCH 06/11] documentation edit + fix comment --- exporter/awss3exporter/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/awss3exporter/README.md b/exporter/awss3exporter/README.md index ba2f1b8cbf92..94da6a20dfcb 100644 --- a/exporter/awss3exporter/README.md +++ b/exporter/awss3exporter/README.md @@ -30,7 +30,7 @@ The following exporter configuration parameters are supported. | `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` | overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket` | | +| `endpoint` | overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket`. region still needs to be provided. | | | `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 | From 7b60ae7e027de11b78c43d49db528949adb9ecba Mon Sep 17 00:00:00 2001 From: daboucha Date: Fri, 3 May 2024 14:02:24 +0200 Subject: [PATCH 07/11] docs(readme): minor edits --- exporter/awss3exporter/README.md | 2 +- exporter/awss3exporter/config_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/exporter/awss3exporter/README.md b/exporter/awss3exporter/README.md index 94da6a20dfcb..7d86650403a9 100644 --- a/exporter/awss3exporter/README.md +++ b/exporter/awss3exporter/README.md @@ -30,7 +30,7 @@ The following exporter configuration parameters are supported. | `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` | overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket`. region still needs to be provided. | | +| `endpoint` | overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket`. Overrides `region` and `s3_bucket` when provided. | | | `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 | diff --git a/exporter/awss3exporter/config_test.go b/exporter/awss3exporter/config_test.go index 63a9a2d13894..7be8626b261a 100644 --- a/exporter/awss3exporter/config_test.go +++ b/exporter/awss3exporter/config_test.go @@ -109,7 +109,7 @@ func TestConfig_Validate(t *testing.T) { errExpected error }{ { - // endpoint overrides bucket name. + // endpoint overrides region and bucket name. name: "valid with endpoint and region", config: func() *Config { c := createDefaultConfig().(*Config) From fa5ca3fa494fe279609751338da2da21ae98a60e Mon Sep 17 00:00:00 2001 From: daboucha Date: Tue, 7 May 2024 17:59:49 +0200 Subject: [PATCH 08/11] docs(readme): clarify endpoint --- exporter/awss3exporter/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/awss3exporter/README.md b/exporter/awss3exporter/README.md index 7d86650403a9..6282b428538f 100644 --- a/exporter/awss3exporter/README.md +++ b/exporter/awss3exporter/README.md @@ -30,7 +30,7 @@ The following exporter configuration parameters are supported. | `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` | overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket`. Overrides `region` and `s3_bucket` when provided. | | +| `endpoint` | (REST API endpoint) overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket` | | | `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 | From 362a623825387ee92f13990fd524a4356f93b49e Mon Sep 17 00:00:00 2001 From: daboucha Date: Mon, 13 May 2024 10:26:51 +0200 Subject: [PATCH 09/11] docs(changelog): add entry --- .chloggen/awss3exporter_config_endpoint.yaml | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 .chloggen/awss3exporter_config_endpoint.yaml diff --git a/.chloggen/awss3exporter_config_endpoint.yaml b/.chloggen/awss3exporter_config_endpoint.yaml new file mode 100644 index 000000000000..da2156d92b99 --- /dev/null +++ b/.chloggen/awss3exporter_config_endpoint.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: enhancement + +# 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: endpoint should contain the S3 bucket + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [32774] + +# (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: + +# 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: [] From b3f2f61e1cb4860cddae9462fc7e6c813cbb6b5c Mon Sep 17 00:00:00 2001 From: daboucha Date: Tue, 11 Jun 2024 16:56:21 +0200 Subject: [PATCH 10/11] fix(test): reflect the error message in the config --- exporter/awss3exporter/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/awss3exporter/config.go b/exporter/awss3exporter/config.go index 87ef597bab52..0db41271ff09 100644 --- a/exporter/awss3exporter/config.go +++ b/exporter/awss3exporter/config.go @@ -51,7 +51,7 @@ func (c *Config) Validate() error { errs = multierr.Append(errs, errors.New("region is required")) } if c.S3Uploader.S3Bucket == "" && c.S3Uploader.Endpoint == "" { - errs = multierr.Append(errs, errors.New("bucket is required")) + errs = multierr.Append(errs, errors.New("bucket or endpoint is required")) } compression := c.S3Uploader.Compression if compression.IsCompressed() { From e7c0a6acdbb0fa8c19c8244a7ef3ee40a4d47599 Mon Sep 17 00:00:00 2001 From: daboucha Date: Tue, 11 Jun 2024 16:58:52 +0200 Subject: [PATCH 11/11] fix(test): reflect the error message in the test too --- exporter/awss3exporter/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/awss3exporter/config_test.go b/exporter/awss3exporter/config_test.go index 7be8626b261a..f9c6011ea7b4 100644 --- a/exporter/awss3exporter/config_test.go +++ b/exporter/awss3exporter/config_test.go @@ -151,7 +151,7 @@ func TestConfig_Validate(t *testing.T) { c.S3Uploader.S3Bucket = "" return c }(), - errExpected: errors.New("bucket is required"), + errExpected: errors.New("bucket or endpoint is required"), }, { name: "bucket only",