Environment variables with 'https_address' in them should have 'https://' scheme

hack/prepare-for-integration-tests.sh

@@ -414,7 +414,7 @@ export PINNIPED_TEST_WEBHOOK_CA_BUNDLE=${webhook_ca_bundle}
 export PINNIPED_TEST_SUPERVISOR_NAMESPACE=${supervisor_namespace}
 export PINNIPED_TEST_SUPERVISOR_APP_NAME=${supervisor_app_name}
 export PINNIPED_TEST_SUPERVISOR_CUSTOM_LABELS='${supervisor_custom_labels}'
-export PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS="localhost:12344"
+export PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS="https://localhost:12344"
 export PINNIPED_TEST_PROXY=http://127.0.0.1:12346
 export PINNIPED_TEST_LDAP_HOST=ldap.tools.svc.cluster.local
 export PINNIPED_TEST_LDAP_STARTTLS_ONLY_HOST=ldapstarttls.tools.svc.cluster.local

test/integration/supervisor_discovery_test.go

@@ -52,17 +52,12 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
 	defer cancel()
 
-	httpsAddress := env.SupervisorHTTPSAddress
-	if host, _, err := net.SplitHostPort(httpsAddress); err == nil {
-		httpsAddress = host
-	}
-
 	temporarilyRemoveAllFederationDomainsAndDefaultTLSCertSecret(ctx, t, ns, env.DefaultTLSCertSecretName(), client, testlib.NewKubernetesClientset(t))
 	defaultCA := createTLSCertificateSecret(
 		ctx,
 		t,
 		ns,
-		testlib.NewSupervisorIssuer(t, httpsAddress),
+		testlib.NewSupervisorIssuer(t, env.SupervisorHTTPSAddress),
 		env.DefaultTLSCertSecretName(),
 		kubeClient,
 	)
@@ -88,6 +83,8 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
 				t.Skip("no address defined")
 			}
 
+			addr, _ = strings.CutPrefix(addr, "https://")
+
 			// Create any IDP so that any FederationDomain created later by this test will see that exactly one IDP exists.
 			testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{
 				Issuer: "https://example.cluster.local/fake-issuer-url-does-not-matter",
@@ -188,9 +185,9 @@ func TestSupervisorTLSTerminationWithSNI_Disruptive(t *testing.T) {
 	temporarilyRemoveAllFederationDomainsAndDefaultTLSCertSecret(ctx, t, ns, env.DefaultTLSCertSecretName(), pinnipedClient, kubeClient)
 
 	scheme := "https"
-	address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 8443
+	supervisorIssuer := testlib.NewSupervisorIssuer(t, env.SupervisorHTTPSAddress)
+	address := supervisorIssuer.Address() // hostname and port WITHOUT SCHEME for direct access to the supervisor's port 8443
 
-	hostname1 := strings.Split(address, ":")[0]
 	issuer1 := fmt.Sprintf("%s://%s/issuer1", scheme, address)
 	certSecretName1 := "integration-test-cert-1"
 
@@ -206,7 +203,7 @@ func TestSupervisorTLSTerminationWithSNI_Disruptive(t *testing.T) {
 	requireEndpointHasBootstrapTLSErrorBecauseCertificatesAreNotReady(t, issuer1)
 
 	// Create the Secret.
-	ca1 := createTLSCertificateSecret(ctx, t, ns, testlib.NewSupervisorIssuer(t, hostname1), certSecretName1, kubeClient)
+	ca1 := createTLSCertificateSecret(ctx, t, ns, supervisorIssuer, certSecretName1, kubeClient)
 
 	// Now that the Secret exists, we should be able to access the endpoints by hostname using the CA.
 	_ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1.Bundle()), issuer1, nil)
@@ -227,7 +224,7 @@ func TestSupervisorTLSTerminationWithSNI_Disruptive(t *testing.T) {
 	requireEndpointHasBootstrapTLSErrorBecauseCertificatesAreNotReady(t, issuer1)
 
 	// Create a Secret at the updated name.
-	ca1update := createTLSCertificateSecret(ctx, t, ns, testlib.NewSupervisorIssuer(t, hostname1), certSecretName1update, kubeClient)
+	ca1update := createTLSCertificateSecret(ctx, t, ns, supervisorIssuer, certSecretName1update, kubeClient)
 
 	// Now that the Secret exists at the new name, we should be able to access the endpoints by hostname using the CA.
 	_ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1update.Bundle()), issuer1, nil)
@@ -247,7 +244,7 @@ func TestSupervisorTLSTerminationWithSNI_Disruptive(t *testing.T) {
 	requireStatus(t, pinnipedClient, federationDomain2.Namespace, federationDomain2.Name, supervisorconfigv1alpha1.FederationDomainPhaseReady, withAllSuccessfulConditions())
 
 	// Create the Secret.
-	ca2 := createTLSCertificateSecret(ctx, t, ns, testlib.NewSupervisorIssuer(t, hostname2), certSecretName2, kubeClient)
+	ca2 := createTLSCertificateSecret(ctx, t, ns, testlib.NewSupervisorIssuer(t, issuer2), certSecretName2, kubeClient)
 
 	// Now that the Secret exists, we should be able to access the endpoints by hostname using the CA.
 	_ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, hostname2+":"+hostnamePort2, string(ca2.Bundle()), issuer2, map[string]string{
@@ -274,15 +271,12 @@ func TestSupervisorTLSTerminationWithDefaultCerts_Disruptive(t *testing.T) {
 	temporarilyRemoveAllFederationDomainsAndDefaultTLSCertSecret(ctx, t, ns, env.DefaultTLSCertSecretName(), pinnipedClient, kubeClient)
 
 	scheme := "https"
-	address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 8443
+	supervisorIssuer := testlib.NewSupervisorIssuer(t, env.SupervisorHTTPSAddress)
+	address := supervisorIssuer.Address() // hostname and port WITHOUT SCHEME for direct access to the supervisor's port 8443
 
-	hostAndPortSegments := strings.Split(address, ":")
 	// hostnames are case-insensitive, so test mis-matching the case of the issuer URL and the request URL
-	hostname := strings.ToLower(hostAndPortSegments[0])
-	port := "8443"
-	if len(hostAndPortSegments) > 1 {
-		port = hostAndPortSegments[1]
-	}
+	hostname := strings.ToLower(supervisorIssuer.Hostname())
+	port := supervisorIssuer.Port("8443")
 
 	ips, err := testlib.LookupIP(ctx, hostname)
 	require.NoError(t, err)
@@ -302,7 +296,7 @@ func TestSupervisorTLSTerminationWithDefaultCerts_Disruptive(t *testing.T) {
 	requireEndpointHasBootstrapTLSErrorBecauseCertificatesAreNotReady(t, issuerUsingIPAddress)
 
 	// Create a Secret at the special name which represents the default TLS cert.
-	defaultCA := createTLSCertificateSecret(ctx, t, ns, testlib.NewSupervisorIssuer(t, ipAsHostname), env.DefaultTLSCertSecretName(), kubeClient)
+	defaultCA := createTLSCertificateSecret(ctx, t, ns, testlib.NewSupervisorIssuer(t, issuerUsingIPAddress), env.DefaultTLSCertSecretName(), kubeClient)
 
 	// Now that the Secret exists, we should be able to access the endpoints by IP address using the CA.
 	_ = requireStandardDiscoveryEndpointsAreWorking(t, scheme, ipWithPort, string(defaultCA.Bundle()), issuerUsingIPAddress, nil)
@@ -317,7 +311,7 @@ func TestSupervisorTLSTerminationWithDefaultCerts_Disruptive(t *testing.T) {
 	requireStatus(t, pinnipedClient, federationDomain2.Namespace, federationDomain2.Name, supervisorconfigv1alpha1.FederationDomainPhaseReady, withAllSuccessfulConditions())
 
 	// Create the Secret.
-	certCA := createTLSCertificateSecret(ctx, t, ns, testlib.NewSupervisorIssuer(t, hostname), certSecretName, kubeClient)
+	certCA := createTLSCertificateSecret(ctx, t, ns, supervisorIssuer, certSecretName, kubeClient)
 
 	// Now that the Secret exists, we should be able to access the endpoints by hostname using the CA from the SNI cert.
 	// Hostnames are case-insensitive, so the request should still work even if the case of the hostname is different

test/integration/supervisor_healthz_test.go

@@ -37,10 +37,10 @@ func TestSupervisorHealthzBootstrap_Disruptive(t *testing.T) {
 
 	const badTLSConfigBody = "pinniped supervisor has invalid TLS serving certificate configuration\n"
 
-	httpGet(ctx, t, httpClient, fmt.Sprintf("https://%s/healthz", env.SupervisorHTTPSAddress), http.StatusOK, "ok")
-	httpGet(ctx, t, httpClient, fmt.Sprintf("https://%s", env.SupervisorHTTPSAddress), http.StatusInternalServerError, badTLSConfigBody)
-	httpGet(ctx, t, httpClient, fmt.Sprintf("https://%s/nothealthz", env.SupervisorHTTPSAddress), http.StatusInternalServerError, badTLSConfigBody)
-	httpGet(ctx, t, httpClient, fmt.Sprintf("https://%s/healthz/something", env.SupervisorHTTPSAddress), http.StatusInternalServerError, badTLSConfigBody)
+	httpGet(ctx, t, httpClient, fmt.Sprintf("%s/healthz", env.SupervisorHTTPSAddress), http.StatusOK, "ok")
+	httpGet(ctx, t, httpClient, env.SupervisorHTTPSAddress, http.StatusInternalServerError, badTLSConfigBody)
+	httpGet(ctx, t, httpClient, fmt.Sprintf("%s/nothealthz", env.SupervisorHTTPSAddress), http.StatusInternalServerError, badTLSConfigBody)
+	httpGet(ctx, t, httpClient, fmt.Sprintf("%s/healthz/something", env.SupervisorHTTPSAddress), http.StatusInternalServerError, badTLSConfigBody)
 }
 
 func httpGet(ctx context.Context, t *testing.T, client *http.Client, url string, expectedStatus int, expectedBody string) {

test/testlib/env.go

@@ -5,20 +5,16 @@ package testlib
 
 import (
 	"encoding/base64"
-	"net"
-	"net/url"
 	"os"
 	"sort"
 	"strings"
 	"sync"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 	"sigs.k8s.io/yaml"
 
 	authenticationv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1"
-	"go.pinniped.dev/internal/certauthority"
 )
 
 type Capability string
@@ -87,63 +83,6 @@ type TestOIDCUpstream struct {
 	ExpectedGroups   []string `json:"expectedGroups"`
 }
 
-type SupervisorIssuer struct {
-	issuerURL *url.URL
-}
-
-func NewSupervisorIssuer(t *testing.T, issuer string) SupervisorIssuer {
-	t.Helper()
-
-	t.Logf("NewSupervisorIssuer: %s", issuer)
-
-	issuerURL, err := url.Parse(issuer)
-	require.NoError(t, err)
-
-	return SupervisorIssuer{
-		issuerURL: issuerURL,
-	}
-}
-
-func (s SupervisorIssuer) Issuer() string {
-	return s.issuerURL.String()
-}
-
-func (s SupervisorIssuer) Hostnames() []string {
-	// TODO: Why does this happen?
-	if s.issuerURL.Hostname() == "" {
-		return []string{"localhost"}
-	}
-	return []string{s.issuerURL.Hostname()}
-}
-
-func (s SupervisorIssuer) IPs() []net.IP {
-	var ips []net.IP
-	if ip := net.ParseIP(s.issuerURL.Hostname()); ip != nil {
-		ips = append(ips, ip)
-	}
-	return ips
-}
-
-func (s SupervisorIssuer) IssuerServerCert(
-	t *testing.T,
-	ca *certauthority.CA,
-) ([]byte, []byte) {
-	t.Helper()
-
-	t.Logf("issuing server cert for Supervisor: hostname=%+v, ips=%+v",
-		s.Hostnames(), s.IPs())
-
-	cert, err := ca.IssueServerCert(s.Hostnames(), s.IPs(), 24*time.Hour)
-	require.NoError(t, err)
-	certPEM, keyPEM, err := certauthority.ToPEM(cert)
-	require.NoError(t, err)
-	return certPEM, keyPEM
-}
-
-func (s SupervisorIssuer) IsIPAddress() bool {
-	return len(s.IPs()) > 0
-}
-
 // InferSupervisorIssuerURL infers the downstream issuer URL from the callback associated with the upstream test client registration.
 func (e *TestEnv) InferSupervisorIssuerURL(t *testing.T) SupervisorIssuer {
 	t.Helper()

test/testlib/supervisor_issuer.go

@@ -0,0 +1,91 @@
+// Copyright 2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package testlib
+
+import (
+	"net"
+	"net/url"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"go.pinniped.dev/internal/certauthority"
+)
+
+type SupervisorIssuer struct {
+	issuerURL *url.URL
+	ip        net.IP
+}
+
+func NewSupervisorIssuer(t *testing.T, issuer string) SupervisorIssuer {
+	t.Helper()
+
+	t.Logf("NewSupervisorIssuer: %s", issuer)
+
+	issuerURL, err := url.Parse(issuer)
+	require.NoError(t, err)
+	require.NotEmpty(t, issuerURL.Hostname(), "hostname cannot be empty, usually this happens when the scheme is empty. issuer=%q", issuer)
+
+	ip := net.ParseIP(issuerURL.Hostname())
+
+	return SupervisorIssuer{
+		issuerURL: issuerURL,
+		ip:        ip,
+	}
+}
+
+func (s SupervisorIssuer) Issuer() string {
+	return s.issuerURL.String()
+}
+
+func (s SupervisorIssuer) Address() string {
+	return s.issuerURL.Host
+}
+
+func (s SupervisorIssuer) Hostname() string {
+	return s.issuerURL.Hostname()
+}
+
+func (s SupervisorIssuer) Port(defaultPort string) string {
+	port := s.issuerURL.Port()
+	if port == "" {
+		return defaultPort
+	}
+	return s.issuerURL.Port()
+}
+
+func (s SupervisorIssuer) Hostnames() []string {
+	if s.IsIPAddress() {
+		return nil // don't want DNS records in the cert when using IP address
+	}
+	return []string{s.issuerURL.Hostname()}
+}
+
+func (s SupervisorIssuer) IPs() []net.IP {
+	if !s.IsIPAddress() {
+		return nil
+	}
+	return []net.IP{s.ip}
+}
+
+func (s SupervisorIssuer) IssuerServerCert(
+	t *testing.T,
+	ca *certauthority.CA,
+) ([]byte, []byte) {
+	t.Helper()
+
+	t.Logf("issuing server cert for Supervisor: hostname=%+v, ips=%+v",
+		s.Hostnames(), s.IPs())
+
+	cert, err := ca.IssueServerCert(s.Hostnames(), s.IPs(), 24*time.Hour)
+	require.NoError(t, err)
+	certPEM, keyPEM, err := certauthority.ToPEM(cert)
+	require.NoError(t, err)
+	return certPEM, keyPEM
+}
+
+func (s SupervisorIssuer) IsIPAddress() bool {
+	return s.ip != nil
+}

test/testlib/supervisor_issuer_test.go

@@ -0,0 +1,82 @@
+// Copyright 2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package testlib
+
+import (
+	"net"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestSupervisorIssuer(t *testing.T) {
+	tests := []struct {
+		name            string
+		issuer          string
+		wantHostname    string
+		wantAddress     string
+		wantIP          net.IP
+		wantIsIPAddress bool
+	}{
+		{
+			name:         "works for localhost",
+			issuer:       "https://localhost:443",
+			wantHostname: "localhost",
+			wantAddress:  "localhost:443",
+		},
+		{
+			name:         "works for localhost with path",
+			issuer:       "https://localhost:443/some/path",
+			wantHostname: "localhost",
+			wantAddress:  "localhost:443",
+		},
+		{
+			name:         "works for domain",
+			issuer:       "https://example.com:443",
+			wantHostname: "example.com",
+			wantAddress:  "example.com:443",
+		},
+		{
+			name:         "works for domain with path",
+			issuer:       "https://example.com:443/some/path",
+			wantHostname: "example.com",
+			wantAddress:  "example.com:443",
+		},
+		{
+			name:            "works for IPv4",
+			issuer:          "https://1.2.3.4:443",
+			wantHostname:    "", // don't want DNS records in the cert when using IP address
+			wantAddress:     "1.2.3.4:443",
+			wantIP:          net.ParseIP("1.2.3.4"),
+			wantIsIPAddress: true,
+		},
+		{
+			name:            "works for IPv4 with path",
+			issuer:          "https://1.2.3.4:443/some/path",
+			wantHostname:    "", // don't want DNS records in the cert when using IP address
+			wantAddress:     "1.2.3.4:443",
+			wantIP:          net.ParseIP("1.2.3.4"),
+			wantIsIPAddress: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			supervisorIssuer := NewSupervisorIssuer(t, test.issuer)
+			require.Equal(t, test.issuer, supervisorIssuer.Issuer())
+			require.Equal(t, test.wantAddress, supervisorIssuer.Address())
+			if test.wantHostname != "" {
+				require.Equal(t, []string{test.wantHostname}, supervisorIssuer.Hostnames())
+			} else {
+				require.Nil(t, supervisorIssuer.Hostnames())
+			}
+			if test.wantIP != nil {
+				require.Equal(t, []net.IP{test.wantIP}, supervisorIssuer.IPs())
+			} else {
+				require.Nil(t, supervisorIssuer.IPs())
+			}
+			require.Equal(t, test.wantIsIPAddress, supervisorIssuer.IsIPAddress())
+		})
+	}
+}