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

feat: Add objstore support for Swift using thanos.io/objstore #11672

Merged
merged 35 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
0a946a2
add TLS support for swift
btaani Jan 12, 2024
88df54d
fix typo
btaani Jan 12, 2024
5f821dd
fix error from unit test
btaani Jan 12, 2024
749f64b
Merge branch 'main' into swift-tls
btaani Jan 12, 2024
f5bcc9c
Merge branch 'main' into swift-tls
btaani Jan 16, 2024
06ef15b
Merge branch 'main' into swift-tls
btaani Jan 18, 2024
fb562c4
remove condition on transport type
btaani Jan 18, 2024
b1b203b
remove duplicate initialization
btaani Jan 18, 2024
7b23e51
modify swift config in object client
btaani Feb 5, 2024
d5ad3e7
use bucket_http config in swift bucket client
btaani Feb 5, 2024
6b70293
Merge branch 'main' into swift-tls
btaani Feb 5, 2024
6a266e0
add swift thanos object client implementation
btaani Feb 9, 2024
fd98f34
undo docs change
btaani Feb 13, 2024
959aa77
change all objectClient implementations
btaani Feb 13, 2024
05acfd9
add hedged client to swift thanos client
btaani Feb 13, 2024
ce0c7e6
add hedged client to swift thanos client
btaani Feb 13, 2024
3659f19
Merge branch 'main' into swift-tls
btaani Feb 14, 2024
7fe24b8
update thanos-io/objstore
btaani Feb 14, 2024
53fd4ee
add TLS support
btaani Feb 14, 2024
b02afa8
Merge branch 'main' into swift-tls
btaani Feb 15, 2024
0c6526e
Merge branch 'main' into swift-tls
btaani Mar 4, 2024
5a511b9
Merge branch 'main' into swift-tls
JoaoBraveCoding Oct 30, 2024
bc2eeb8
chore: Move Swift client to new ObjectClientAdapter
JoaoBraveCoding Oct 30, 2024
e64f09e
docs: fixed docs
JoaoBraveCoding Oct 30, 2024
11970b9
Merge branch 'main' into swift-tls
JoaoBraveCoding Oct 30, 2024
380c0b8
fix: fix tests
JoaoBraveCoding Oct 30, 2024
61c50f3
address review comments
JoaoBraveCoding Nov 11, 2024
776c5c4
make s3 use http config
JoaoBraveCoding Nov 11, 2024
99a0cf3
Merge branch 'main' into swift-tls
JoaoBraveCoding Nov 15, 2024
32f92fe
address review comments and fix merge result
JoaoBraveCoding Nov 15, 2024
ad09648
Duplicate Swift config to old client
JoaoBraveCoding Nov 15, 2024
bc652bd
Merge branch 'main' into swift-tls
ashwanthgoli Nov 25, 2024
340b953
fix build
ashwanthgoli Nov 25, 2024
fb3f953
make format
ashwanthgoli Nov 25, 2024
dd26273
ensure cli flags are same as existing ones
ashwanthgoli Nov 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/sources/configure/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -5150,6 +5150,16 @@ The `swift_storage_config` block configures the connection to OpenStack Object S
# is received on a request.
# CLI flag: -<prefix>.swift.request-timeout
[request_timeout: <duration> | default = 5s]

# Set to false to skip verifying the certificate chain and hostname.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you edit this file directly? Or is this change automatically generated? The configuration reference should be updated in docs/sources/configure/index.template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did so directly.. thanks for pointing this out!

# Set to true to skip verifying the certificate chain and hostname.
# CLI flag: -<prefix>.swift.insecure-skip-verify
[insecure_skip_verify: <boolean> | default = false]

# Path to the trusted CA file that signed the SSL certificate of the swift
# endpoint.
# CLI flag: -<prefix>.swift.ca-file
[ca_file: <string> | default = ""]
```

### cos_storage_config
Expand Down
15 changes: 15 additions & 0 deletions pkg/storage/bucket/swift/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@ type Config struct {
MaxRetries int `yaml:"max_retries"`
ConnectTimeout time.Duration `yaml:"connect_timeout"`
RequestTimeout time.Duration `yaml:"request_timeout"`
HTTPConfig HTTPConfig `yaml:"http_config"`
}

// HTTPConfig stores the http.Transport configuration
type HTTPConfig struct {
Timeout time.Duration `yaml:"timeout"`
IdleConnTimeout time.Duration `yaml:"idle_conn_timeout"`
ResponseHeaderTimeout time.Duration `yaml:"response_header_timeout"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

These values are define here but I don't see anywhere they are actually being used to configure the underlying transport?

Copy link
Contributor Author

@btaani btaani Feb 5, 2024

Choose a reason for hiding this comment

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

Thanks, you're right I had assumed that this was the struct being used inside the chunks package. I've moved it there already.

Following @periklis 's advice to reuse the bucket_http config inside the swift bucket config, I've noticed that the swift config used is coming from thanos-io/objstore, which doesn't have an HTTP field built into it (in contrary to s3's thanos/objstore config struct which does have the HTTP field).
This limits the addition of the HTTPConfig.Transport field to the swift config.

So I need your advice here since I lack experience in this codebase, do you suggest that we wait until it's added to thanos-io/objstore's swift config? Or should we proceed with this PR only with the object client, while leaving the bucket client until either a restructure of the config is done, or till the correct fields are added to the thanos/objstore config?
Edit: after discussing with @periklis, we agreed to 1) add the required HTTP config in thanos-io/objstore and 2) use the thanos-io/objstore config in both the bucket and object client, for the sake of unifying all storage clients.

InsecureSkipVerify bool `yaml:"insecure_skip_verify"`
CAFile string `yaml:"ca_file"`
}

// RegisterFlags registers the flags for Swift storage
Expand Down Expand Up @@ -54,6 +64,11 @@ func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
f.IntVar(&cfg.MaxRetries, prefix+"swift.max-retries", 3, "Max retries on requests error.")
f.DurationVar(&cfg.ConnectTimeout, prefix+"swift.connect-timeout", 10*time.Second, "Time after which a connection attempt is aborted.")
f.DurationVar(&cfg.RequestTimeout, prefix+"swift.request-timeout", 5*time.Second, "Time after which an idle request is aborted. The timeout watchdog is reset each time some data is received, so the timeout triggers after X time no data is received on a request.")
f.DurationVar(&cfg.HTTPConfig.IdleConnTimeout, prefix+"swift.http.idle-conn-timeout", 90*time.Second, "The maximum amount of time an idle connection will be held open.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be a change from the currently used default value of 0 for unlimited, I think as we are trying to get this change into a patch fix release it would be better to keep the current default value.

Also as mentioned above I don't see where this value actually configures the underlying transport

Copy link
Collaborator

Choose a reason for hiding this comment

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

@slim-bean AFAICS, GCS, Azure and S3 all define as default 90sec. Maybe you refer to some different place?

f.DurationVar(&cfg.HTTPConfig.Timeout, prefix+"swift.http.timeout", 0, "Timeout specifies a time limit for requests made by swift Client.")
f.DurationVar(&cfg.HTTPConfig.ResponseHeaderTimeout, prefix+"swift.http.response-header-timeout", 0, "If non-zero, specifies the amount of time to wait for a server's response headers after fully writing the request.")
f.BoolVar(&cfg.HTTPConfig.InsecureSkipVerify, prefix+"swift.http.insecure-skip-verify", false, "Set to true to skip verifying the certificate chain and hostname.")
f.StringVar(&cfg.HTTPConfig.CAFile, prefix+"swift.http.ca-file", "", "Path to the trusted CA file that signed the SSL certificate of the swift endpoint.")
}

func (cfg *Config) Validate() error {
Expand Down
22 changes: 20 additions & 2 deletions pkg/storage/chunk/client/openstack/swift_object_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@ package openstack
import (
"bytes"
"context"
"crypto/tls"
"crypto/x509"
"flag"
"fmt"
"io"
"net/http"
"os"
"time"

"github.com/ncw/swift"
Expand Down Expand Up @@ -76,7 +79,20 @@ func NewSwiftObjectClient(cfg SwiftConfig, hedgingCfg hedging.Config) (*SwiftObj
}

func createConnection(cfg SwiftConfig, hedgingCfg hedging.Config, hedging bool) (*swift.Connection, error) {
// Create a connection
tlsConfig := &tls.Config{
InsecureSkipVerify: cfg.HTTPConfig.InsecureSkipVerify,
}
if cfg.HTTPConfig.CAFile != "" {
tlsConfig.RootCAs = x509.NewCertPool()
data, err := os.ReadFile(cfg.HTTPConfig.CAFile)
if err != nil {
return nil, err
}
tlsConfig.RootCAs.AppendCertsFromPEM(data)
}

newTransport := defaultTransport.(*http.Transport)
newTransport.TLSClientConfig = tlsConfig
c := &swift.Connection{
AuthVersion: cfg.AuthVersion,
AuthUrl: cfg.AuthURL,
Expand All @@ -94,9 +110,11 @@ func createConnection(cfg SwiftConfig, hedgingCfg hedging.Config, hedging bool)
Domain: cfg.DomainName,
DomainId: cfg.DomainID,
Region: cfg.RegionName,
Transport: defaultTransport,
Transport: newTransport,
}

// Create a connection

switch {
JoaoBraveCoding marked this conversation as resolved.
Show resolved Hide resolved
case cfg.UserDomainName != "":
c.Domain = cfg.UserDomainName
Expand Down
Loading