S3 dualstack mode check #3721

sidkram · 2024-05-28T11:42:56Z

What this PR does: Adds a boolean param enabling (disabled by default) the use of dualstack mode on s3 which minio does by default

Which issue(s) this PR fixes:
Fixes #3720

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

CLAassistant · 2024-05-28T11:43:02Z

All committers have signed the CLA.

tempodb/backend/s3/s3.go

tempodb/backend/s3/config.go

Co-authored-by: Mario <mariorvinas@gmail.com>

sidkram

Made changes as suggested @mapno

mapno

Left a few more comments. Could you add a changelog entry as well?

Also, you'll need to sign the CLA. Thanks!

docs/sources/tempo/configuration/manifest.md

docs/sources/tempo/setup/linux.md

docs/sources/tempo/configuration/_index.md

mapno · 2024-05-28T13:46:30Z

Have you run make vendor-check? Pipeline is failing because of that.

Co-authored-by: Mario <mariorvinas@gmail.com>

sidkram · 2024-05-28T14:26:10Z

make vendor-check in my case is failing and Im unable to makeout much from the error log. I shall attach it over here, hoping you'd be able to help. Thanks @mapno
out.txt

mapno · 2024-05-28T14:28:14Z

Run make update-mod instead, my bad.

sidkram · 2024-05-29T05:34:21Z

@mapno I've made the necessary changes. Let me know if it looks good

mapno · 2024-05-29T05:43:11Z

You need to sign the CLA. It's in the first comment #3721 (comment). Thanks

tempodb/backend/s3/s3.go

rohan-changejar · 2024-05-29T07:53:02Z

@mapno any other changes required from our end

mapno · 2024-05-29T09:41:13Z

tempodb/backend/s3/s3.go

@@ -660,6 +660,8 @@ func createCore(cfg *Config, hedge bool) (*minio.Core, error) {
 		opts.BucketLookup = minio.BucketLookupType(cfg.BucketLookupType)
 	}

+	opts.UseDualStack = cfg.UseDualStack


This option does not exist. Disabling dualstack endpoints is configured via a method in the client https://github.com/minio/minio-go/pull/1945/files#diff-f7bb71ca856c07b3329265f26b1e577e00b71e4792550cf14346bdbe0e7bd18dR342-R346.

This is why the pipeline is failing

So in this case, according to this function, the minio is going to use the dualstack endpoint everytime there's an s3 url present, right ?. How can we override this ?

You just need to call that function with false

mapno · 2024-05-29T09:43:00Z

Although not mandatory in this case, a unit test would be appreciated.

sidkram · 2024-05-29T19:29:40Z

@mapno , I've made the suggested changes. Does it look fine now ?
I tried mapping the endpointURL: cfg.Endpoint when initializing the minio.Client struct, but its not able to accept the endpoint as a valid struct value. The same case when I parsed the endpoint(str) to url.URL

mapno · 2024-05-30T06:14:55Z

tempodb/backend/s3/s3.go

+	clnt := &minio.Client{}
+
+	if cfg.UseDualStack {
+		clnt.SetS3EnableDualstack(false)
+	}


This client is never returned. You need to apply the config to the client returned by this function newCore().

@mapno , I have modified the core and api logic. But make vendor-check both on local and on the workflow deletes the changes made. Can you help me here

mapno

I appreciate you might not be familiar with Go or Tempo's codebase, but please stop pushing broken commits. The contributing guide I initially linked describes how you can make sure that the build at least compiles and passes the tests.

If you don't understand the change that you need to do, please let us know and we'd be glad to help.

mapno · 2024-06-03T11:33:41Z

tempodb/backend/s3/s3.go

+		Secure:           !cfg.Insecure,
+		Creds:            creds,
+		Transport:        transport,
+		DualStackEnabled: cfg.UseDualStack,


This option doesn't exist, it's configured via a method SetS3EnableDualstack on the client (what's returned by minio.NewCore())

Sure @mapno , I appreciate your help throughout and would be glad if you're able to find time to fix this. I'll try to learn better and contribute. Thanks

mapno

We're very close now

mapno · 2024-06-06T15:06:38Z

tempodb/backend/s3/s3.go

+	if cfg.UseDualStack {
+		clnt.SetS3EnableDualstack(true)
+	} else {
+		clnt.SetS3EnableDualstack(false)
+	}


Suggested change

if cfg.UseDualStack {

clnt.SetS3EnableDualstack(true)

} else {

clnt.SetS3EnableDualstack(false)

}

clnt.SetS3EnableDualstack(cfg.UseDualStack)

sidkram · 2024-06-06T17:02:10Z

the make vendor-check still seems to fail. not sure how to get through that

mapno · 2024-06-07T09:17:31Z

You can run make update-mod

sidkram · 2024-06-07T11:36:48Z

You can run make update-mod

Yes, I did update-mod followed by a vendor-check. I assume its some sort of a warning and not any error from what I see.

make: Entering directory '/src/loki'
goyacc -o pkg/traceql/expr.y.go pkg/traceql/expr.y && rm y.output

conflicts: 1 shift/reduce
make: Leaving directory '/src/loki'
go run ./pkg/parquetquerygen/predicates.go > ./pkg/parquetquery/predicates.gen.go
git diff --exit-code -- **/go.sum **/go.mod vendor/ pkg/tempopb/ pkg/traceql/

sikriJar · 2024-06-13T13:07:48Z

I'm facing a similar problem. Hope this gets resolved soon! @mapno

sidkram · 2024-06-14T11:05:47Z

@mapno , Can you guide me on what went wrong on the last pipeline. I can quickly make further changes as required..

rohan-changejar · 2024-06-17T06:10:58Z

Hi @mapno , if this pr is not upto your expectations, I can raise a new pr as well. Please let me know

mapno

I was away last week. The PR is now close to being approvable, there is only one comment left. Also, could you add a changelog entry?

My only worry is if the update is going to mess with S3 backends, now that the expiration for the credentials is being honoured. I suppose that's the intended behaviour, so that's OK.

docs/sources/tempo/configuration/_index.md

knylander-grafana

Thank you for updating the documentation.

mapno

LGTM

sidkram · 2024-06-19T10:24:56Z

Thanks @mapno, @knylander-grafana for the approval and guidance. One more thing, I don't see a way to draft a release (as mentioned on the releases.md), so I assume its done from the codeowners side. Is there a time bound on when can we expect a new release as this alleviates a huge cost infliction on our systems?

mapno · 2024-06-19T10:56:33Z

Is there a time bound on when can we expect a new release as this alleviates a huge cost infliction on our systems?

We aim to do a release roughly once every quarter (3 months). We released v2.5 less then a month ago, so I wouldn't expect to have v2.6 before a couple of months.

In the meantime, we publish builds for all commits in main in DockerHub.

sidkram added 2 commits May 28, 2024 17:02

added files to handle dualstack mode for minio s3 handler

b31671d

added changes on api.go

bfbef6c

sidkram requested review from knylander-grafana, joe-elliott, annanay25, mdisibio, mapno, kvrhdn, zalegrala, electron0zero, ie-pham and stoewer as code owners May 28, 2024 11:42

mapno requested changes May 28, 2024

View reviewed changes

tempodb/backend/s3/s3.go Outdated Show resolved Hide resolved

tempodb/backend/s3/config.go Outdated Show resolved Hide resolved

sidkram and others added 2 commits May 28, 2024 18:58

Update tempodb/backend/s3/s3.go

ecc78c7

Co-authored-by: Mario <mariorvinas@gmail.com>

Update tempodb/backend/s3/config.go

6886d74

Co-authored-by: Mario <mariorvinas@gmail.com>

sidkram commented May 28, 2024

View reviewed changes

mapno requested changes May 28, 2024

View reviewed changes

sidkram and others added 5 commits May 28, 2024 19:42

Update docs/sources/tempo/configuration/manifest.md

c23ce81

Co-authored-by: Mario <mariorvinas@gmail.com>

Update docs/sources/tempo/setup/linux.md

670c90d

Co-authored-by: Mario <mariorvinas@gmail.com>

Update docs/sources/tempo/configuration/manifest.md

e2851f0

Co-authored-by: Mario <mariorvinas@gmail.com>

Update docs/sources/tempo/configuration/_index.md

7d39ec1

Co-authored-by: Mario <mariorvinas@gmail.com>

added docs for dualstack

7035066

mapno reviewed May 29, 2024

View reviewed changes

tempodb/backend/s3/s3.go Outdated Show resolved Hide resolved

mapno reviewed May 29, 2024

View reviewed changes

mapno reviewed May 30, 2024

View reviewed changes

mapno requested changes Jun 3, 2024

View reviewed changes

mapno mentioned this pull request Jun 6, 2024

Need a way to stop tempo from using dual stack urls for s3 backend #3720

Closed

final commit

9de2677

sidkram force-pushed the s3-dualstack-mode-check branch from aa659dc to 9de2677 Compare June 6, 2024 14:50

mapno requested changes Jun 6, 2024

View reviewed changes

initiate client object

f64b162

Fix IAM test

4032a7f

mapno reviewed Jun 18, 2024

View reviewed changes

docs/sources/tempo/configuration/_index.md Outdated Show resolved Hide resolved

Added changelog entry

fa14594

knylander-grafana reviewed Jun 18, 2024

View reviewed changes

docs/sources/tempo/configuration/_index.md Outdated Show resolved Hide resolved

Update docs/sources/tempo/configuration/_index.md

b5e221d

knylander-grafana reviewed Jun 18, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into s3-dualstack-mode-check

8900ad9

mapno approved these changes Jun 19, 2024

View reviewed changes

mapno merged commit 5e4eafd into grafana:main Jun 19, 2024
14 of 15 checks passed

joe-elliott mentioned this pull request Jul 23, 2024

Support EKS Pod Identity feature #3899

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 dualstack mode check #3721

S3 dualstack mode check #3721

sidkram commented May 28, 2024 •

edited by mapno

Loading

CLAassistant commented May 28, 2024 •

edited

Loading

sidkram left a comment •

edited

Loading

mapno left a comment

mapno commented May 28, 2024

sidkram commented May 28, 2024

mapno commented May 28, 2024

sidkram commented May 29, 2024

mapno commented May 29, 2024

rohan-changejar commented May 29, 2024

mapno May 29, 2024

mapno May 29, 2024

sidkram May 29, 2024

mapno May 29, 2024

mapno commented May 29, 2024

sidkram commented May 29, 2024 •

edited

Loading

mapno May 30, 2024

sidkram Jun 3, 2024

mapno left a comment •

edited

Loading

mapno Jun 3, 2024

sidkram Jun 3, 2024

mapno left a comment

mapno Jun 6, 2024

sidkram commented Jun 6, 2024

mapno commented Jun 7, 2024

sidkram commented Jun 7, 2024 •

edited

Loading

sikriJar commented Jun 13, 2024

sidkram commented Jun 14, 2024

rohan-changejar commented Jun 17, 2024

mapno left a comment

knylander-grafana left a comment

mapno left a comment

sidkram commented Jun 19, 2024 •

edited

Loading

mapno commented Jun 19, 2024

S3 dualstack mode check #3721

S3 dualstack mode check #3721

Conversation

sidkram commented May 28, 2024 • edited by mapno Loading

CLAassistant commented May 28, 2024 • edited Loading

sidkram left a comment • edited Loading

Choose a reason for hiding this comment

mapno left a comment

Choose a reason for hiding this comment

mapno commented May 28, 2024

sidkram commented May 28, 2024

mapno commented May 28, 2024

sidkram commented May 29, 2024

mapno commented May 29, 2024

rohan-changejar commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapno commented May 29, 2024

sidkram commented May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapno left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidkram commented Jun 6, 2024

mapno commented Jun 7, 2024

sidkram commented Jun 7, 2024 • edited Loading

sikriJar commented Jun 13, 2024

sidkram commented Jun 14, 2024

rohan-changejar commented Jun 17, 2024

mapno left a comment

Choose a reason for hiding this comment

knylander-grafana left a comment

Choose a reason for hiding this comment

mapno left a comment

Choose a reason for hiding this comment

sidkram commented Jun 19, 2024 • edited Loading

mapno commented Jun 19, 2024

sidkram commented May 28, 2024 •

edited by mapno

Loading

CLAassistant commented May 28, 2024 •

edited

Loading

sidkram left a comment •

edited

Loading

sidkram commented May 29, 2024 •

edited

Loading

mapno left a comment •

edited

Loading

sidkram commented Jun 7, 2024 •

edited

Loading

sidkram commented Jun 19, 2024 •

edited

Loading