Add impersonationProxyServerPort to the Concierge's static ConfigMap

- Used to determine on which port the impersonation proxy will bind
- Defaults to 8444, which is the old hard-coded port value
- Allow the port number to be configured to any value within the
  range 1024 to 65535
- This commit does not include adding new config knobs to the ytt
  values file, so while it is possible to change this port without
  needing to recompile, it is not convenient

deploy/concierge/deployment.yaml

@@ -59,6 +59,7 @@ data:
         renewBeforeSeconds: (@= str(data.values.api_serving_certificate_renew_before_seconds) @)
     apiGroupSuffix: (@= data.values.api_group_suffix @)
     # aggregatedAPIServerPort may be set here, although other YAML references to the default port (10250) may also need to be updated
+    # impersonationProxyServerPort may be set here, although other YAML references to the default port (8444) may also need to be updated
     names:
       servingCertificateSecret: (@= defaultResourceNameWithSuffix("api-tls-serving-certificate") @)
       credentialIssuer: (@= defaultResourceNameWithSuffix("config") @)

internal/concierge/server/server.go

@@ -150,6 +150,8 @@ func (a *App) runServer(ctx context.Context) error {
 			ServingCertDuration:              time.Duration(*cfg.APIConfig.ServingCertificateConfig.DurationSeconds) * time.Second,
 			ServingCertRenewBefore:           time.Duration(*cfg.APIConfig.ServingCertificateConfig.RenewBeforeSeconds) * time.Second,
 			AuthenticatorCache:               authenticators,
+			// This port should be safe to cast because the config reader already validated it.
+			ImpersonationProxyServerPort: int(*cfg.ImpersonationProxyServerPort),
 		},
 	)
 	if err != nil {

internal/config/concierge/config.go

@@ -27,6 +27,11 @@ const (
 	// allow traffic from the control plane to most ports, but do allow traffic to port 10250. This allows
 	// the Concierge to work without additional configuration on these types of clusters.
 	aggregatedAPIServerPortDefault = 10250
+
+	// Use port 8444 because that is the port that was selected for the first released version of the
+	// impersonation proxy, and has been the value since. It was originally selected because the
+	// aggregated API server used to run on 8443 (has since changed), so 8444 was the next available port.
+	impersonationProxyPortDefault = 8444
 )
 
 // FromPath loads an Config from a provided local file path, inserts any
@@ -49,6 +54,7 @@ func FromPath(path string) (*Config, error) {
 
 	maybeSetAPIDefaults(&config.APIConfig)
 	maybeSetAggregatedAPIServerPortDefaults(&config.AggregatedAPIServerPort)
+	maybeSetImpersonationProxyServerPortDefaults(&config.ImpersonationProxyServerPort)
 	maybeSetAPIGroupSuffixDefault(&config.APIGroupSuffix)
 	maybeSetKubeCertAgentDefaults(&config.KubeCertAgentConfig)
 
@@ -60,10 +66,14 @@ func FromPath(path string) (*Config, error) {
 		return nil, fmt.Errorf("validate apiGroupSuffix: %w", err)
 	}
 
-	if err := validateAggregatedAPIServerPort(config.AggregatedAPIServerPort); err != nil {
+	if err := validateServerPort(config.AggregatedAPIServerPort); err != nil {
 		return nil, fmt.Errorf("validate aggregatedAPIServerPort: %w", err)
 	}
 
+	if err := validateServerPort(config.ImpersonationProxyServerPort); err != nil {
+		return nil, fmt.Errorf("validate impersonationProxyServerPort: %w", err)
+	}
+
 	if err := validateNames(&config.NamesConfig); err != nil {
 		return nil, fmt.Errorf("validate names: %w", err)
 	}
@@ -95,9 +105,15 @@ func maybeSetAPIGroupSuffixDefault(apiGroupSuffix **string) {
 	}
 }
 
-func maybeSetAggregatedAPIServerPortDefaults(aggregatedAPIServerPort **int64) {
-	if *aggregatedAPIServerPort == nil {
-		*aggregatedAPIServerPort = pointer.Int64Ptr(aggregatedAPIServerPortDefault)
+func maybeSetAggregatedAPIServerPortDefaults(port **int64) {
+	if *port == nil {
+		*port = pointer.Int64Ptr(aggregatedAPIServerPortDefault)
+	}
+}
+
+func maybeSetImpersonationProxyServerPortDefaults(port **int64) {
+	if *port == nil {
+		*port = pointer.Int64Ptr(impersonationProxyPortDefault)
 	}
 }
 
@@ -165,9 +181,9 @@ func validateAPIGroupSuffix(apiGroupSuffix string) error {
 	return groupsuffix.Validate(apiGroupSuffix)
 }
 
-func validateAggregatedAPIServerPort(aggregatedAPIServerPort *int64) error {
+func validateServerPort(port *int64) error {
 	// It cannot be below 1024 because the container is not running as root.
-	if *aggregatedAPIServerPort < 1024 || *aggregatedAPIServerPort > 65535 {
+	if *port < 1024 || *port > 65535 {
 		return constable.Error("must be within range 1024 to 65535")
 	}
 	return nil

internal/config/concierge/config_test.go

@@ -34,6 +34,7 @@ func TestFromPath(t *testing.T) {
 					renewBeforeSeconds: 2400
 				apiGroupSuffix: some.suffix.com
 				aggregatedAPIServerPort: 12345
+				impersonationProxyServerPort: 4242
 				names:
 				  servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate
 				  credentialIssuer: pinniped-config
@@ -68,6 +69,7 @@ func TestFromPath(t *testing.T) {
 				},
 				APIGroupSuffix:               pointer.StringPtr("some.suffix.com"),
 				AggregatedAPIServerPort:      pointer.Int64Ptr(12345),
+				ImpersonationProxyServerPort: pointer.Int64Ptr(4242),
 				NamesConfig: NamesConfigSpec{
 					ServingCertificateSecret:          "pinniped-concierge-api-tls-serving-certificate",
 					CredentialIssuer:                  "pinniped-config",
@@ -112,6 +114,7 @@ func TestFromPath(t *testing.T) {
 				},
 				APIGroupSuffix:               pointer.StringPtr("pinniped.dev"),
 				AggregatedAPIServerPort:      pointer.Int64Ptr(10250),
+				ImpersonationProxyServerPort: pointer.Int64Ptr(8444),
 				APIConfig: APIConfigSpec{
 					ServingCertificateConfig: ServingCertificateConfigSpec{
 						DurationSeconds:    pointer.Int64Ptr(60 * 60 * 24 * 365),    // about a year
@@ -342,6 +345,22 @@ func TestFromPath(t *testing.T) {
 			`),
 			wantError: "validate aggregatedAPIServerPort: must be within range 1024 to 65535",
 		},
+		{
+			name: "ImpersonationProxyServerPort too small",
+			yaml: here.Doc(`
+				---
+				impersonationProxyServerPort: 1023
+			`),
+			wantError: "validate impersonationProxyServerPort: must be within range 1024 to 65535",
+		},
+		{
+			name: "ImpersonationProxyServerPort too large",
+			yaml: here.Doc(`
+				---
+				impersonationProxyServerPort: 65536
+			`),
+			wantError: "validate impersonationProxyServerPort: must be within range 1024 to 65535",
+		},
 		{
 			name: "ZeroRenewBefore",
 			yaml: here.Doc(`

internal/config/concierge/types.go

@@ -11,6 +11,7 @@ type Config struct {
 	APIConfig                    APIConfigSpec     `json:"api"`
 	APIGroupSuffix               *string           `json:"apiGroupSuffix,omitempty"`
 	AggregatedAPIServerPort      *int64            `json:"aggregatedAPIServerPort"`
+	ImpersonationProxyServerPort *int64            `json:"impersonationProxyServerPort"`
 	NamesConfig                  NamesConfigSpec   `json:"names"`
 	KubeCertAgentConfig          KubeCertAgentSpec `json:"kubeCertAgent"`
 	Labels                       map[string]string `json:"labels"`

internal/controller/impersonatorconfig/impersonator_config.go

@@ -48,7 +48,6 @@ import (
 )
 
 const (
-	impersonationProxyPort       = 8444
 	defaultHTTPSPort             = 443
 	approximatelyOneHundredYears = 100 * 365 * 24 * time.Hour
 	caCommonName                 = "Pinniped Impersonation Proxy Serving CA"
@@ -61,6 +60,7 @@ const (
 type impersonatorConfigController struct {
 	namespace                        string
 	credentialIssuerResourceName     string
+	impersonationProxyPort           int
 	generatedLoadBalancerServiceName string
 	generatedClusterIPServiceName    string
 	tlsSecretName                    string
@@ -96,6 +96,7 @@ func NewImpersonatorConfigController(
 	servicesInformer corev1informers.ServiceInformer,
 	secretsInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
+	impersonationProxyPort int,
 	generatedLoadBalancerServiceName string,
 	generatedClusterIPServiceName string,
 	tlsSecretName string,
@@ -115,6 +116,7 @@ func NewImpersonatorConfigController(
 			Syncer: &impersonatorConfigController{
 				namespace:                         namespace,
 				credentialIssuerResourceName:      credentialIssuerResourceName,
+				impersonationProxyPort:            impersonationProxyPort,
 				generatedLoadBalancerServiceName:  generatedLoadBalancerServiceName,
 				generatedClusterIPServiceName:     generatedClusterIPServiceName,
 				tlsSecretName:                     tlsSecretName,
@@ -401,9 +403,9 @@ func (c *impersonatorConfigController) ensureImpersonatorIsStarted(syncCtx contr
 		}
 	}
 
-	c.infoLog.Info("starting impersonation proxy", "port", impersonationProxyPort)
+	c.infoLog.Info("starting impersonation proxy", "port", c.impersonationProxyPort)
 	startImpersonatorFunc, err := c.impersonatorFunc(
-		impersonationProxyPort,
+		c.impersonationProxyPort,
 		c.tlsServingCertDynamicCertProvider,
 		c.impersonationSigningCertProvider,
 	)
@@ -436,7 +438,7 @@ func (c *impersonatorConfigController) ensureImpersonatorIsStopped(shouldCloseEr
 		return nil
 	}
 
-	c.infoLog.Info("stopping impersonation proxy", "port", impersonationProxyPort)
+	c.infoLog.Info("stopping impersonation proxy", "port", c.impersonationProxyPort)
 	close(c.serverStopCh)
 	stopErr := <-c.errorCh
 
@@ -457,7 +459,7 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C
 			Type: v1.ServiceTypeLoadBalancer,
 			Ports: []v1.ServicePort{
 				{
-					TargetPort: intstr.FromInt(impersonationProxyPort),
+					TargetPort: intstr.FromInt(c.impersonationProxyPort),
 					Port:       defaultHTTPSPort,
 					Protocol:   v1.ProtocolTCP,
 				},
@@ -503,7 +505,7 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStarted(ctx conte
 			Type: v1.ServiceTypeClusterIP,
 			Ports: []v1.ServicePort{
 				{
-					TargetPort: intstr.FromInt(impersonationProxyPort),
+					TargetPort: intstr.FromInt(c.impersonationProxyPort),
 					Port:       defaultHTTPSPort,
 					Protocol:   v1.ProtocolTCP,
 				},

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -51,6 +51,7 @@ import (
 func TestImpersonatorConfigControllerOptions(t *testing.T) {
 	spec.Run(t, "options", func(t *testing.T, when spec.G, it spec.S) {
 		const installedInNamespace = "some-namespace"
+		const impersonationProxyPort = 8444
 		const credentialIssuerResourceName = "some-credential-issuer-resource-name"
 		const generatedLoadBalancerServiceName = "some-service-resource-name"
 		const generatedClusterIPServiceName = "some-cluster-ip-resource-name"
@@ -84,6 +85,7 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) {
 				servicesInformer,
 				secretsInformer,
 				observableWithInformerOption.WithInformer,
+				impersonationProxyPort,
 				generatedLoadBalancerServiceName,
 				generatedClusterIPServiceName,
 				tlsSecretName,
@@ -252,6 +254,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 	name := t.Name()
 	spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) {
 		const installedInNamespace = "some-namespace"
+		const impersonationProxyPort = 8444
 		const credentialIssuerResourceName = "some-credential-issuer-resource-name"
 		const loadBalancerServiceName = "some-service-resource-name"
 		const clusterIPServiceName = "some-cluster-ip-resource-name"
@@ -553,6 +556,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				kubeInformers.Core().V1().Services(),
 				kubeInformers.Core().V1().Secrets(),
 				controllerlib.WithInformer,
+				impersonationProxyPort,
 				loadBalancerServiceName,
 				clusterIPServiceName,
 				tlsSecretName,

internal/controllermanager/prepare_controllers.go

@@ -59,6 +59,9 @@ type Config struct {
 	// the kubecertagent package's controllers should manage the agent pods.
 	KubeCertAgentConfig *concierge.KubeCertAgentSpec
 
+	// ImpersonationProxyServerPort decides which port the impersonation proxy should bind.
+	ImpersonationProxyServerPort int
+
 	// DiscoveryURLOverride allows a caller to inject a hardcoded discovery URL into Pinniped
 	// discovery document.
 	DiscoveryURLOverride *string
@@ -93,7 +96,7 @@ type Config struct {
 	Labels map[string]string
 }
 
-// Prepare the controllers and their informers and return a function that will start them when called.
+// PrepareControllers prepares the controllers and their informers and returns a function that will start them when called.
 //nolint:funlen // Eh, fair, it is a really long function...but it is wiring the world...so...
 func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) {
 	loginConciergeGroupData, identityConciergeGroupData := groupsuffix.ConciergeAggregatedGroups(c.APIGroupSuffix)
@@ -262,6 +265,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) {
 				informers.installationNamespaceK8s.Core().V1().Services(),
 				informers.installationNamespaceK8s.Core().V1().Secrets(),
 				controllerlib.WithInformer,
+				c.ImpersonationProxyServerPort,
 				c.NamesConfig.ImpersonationLoadBalancerService,
 				c.NamesConfig.ImpersonationClusterIPService,
 				c.NamesConfig.ImpersonationTLSCertificateSecret,