Skip to content

fix: preserve multi-host HA DSN addresses (Go 1.26) - #1946

Open
sankalpsthakur wants to merge 2 commits into
ClickHouse:mainfrom
sankalpsthakur:fix/1784-ha-dsn-go126
Open

fix: preserve multi-host HA DSN addresses (Go 1.26)#1946
sankalpsthakur wants to merge 2 commits into
ClickHouse:mainfrom
sankalpsthakur:fix/1784-ha-dsn-go126

Conversation

@sankalpsthakur

Copy link
Copy Markdown
Contributor

Summary

Hardens multi-host (HA) DSN address parsing so host lists stay intact across net/url host-normalization differences (including Go 1.26-era multi-host / HA DSN breakage reported in #1784).

fromDSN already uses churl.Parse (from #1787). This change additionally extracts the HA host list from the raw DSN authority instead of only strings.Split(dsn.Host, ","), so:

  • IPv4 multi-host DSNs keep every peer (host1:9440,host2:9440)
  • Bracketed IPv6 multi-host DSNs keep every peer ([::1]:9440,[2001:db8::1]:9440) instead of collapsing to the last host
  • Auth / query / path continue to come from the parsed URL

Changes

  • dsnAddrList / addrListFromDSN / splitHostList in clickhouse_options.go
  • ParseDSN regression cases for issue 1784 (auth+secure, HTTP HA, IPv6 HA)
  • Unit tests for host-list helpers
  • lib/churl multi-host parse tests

Fixes #1784

Checklist

Validation

go test -count=1 -run 'TestParseDSN|TestDSNAddrList|TestSplitHostList' .
go test -count=1 ./lib/churl/
go test -count=1 .

Results (Go 1.26.0):

ok  github.com/ClickHouse/clickhouse-go/v2          (TestParseDSN + helpers PASS)
ok  github.com/ClickHouse/clickhouse-go/v2/lib/churl (TestParseMultiHost PASS)
ok  github.com/ClickHouse/clickhouse-go/v2          (package unit tests PASS)

AI/LLM disclosure

  • AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
  • I reviewed the complete change, understand the reasoning, and ran the reported local tests before submitting.
  • This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

Extract HA host lists from the raw DSN authority instead of only
splitting url.Host, so multi-host DSNs (including bracketed IPv6)
are not collapsed by URL host normalization. Add ParseDSN and churl
regression tests for the issue 1784 multi-host cases.
Comment thread clickhouse_options_test.go Outdated
},
"",
},
// Regression for https://github.com/ClickHouse/clickhouse-go/issues/1784

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we do not name tests after issues - tests should match functionality
please remove.

// (Go 1.26 net/url multi-host / HA DSN). Also covers auth + query.
{
"HA multi-host with auth and secure (issue 1784)",
"clickhouse://user:pass@host1:9440,host2:9440/database?secure=true",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what user and password would be used for second host?
what if credentials are overridden in options?
There should be tests for all this cases.

Comment thread clickhouse_options.go

// addrListFromDSN extracts host[:port] entries from the DSN authority.
// Supports comma-separated HA hosts and bracketed IPv6 literals.
func addrListFromDSN(raw string) []string {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are already tools that can parse such form of url:

package main

import (
	"fmt"
	"net"
	"net/url"
	"strings"
)

type TargetHost struct {
	Host string
	Port string
}

func main() {
	rawURL := "clickhouse://user:pass@host1:9440,host2:9440/database?secure=true"

	// 1. Parse into a standard URL structure
	u, err := url.Parse(rawURL)
	if err != nil {
		panic(err)
	}

	// 2. Extract Username
	username := ""
	if u.User != nil {
		username = u.User.Username()
	}

	// 3. Extract the complete host cluster string
	// u.Host will contain "host1:9440,host2:9440"
	clusterStr := u.Host

	// 4. Split the individual hosts by comma
	hostTokens := strings.Split(clusterStr, ",")
	var hosts []TargetHost

	for _, token := range hostTokens {
		h, p, err := net.SplitHostPort(token)
		if err != nil {
			// Fallback if port is missing from a specific host token
			h = token
			p = ""
		}
		hosts = append(hosts, TargetHost{Host: h, Port: p})
	}

	// Output results
	fmt.Println("Username:", username)
	fmt.Println("Database:", strings.TrimPrefix(u.Path, "/"))
	fmt.Println("Hosts found:")
	for i, h := range hosts {
		fmt.Printf("  [%d] Host: %s, Port: %s\n", i, h.Host, h.Port)
	}
}

@chernser chernser left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • correct implementation - use std lib
  • documentation how it works and changelog
  • more tests with authentication
  • ideally integration test

- Use stdlib net.SplitHostPort/net.JoinHostPort in splitHostList for
  per-host validation/normalization; document HA handling and Go 1.26
  net/url tightening (churl as Go 1.25.7 copy plus raw-authority
  extraction). Clarify single cluster-wide Auth and query override.
- Add CHANGELOG Unreleased entry describing fix.
- Expand ParseDSN tests: rename from issue-number style to functional
  names, add multi-host cases for auth overridden via query, auth via
  query only, encoded passwords, IPv6 with auth, and 3-host preservation.
- Expand host-list helper tests: rename, add auth-override case and
  additional splitHostList coverage.

Fixes ClickHouse#1784
@sankalpsthakur

sankalpsthakur commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @chernser, addressed the feedback in the latest push. Documented lib/churl and refactored splitHostList to use net.SplitHostPort and JoinHostPort, expanded docs and added a changelog entry. Added more auth tests for multi-host cases including query overrides and IPv6. Unit tests pass. Let me know if you want integration coverage as well.

@sankalpsthakur

Copy link
Copy Markdown
Contributor Author

Thanks @chernser, pushed c54255c to address the checklist. Please take another look when you have a moment.

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.

HA DSN parsing fails with Go 1.26

2 participants