OCPBUGS-100140: bracket IPv6 in OAuth issuer and callback URLs

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"math/big"
+	"net"
 	"net/http"
 	"os"
 	"sort"
@@ -761,7 +762,9 @@ func (r *HostedControlPlaneReconciler) reconcileInfrastructureStatusCondition(ct
 			Reason:  hyperv1.AsExpectedReason,
 		}
 		if util.HCPOAuthEnabled(hostedControlPlane) {
-			hostedControlPlane.Status.OAuthCallbackURLTemplate = fmt.Sprintf("https://%s:%d/oauth2callback/[identity-provider-name]", infraStatus.OAuthHost, infraStatus.OAuthPort)
+			// JoinHostPort brackets IPv6 literals so url.Parse accepts the callback template.
+			hostedControlPlane.Status.OAuthCallbackURLTemplate = fmt.Sprintf("https://%s/oauth2callback/[identity-provider-name]",
+				net.JoinHostPort(infraStatus.OAuthHost, strconv.Itoa(int(infraStatus.OAuthPort))))
 		}
 	} else {
 		message := "Cluster infrastructure is still provisioning"

control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.go

@@ -3,6 +3,9 @@ package kas
 import (
 	"encoding/json"
 	"fmt"
+	"net"
+	"net/url"
+	"strconv"
 
 	"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests"
 	"github.com/openshift/hypershift/support/certs"
@@ -39,7 +42,11 @@ func adaptOauthMetadata(cpContext component.WorkloadContext, cfg *corev1.ConfigM
 		return fmt.Errorf("failed to unmarshal oauth metadata: %w", err)
 	}
 
-	oauthURL := fmt.Sprintf("https://%s:%d", cpContext.InfraStatus.OAuthHost, cpContext.InfraStatus.OAuthPort)
+	// Use net.JoinHostPort so IPv6 NodePort addresses are bracketed (RFC 3986).
+	oauthURL := (&url.URL{
+		Scheme: "https",
+		Host:   net.JoinHostPort(cpContext.InfraStatus.OAuthHost, strconv.Itoa(int(cpContext.InfraStatus.OAuthPort))),
+	}).String()
 	oauthMetadata["issuer"] = oauthURL
 	oauthMetadata["authorization_endpoint"] = fmt.Sprintf("%s/oauth/authorize", oauthURL)
 	oauthMetadata["token_endpoint"] = fmt.Sprintf("%s/oauth/token", oauthURL)

control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go

@@ -1,11 +1,13 @@
 package kas
 
 import (
+	"encoding/json"
 	"testing"
 
 	. "github.com/onsi/gomega"
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/infra"
 	controlplanecomponent "github.com/openshift/hypershift/support/controlplane-component"
 
 	corev1 "k8s.io/api/core/v1"
@@ -16,10 +18,15 @@ import (
 func TestAdaptOauthMetadata(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
-		name      string
-		cfg       *corev1.ConfigMap
-		wantErr   bool
-		errSubstr string
+		name       string
+		cfg        *corev1.ConfigMap
+		oauthHost  string
+		oauthPort  int32
+		wantErr    bool
+		errSubstr  string
+		wantIssuer string
+		wantAuthz  string
+		wantToken  string
 	}{
 		{
 			name: "When ConfigMap contains invalid JSON, it should return an unmarshal error",
@@ -31,6 +38,45 @@ func TestAdaptOauthMetadata(t *testing.T) {
 			wantErr:   true,
 			errSubstr: "failed to unmarshal oauth metadata",
 		},
+		{
+			name: "When OAuth host is IPv4, it should leave issuer URLs unbracketed",
+			cfg: &corev1.ConfigMap{
+				Data: map[string]string{
+					OauthMetadataConfigKey: `{}`,
+				},
+			},
+			oauthHost:  "192.0.2.10",
+			oauthPort:  32047,
+			wantIssuer: "https://192.0.2.10:32047",
+			wantAuthz:  "https://192.0.2.10:32047/oauth/authorize",
+			wantToken:  "https://192.0.2.10:32047/oauth/token",
+		},
+		{
+			name: "When OAuth host is IPv6, it should bracket the address",
+			cfg: &corev1.ConfigMap{
+				Data: map[string]string{
+					OauthMetadataConfigKey: `{}`,
+				},
+			},
+			oauthHost:  "fd2e:6f44:5dd8:c956::14",
+			oauthPort:  32047,
+			wantIssuer: "https://[fd2e:6f44:5dd8:c956::14]:32047",
+			wantAuthz:  "https://[fd2e:6f44:5dd8:c956::14]:32047/oauth/authorize",
+			wantToken:  "https://[fd2e:6f44:5dd8:c956::14]:32047/oauth/token",
+		},
+		{
+			name: "When OAuth host is a hostname, it should leave issuer URLs unbracketed",
+			cfg: &corev1.ConfigMap{
+				Data: map[string]string{
+					OauthMetadataConfigKey: `{}`,
+				},
+			},
+			oauthHost:  "oauth.example.com",
+			oauthPort:  443,
+			wantIssuer: "https://oauth.example.com:443",
+			wantAuthz:  "https://oauth.example.com:443/oauth/authorize",
+			wantToken:  "https://oauth.example.com:443/oauth/token",
+		},
 	}
 
 	for _, tt := range tests {
@@ -41,15 +87,25 @@ func TestAdaptOauthMetadata(t *testing.T) {
 				Context: t.Context(),
 				HCP:     &hyperv1.HostedControlPlane{},
 				Client:  fake.NewClientBuilder().Build(),
+				InfraStatus: infra.InfrastructureStatus{
+					OAuthHost: tt.oauthHost,
+					OAuthPort: tt.oauthPort,
+				},
 			}
 
 			err := adaptOauthMetadata(cpContext, tt.cfg)
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())
 				g.Expect(err.Error()).To(ContainSubstring(tt.errSubstr))
-			} else {
-				g.Expect(err).ToNot(HaveOccurred())
+				return
 			}
+
+			g.Expect(err).ToNot(HaveOccurred())
+			var oauthMetadata map[string]interface{}
+			g.Expect(json.Unmarshal([]byte(tt.cfg.Data[OauthMetadataConfigKey]), &oauthMetadata)).To(Succeed())
+			g.Expect(oauthMetadata["issuer"]).To(Equal(tt.wantIssuer))
+			g.Expect(oauthMetadata["authorization_endpoint"]).To(Equal(tt.wantAuthz))
+			g.Expect(oauthMetadata["token_endpoint"]).To(Equal(tt.wantToken))
 		})
 	}
 }

control-plane-operator/hostedclusterconfigoperator/controllers/resources/oauth/reconcile.go

@@ -4,6 +4,8 @@ import (
 	"crypto/rand"
 	"encoding/base64"
 	"fmt"
+	"net"
+	"strconv"
 
 	"github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests"
 	"github.com/openshift/hypershift/support/certs"
@@ -20,16 +22,20 @@ func ReconcileOAuthServerCertCABundle(cm *corev1.ConfigMap, sourceBundle *corev1
 	return nil
 }
 
+func oauthRedirectURI(path, externalHost string, externalPort int32) string {
+	return fmt.Sprintf("https://%s%s", net.JoinHostPort(externalHost, strconv.Itoa(int(externalPort))), path)
+}
+
 func ReconcileBrowserClient(client *oauthv1.OAuthClient, externalHost string, externalPort int32) error {
 	redirectURIs := []string{
-		fmt.Sprintf("https://%s:%d/oauth/token/display", externalHost, externalPort),
+		oauthRedirectURI("/oauth/token/display", externalHost, externalPort),
 	}
 	return reconcileOAuthClient(client, redirectURIs, false, true)
 }
 
 func ReconcileChallengingClient(client *oauthv1.OAuthClient, externalHost string, externalPort int32) error {
 	redirectURIs := []string{
-		fmt.Sprintf("https://%s:%d/oauth/token/implicit", externalHost, externalPort),
+		oauthRedirectURI("/oauth/token/implicit", externalHost, externalPort),
 	}
 	return reconcileOAuthClient(client, redirectURIs, true, false)
 }

control-plane-operator/hostedclusterconfigoperator/controllers/resources/oauth/reconcile_test.go

@@ -7,6 +7,8 @@ import (
 
 	"github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests"
 
+	oauthv1 "github.com/openshift/api/oauth/v1"
+
 	rbacv1 "k8s.io/api/rbac/v1"
 )
 
@@ -77,3 +79,65 @@ func TestReconcileOauthServingCertRoleBinding(t *testing.T) {
 		})
 	}
 }
+
+func TestReconcileBrowserClient(t *testing.T) {
+	testsCases := []struct {
+		name         string
+		externalHost string
+		externalPort int32
+		wantURIs     []string
+	}{
+		{
+			name:         "When OAuth host is IPv4, it should leave the redirect URI unbracketed",
+			externalHost: "192.0.2.10",
+			externalPort: 32047,
+			wantURIs:     []string{"https://192.0.2.10:32047/oauth/token/display"},
+		},
+		{
+			name:         "When OAuth host is IPv6, it should bracket the address in the redirect URI",
+			externalHost: "fd2e:6f44:5dd8:c956::14",
+			externalPort: 32047,
+			wantURIs:     []string{"https://[fd2e:6f44:5dd8:c956::14]:32047/oauth/token/display"},
+		},
+	}
+	for _, tc := range testsCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			client := &oauthv1.OAuthClient{}
+			err := ReconcileBrowserClient(client, tc.externalHost, tc.externalPort)
+			g.Expect(err).To(Not(HaveOccurred()))
+			g.Expect(client.RedirectURIs).To(Equal(tc.wantURIs))
+		})
+	}
+}
+
+func TestReconcileChallengingClient(t *testing.T) {
+	testsCases := []struct {
+		name         string
+		externalHost string
+		externalPort int32
+		wantURIs     []string
+	}{
+		{
+			name:         "When OAuth host is IPv4, it should leave the redirect URI unbracketed",
+			externalHost: "192.0.2.10",
+			externalPort: 32047,
+			wantURIs:     []string{"https://192.0.2.10:32047/oauth/token/implicit"},
+		},
+		{
+			name:         "When OAuth host is IPv6, it should bracket the address in the redirect URI",
+			externalHost: "fd2e:6f44:5dd8:c956::14",
+			externalPort: 32047,
+			wantURIs:     []string{"https://[fd2e:6f44:5dd8:c956::14]:32047/oauth/token/implicit"},
+		},
+	}
+	for _, tc := range testsCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			client := &oauthv1.OAuthClient{}
+			err := ReconcileChallengingClient(client, tc.externalHost, tc.externalPort)
+			g.Expect(err).To(Not(HaveOccurred()))
+			g.Expect(client.RedirectURIs).To(Equal(tc.wantURIs))
+		})
+	}
+}

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

@@ -1115,7 +1115,7 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
 				}
 			}
 			if err == nil && serviceFirstNodePortAvailable(ignitionService) {
-				hcluster.Status.IgnitionEndpoint = fmt.Sprintf("%s:%d", serviceStrategy.NodePort.Address, ignitionService.Spec.Ports[0].NodePort)
+				hcluster.Status.IgnitionEndpoint = net.JoinHostPort(serviceStrategy.NodePort.Address, strconv.Itoa(int(ignitionService.Spec.Ports[0].NodePort)))
 			}
 		default:
 			// We don't return the error here as reconciling won't solve the input problem.

hypershift-operator/controllers/hostedcluster/internal/platform/agent/agent_test.go

@@ -181,6 +181,42 @@ func TestReconcileCAPIInfraCR(t *testing.T) {
 				Status: agentv1.AgentClusterStatus{},
 			},
 		},
+		"When ign endpoint is IPv6 host:port, it should create infra cluster with bracketed URL": {
+			controlPlaneNamespace: controlPlaneNamespace,
+			hostedCluster: &hyperv1.HostedCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      hostedClusterName,
+					Namespace: hostedClusterNamespace,
+				},
+				Spec: hyperv1.HostedClusterSpec{
+					Platform: hyperv1.PlatformSpec{
+						Type: hyperv1.AgentPlatform,
+					},
+				},
+				Status: hyperv1.HostedClusterStatus{
+					IgnitionEndpoint: "[fd2e:6f44:5dd8:c956::14]:31936",
+				},
+			},
+			APIEndpoint: APIEndpoint,
+			expectedObject: &agentv1.AgentCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace:       controlPlaneNamespace,
+					Name:            hostedClusterName,
+					ResourceVersion: "1",
+				},
+				Spec: agentv1.AgentClusterSpec{
+					IgnitionEndpoint: &agentv1.IgnitionEndpoint{
+						Url:                    "https://[fd2e:6f44:5dd8:c956::14]:31936/ignition",
+						CaCertificateReference: &agentv1.CaCertificateReference{Name: caSecret.Name, Namespace: caSecret.Namespace},
+					},
+					ControlPlaneEndpoint: capiv1.APIEndpoint{
+						Port: APIEndpoint.Port,
+						Host: APIEndpoint.Host,
+					},
+				},
+				Status: agentv1.AgentClusterStatus{},
+			},
+		},
 	}
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {

hypershift-operator/controllers/hostedcluster/reconcile_legacy.go

@@ -3,6 +3,7 @@ package hostedcluster
 import (
 	"context"
 	"fmt"
+	"net"
 	"strconv"
 	"strings"
 	"time"
@@ -795,7 +796,7 @@ func (r *HostedClusterReconciler) reconcileLegacy(ctx context.Context, req ctrl.
 				}
 			}
 			if err == nil && serviceFirstNodePortAvailable(ignitionService) {
-				hcluster.Status.IgnitionEndpoint = fmt.Sprintf("%s:%d", serviceStrategy.NodePort.Address, ignitionService.Spec.Ports[0].NodePort)
+				hcluster.Status.IgnitionEndpoint = net.JoinHostPort(serviceStrategy.NodePort.Address, strconv.Itoa(int(ignitionService.Spec.Ports[0].NodePort)))
 			}
 		default:
 			// We don't return the error here as reconciling won't solve the input problem.

support/globalconfig/infrastructure.go

@@ -2,6 +2,8 @@ package globalconfig
 
 import (
 	"fmt"
+	"net"
+	"strconv"
 	"strings"
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
@@ -13,6 +15,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+func httpsURL(host string, port int32) string {
+	return fmt.Sprintf("https://%s", net.JoinHostPort(host, strconv.Itoa(int(port))))
+}
+
 func InfrastructureConfig() *configv1.Infrastructure {
 	infra := &configv1.Infrastructure{
 		ObjectMeta: metav1.ObjectMeta{
@@ -30,14 +36,14 @@ func ReconcileInfrastructure(infra *configv1.Infrastructure, hcp *hyperv1.Hosted
 	apiServerPort := hcp.Status.ControlPlaneEndpoint.Port
 
 	infra.Spec.PlatformSpec.Type = configv1.PlatformType(platformType)
-	infra.Status.APIServerInternalURL = fmt.Sprintf("https://%s:%d", apiServerAddress, apiServerPort)
+	infra.Status.APIServerInternalURL = httpsURL(apiServerAddress, apiServerPort)
 	if netutil.IsPrivateHCP(hcp) {
-		infra.Status.APIServerInternalURL = fmt.Sprintf("https://api.%s.hypershift.local:%d", hcp.Name, apiServerPort)
+		infra.Status.APIServerInternalURL = httpsURL(fmt.Sprintf("api.%s.hypershift.local", hcp.Name), apiServerPort)
 	}
 
-	infra.Status.APIServerURL = fmt.Sprintf("https://%s:%d", apiServerAddress, apiServerPort)
+	infra.Status.APIServerURL = httpsURL(apiServerAddress, apiServerPort)
 	if len(hcp.Spec.KubeAPIServerDNSName) > 0 {
-		infra.Status.APIServerURL = fmt.Sprintf("https://%s:%d", hcp.Spec.KubeAPIServerDNSName, apiServerPort)
+		infra.Status.APIServerURL = httpsURL(hcp.Spec.KubeAPIServerDNSName, apiServerPort)
 	}
 	infra.Status.EtcdDiscoveryDomain = BaseDomain(hcp)
 	infra.Status.InfrastructureName = hcp.Spec.InfraID

support/globalconfig/infrastructure_test.go

@@ -100,6 +100,20 @@ func TestReconcileInfrastructure(t *testing.T) {
 				g.Expect(infra.Status.APIServerInternalURL).To(Equal(wantURL))
 			},
 		},
+		{
+			name:       "When control plane endpoint is IPv6, it should bracket the address in API server URLs",
+			inputInfra: InfrastructureConfig(),
+			inputHCP: func() *hyperv1.HostedControlPlane {
+				hcp := baseHCP(hyperv1.NonePlatform)
+				hcp.Status.ControlPlaneEndpoint.Host = "fd2e:6f44:5dd8:c956::14"
+				return hcp
+			}(),
+			verify: func(g Gomega, infra *configv1.Infrastructure) {
+				wantURL := "https://[fd2e:6f44:5dd8:c956::14]:6443"
+				g.Expect(infra.Status.APIServerURL).To(Equal(wantURL))
+				g.Expect(infra.Status.APIServerInternalURL).To(Equal(wantURL))
+			},
+		},
 		{
 			name:       "When HCP has DNS config, it should set EtcdDiscoveryDomain from BaseDomain",
 			inputInfra: InfrastructureConfig(),