From 52b0cf43ca779da37af09cd0a550b1f928bc8eb9 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 7 Jun 2023 18:07:43 -0500
Subject: [PATCH 01/10] Fix godoc

---
 internal/certauthority/certauthority.go           |  8 ++++----
 internal/concierge/server/server.go               | 15 ++++++++-------
 internal/config/concierge/config.go               |  4 ++--
 internal/config/concierge/types.go                |  6 +++---
 internal/controllerlib/manager.go                 |  6 +++---
 .../oidc/provider/federation_domain_issuer.go     |  9 +++++++--
 site/content/docs/howto/install-supervisor.md     |  2 +-
 7 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go
index 9fcfdcfb..ab9e086d 100644
--- a/internal/certauthority/certauthority.go
+++ b/internal/certauthority/certauthority.go
@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package certauthority implements a simple x509 certificate authority suitable for use in an aggregated API service.
@@ -179,13 +179,13 @@ func (c *CA) IssueServerCert(dnsNames []string, ips []net.IP, ttl time.Duration)
 	return c.issueCert(x509.ExtKeyUsageServerAuth, pkix.Name{}, dnsNames, ips, ttl)
 }
 
-// Similar to IssueClientCert, but returning the new cert as a pair of PEM-formatted byte slices
+// IssueClientCertPEM is similar to IssueClientCert, but returns the new cert as a pair of PEM-formatted byte slices
 // for the certificate and private key.
 func (c *CA) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) {
 	return toPEM(c.IssueClientCert(username, groups, ttl))
 }
 
-// Similar to IssueServerCert, but returning the new cert as a pair of PEM-formatted byte slices
+// IssueServerCertPEM is similar to IssueServerCert, but returns the new cert as a pair of PEM-formatted byte slices
 // for the certificate and private key.
 func (c *CA) IssueServerCertPEM(dnsNames []string, ips []net.IP, ttl time.Duration) ([]byte, []byte, error) {
 	return toPEM(c.IssueServerCert(dnsNames, ips, ttl))
@@ -260,7 +260,7 @@ func toPEM(cert *tls.Certificate, err error) ([]byte, []byte, error) {
 	return certPEM, keyPEM, nil
 }
 
-// Encode a tls.Certificate into a private key PEM and a cert chain PEM.
+// ToPEM encodes a tls.Certificate into a private key PEM and a cert chain PEM.
 func ToPEM(cert *tls.Certificate) ([]byte, []byte, error) {
 	// Encode the certificate(s) to PEM.
 	certPEMBlocks := make([][]byte, 0, len(cert.Certificate))
diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go
index 906a727d..a3ede977 100644
--- a/internal/concierge/server/server.go
+++ b/internal/concierge/server/server.go
@@ -1,4 +1,4 @@
-// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package server is the command line entry point for pinniped-concierge.
@@ -118,17 +118,17 @@ func (a *App) runServer(ctx context.Context) error {
 
 	// This cert provider will provide certs to the API server and will
 	// be mutated by a controller to keep the certs up to date with what
-	// is stored in a k8s Secret. Therefore it also effectively acting as
-	// an in-memory cache of what is stored in the k8s Secret, helping to
-	// keep incoming requests fast.
+	// is stored in a k8s Secret. Therefore, it acts as an in-memory cache
+	// of what is stored in the k8s Secret, helping to keep incoming requests
+	// fast.
 	dynamicServingCertProvider := dynamiccert.NewServingCert("concierge-serving-cert")
 
 	// This cert provider will be used to provide the Kube signing key to the
-	// cert issuer used to issue certs to Pinniped clients wishing to login.
+	// cert issuer used to issue certs to Pinniped clients wishing to log in.
 	dynamicSigningCertProvider := dynamiccert.NewCA("concierge-kube-signing-cert")
 
 	// This cert provider will be used to provide the impersonation proxy signing key to the
-	// cert issuer used to issue certs to Pinniped clients wishing to login.
+	// cert issuer used to issue certs to Pinniped clients wishing to log in.
 	impersonationProxySigningCertProvider := dynamiccert.NewCA("impersonation-proxy-signing-cert")
 
 	// Get the "real" name of the login concierge API group (i.e., the API group name with the
@@ -256,7 +256,8 @@ func getAggregatedAPIServerConfig(
 	return apiServerConfig, nil
 }
 
-func main() error { // return an error instead of plog.Fatal to allow defer statements to run
+// main returns an error instead of calling plog.Fatal to allow defer statements to run
+func main() error {
 	defer plog.Setup()()
 
 	// Dump out the time since compile (mostly useful for benchmarking our local development cycle latency).
diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go
index 9a4e6aea..c0881a05 100644
--- a/internal/config/concierge/config.go
+++ b/internal/config/concierge/config.go
@@ -1,4 +1,4 @@
-// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package concierge contains functionality to load/store Config's from/to
@@ -35,7 +35,7 @@ const (
 	impersonationProxyPortDefault = 8444
 )
 
-// FromPath loads an Config from a provided local file path, inserts any
+// FromPath loads a Config from a provided local file path, inserts any
 // defaults (from the Config documentation), and verifies that the config is
 // valid (per the Config documentation).
 //
diff --git a/internal/config/concierge/types.go b/internal/config/concierge/types.go
index f444dc82..6e818704 100644
--- a/internal/config/concierge/types.go
+++ b/internal/config/concierge/types.go
@@ -1,11 +1,11 @@
-// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package concierge
 
 import "go.pinniped.dev/internal/plog"
 
-// Config contains knobs to setup an instance of the Pinniped Concierge.
+// Config contains knobs to set up an instance of the Pinniped Concierge.
 type Config struct {
 	DiscoveryInfo                DiscoveryInfoSpec `json:"discovery"`
 	APIConfig                    APIConfigSpec     `json:"api"`
@@ -21,7 +21,7 @@ type Config struct {
 }
 
 // DiscoveryInfoSpec contains configuration knobs specific to
-// pinniped's publishing of discovery information. These values can be
+// Pinniped's publishing of discovery information. These values can be
 // viewed as overrides, i.e., if these are set, then Pinniped will
 // publish these values in its discovery document instead of the ones it finds.
 type DiscoveryInfoSpec struct {
diff --git a/internal/controllerlib/manager.go b/internal/controllerlib/manager.go
index d0babbbe..69647fbb 100644
--- a/internal/controllerlib/manager.go
+++ b/internal/controllerlib/manager.go
@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package controllerlib
@@ -39,8 +39,8 @@ func (c *controllerManager) WithController(controller Controller, workers int) M
 	return c
 }
 
-// Start will run all managed controllers and block until all controllers shutdown.
-// When the context passed is cancelled, all controllers are signalled to shutdown.
+// Start will run all managed controllers and block until all controllers have shut down.
+// When the context passed is cancelled, all controllers are signalled to shut down.
 func (c *controllerManager) Start(ctx context.Context) {
 	var wg sync.WaitGroup
 	wg.Add(len(c.controllers))
diff --git a/internal/oidc/provider/federation_domain_issuer.go b/internal/oidc/provider/federation_domain_issuer.go
index be683c69..a5f19f8c 100644
--- a/internal/oidc/provider/federation_domain_issuer.go
+++ b/internal/oidc/provider/federation_domain_issuer.go
@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package provider
@@ -11,7 +11,7 @@ import (
 	"go.pinniped.dev/internal/constable"
 )
 
-// FederationDomainIssuer represents all of the settings and state for a downstream OIDC provider
+// FederationDomainIssuer represents all the settings and state for a downstream OIDC provider
 // as defined by a FederationDomain.
 type FederationDomainIssuer struct {
 	issuer     string
@@ -19,6 +19,8 @@ type FederationDomainIssuer struct {
 	issuerPath string
 }
 
+// NewFederationDomainIssuer returns a FederationDomainIssuer.
+// Performs validation, and returns any error from validation.
 func NewFederationDomainIssuer(issuer string) (*FederationDomainIssuer, error) {
 	p := FederationDomainIssuer{issuer: issuer}
 	err := p.validate()
@@ -64,14 +66,17 @@ func (p *FederationDomainIssuer) validate() error {
 	return nil
 }
 
+// Issuer returns the issuer
 func (p *FederationDomainIssuer) Issuer() string {
 	return p.issuer
 }
 
+// IssuerHost returns the issuerHost
 func (p *FederationDomainIssuer) IssuerHost() string {
 	return p.issuerHost
 }
 
+// IssuerPath returns the issuerPath
 func (p *FederationDomainIssuer) IssuerPath() string {
 	return p.issuerPath
 }
diff --git a/site/content/docs/howto/install-supervisor.md b/site/content/docs/howto/install-supervisor.md
index 15c38766..376e2a72 100644
--- a/site/content/docs/howto/install-supervisor.md
+++ b/site/content/docs/howto/install-supervisor.md
@@ -59,7 +59,7 @@ Pinniped uses [ytt](https://carvel.dev/ytt/) from [Carvel](https://carvel.dev/)
 1. Customize configuration parameters:
 
     - See the [default values](http://github.com/vmware-tanzu/pinniped/tree/main/deploy/supervisor/values.yaml) for documentation about individual configuration parameters.
-      For example, you can change the number of Concierge pods by setting `replicas` or apply custom annotations to the impersonation proxy service using `impersonation_proxy_spec`.
+      For example, you can change the number of Supervisor pods by setting `replicas` or install into a non-default namespace using `into_namespace`.
 
     - In a different directory, create a new YAML file to contain your site-specific configuration. For example, you might call this file `site/dev-env.yaml`.
 

From f8ce2af08c0e0a2fb6b1700249026ab4df1530a8 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 7 Jun 2023 18:08:23 -0500
Subject: [PATCH 02/10] Use go:embed for easier to read tests

---
 internal/certauthority/certauthority_test.go | 82 +++++++++++---------
 1 file changed, 44 insertions(+), 38 deletions(-)

diff --git a/internal/certauthority/certauthority_test.go b/internal/certauthority/certauthority_test.go
index 7109f669..ce5e24ed 100644
--- a/internal/certauthority/certauthority_test.go
+++ b/internal/certauthority/certauthority_test.go
@@ -7,10 +7,10 @@ import (
 	"crypto"
 	"crypto/tls"
 	"crypto/x509"
+	_ "embed"
 	"fmt"
 	"io"
 	"net"
-	"os"
 	"strings"
 	"testing"
 	"time"
@@ -20,60 +20,66 @@ import (
 	"go.pinniped.dev/internal/testutil"
 )
 
-func loadFromFiles(t *testing.T, certPath string, keyPath string) (*CA, error) {
-	t.Helper()
-
-	certPEM, err := os.ReadFile(certPath)
-	require.NoError(t, err)
-
-	keyPEM, err := os.ReadFile(keyPath)
-	require.NoError(t, err)
-
-	ca, err := Load(string(certPEM), string(keyPEM))
-	return ca, err
-}
+var (
+	//go:embed testdata/empty
+	empty string
+	//go:embed testdata/invalid
+	invalid string
+	//go:embed testdata/multiple.crt
+	multiple string
+	//go:embed testdata/test.crt
+	testCert string
+	//go:embed testdata/test.key
+	testKey string
+	//go:embed testdata/test2.key
+	testKey2 string
+)
 
 func TestLoad(t *testing.T) {
+
 	tests := []struct {
-		name     string
-		certPath string
-		keyPath  string
-		wantErr  string
+		name    string
+		cert    string
+		key     string
+		wantErr string
+		test    []byte
 	}{
 		{
-			name:     "empty key",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/empty",
-			wantErr:  "could not load CA: tls: failed to find any PEM data in key input",
+			name:    "empty key",
+			cert:    testCert,
+			key:     empty,
+			wantErr: "could not load CA: tls: failed to find any PEM data in key input",
 		},
 		{
-			name:     "invalid key",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/invalid",
-			wantErr:  "could not load CA: tls: failed to find any PEM data in key input",
+			name:    "invalid key",
+			cert:    testCert,
+			key:     invalid,
+			wantErr: "could not load CA: tls: failed to find any PEM data in key input",
 		},
 		{
-			name:     "mismatched cert and key",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/test2.key",
-			wantErr:  "could not load CA: tls: private key does not match public key",
+			name:    "mismatched cert and key",
+			cert:    testCert,
+			key:     testKey2,
+			wantErr: "could not load CA: tls: private key does not match public key",
 		},
 		{
-			name:     "multiple certs",
-			certPath: "./testdata/multiple.crt",
-			keyPath:  "./testdata/test.key",
-			wantErr:  "invalid CA certificate: expected a single certificate, found 2 certificates",
+			name:    "multiple certs",
+			cert:    multiple,
+			key:     testKey,
+			wantErr: "invalid CA certificate: expected a single certificate, found 2 certificates",
 		},
 		{
-			name:     "success",
-			certPath: "./testdata/test.crt",
-			keyPath:  "./testdata/test.key",
+			name: "success",
+			cert: testCert,
+			key:  testKey,
 		},
 	}
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			ca, err := loadFromFiles(t, tt.certPath, tt.keyPath)
+			t.Parallel()
+
+			ca, err := Load(tt.cert, tt.key)
 			if tt.wantErr != "" {
 				require.EqualError(t, err, tt.wantErr)
 				return
@@ -226,7 +232,7 @@ func TestIssue(t *testing.T) {
 
 	now := time.Date(2020, 7, 10, 12, 41, 12, 1234, time.UTC)
 
-	realCA, err := loadFromFiles(t, "./testdata/test.crt", "./testdata/test.key")
+	realCA, err := Load(testCert, testKey)
 	require.NoError(t, err)
 
 	tests := []struct {

From 8d8e1f3abdd865e2248aa5bbaf8354cd62cff980 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 7 Jun 2023 18:08:33 -0500
Subject: [PATCH 03/10] Backfill issuer tests

---
 internal/issuer/issuer_test.go | 168 +++++++++++++++++++++++++++++++++
 1 file changed, 168 insertions(+)
 create mode 100644 internal/issuer/issuer_test.go

diff --git a/internal/issuer/issuer_test.go b/internal/issuer/issuer_test.go
new file mode 100644
index 00000000..6ce628bc
--- /dev/null
+++ b/internal/issuer/issuer_test.go
@@ -0,0 +1,168 @@
+// Copyright 2023 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package issuer
+
+import (
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/require"
+	"go.pinniped.dev/internal/mocks/issuermocks"
+)
+
+func TestName(t *testing.T) {
+	ctrl := gomock.NewController(t)
+
+	tests := []struct {
+		name             string
+		buildIssuerMocks func() ClientCertIssuers
+		want             string
+	}{
+		{
+			name:             "empty issuers",
+			buildIssuerMocks: func() ClientCertIssuers { return ClientCertIssuers{} },
+			want:             "empty-client-cert-issuers",
+		},
+		{
+			name: "foo issuer",
+			buildIssuerMocks: func() ClientCertIssuers {
+				fooClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				fooClientCertIssuer.EXPECT().Name().Return("foo")
+
+				return ClientCertIssuers{fooClientCertIssuer}
+			},
+			want: "foo",
+		},
+		{
+			name: "foo and bar issuers",
+			buildIssuerMocks: func() ClientCertIssuers {
+				fooClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				fooClientCertIssuer.EXPECT().Name().Return("foo")
+
+				barClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				barClientCertIssuer.EXPECT().Name().Return("bar")
+
+				return ClientCertIssuers{fooClientCertIssuer, barClientCertIssuer}
+			},
+			want: "foo,bar",
+		},
+	}
+
+	for _, tTemp := range tests {
+		testcase := tTemp
+		t.Run(testcase.name, func(t *testing.T) {
+			t.Parallel()
+
+			name := testcase.buildIssuerMocks().Name()
+			require.Equal(t, testcase.want, name)
+		})
+	}
+}
+
+func TestIssueClientCertPEM(t *testing.T) {
+	ctrl := gomock.NewController(t)
+
+	tests := []struct {
+		name             string
+		buildIssuerMocks func() ClientCertIssuers
+		wantErrorMessage string
+		wantCert         []byte
+		wantKey          []byte
+	}{
+		{
+			name:             "empty issuers",
+			buildIssuerMocks: func() ClientCertIssuers { return ClientCertIssuers{} },
+			wantErrorMessage: "failed to issue cert",
+		},
+		{
+			name: "issuers with error",
+			buildIssuerMocks: func() ClientCertIssuers {
+				errClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				errClientCertIssuer.EXPECT().Name().Return("error cert issuer")
+				errClientCertIssuer.EXPECT().
+					IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second).
+					Return(nil, nil, errors.New("error from wrapped cert issuer"))
+				return ClientCertIssuers{errClientCertIssuer}
+			},
+			wantErrorMessage: "error cert issuer failed to issue client cert: error from wrapped cert issuer",
+		},
+		{
+			name: "valid issuer",
+			buildIssuerMocks: func() ClientCertIssuers {
+				validClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				validClientCertIssuer.EXPECT().
+					IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second).
+					Return([]byte("cert"), []byte("key"), nil)
+				return ClientCertIssuers{validClientCertIssuer}
+			},
+			wantCert: []byte("cert"),
+			wantKey:  []byte("key"),
+		},
+		{
+			name: "fallthrough issuer",
+			buildIssuerMocks: func() ClientCertIssuers {
+				errClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				errClientCertIssuer.EXPECT().Name().Return("error cert issuer")
+				errClientCertIssuer.EXPECT().
+					IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second).
+					Return(nil, nil, errors.New("error from wrapped cert issuer"))
+
+				validClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				validClientCertIssuer.EXPECT().
+					IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second).
+					Return([]byte("cert"), []byte("key"), nil)
+				return ClientCertIssuers{
+					errClientCertIssuer,
+					validClientCertIssuer,
+				}
+			},
+			wantCert: []byte("cert"),
+			wantKey:  []byte("key"),
+		},
+		{
+			name: "multiple error issuers",
+			buildIssuerMocks: func() ClientCertIssuers {
+				err1ClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				err1ClientCertIssuer.EXPECT().Name().Return("error1 cert issuer")
+				err1ClientCertIssuer.EXPECT().
+					IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second).
+					Return(nil, nil, errors.New("error1 from wrapped cert issuer"))
+
+				err2ClientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl)
+				err2ClientCertIssuer.EXPECT().Name().Return("error2 cert issuer")
+				err2ClientCertIssuer.EXPECT().
+					IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second).
+					Return(nil, nil, errors.New("error2 from wrapped cert issuer"))
+
+				return ClientCertIssuers{
+					err1ClientCertIssuer,
+					err2ClientCertIssuer,
+				}
+			},
+			wantErrorMessage: "[error1 cert issuer failed to issue client cert: error1 from wrapped cert issuer, error2 cert issuer failed to issue client cert: error2 from wrapped cert issuer]",
+		},
+	}
+
+	for _, tTemp := range tests {
+		testcase := tTemp
+		t.Run(testcase.name, func(t *testing.T) {
+			t.Parallel()
+
+			certPEM, keyPEM, err := testcase.buildIssuerMocks().
+				IssueClientCertPEM("username", []string{"group1", "group2"}, 32*time.Second)
+
+			if testcase.wantErrorMessage != "" {
+				require.ErrorContains(t, err, testcase.wantErrorMessage)
+				require.Empty(t, certPEM)
+				require.Empty(t, keyPEM)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, testcase.wantCert, certPEM)
+				require.Equal(t, testcase.wantKey, keyPEM)
+			}
+		})
+	}
+}

From 10c3e482b4407f700a85209b5b6c64543326e5e0 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 7 Jun 2023 20:50:09 -0500
Subject: [PATCH 04/10] Prefer early return

---
 internal/issuer/issuer.go | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/internal/issuer/issuer.go b/internal/issuer/issuer.go
index ef563e99..f4a392c5 100644
--- a/internal/issuer/issuer.go
+++ b/internal/issuer/issuer.go
@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package issuer
@@ -42,11 +42,10 @@ func (c ClientCertIssuers) IssueClientCertPEM(username string, groups []string,
 
 	for _, issuer := range c {
 		certPEM, keyPEM, err := issuer.IssueClientCertPEM(username, groups, ttl)
-		if err != nil {
-			errs = append(errs, fmt.Errorf("%s failed to issue client cert: %w", issuer.Name(), err))
-			continue
+		if err == nil {
+			return certPEM, keyPEM, nil
 		}
-		return certPEM, keyPEM, nil
+		errs = append(errs, fmt.Errorf("%s failed to issue client cert: %w", issuer.Name(), err))
 	}
 
 	if err := errors.NewAggregate(errs); err != nil {

From 500492544486485b4d393aad4c805a828ec409d1 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 7 Jun 2023 21:33:54 -0500
Subject: [PATCH 05/10] Backfill test cases

---
 internal/oidc/provider/federation_domain_issuer.go   |  4 ++++
 .../oidc/provider/federation_domain_issuer_test.go   | 12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/internal/oidc/provider/federation_domain_issuer.go b/internal/oidc/provider/federation_domain_issuer.go
index a5f19f8c..2cd04e8a 100644
--- a/internal/oidc/provider/federation_domain_issuer.go
+++ b/internal/oidc/provider/federation_domain_issuer.go
@@ -44,6 +44,10 @@ func (p *FederationDomainIssuer) validate() error {
 		return constable.Error(`issuer must have "https" scheme`)
 	}
 
+	if issuerURL.Hostname() == "" {
+		return constable.Error(`issuer must have a hostname`)
+	}
+
 	if issuerURL.User != nil {
 		return constable.Error(`issuer must not have username or password`)
 	}
diff --git a/internal/oidc/provider/federation_domain_issuer_test.go b/internal/oidc/provider/federation_domain_issuer_test.go
index 4f7c06e9..7f10dd33 100644
--- a/internal/oidc/provider/federation_domain_issuer_test.go
+++ b/internal/oidc/provider/federation_domain_issuer_test.go
@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package provider
@@ -20,6 +20,16 @@ func TestFederationDomainIssuerValidations(t *testing.T) {
 			issuer:    "",
 			wantError: "federation domain must have an issuer",
 		},
+		{
+			name:      "returns url.Parse errors",
+			issuer:    "https://example.com" + string(byte(0x7f)),
+			wantError: "could not parse issuer as URL: parse \"https://example.com\\x7f\": net/url: invalid control character in URL",
+		},
+		{
+			name:      "no hostname",
+			issuer:    "https://",
+			wantError: `issuer must have a hostname`,
+		},
 		{
 			name:      "no scheme",
 			issuer:    "tuna.com",

From 3d7eb55fc2d5304cf02199c26aa87a10f7836287 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Fri, 23 Jun 2023 13:52:32 -0500
Subject: [PATCH 06/10] Pass caBundle instead of an object

---
 .../impersonatorconfig/impersonator_config.go | 33 ++++++++++++++-----
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go
index aa261e42..5cc68f35 100644
--- a/internal/controller/impersonatorconfig/impersonator_config.go
+++ b/internal/controller/impersonatorconfig/impersonator_config.go
@@ -1,4 +1,4 @@
-// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonatorconfig
@@ -285,12 +285,10 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 		return nil, err
 	}
 
-	var impersonationCA *certauthority.CA
+	var impersonationCABundle []byte
 	if c.shouldHaveImpersonator(impersonationSpec) {
-		if impersonationCA, err = c.ensureCASecretIsCreated(ctx); err != nil {
-			return nil, err
-		}
-		if err = c.ensureTLSSecret(ctx, nameInfo, impersonationCA); err != nil {
+		impersonationCABundle, err = c.ensureCAAndTLSSecrets(ctx, nameInfo)
+		if err != nil {
 			return nil, err
 		}
 	} else {
@@ -300,7 +298,7 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 		c.clearTLSSecret()
 	}
 
-	credentialIssuerStrategyResult := c.doSyncResult(nameInfo, impersonationSpec, impersonationCA)
+	credentialIssuerStrategyResult := c.doSyncResult(nameInfo, impersonationSpec, impersonationCABundle)
 
 	if c.shouldHaveImpersonator(impersonationSpec) {
 		if err = c.loadSignerCA(); err != nil {
@@ -313,6 +311,23 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 	return credentialIssuerStrategyResult, nil
 }
 
+func (c *impersonatorConfigController) ensureCAAndTLSSecrets(ctx context.Context, nameInfo *certNameInfo) (impersonationCABundle []byte, err error) {
+	var impersonationCA *certauthority.CA
+	if impersonationCA, err = c.ensureCASecretIsCreated(ctx); err != nil {
+		return nil, err
+	}
+	if err = c.ensureTLSSecret(ctx, nameInfo, impersonationCA); err != nil {
+		return nil, err
+	}
+
+	if impersonationCA != nil {
+		return impersonationCA.Bundle(), nil
+	}
+
+	// untested
+	return nil, nil
+}
+
 func (c *impersonatorConfigController) loadImpersonationProxyConfiguration(credIssuer *v1alpha1.CredentialIssuer) (*v1alpha1.ImpersonationProxySpec, error) {
 	// Make a copy of the spec since we got this object from informer cache.
 	spec := credIssuer.Spec.DeepCopy().ImpersonationProxy
@@ -1018,7 +1033,7 @@ func (c *impersonatorConfigController) clearSignerCA() {
 	c.impersonationSigningCertProvider.UnsetCertKeyContent()
 }
 
-func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *v1alpha1.ImpersonationProxySpec, ca *certauthority.CA) *v1alpha1.CredentialIssuerStrategy {
+func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *v1alpha1.ImpersonationProxySpec, caBundle []byte) *v1alpha1.CredentialIssuerStrategy {
 	switch {
 	case c.disabledExplicitly(config):
 		return &v1alpha1.CredentialIssuerStrategy{
@@ -1055,7 +1070,7 @@ func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, conf
 				Type: v1alpha1.ImpersonationProxyFrontendType,
 				ImpersonationProxyInfo: &v1alpha1.ImpersonationProxyInfo{
 					Endpoint:                 "https://" + nameInfo.clientEndpoint,
-					CertificateAuthorityData: base64.StdEncoding.EncodeToString(ca.Bundle()),
+					CertificateAuthorityData: base64.StdEncoding.EncodeToString(caBundle),
 				},
 			},
 		}

From 183c771d4e8156471f9ffa12efbd12d6d7e1d42c Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Thu, 6 Jul 2023 17:14:39 -0700
Subject: [PATCH 07/10] Mark untested code paths

---
 .../impersonatorconfig/impersonator_config.go | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go
index 5cc68f35..a05dff37 100644
--- a/internal/controller/impersonatorconfig/impersonator_config.go
+++ b/internal/controller/impersonatorconfig/impersonator_config.go
@@ -256,6 +256,7 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 		}
 	} else {
 		if err = c.ensureImpersonatorIsStopped(true); err != nil {
+			// untested
 			return nil, err
 		}
 	}
@@ -378,6 +379,7 @@ func (c *impersonatorConfigController) serviceExists(serviceName string) (bool,
 		return false, nil, nil
 	}
 	if err != nil {
+		// untested
 		return false, nil, err
 	}
 	return true, service, nil
@@ -390,6 +392,7 @@ func (c *impersonatorConfigController) tlsSecretExists() (bool, *v1.Secret, erro
 		return false, nil, nil
 	}
 	if err != nil {
+		// untested
 		return false, nil, err
 	}
 	return true, secret, nil
@@ -496,6 +499,7 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C
 func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.Context) error {
 	running, service, err := c.serviceExists(c.generatedLoadBalancerServiceName)
 	if err != nil {
+		// untested
 		return err
 	}
 	if !running {
@@ -541,6 +545,7 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStarted(ctx conte
 func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx context.Context) error {
 	running, service, err := c.serviceExists(c.generatedClusterIPServiceName)
 	if err != nil {
+		// untested
 		return err
 	}
 	if !running {
@@ -576,6 +581,7 @@ func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context
 		sort.Strings(desiredAnnotationKeys)
 		keysJSONArray, err := json.Marshal(desiredAnnotationKeys)
 		if err != nil {
+			// untested
 			return err // This shouldn't really happen. We should always be able to marshal an array of strings.
 		}
 		// Save the desired annotations to a bookkeeping annotation.
@@ -590,6 +596,7 @@ func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context
 		return err
 	}
 	if err != nil {
+		// untested
 		return err
 	}
 
@@ -653,6 +660,7 @@ func (c *impersonatorConfigController) ensureTLSSecret(ctx context.Context, name
 	secretFromInformer, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.tlsSecretName)
 	notFound := k8serrors.IsNotFound(err)
 	if !notFound && err != nil {
+		// untested
 		return err
 	}
 
@@ -722,9 +730,10 @@ func (c *impersonatorConfigController) deleteTLSSecretWhenCertificateDoesNotMatc
 	}
 
 	if !nameInfo.ready {
-		// We currently have a secret but we are waiting for a load balancer to be assigned an ingress, so
+		// We currently have a secret, but we are waiting for a load balancer to be assigned an ingress, so
 		// our current secret must be old/unwanted.
 		if err = c.ensureTLSSecretIsRemoved(ctx); err != nil {
+			// untested
 			return false, err
 		}
 		return true, nil
@@ -770,6 +779,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 	if secret != nil {
 		err := c.loadTLSCertFromSecret(secret)
 		if err != nil {
+			// untested
 			return err
 		}
 		return nil
@@ -786,6 +796,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 
 	err = c.loadTLSCertFromSecret(newTLSSecret)
 	if err != nil {
+		// untested
 		return err
 	}
 
@@ -795,6 +806,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 func (c *impersonatorConfigController) ensureCASecretIsCreated(ctx context.Context) (*certauthority.CA, error) {
 	caSecret, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.caSecretName)
 	if err != nil && !k8serrors.IsNotFound(err) {
+		// untested
 		return nil, err
 	}
 
@@ -816,11 +828,13 @@ func (c *impersonatorConfigController) ensureCASecretIsCreated(ctx context.Conte
 func (c *impersonatorConfigController) createCASecret(ctx context.Context) (*certauthority.CA, error) {
 	impersonationCA, err := certauthority.New(caCommonName, approximatelyOneHundredYears)
 	if err != nil {
+		// untested
 		return nil, fmt.Errorf("could not create impersonation CA: %w", err)
 	}
 
 	caPrivateKeyPEM, err := impersonationCA.PrivateKeyToPEM()
 	if err != nil {
+		// untested
 		return nil, err
 	}
 
@@ -874,6 +888,7 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer()
 		return &certNameInfo{ready: false}, nil
 	}
 	if err != nil {
+		// untested
 		return nil, err
 	}
 	ingresses := lb.Status.LoadBalancer.Ingress
@@ -908,6 +923,7 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromClusterIPServic
 		return &certNameInfo{ready: false}, nil
 	}
 	if err != nil {
+		// untested
 		return nil, err
 	}
 	ip := clusterIP.Spec.ClusterIP
@@ -924,6 +940,7 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromClusterIPServic
 		}
 		return &certNameInfo{ready: true, selectedIPs: parsedIPs, clientEndpoint: ip}, nil
 	}
+	// untested
 	return &certNameInfo{ready: false}, nil
 }
 
@@ -935,11 +952,13 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c
 
 	impersonationCert, err := ca.IssueServerCert(hostnames, ips, approximatelyOneHundredYears)
 	if err != nil {
+		// untested
 		return nil, fmt.Errorf("could not create impersonation cert: %w", err)
 	}
 
 	certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert)
 	if err != nil {
+		// untested
 		return nil, err
 	}
 
@@ -969,6 +988,7 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre
 	keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey]
 
 	if err := c.tlsServingCertDynamicCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil {
+		// untested
 		return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err)
 	}
 
@@ -983,6 +1003,7 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre
 func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Context) error {
 	tlsSecretExists, secret, err := c.tlsSecretExists()
 	if err != nil {
+		// untested
 		return err
 	}
 	if !tlsSecretExists {

From 741ccfd2ce0ce4778bde101937fc9e15c93a74d8 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Fri, 7 Jul 2023 09:52:32 -0700
Subject: [PATCH 08/10] Fix lint

---
 internal/certauthority/certauthority_test.go       | 1 -
 internal/concierge/server/server.go                | 2 +-
 internal/issuer/issuer.go                          | 2 +-
 internal/issuer/issuer_test.go                     | 1 +
 internal/oidc/provider/federation_domain_issuer.go | 6 +++---
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/internal/certauthority/certauthority_test.go b/internal/certauthority/certauthority_test.go
index ce5e24ed..31251206 100644
--- a/internal/certauthority/certauthority_test.go
+++ b/internal/certauthority/certauthority_test.go
@@ -36,7 +36,6 @@ var (
 )
 
 func TestLoad(t *testing.T) {
-
 	tests := []struct {
 		name    string
 		cert    string
diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go
index a3ede977..fa2e07d3 100644
--- a/internal/concierge/server/server.go
+++ b/internal/concierge/server/server.go
@@ -256,7 +256,7 @@ func getAggregatedAPIServerConfig(
 	return apiServerConfig, nil
 }
 
-// main returns an error instead of calling plog.Fatal to allow defer statements to run
+// main returns an error instead of calling plog.Fatal to allow defer statements to run.
 func main() error {
 	defer plog.Setup()()
 
diff --git a/internal/issuer/issuer.go b/internal/issuer/issuer.go
index f4a392c5..8023b7e7 100644
--- a/internal/issuer/issuer.go
+++ b/internal/issuer/issuer.go
@@ -38,7 +38,7 @@ func (c ClientCertIssuers) Name() string {
 }
 
 func (c ClientCertIssuers) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) {
-	var errs []error
+	errs := make([]error, 0, len(c))
 
 	for _, issuer := range c {
 		certPEM, keyPEM, err := issuer.IssueClientCertPEM(username, groups, ttl)
diff --git a/internal/issuer/issuer_test.go b/internal/issuer/issuer_test.go
index 6ce628bc..114e5f15 100644
--- a/internal/issuer/issuer_test.go
+++ b/internal/issuer/issuer_test.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
+
 	"go.pinniped.dev/internal/mocks/issuermocks"
 )
 
diff --git a/internal/oidc/provider/federation_domain_issuer.go b/internal/oidc/provider/federation_domain_issuer.go
index 2cd04e8a..29e3683c 100644
--- a/internal/oidc/provider/federation_domain_issuer.go
+++ b/internal/oidc/provider/federation_domain_issuer.go
@@ -70,17 +70,17 @@ func (p *FederationDomainIssuer) validate() error {
 	return nil
 }
 
-// Issuer returns the issuer
+// Issuer returns the issuer.
 func (p *FederationDomainIssuer) Issuer() string {
 	return p.issuer
 }
 
-// IssuerHost returns the issuerHost
+// IssuerHost returns the issuerHost.
 func (p *FederationDomainIssuer) IssuerHost() string {
 	return p.issuerHost
 }
 
-// IssuerPath returns the issuerPath
+// IssuerPath returns the issuerPath.
 func (p *FederationDomainIssuer) IssuerPath() string {
 	return p.issuerPath
 }

From c142c52258ccff45fc7d9394b7f5400ba34d583e Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 19 Jul 2023 15:49:22 -0500
Subject: [PATCH 09/10] Do not name return variables

---
 .../controller/impersonatorconfig/impersonator_config.go   | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go
index a05dff37..2d6decf7 100644
--- a/internal/controller/impersonatorconfig/impersonator_config.go
+++ b/internal/controller/impersonatorconfig/impersonator_config.go
@@ -312,8 +312,11 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 	return credentialIssuerStrategyResult, nil
 }
 
-func (c *impersonatorConfigController) ensureCAAndTLSSecrets(ctx context.Context, nameInfo *certNameInfo) (impersonationCABundle []byte, err error) {
-	var impersonationCA *certauthority.CA
+func (c *impersonatorConfigController) ensureCAAndTLSSecrets(ctx context.Context, nameInfo *certNameInfo) ([]byte, error) {
+	var (
+		impersonationCA *certauthority.CA
+		err             error
+	)
 	if impersonationCA, err = c.ensureCASecretIsCreated(ctx); err != nil {
 		return nil, err
 	}

From 39912060f729d96b12f2f4adbaa39b9a056c1490 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 19 Jul 2023 15:50:12 -0500
Subject: [PATCH 10/10] Remove untested comments

---
 .../impersonatorconfig/impersonator_config.go | 22 -------------------
 1 file changed, 22 deletions(-)

diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go
index 2d6decf7..90f51b06 100644
--- a/internal/controller/impersonatorconfig/impersonator_config.go
+++ b/internal/controller/impersonatorconfig/impersonator_config.go
@@ -256,7 +256,6 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context, cre
 		}
 	} else {
 		if err = c.ensureImpersonatorIsStopped(true); err != nil {
-			// untested
 			return nil, err
 		}
 	}
@@ -328,7 +327,6 @@ func (c *impersonatorConfigController) ensureCAAndTLSSecrets(ctx context.Context
 		return impersonationCA.Bundle(), nil
 	}
 
-	// untested
 	return nil, nil
 }
 
@@ -382,7 +380,6 @@ func (c *impersonatorConfigController) serviceExists(serviceName string) (bool,
 		return false, nil, nil
 	}
 	if err != nil {
-		// untested
 		return false, nil, err
 	}
 	return true, service, nil
@@ -395,7 +392,6 @@ func (c *impersonatorConfigController) tlsSecretExists() (bool, *v1.Secret, erro
 		return false, nil, nil
 	}
 	if err != nil {
-		// untested
 		return false, nil, err
 	}
 	return true, secret, nil
@@ -502,7 +498,6 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C
 func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.Context) error {
 	running, service, err := c.serviceExists(c.generatedLoadBalancerServiceName)
 	if err != nil {
-		// untested
 		return err
 	}
 	if !running {
@@ -548,7 +543,6 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStarted(ctx conte
 func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx context.Context) error {
 	running, service, err := c.serviceExists(c.generatedClusterIPServiceName)
 	if err != nil {
-		// untested
 		return err
 	}
 	if !running {
@@ -584,7 +578,6 @@ func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context
 		sort.Strings(desiredAnnotationKeys)
 		keysJSONArray, err := json.Marshal(desiredAnnotationKeys)
 		if err != nil {
-			// untested
 			return err // This shouldn't really happen. We should always be able to marshal an array of strings.
 		}
 		// Save the desired annotations to a bookkeeping annotation.
@@ -599,7 +592,6 @@ func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context
 		return err
 	}
 	if err != nil {
-		// untested
 		return err
 	}
 
@@ -663,7 +655,6 @@ func (c *impersonatorConfigController) ensureTLSSecret(ctx context.Context, name
 	secretFromInformer, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.tlsSecretName)
 	notFound := k8serrors.IsNotFound(err)
 	if !notFound && err != nil {
-		// untested
 		return err
 	}
 
@@ -736,7 +727,6 @@ func (c *impersonatorConfigController) deleteTLSSecretWhenCertificateDoesNotMatc
 		// We currently have a secret, but we are waiting for a load balancer to be assigned an ingress, so
 		// our current secret must be old/unwanted.
 		if err = c.ensureTLSSecretIsRemoved(ctx); err != nil {
-			// untested
 			return false, err
 		}
 		return true, nil
@@ -782,7 +772,6 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 	if secret != nil {
 		err := c.loadTLSCertFromSecret(secret)
 		if err != nil {
-			// untested
 			return err
 		}
 		return nil
@@ -799,7 +788,6 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 
 	err = c.loadTLSCertFromSecret(newTLSSecret)
 	if err != nil {
-		// untested
 		return err
 	}
 
@@ -809,7 +797,6 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 func (c *impersonatorConfigController) ensureCASecretIsCreated(ctx context.Context) (*certauthority.CA, error) {
 	caSecret, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.caSecretName)
 	if err != nil && !k8serrors.IsNotFound(err) {
-		// untested
 		return nil, err
 	}
 
@@ -831,13 +818,11 @@ func (c *impersonatorConfigController) ensureCASecretIsCreated(ctx context.Conte
 func (c *impersonatorConfigController) createCASecret(ctx context.Context) (*certauthority.CA, error) {
 	impersonationCA, err := certauthority.New(caCommonName, approximatelyOneHundredYears)
 	if err != nil {
-		// untested
 		return nil, fmt.Errorf("could not create impersonation CA: %w", err)
 	}
 
 	caPrivateKeyPEM, err := impersonationCA.PrivateKeyToPEM()
 	if err != nil {
-		// untested
 		return nil, err
 	}
 
@@ -891,7 +876,6 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer()
 		return &certNameInfo{ready: false}, nil
 	}
 	if err != nil {
-		// untested
 		return nil, err
 	}
 	ingresses := lb.Status.LoadBalancer.Ingress
@@ -926,7 +910,6 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromClusterIPServic
 		return &certNameInfo{ready: false}, nil
 	}
 	if err != nil {
-		// untested
 		return nil, err
 	}
 	ip := clusterIP.Spec.ClusterIP
@@ -943,7 +926,6 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromClusterIPServic
 		}
 		return &certNameInfo{ready: true, selectedIPs: parsedIPs, clientEndpoint: ip}, nil
 	}
-	// untested
 	return &certNameInfo{ready: false}, nil
 }
 
@@ -955,13 +937,11 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c
 
 	impersonationCert, err := ca.IssueServerCert(hostnames, ips, approximatelyOneHundredYears)
 	if err != nil {
-		// untested
 		return nil, fmt.Errorf("could not create impersonation cert: %w", err)
 	}
 
 	certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert)
 	if err != nil {
-		// untested
 		return nil, err
 	}
 
@@ -991,7 +971,6 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre
 	keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey]
 
 	if err := c.tlsServingCertDynamicCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil {
-		// untested
 		return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err)
 	}
 
@@ -1006,7 +985,6 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre
 func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Context) error {
 	tlsSecretExists, secret, err := c.tlsSecretExists()
 	if err != nil {
-		// untested
 		return err
 	}
 	if !tlsSecretExists {