Skip to content

Conversation

JoaoBraveCoding
Copy link
Collaborator

@JoaoBraveCoding JoaoBraveCoding commented Nov 13, 2023

What this PR does / why we need it:

This PR adds support to use the thanos.io/objstore backend for the AWS provider for all components excluding the Ruler since it's similar to the work done in #11132.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

  • We tried to the best of our efforts to preserve feature parity so the new backend still uses the ObjectClient interface
  • Fix how some CLI variables we being passed (some HTTP CLI variables were under .http while others weren't)
  • Thanos.io/objstore doesn't support all the CLI flags Loki supports nowadays (see snippet below)
  • This PR was inspired by the way Mimir uses thanos.io/objstore with the goal of making this part of the codebase as similar as possible

CLI Table

s3.bucket (new, before it was buckets)
s3.endpoint
s3.region
s3.access-key-id
s3.secret-access-key
s3.session-token
s3.insecure
s3.storage-class 
s3.sse
s3.http.idle-conn-timeout
s3.http.response-header-timeout
s3.http.insecure-skip-verify
s3.http.ca-file
s3.signature-version (possible but I've deprecated it)

NOT PRESENT AT THE MOMENT
s3.url (replaced by endpoint)
s3.force-path-style (deprecated since we use endpoint)
s3.buckets
s3.http.timeout (only possible if we were to always pass full transport)
s3.min-backoff (Would need objstore patch)
s3.max-backoff (Would need objstore patch)
s3.max-retries (Would need objstore patch)

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@JoaoBraveCoding JoaoBraveCoding force-pushed the log-4550-aws branch 2 times, most recently from fd69ec7 to 4bc8466 Compare November 14, 2023 12:07
@JoaoBraveCoding JoaoBraveCoding changed the title WIP: AWS implementation using thanos.io/objstore WIP: storage: AWS backend using thanos.io/objstore Nov 14, 2023
Copy link
Contributor

github-actions bot commented Nov 20, 2023

Trivy scan found the following vulnerabilities:

@JoaoBraveCoding JoaoBraveCoding force-pushed the log-4550-aws branch 4 times, most recently from b8f05d5 to 41982d2 Compare November 21, 2023 10:09
@JoaoBraveCoding JoaoBraveCoding marked this pull request as ready for review November 23, 2023 10:48
@JoaoBraveCoding JoaoBraveCoding requested a review from a team as a code owner November 23, 2023 10:48
@JoaoBraveCoding JoaoBraveCoding changed the title WIP: storage: AWS backend using thanos.io/objstore storage: AWS backend using thanos.io/objstore Nov 23, 2023
@JoaoBraveCoding JoaoBraveCoding changed the title storage: AWS backend using thanos.io/objstore feat(storage): AWS backend using thanos.io/objstore Mar 14, 2024
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Mar 14, 2024
Copy link
Collaborator Author

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

On pkg/storage/bucket/client.go we should also uncomment lines 88 to 90.

// RegisterFlagsWithPrefix registers the flags for s3 storage with the provided prefix
func (cfg *HTTPConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
cfg.Config.RegisterFlagsWithPrefix(prefix+"s3.", f)
f.DurationVar(&cfg.IdleConnTimeout, prefix+"s3.http.idle-conn-timeout", 90*time.Second, "The time an idle connection will remain idle before closing.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f.DurationVar(&cfg.IdleConnTimeout, prefix+"s3.http.idle-conn-timeout", 90*time.Second, "The time an idle connection will remain idle before closing.")
s3prefix := prefix + "s3."
f.DurationVar(&cfg.IdleConnTimeout, s3prefix+"http.idle-conn-timeout", 90*time.Second, "The time an idle connection will remain idle before closing.")

f.IntVar(&cfg.MaxIdleConns, prefix+"s3.max-idle-connections", 100, "Maximum number of idle (keep-alive) connections across all hosts. 0 means no limit.")
f.IntVar(&cfg.MaxIdleConnsPerHost, prefix+"s3.max-idle-connections-per-host", 100, "Maximum number of idle (keep-alive) connections to keep per-host. If 0, a built-in default value is used.")
f.IntVar(&cfg.MaxConnsPerHost, prefix+"s3.max-connections-per-host", 0, "Maximum number of connections per host. 0 means no limit.")
cfg.TLSConfig.RegisterFlagsWithPrefix(prefix, f)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also pass the prefix + "s3." as prefix here.

@ashwanthgoli ashwanthgoli merged commit b872246 into grafana:main Oct 28, 2024
60 checks passed
loki-gh-app bot pushed a commit that referenced this pull request Oct 28, 2024
Co-authored-by: Ashwanth Goli <[email protected]>
(cherry picked from commit b872246)
@JoaoBraveCoding JoaoBraveCoding deleted the log-4550-aws branch November 8, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport k226 size/XL type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants