Merge pull request #888 from vmware-tanzu/customize_ports

Make Concierge server port numbers configurable

Dockerfile

@@ -29,8 +29,8 @@ FROM gcr.io/distroless/static:nonroot@sha256:bca3c203cdb36f5914ab8568e4c25165643
 # Copy the server binary from the build-env stage.
 COPY --from=build-env /usr/local/bin /usr/local/bin
 
-# Document the ports
+# Document the default server ports for the various server apps
-EXPOSE 8080 8443
+EXPOSE 8080 8443 8444 10250
 
 # Run as non-root for security posture
 # Use the same non-root user as https://github.com/GoogleContainerTools/distroless/blob/fc3c4eaceb0518900f886aae90407c43be0a42d9/base/base.bzl#L9

deploy/concierge/deployment.yaml

@@ -58,6 +58,8 @@ data:
         durationSeconds: (@= str(data.values.api_serving_certificate_duration_seconds) @)
         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") @)
@@ -175,7 +177,7 @@ spec:
           livenessProbe:
             httpGet:
               path: /healthz
-              port: 8443
+              port: 10250
               scheme: HTTPS
             initialDelaySeconds: 2
             timeoutSeconds: 15
@@ -184,7 +186,7 @@ spec:
           readinessProbe:
             httpGet:
               path: /healthz
-              port: 8443
+              port: 10250
               scheme: HTTPS
             initialDelaySeconds: 2
             timeoutSeconds: 3
@@ -251,7 +253,7 @@ spec:
   ports:
     - protocol: TCP
       port: 443
-      targetPort: 8443
+      targetPort: 10250
 ---
 apiVersion: v1
 kind: Service

deploy/concierge/values.yaml

@@ -41,7 +41,7 @@ kube_cert_agent_image:
 image_pull_dockerconfigjson: #! e.g. {"auths":{"https://registry.example.com":{"username":"USERNAME","password":"PASSWORD","auth":"BASE64_ENCODED_USERNAME_COLON_PASSWORD"}}}
 
 #! Pinniped will try to guess the right K8s API URL for sharing that information with potential clients.
-#! This settings allows the guess to be overridden.
+#! This setting allows the guess to be overridden.
 #! Optional.
 discovery_url: #! e.g., https://example.com
 

internal/concierge/server/server.go

@@ -152,6 +152,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 {
@@ -170,6 +172,7 @@ func (a *App) runServer(ctx context.Context) error {
 		certIssuer,
 		buildControllers,
 		*cfg.APIGroupSuffix,
+		*cfg.AggregatedAPIServerPort,
 		scheme,
 		loginGV,
 		identityGV,
@@ -195,6 +198,7 @@ func getAggregatedAPIServerConfig(
 	issuer issuer.ClientCertIssuer,
 	buildControllers controllerinit.RunnerBuilder,
 	apiGroupSuffix string,
+	aggregatedAPIServerPort int64,
 	scheme *runtime.Scheme,
 	loginConciergeGroupVersion, identityConciergeGroupVersion schema.GroupVersion,
 ) (*apiserver.Config, error) {
@@ -209,7 +213,9 @@ func getAggregatedAPIServerConfig(
 	)
 	recommendedOptions.Etcd = nil // turn off etcd storage because we don't need it yet
 	recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider
-	recommendedOptions.SecureServing.BindPort = 8443 // Don't run on default 443 because that requires root
+
+	// This port is configurable. It should be safe to cast because the config reader already validated it.
+	recommendedOptions.SecureServing.BindPort = int(aggregatedAPIServerPort)
 
 	// secure TLS for connections coming from and going to the Kube API server
 	// this is best effort because not all options provide the right hooks to override TLS config

internal/config/concierge/config.go

@@ -21,6 +21,17 @@ import (
 const (
 	aboutAYear   = 60 * 60 * 24 * 365
 	about9Months = 60 * 60 * 24 * 30 * 9
+
+	// Use 10250 because it happens to be the same port on which the Kubelet listens, so some cluster types
+	// are more permissive with servers that run on this port. For example, GKE private clusters do not
+	// 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
@@ -42,6 +53,8 @@ func FromPath(path string) (*Config, error) {
 	}
 
 	maybeSetAPIDefaults(&config.APIConfig)
+	maybeSetAggregatedAPIServerPortDefaults(&config.AggregatedAPIServerPort)
+	maybeSetImpersonationProxyServerPortDefaults(&config.ImpersonationProxyServerPort)
 	maybeSetAPIGroupSuffixDefault(&config.APIGroupSuffix)
 	maybeSetKubeCertAgentDefaults(&config.KubeCertAgentConfig)
 
@@ -53,6 +66,14 @@ func FromPath(path string) (*Config, error) {
 		return nil, fmt.Errorf("validate apiGroupSuffix: %w", err)
 	}
 
+	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)
 	}
@@ -84,6 +105,18 @@ func maybeSetAPIGroupSuffixDefault(apiGroupSuffix **string) {
 	}
 }
 
+func maybeSetAggregatedAPIServerPortDefaults(port **int64) {
+	if *port == nil {
+		*port = pointer.Int64Ptr(aggregatedAPIServerPortDefault)
+	}
+}
+
+func maybeSetImpersonationProxyServerPortDefaults(port **int64) {
+	if *port == nil {
+		*port = pointer.Int64Ptr(impersonationProxyPortDefault)
+	}
+}
+
 func maybeSetKubeCertAgentDefaults(cfg *KubeCertAgentSpec) {
 	if cfg.NamePrefix == nil {
 		cfg.NamePrefix = pointer.StringPtr("pinniped-kube-cert-agent-")
@@ -147,3 +180,11 @@ func validateAPI(apiConfig *APIConfigSpec) error {
 func validateAPIGroupSuffix(apiGroupSuffix string) error {
 	return groupsuffix.Validate(apiGroupSuffix)
 }
+
+func validateServerPort(port *int64) error {
+	// It cannot be below 1024 because the container is not running as root.
+	if *port < 1024 || *port > 65535 {
+		return constable.Error("must be within range 1024 to 65535")
+	}
+	return nil
+}

internal/config/concierge/config_test.go

@@ -33,6 +33,8 @@ func TestFromPath(t *testing.T) {
 					durationSeconds: 3600
 					renewBeforeSeconds: 2400
 				apiGroupSuffix: some.suffix.com
+				aggregatedAPIServerPort: 12345
+				impersonationProxyServerPort: 4242
 				names:
 				  servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate
 				  credentialIssuer: pinniped-config
@@ -65,7 +67,9 @@ func TestFromPath(t *testing.T) {
 						RenewBeforeSeconds: pointer.Int64Ptr(2400),
 					},
 				},
-				APIGroupSuffix: pointer.StringPtr("some.suffix.com"),
+				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",
@@ -108,7 +112,9 @@ func TestFromPath(t *testing.T) {
 				DiscoveryInfo: DiscoveryInfoSpec{
 					URL: nil,
 				},
-				APIGroupSuffix: pointer.StringPtr("pinniped.dev"),
+				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
@@ -323,6 +329,38 @@ func TestFromPath(t *testing.T) {
 			`),
 			wantError: "validate api: renewBefore must be positive",
 		},
+		{
+			name: "AggregatedAPIServerPortDefault too small",
+			yaml: here.Doc(`
+				---
+				aggregatedAPIServerPort: 1023
+			`),
+			wantError: "validate aggregatedAPIServerPort: must be within range 1024 to 65535",
+		},
+		{
+			name: "AggregatedAPIServerPortDefault too large",
+			yaml: here.Doc(`
+				---
+				aggregatedAPIServerPort: 65536
+			`),
+			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

@@ -7,13 +7,15 @@ import "go.pinniped.dev/internal/plog"
 
 // Config contains knobs to setup an instance of the Pinniped Concierge.
 type Config struct {
-	DiscoveryInfo       DiscoveryInfoSpec `json:"discovery"`
+	DiscoveryInfo                DiscoveryInfoSpec `json:"discovery"`
-	APIConfig           APIConfigSpec     `json:"api"`
+	APIConfig                    APIConfigSpec     `json:"api"`
-	APIGroupSuffix      *string           `json:"apiGroupSuffix,omitempty"`
+	APIGroupSuffix               *string           `json:"apiGroupSuffix,omitempty"`
-	NamesConfig         NamesConfigSpec   `json:"names"`
+	AggregatedAPIServerPort      *int64            `json:"aggregatedAPIServerPort"`
-	KubeCertAgentConfig KubeCertAgentSpec `json:"kubeCertAgent"`
+	ImpersonationProxyServerPort *int64            `json:"impersonationProxyServerPort"`
-	Labels              map[string]string `json:"labels"`
+	NamesConfig                  NamesConfigSpec   `json:"names"`
-	LogLevel            plog.LogLevel     `json:"logLevel"`
+	KubeCertAgentConfig          KubeCertAgentSpec `json:"kubeCertAgent"`
+	Labels                       map[string]string `json:"labels"`
+	LogLevel                     plog.LogLevel     `json:"logLevel"`
 }
 
 // DiscoveryInfoSpec contains configuration knobs specific to

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,