Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
108 changes: 82 additions & 26 deletions cli/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,21 @@ const (
hostSelectTypeWeighed hostSelectType = "weighed"
)

// hostPair holds a resolved host and the original hostname it was resolved from.
// originalHost is "" when --resolve-host is not used (no pinning needed).
type hostPair struct {
resolved string // IP:port to dial
originalHost string // original hostname (for S3 signing + SNI)
}

func newClient(ctx *cli.Context) func() (cl *minio.Client, done func()) {
hosts := parseHosts(ctx.String("host"), ctx.Bool("resolve-host"))
switch len(hosts) {
pairs := parseHostPairs(ctx.String("host"), ctx.Bool("resolve-host"))

switch len(pairs) {
case 0:
fatalIf(probe.NewError(errors.New("no host defined")), "Unable to create MinIO client")
case 1:
cl, err := getClient(ctx, hosts[0])
cl, err := getClient(ctx, pairs[0].resolved, pairs[0].originalHost)
fatalIf(probe.NewError(err), "Unable to create MinIO client")

return func() (*minio.Client, func()) {
Expand All @@ -70,9 +78,9 @@ func newClient(ctx *cli.Context) func() (cl *minio.Client, done func()) {
// Do round-robin.
var current int
var mu sync.Mutex
clients := make([]*minio.Client, len(hosts))
for i := range hosts {
cl, err := getClient(ctx, hosts[i])
clients := make([]*minio.Client, len(pairs))
for i := range pairs {
cl, err := getClient(ctx, pairs[i].resolved, pairs[i].originalHost)
fatalIf(probe.NewError(err), "Unable to create MinIO client")
clients[i] = cl
}
Expand All @@ -87,20 +95,20 @@ func newClient(ctx *cli.Context) func() (cl *minio.Client, done func()) {
// Keep track of handed out clients.
// Select random between the clients that have the fewest handed out.
var mu sync.Mutex
clients := make([]*minio.Client, len(hosts))
for i := range hosts {
cl, err := getClient(ctx, hosts[i])
clients := make([]*minio.Client, len(pairs))
for i := range pairs {
cl, err := getClient(ctx, pairs[i].resolved, pairs[i].originalHost)
fatalIf(probe.NewError(err), "Unable to create MinIO client")
clients[i] = cl
}
running := make([]int, len(hosts))
lastFinished := make([]time.Time, len(hosts))
running := make([]int, len(pairs))
lastFinished := make([]time.Time, len(pairs))
{
// Start with a random host
now := time.Now()
off := rand.New(rand.NewSource(time.Now().UnixNano())).Intn(len(hosts))
off := rand.New(rand.NewSource(time.Now().UnixNano())).Intn(len(pairs))
for i := range lastFinished {
lastFinished[i] = now.Add(time.Duration(i + off%len(hosts)))
lastFinished[i] = now.Add(time.Duration(i + off%len(pairs)))
}
Comment on lines +109 to 112

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Random start offset is not actually randomizing the initial host.

On Line 111, i + off%len(pairs) adds the same constant shift to all entries, so relative order is unchanged and index 0 still starts earliest.

Suggested fix
 		{
 			// Start with a random host
 			now := time.Now()
 			off := rand.New(rand.NewSource(time.Now().UnixNano())).Intn(len(pairs))
 			for i := range lastFinished {
-				lastFinished[i] = now.Add(time.Duration(i + off%len(pairs)))
+				lastFinished[(i+off)%len(pairs)] = now.Add(time.Duration(i))
 			}
 		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
off := rand.New(rand.NewSource(time.Now().UnixNano())).Intn(len(pairs))
for i := range lastFinished {
lastFinished[i] = now.Add(time.Duration(i + off%len(hosts)))
lastFinished[i] = now.Add(time.Duration(i + off%len(pairs)))
}
off := rand.New(rand.NewSource(time.Now().UnixNano())).Intn(len(pairs))
for i := range lastFinished {
lastFinished[(i+off)%len(pairs)] = now.Add(time.Duration(i))
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/client.go` around lines 109 - 112, The current offset calculation uses i
+ off%len(pairs) which applies the same constant shift to every entry; change it
to compute a rotated index using (i + off) % len(pairs) so each entry is shifted
relative to others (e.g., compute idx := (i+off)%len(pairs) and use idx when
setting lastFinished[i] = now.Add(time.Duration(idx))). Update the code that
sets lastFinished to use this rotated index (variables: off, lastFinished,
pairs, now).

}
find := func() int {
Expand Down Expand Up @@ -159,13 +167,19 @@ func detectLocalIP(host string) string {
}

// getClient creates a client with the specified host and the options set in the context.
func getClient(ctx *cli.Context, host string) (*minio.Client, error) {
// host is the resolved IP:port to dial; originalHost is the logical hostname for S3 signing
// and SNI (empty when --resolve-host is not used).
func getClient(ctx *cli.Context, host, originalHost string) (*minio.Client, error) {
var creds *credentials.Credentials
localIP := clientListenIP
if localIP == "" {
localIP = detectLocalIP(host)
}
transport := clientTransportWithLocalIP(ctx, localIP)
endpoint := host
if originalHost != "" {
endpoint = originalHost
}
transport := clientTransportWithLocalIP(ctx, localIP, host, originalHost)
switch strings.ToUpper(ctx.String("signature")) {
case "S3V4":
// if Signature version '4' use NewV4 directly.
Expand All @@ -189,7 +203,7 @@ func getClient(ctx *cli.Context, host string) (*minio.Client, error) {
if ctx.Bool("tls") || ctx.Bool("ktls") {
proto = "https"
}
stsEndPoint := fmt.Sprintf("%s://%s", proto, host)
stsEndPoint := fmt.Sprintf("%s://%s", proto, endpoint)
creds, err = credentials.NewSTSWebIdentity(stsEndPoint, func() (*credentials.WebIdentityToken, error) {
stsToken := ctx.String("sts-web-token")
if stsTokenFile, hasFilePrefix := strings.CutPrefix(stsToken, "file:"); hasFilePrefix {
Expand All @@ -214,7 +228,7 @@ func getClient(ctx *cli.Context, host string) (*minio.Client, error) {
} else if ctx.String("lookup") == "path" {
lookup = minio.BucketLookupPath
}
cl, err := minio.New(host, &minio.Options{
cl, err := minio.New(endpoint, &minio.Options{
Creds: creds,
Secure: ctx.Bool("tls") || ctx.Bool("ktls"),
Region: ctx.String("region"),
Expand All @@ -236,19 +250,21 @@ func getClient(ctx *cli.Context, host string) (*minio.Client, error) {
}

func clientTransport(ctx *cli.Context) http.RoundTripper {
return clientTransportWithLocalIP(ctx, "")
return clientTransportWithLocalIP(ctx, "", "", "")
}

// clientTransportWithLocalIP creates a transport that binds outbound connections
// to localIP (empty string means no binding, OS picks the source address).
func clientTransportWithLocalIP(ctx *cli.Context, localIP string) http.RoundTripper {
// When resolvedHost and originalHost are both non-empty, the transport also rewrites
// dial addresses from originalHost to resolvedHost and sets TLS SNI from originalHost.
func clientTransportWithLocalIP(ctx *cli.Context, localIP, resolvedHost, originalHost string) http.RoundTripper {
switch {
case ctx.Bool("ktls"):
return clientTransportKTLS(ctx, localIP)
return clientTransportKTLS(ctx, localIP, resolvedHost, originalHost)
case ctx.Bool("tls"):
return clientTransportTLS(ctx, localIP)
return clientTransportTLS(ctx, localIP, resolvedHost, originalHost)
default:
return clientTransportDefault(ctx, localIP)
return clientTransportDefault(ctx, localIP, resolvedHost, originalHost)
}
}

Expand Down Expand Up @@ -325,6 +341,42 @@ func parseHosts(h string, resolveDNS bool) []string {
return resolved
}

// parseHostPairs parses the host string into hostPair slices. When resolveDNS is true,
// each hostname is resolved to its IPs and each IP becomes a separate pair carrying the
// original hostname so that S3 signing and SNI remain correct.
func parseHostPairs(h string, resolveDNS bool) []hostPair {
raw := parseHosts(h, false)
if !resolveDNS {
pairs := make([]hostPair, len(raw))
for i, r := range raw {
pairs[i] = hostPair{resolved: r}
}
return pairs
}
var pairs []hostPair
for _, hostport := range raw {
host, port, _ := net.SplitHostPort(hostport)
if host == "" {
host = hostport
}
// IPs are resolved once at startup and pinned for the duration of the
// benchmark run. TTL-based refresh is intentionally not supported.
ips, err := net.LookupIP(host)
if err != nil {
fatalIf(probe.NewError(err), "Could not get IPs for "+hostport)
}
for _, ip := range ips {
resolved := ip.String()
if port != "" {
// Use net.JoinHostPort so IPv6 addresses get correct bracket formatting.
resolved = net.JoinHostPort(ip.String(), port)
}
pairs = append(pairs, hostPair{resolved: resolved, originalHost: hostport})
}
}
return pairs
}

// mustGetSystemCertPool - return system CAs or empty pool in case of error (or windows)
func mustGetSystemCertPool() *x509.CertPool {
rootCAs, err := certs.GetRootCAs("")
Expand All @@ -338,15 +390,19 @@ func mustGetSystemCertPool() *x509.CertPool {
}

func newAdminClient(ctx *cli.Context) *madmin.AdminClient {
hosts := parseHosts(ctx.String("host"), ctx.Bool("resolve-host"))
if len(hosts) == 0 {
pairs := parseHostPairs(ctx.String("host"), ctx.Bool("resolve-host"))
if len(pairs) == 0 {
fatalIf(probe.NewError(errors.New("no host defined")), "Unable to create MinIO admin client")
}

cl, err := madmin.NewWithOptions(hosts[0], &madmin.Options{
endpoint := pairs[0].resolved
if pairs[0].originalHost != "" {
endpoint = pairs[0].originalHost
}
cl, err := madmin.NewWithOptions(endpoint, &madmin.Options{
Creds: credentials.NewStaticV4(ctx.String("access-key"), ctx.String("secret-key"), ""),
Secure: ctx.Bool("tls") || ctx.Bool("ktls"),
Transport: clientTransport(ctx),
Transport: clientTransportWithLocalIP(ctx, "", pairs[0].resolved, pairs[0].originalHost),
})
fatalIf(probe.NewError(err), "Unable to create MinIO admin client")
cl.SetAppInfo(appName, pkg.Version)
Expand Down
6 changes: 5 additions & 1 deletion cli/client_default.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ import (
"github.com/minio/cli"
)

func clientTransportDefault(ctx *cli.Context, localIP string) http.RoundTripper {
func clientTransportDefault(ctx *cli.Context, localIP, resolvedHost, originalHost string) http.RoundTripper {
dialer := makeDialer(localIP)
if resolvedHost != "" {
return newClientTransport(ctx, withResolveHost(resolvedHost, originalHost, dialer, false))
}
Comment on lines +28 to +30

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Line 28: Gate resolve-host transport setup on originalHost, not resolvedHost.

resolvedHost is populated even when --resolve-host is disabled (from parseHostPairs), so this branch can skip the withLocalAddr(localIP) path unintentionally. Use originalHost (or both fields) as the activation condition for dial rewriting.

Suggested fix
 func clientTransportDefault(ctx *cli.Context, localIP, resolvedHost, originalHost string) http.RoundTripper {
 	dialer := makeDialer(localIP)
-	if resolvedHost != "" {
+	if originalHost != "" && resolvedHost != "" {
 		return newClientTransport(ctx, withResolveHost(resolvedHost, originalHost, dialer, false))
 	}
 	return newClientTransport(ctx, withLocalAddr(localIP))
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/client_default.go` around lines 28 - 30, The branch that decides to
return newClientTransport(...) is gating on resolvedHost, but resolvedHost can
be set even when --resolve-host was not intended (parseHostPairs), causing the
withLocalAddr(localIP) path to be skipped; change the condition to check
originalHost (or check both originalHost and resolvedHost) so dial rewriting is
only activated when the original host mapping was provided by the user, and
ensure newClientTransport is invoked with withResolveHost(originalHost,
resolvedHost, dialer, false) only when the originalHost mapping is present;
update the conditional around resolvedHost in the code that constructs the
client transport (the if that calls newClientTransport and withResolveHost) to
use originalHost (or a combined check) so withLocalAddr(localIP) is not
inadvertently bypassed.

return newClientTransport(ctx, withLocalAddr(localIP))
}
45 changes: 40 additions & 5 deletions cli/client_ktls.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
package cli

import (
"context"
"net"
stdHttp "net/http"
"os"
"time"
Expand All @@ -27,7 +29,8 @@ import (
"gitlab.com/go-extension/tls"
)

func clientTransportKTLS(ctx *cli.Context, localIP string) stdHttp.RoundTripper {
func clientTransportKTLS(ctx *cli.Context, localIP, resolvedHost, originalHost string) stdHttp.RoundTripper {
sni := sniFromHost(originalHost)
// Keep TLS config.
tlsConfig := &tls.Config{
RootCAs: mustGetSystemCertPool(),
Expand All @@ -36,6 +39,7 @@ func clientTransportKTLS(ctx *cli.Context, localIP string) stdHttp.RoundTripper
// Can't use TLSv1.1 because of RC4 cipher usage
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: ctx.Bool("insecure"),
ServerName: sni,
ClientSessionCache: tls.NewLRUClientSessionCache(1024), // up to 1024 nodes

// Extra configs
Expand All @@ -54,16 +58,47 @@ func clientTransportKTLS(ctx *cli.Context, localIP string) stdHttp.RoundTripper

netD := makeDialer(localIP)

// origHostname is the bare hostname used to identify which dial addresses
// should be rewritten to the resolved IP. Proxy addresses won't match.
origHostname := sniFromHost(originalHost)
getDialAddr := func(addr string) string {
if originalHost == "" || resolvedHost == "" {
return addr
}
host, port, err := net.SplitHostPort(addr)
if err != nil {
host = addr
port = "443"
}
targetHost, _, err := net.SplitHostPort(resolvedHost)
if err != nil {
targetHost = resolvedHost
}
// Only rewrite when the target is our original hostname.
// Proxy addresses won't match and are left untouched.
if host == origHostname {
return net.JoinHostPort(targetHost, port)
}
return addr
}

// If we don't enable http/2, then using a custom DialTLSConext is the best choice.
// It can improve performance by not using a compatibility layer.
if !ctx.Bool("http2") {
dialer := &tls.Dialer{NetDialer: netD, Config: tlsConfig}
return newClientTransport(ctx, withDialTLSContext(dialer.DialContext))
tlsDialer := &tls.Dialer{NetDialer: netD, Config: tlsConfig}
h1Dialer := func(ctx context.Context, network, addr string) (net.Conn, error) {
dialAddr := getDialAddr(addr)
return tlsDialer.DialContext(ctx, network, dialAddr)
}
return newClientTransport(ctx, withDialTLSContext(h1Dialer))
}

tr := &http.Transport{
Proxy: http.ProxyFromEnvironment,
DialContext: netD.DialContext,
Proxy: http.ProxyFromEnvironment,
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
dialAddr := getDialAddr(addr)
return netD.DialContext(ctx, network, dialAddr)
},
MaxIdleConnsPerHost: ctx.Int("concurrent"),
WriteBufferSize: ctx.Int("sndbuf"), // Configure beyond 4KiB default buffer size.
ReadBufferSize: ctx.Int("rcvbuf"), // Configure beyond 4KiB default buffer size.
Expand Down
13 changes: 11 additions & 2 deletions cli/client_tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import (
"github.com/minio/cli"
)

func clientTransportTLS(ctx *cli.Context, localIP string) http.RoundTripper {
func clientTransportTLS(ctx *cli.Context, localIP, resolvedHost, originalHost string) http.RoundTripper {
sni := sniFromHost(originalHost)
// Keep TLS config.
tlsConfig := &tls.Config{
RootCAs: mustGetSystemCertPool(),
Expand All @@ -34,12 +35,20 @@ func clientTransportTLS(ctx *cli.Context, localIP string) http.RoundTripper {
// Can't use TLSv1.1 because of RC4 cipher usage
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: ctx.Bool("insecure"),
ServerName: sni,
Comment thread
maniche1024 marked this conversation as resolved.
ClientSessionCache: tls.NewLRUClientSessionCache(1024), // up to 1024 nodes
}

if ctx.Bool("debug") {
tlsConfig.KeyLogWriter = os.Stdout
}

return newClientTransport(ctx, withTLSConfig(tlsConfig), withLocalAddr(localIP))
dialer := makeDialer(localIP)
opts := []transportOption{withTLSConfig(tlsConfig)}
if originalHost != "" {
opts = append(opts, withResolveHost(resolvedHost, originalHost, dialer, true))
} else {
opts = append(opts, withLocalAddr(localIP))
}
return newClientTransport(ctx, opts...)
}
49 changes: 49 additions & 0 deletions cli/client_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,55 @@ func withDialTLSContext(dialer func(ctx context.Context, network, addr string) (
}
}

// sniFromHost extracts the bare hostname from a host:port string for use as
// TLS ServerName (SNI). Returns "" if originalHost is empty, which lets Go
// derive SNI automatically from the dial address.
func sniFromHost(originalHost string) string {
if originalHost == "" {
return ""
}
if h, _, err := net.SplitHostPort(originalHost); err == nil {
return h
}
return originalHost
}

// withResolveHost rewrites the dial address from the logical hostname to the
// resolved IP when --resolve-host is active. Only activates when originalHost != "".
// Proxy connections are not rewritten: only connections whose target matches the
// original hostname are rewritten; proxy addresses are left unchanged.
func withResolveHost(resolvedHost, originalHost string, dialer *net.Dialer, isTLS bool) transportOption {
return func(transport *http.Transport) {
if originalHost == "" || resolvedHost == "" {
return
}
targetHost, _, err := net.SplitHostPort(resolvedHost)
if err != nil {
targetHost = resolvedHost
}
// Extract the bare original hostname once, outside the closure,
// so we can correctly identify which connections to rewrite.
origHostname := sniFromHost(originalHost)
transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
host, port, err := net.SplitHostPort(addr)
if err != nil {
host = addr
port = "80"
if isTLS {
port = "443"
}
}
dialAddr := addr
// Only rewrite when the target is our original hostname.
// Proxy addresses won't match and are left untouched.
if host == origHostname {
dialAddr = net.JoinHostPort(targetHost, port)
}
return dialer.DialContext(ctx, network, dialAddr)
}
}
}

func newClientTransport(ctx *cli.Context, options ...transportOption) http.RoundTripper {
tr := &http.Transport{
Proxy: http.ProxyFromEnvironment,
Expand Down
2 changes: 1 addition & 1 deletion cli/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ var ioFlags = []cli.Flag{
},
cli.BoolFlag{
Name: "resolve-host",
Usage: "Resolve the host(s) ip(s) (including multiple A/AAAA records). This can break SSL certificates, use --insecure if so",
Usage: "Resolve the host(s) ip(s) (including multiple A/AAAA records)",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Sync --resolve-host wording with sample config docs.

Line 220 removes the stale SSL warning, but yml-samples/put.yml still tells users resolve-host can break certs and suggests --insecure. Please update that sample text to match this corrected behavior to avoid conflicting guidance.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/flags.go` at line 220, Update the sample config text in
yml-samples/put.yml to match the corrected flag help for --resolve-host: remove
the stale "can break certs" warning and the suggestion to use --insecure, and
instead describe that resolve-host forces hostname resolution to IP(s)
(including multiple A/AAAA records) without implying it breaks TLS; ensure the
sample wording mirrors the Usage string change so users see consistent behavior
between the flag help (Usage: "Resolve the host(s) ip(s) (including multiple
A/AAAA records)") and the sample config entry for resolve-host.

Hidden: true,
},
cli.IntFlag{
Expand Down
Loading