diff --git a/deploy/concierge/deployment.yaml b/deploy/concierge/deployment.yaml index d1999886..9292168f 100644 --- a/deploy/concierge/deployment.yaml +++ b/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") @) diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index ff8e8ebc..f593fd0b 100644 --- a/internal/concierge/server/server.go +++ b/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 { diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go index 6c503178..4749c7ca 100644 --- a/internal/config/concierge/config.go +++ b/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 diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index 77bdf1d8..86139182 100644 --- a/internal/config/concierge/config_test.go +++ b/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 @@ -66,8 +67,9 @@ func TestFromPath(t *testing.T) { RenewBeforeSeconds: pointer.Int64Ptr(2400), }, }, - APIGroupSuffix: pointer.StringPtr("some.suffix.com"), - AggregatedAPIServerPort: pointer.Int64Ptr(12345), + 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", @@ -110,8 +112,9 @@ func TestFromPath(t *testing.T) { DiscoveryInfo: DiscoveryInfoSpec{ URL: nil, }, - APIGroupSuffix: pointer.StringPtr("pinniped.dev"), - AggregatedAPIServerPort: pointer.Int64Ptr(10250), + 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(` diff --git a/internal/config/concierge/types.go b/internal/config/concierge/types.go index cb93cb5c..8f2c15c9 100644 --- a/internal/config/concierge/types.go +++ b/internal/config/concierge/types.go @@ -7,14 +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"` - APIConfig APIConfigSpec `json:"api"` - APIGroupSuffix *string `json:"apiGroupSuffix,omitempty"` - AggregatedAPIServerPort *int64 `json:"aggregatedAPIServerPort"` - NamesConfig NamesConfigSpec `json:"names"` - KubeCertAgentConfig KubeCertAgentSpec `json:"kubeCertAgent"` - Labels map[string]string `json:"labels"` - LogLevel plog.LogLevel `json:"logLevel"` + DiscoveryInfo DiscoveryInfoSpec `json:"discovery"` + 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"` + LogLevel plog.LogLevel `json:"logLevel"` } // DiscoveryInfoSpec contains configuration knobs specific to diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index e7e25e4f..76d5334f 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/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, }, diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 7e17d15e..ec43849c 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/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, diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 6129a974..be3af899 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/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,