Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

S3 dualstack mode check #3721

Merged
merged 15 commits into from
Jun 19, 2024
Merged

S3 dualstack mode check #3721

merged 15 commits into from
Jun 19, 2024

Conversation

sidkram
Copy link
Contributor

@sidkram sidkram commented May 28, 2024

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
Copy link

CLAassistant commented May 28, 2024

CLA assistant check
All committers have signed the CLA.

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
Co-authored-by: Mario <mariorvinas@gmail.com>
Co-authored-by: Mario <mariorvinas@gmail.com>
Copy link
Contributor Author

@sidkram sidkram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made changes as suggested @mapno

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
docs/sources/tempo/configuration/manifest.md Outdated Show resolved Hide resolved
docs/sources/tempo/setup/linux.md Outdated Show resolved Hide resolved
docs/sources/tempo/configuration/_index.md Outdated Show resolved Hide resolved
docs/sources/tempo/configuration/_index.md Outdated Show resolved Hide resolved
@mapno
Copy link
Member

mapno commented May 28, 2024

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

sidkram and others added 5 commits May 28, 2024 19:42
Co-authored-by: Mario <mariorvinas@gmail.com>
Co-authored-by: Mario <mariorvinas@gmail.com>
Co-authored-by: Mario <mariorvinas@gmail.com>
Co-authored-by: Mario <mariorvinas@gmail.com>
@sidkram
Copy link
Contributor Author

sidkram commented May 28, 2024

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
Copy link
Member

mapno commented May 28, 2024

Run make update-mod instead, my bad.

@sidkram
Copy link
Contributor Author

sidkram commented May 29, 2024

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

@mapno
Copy link
Member

mapno commented May 29, 2024

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

tempodb/backend/s3/s3.go Outdated Show resolved Hide resolved
@rohan-changejar
Copy link

@mapno any other changes required from our end

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

opts.UseDualStack = cfg.UseDualStack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why the pipeline is failing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just need to call that function with false

@mapno
Copy link
Member

mapno commented May 29, 2024

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

@sidkram
Copy link
Contributor Author

sidkram commented May 29, 2024

@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

Comment on lines 663 to 670
clnt := &minio.Client{}

if cfg.UseDualStack {
clnt.SetS3EnableDualstack(false)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Secure: !cfg.Insecure,
Creds: creds,
Transport: transport,
DualStackEnabled: cfg.UseDualStack,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're very close now

Comment on lines 666 to 670
if cfg.UseDualStack {
clnt.SetS3EnableDualstack(true)
} else {
clnt.SetS3EnableDualstack(false)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sidkram
Copy link
Contributor Author

sidkram commented Jun 6, 2024

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

@mapno
Copy link
Member

mapno commented Jun 7, 2024

You can run make update-mod

@sidkram
Copy link
Contributor Author

sidkram commented Jun 7, 2024

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
Copy link

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

@sidkram
Copy link
Contributor Author

sidkram commented Jun 14, 2024

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

@rohan-changejar
Copy link

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

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating the documentation.

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mapno mapno merged commit 5e4eafd into grafana:main Jun 19, 2024
14 of 15 checks passed
@sidkram
Copy link
Contributor Author

sidkram commented Jun 19, 2024

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
Copy link
Member

mapno commented Jun 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a way to stop tempo from using dual stack urls for s3 backend
6 participants