Skip to content

Bracket IPv6 hosts when building https:// URLs - #1566

Open
pujitha24 wants to merge 3 commits into
k0sproject:mainfrom
pujitha24:auto/issue-1565
Open

Bracket IPv6 hosts when building https:// URLs#1566
pujitha24 wants to merge 3 commits into
k0sproject:mainfrom
pujitha24:auto/issue-1565

Conversation

@pujitha24

Copy link
Copy Markdown
Contributor

Motivation:
Several places built an https:// URL with fmt.Sprintf("https://%s:%d",
host, port). This is broken when host is a bare IPv6 literal: the
resulting string ("https://2001:db8::1:443") is ambiguous and fails to
parse as host:port. Reported for the CAPI worker join flow, where a
K0smotronControlPlane behind an IPv6 LoadBalancer VIP produces a join
URL the kubelet bootstrap client rejects with "host must be a URL or a
host:port pair". The same fmt.Sprintf pattern, on the same
Ingress.APIHost/Port fields, also appears in the join-token rewrite and
kubeconfig rewrite paths, so it was fixed there too; a sibling file
(k0smotroncluster_configmap.go) already used the correct pattern for
those same fields.

Approach:
Use net.JoinHostPort, which already adds brackets around IPv6 literals
(and is a no-op for IPv4/DNS hosts), instead of raw string formatting.
Added an apiServerURL(host, port string) helper in
internal/controller/bootstrap/common.go and reused it at the two
call sites named in the report (worker join URL and control-plane CA
probe URL). Applied the same net.JoinHostPort fix directly in
internal/controller/k0smotron.io/jointokenrequest_controller.go and
k0smotroncluster_kubeconfig.go, which live in a different package and
have the identical defect on the identical Ingress fields.

Not changed: internal/controller/controlplane/k0s_controlplane_controller.go
builds a similar URL from Tunneling.ServerAddress/TunnelingNodePort, and
k0smotroncluster_ingress.go renders an APIHost:Port pair into an HAProxy
config template. Both are the same underlying colon-ambiguity issue but
in different code paths/fix mechanisms; left out of this change to keep
it focused on the reported defect and its direct duplicates.

Validation:

  • go build ./... passes.
  • go test ./internal/controller/bootstrap/... and
    ./internal/controller/k0smotron.io/... pass, including new regression
    tests: Test_apiServerURL (bootstrap package), Test_updateJoinTokenURL,
    and TestRewriteKubeconfigValuesIngressIPv6HostIsBracketed. Each
    asserts the exact bracketed https://[host]:port form for an IPv6
    host; with the old fmt.Sprintf code these would produce the
    unbracketed, unparseable form instead.
  • make test (repo's documented pre-submit target) passes for the whole
    repo.
  • golangci-lint run --config .golangci.yml --build-tags hack on the
    changed packages reports 0 issues.
  • No live cluster or e2e reproduction was run; the defect is visible
    directly in the code (missing IPv6 bracketing) and is proven by the
    targeted unit tests above, which is the validation this repo's own
    testing guidelines (docs/contributing/contribute-testing.md) call for
    unit-level fixes like this one.

Report: #1565
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Assisted-by: claude-sonnet-5 (via Claude Code)

Fixes #1565

Motivation:
Several places built an https:// URL with fmt.Sprintf("https://%s:%d",
host, port). This is broken when host is a bare IPv6 literal: the
resulting string ("https://2001:db8::1:443") is ambiguous and fails to
parse as host:port. Reported for the CAPI worker join flow, where a
K0smotronControlPlane behind an IPv6 LoadBalancer VIP produces a join
URL the kubelet bootstrap client rejects with "host must be a URL or a
host:port pair". The same fmt.Sprintf pattern, on the same
Ingress.APIHost/Port fields, also appears in the join-token rewrite and
kubeconfig rewrite paths, so it was fixed there too; a sibling file
(k0smotroncluster_configmap.go) already used the correct pattern for
those same fields.

Approach:
Use net.JoinHostPort, which already adds brackets around IPv6 literals
(and is a no-op for IPv4/DNS hosts), instead of raw string formatting.
Added an apiServerURL(host, port string) helper in
internal/controller/bootstrap/common.go and reused it at the two
call sites named in the report (worker join URL and control-plane CA
probe URL). Applied the same net.JoinHostPort fix directly in
internal/controller/k0smotron.io/jointokenrequest_controller.go and
k0smotroncluster_kubeconfig.go, which live in a different package and
have the identical defect on the identical Ingress fields.

Not changed: internal/controller/controlplane/k0s_controlplane_controller.go
builds a similar URL from Tunneling.ServerAddress/TunnelingNodePort, and
k0smotroncluster_ingress.go renders an APIHost:Port pair into an HAProxy
config template. Both are the same underlying colon-ambiguity issue but
in different code paths/fix mechanisms; left out of this change to keep
it focused on the reported defect and its direct duplicates.

Validation:
- go build ./... passes.
- go test ./internal/controller/bootstrap/... and
  ./internal/controller/k0smotron.io/... pass, including new regression
  tests: Test_apiServerURL (bootstrap package), Test_updateJoinTokenURL,
  and TestRewriteKubeconfigValuesIngressIPv6HostIsBracketed. Each
  asserts the exact bracketed https://[host]:port form for an IPv6
  host; with the old fmt.Sprintf code these would produce the
  unbracketed, unparseable form instead.
- make test (repo's documented pre-submit target) passes for the whole
  repo.
- golangci-lint run --config .golangci.yml --build-tags hack on the
  changed packages reports 0 issues.
- No live cluster or e2e reproduction was run; the defect is visible
  directly in the code (missing IPv6 bracketing) and is proven by the
  targeted unit tests above, which is the validation this repo's own
  testing guidelines (docs/contributing/contribute-testing.md) call for
  unit-level fixes like this one.

Report: k0sproject#1565
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
@pujitha24
pujitha24 requested a review from a team as a code owner August 24, 2026 03:46

@makhov makhov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! I think we need a more structured and unified approach to solving this issue across all k0smotron code.

joinURL := apiServerURL(scope.Cluster.Spec.ControlPlaneEndpoint.Host, strconv.Itoa(int(scope.Cluster.Spec.ControlPlaneEndpoint.Port)))
if scope.ingressSpec != nil {
joinURL = fmt.Sprintf("https://%s:%d", scope.ingressSpec.APIHost, scope.ingressSpec.Port)
joinURL = apiServerURL(scope.ingressSpec.APIHost, strconv.FormatInt(scope.ingressSpec.Port, 10))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ingress by definition uses dns names, so it doesn't make much sense to do it here (and below).

}

return fmt.Sprintf("https://%s:%s", firstControllerIP, port), nil
return apiServerURL(firstControllerIP, port), nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

findFirstControllerIP already wraps IPv6 addresses in the brackets, at least in some cases, so this will break the things wrapping them twice.

apiServerURL (added in be648bc) already brackets IPv6 literals via
net.JoinHostPort. findFirstControllerIP was still manually wrapping
IPv6 internal addresses in brackets before returning them, so
detectJoinHost's apiServerURL(firstControllerIP, port) call would
double-bracket them. Return the raw address and let apiServerURL do
the bracketing, per review feedback on k0sproject#1566.

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24

Copy link
Copy Markdown
Contributor Author

@makhov I've pushed changes addressing your review — the branch is now at bec1f5e and CI is green. Could you take another look when you have a moment? Happy to keep iterating if anything is still off.

@makhov

makhov commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

As I said, we need to think it through and take a unified approach to handle URLs across the full codebase. And don't introduce new unnecessary code and tests.

Ingress.APIHost is used directly as a Kubernetes Ingress Host field, so
it can never be a bare IPv6 literal; bracketing it via net.JoinHostPort
was unnecessary. Revert the three Ingress.APIHost call sites this PR
touched back to plain fmt.Sprintf, and drop the tests that only
exercised that reverted behavior. The ControlPlaneEndpoint.Host and
firstControllerIP call sites, which can genuinely be IPv6 literals,
are unchanged.

Per review feedback on k0sproject#1566.

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24

Copy link
Copy Markdown
Contributor Author

Fair point on the ingress case — Ingress.APIHost is always a DNS name, so wrapping it in apiServerURL/net.JoinHostPort there doesn't fix anything real, it's just extra code and a test for a case that can't occur. I fixed the double-bracketing bug you flagged on findFirstControllerIP in bec1f5e, but I haven't touched the ingress call site yet, and I don't want to guess at what "unified" should look like and hand you another patch that misses the mark again.

Would it help if I scoped this back down to just the one call site from the original report (the worker join URL built from ControlPlaneEndpoint, which can actually be a bare IPv6 literal), drop the apiServerURL wrapping on the ingress-derived URLs and the jointoken/kubeconfig paths, and trim the tests to match? Or would you rather this wait for a separate issue/design where the URL-building is unified across all the call sites (including the Tunneling and HAProxy-template ones I left out)? Happy to go either way, just don't want to keep guessing.

@pujitha24

Copy link
Copy Markdown
Contributor Author

@makhov I've pushed changes addressing your review — the branch is now at 0d66bac and CI is green. Could you take another look when you have a moment? Happy to keep iterating if anything is still off.

@makhov

makhov commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What I mean is that we probably should look at the whole codebase and check how we do URLs. Most likely we would need some global helper that we will use basically everywhere when needed.

@pujitha24

Copy link
Copy Markdown
Contributor Author

@[2026-09-02 06:47:21] SWEEP tektoncd/pipelines-as-code#2904: clone/fetch failed I've pushed changes addressing your review — the branch is now at 0d66bac and CI is green. Could you take another look when you have a moment? Happy to keep iterating if anything is still off.

@pujitha24

Copy link
Copy Markdown
Contributor Author

@[2026-09-02 06:47:29] metal3-io/baremetal-operator#3514: base rate on other contributors' PRs (fail/total): 0/9 0/4 0/9 0/9 0/8 0/8 [2026-09-02 06:47:21] SWEEP tektoncd/pipelines-as-code#2904: clone/fetch failed I've pushed changes addressing your review — the branch is now at 0d66bac and CI is green. Could you take another look when you have a moment? Happy to keep iterating if anything is still off.

@pujitha24

Copy link
Copy Markdown
Contributor Author

@[2026-09-02 06:47:30] SWEEP metal3-io/baremetal-operator#3514: clone/fetch failed I've pushed changes addressing your review — the branch is now at 0d66bac and CI is green. Could you take another look when you have a moment? Happy to keep iterating if anything is still off.

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.

IPv6 control-plane endpoint: join URL is not bracketed

2 participants