From 297a484948ab5aa5ab05e424ede69d04ad9ebe40 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 19 May 2021 11:40:32 -0500 Subject: [PATCH] Add more validation and update tests for impersonationProxy as pointer. Signed-off-by: Matt Moyer --- .../impersonatorconfig/impersonator_config.go | 108 +++++++-- .../impersonator_config_test.go | 224 ++++++++++++++---- .../concierge_impersonation_proxy_test.go | 8 +- 3 files changed, 269 insertions(+), 71 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index beb466e1..cf3939fb 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -12,6 +12,7 @@ import ( "fmt" "net" "reflect" + "strconv" "strings" "time" @@ -23,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -262,23 +264,28 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context) (*v func (c *impersonatorConfigController) loadImpersonationProxyConfiguration() (*v1alpha1.ImpersonationProxySpec, error) { credIssuer, err := c.credIssuerInformer.Lister().Get(c.credentialIssuerResourceName) - if err != nil { return nil, fmt.Errorf("failed to get %s CredentialIssuer: %w", c.credentialIssuerResourceName, err) } - credIssuer = credIssuer.DeepCopy() - err = validateCredentialIssuerSpec(credIssuer) - if err != nil { - return nil, fmt.Errorf("invalid impersonator configuration: %v", err) + // Make a copy of the spec since we got this object from informer cache. + spec := credIssuer.Spec.DeepCopy().ImpersonationProxy + if spec == nil { + return nil, fmt.Errorf("could not load CredentialIssuer: spec.impersonationProxy is nil") + } + + // Default service type to LoadBalancer (this is normally already done via CRD defaulting). + if spec.Service.Type == "" { + spec.Service.Type = v1alpha1.ImpersonationProxyServiceTypeLoadBalancer + } + + if err := validateCredentialIssuerSpec(spec); err != nil { + return nil, fmt.Errorf("could not load CredentialIssuer spec.impersonationProxy: %w", err) } plog.Info("Read impersonation proxy config", "credentialIssuer", c.credentialIssuerResourceName, ) - if credIssuer.Spec.ImpersonationProxy.Service.Type == "" { - credIssuer.Spec.ImpersonationProxy.Service.Type = v1alpha1.ImpersonationProxyServiceTypeLoadBalancer - } - return &credIssuer.Spec.ImpersonationProxy, nil + return spec, nil } func (c *impersonatorConfigController) shouldHaveImpersonator(config *v1alpha1.ImpersonationProxySpec) bool { @@ -455,7 +462,7 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C "service", c.generatedLoadBalancerServiceName, "namespace", c.namespace) _, err = c.k8sClient.CoreV1().Services(c.namespace).Update(ctx, &loadBalancer, metav1.UpdateOptions{}) - + // TODO: make sure this error is handled if necessary } return nil } @@ -894,19 +901,80 @@ func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, conf } } -func validateCredentialIssuerSpec(credIssuer *v1alpha1.CredentialIssuer) error { - // TODO check external endpoint for valid ip or hostname - impersonationProxySpec := credIssuer.Spec.ImpersonationProxy - if impersonationProxySpec.Mode != v1alpha1.ImpersonationProxyModeDisabled && - impersonationProxySpec.ExternalEndpoint == "" && impersonationProxySpec.Service.Type == v1alpha1.ImpersonationProxyServiceTypeNone { - return fmt.Errorf("invalid impersonation proxy configuration: must specify an external endpoint or set a service type") - } - switch mode := credIssuer.Spec.ImpersonationProxy.Mode; mode { - case v1alpha1.ImpersonationProxyModeAuto: +func validateCredentialIssuerSpec(spec *v1alpha1.ImpersonationProxySpec) error { + // Validate that the mode is one of our known values. + switch spec.Mode { case v1alpha1.ImpersonationProxyModeDisabled: + case v1alpha1.ImpersonationProxyModeAuto: case v1alpha1.ImpersonationProxyModeEnabled: default: - return fmt.Errorf("invalid impersonation proxy mode %q, valid values are auto, disabled, or enabled", mode) + return fmt.Errorf("invalid proxy mode %q (expected auto, disabled, or enabled)", spec.Mode) } + + // If disabled, ignore all other fields and consider the configuration valid. + if spec.Mode == v1alpha1.ImpersonationProxyModeDisabled { + return nil + } + + // Validate that the service type is one of our known values. + switch spec.Service.Type { + case v1alpha1.ImpersonationProxyServiceTypeNone: + case v1alpha1.ImpersonationProxyServiceTypeLoadBalancer: + case v1alpha1.ImpersonationProxyServiceTypeClusterIP: + default: + return fmt.Errorf("invalid service type %q (expected None, LoadBalancer, or ClusterIP)", spec.Service.Type) + } + + // If specified, validate that the LoadBalancerIP is a valid IPv4 or IPv6 address. + if ip := spec.Service.LoadBalancerIP; ip != "" && len(validation.IsValidIP(ip)) > 0 { + return fmt.Errorf("invalid LoadBalancerIP %q", spec.Service.LoadBalancerIP) + } + + // If service is type "None", a non-empty external endpoint must be specified. + if spec.ExternalEndpoint == "" && spec.Service.Type == v1alpha1.ImpersonationProxyServiceTypeNone { + return fmt.Errorf("externalEndpoint must be set when service.type is None") + } + + if err := validateExternalEndpoint(spec.ExternalEndpoint); err != nil { + return fmt.Errorf("invalid ExternalEndpoint %q: %w", spec.ExternalEndpoint, err) + } + return nil } + +func validateExternalEndpoint(endpoint string) error { + // Empty string is valid (no external endpoint, default to service name) + if endpoint == "" { + return nil + } + + // Try parsing it both with and without an implicit port 443 at the end. + host, port, err := net.SplitHostPort(endpoint) + + // If we got an error parsing the raw input, try adding an implicit port 443. + if err != nil { + host, port, err = net.SplitHostPort(net.JoinHostPort(endpoint, "443")) + } + + // If there's still an error, fail now. + if err != nil { + return err + } + + portInt, _ := strconv.Atoi(port) + if len(validation.IsValidPortNum(portInt)) > 0 { + return fmt.Errorf("invalid port %q", port) + } + + // Check if the host part is a valid IP address. + if len(validation.IsValidIP(host)) == 0 { + return nil + } + + // Check if the host part is a valid hostname according to RFC 1123. + if len(validation.IsDNS1123Subdomain(host)) == 0 { + return nil + } + + return fmt.Errorf("host %q is not a valid hostname or IP address", host) +} diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 3fa247e2..4f84b799 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -1019,7 +1019,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, ExternalEndpoint: localhostIP, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1068,7 +1068,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -1360,14 +1360,13 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireSigningCertProviderIsEmpty() }) }) - }) when("the configuration is disabled mode", func() { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeDisabled, }, }, pinnipedInformerClient) @@ -1392,7 +1391,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("no load balancer", func() { it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, }, }, pinnipedInformerClient) @@ -1423,7 +1422,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("a loadbalancer already exists", func() { it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, }, }, pinnipedInformerClient) @@ -1456,7 +1455,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { var caCrt []byte it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, }, }, pinnipedInformerClient) @@ -1486,7 +1485,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { annotations := map[string]string{"some-annotation-key": "some-annotation-value"} it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, Service: v1alpha1.ImpersonationProxyServiceSpec{ Type: v1alpha1.ImpersonationProxyServiceTypeLoadBalancer, @@ -1515,7 +1514,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeHostname = "fake.example.com" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostname, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1544,7 +1543,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeHostname = "fake.example.com" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostname, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1574,7 +1573,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeIPWithPort = "127.0.0.1:3000" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeIPWithPort, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1603,7 +1602,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeHostnameWithPort = "fake.example.com:3000" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostnameWithPort, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1632,7 +1631,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeHostnameWithPort = "fake.example.com:3000" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostnameWithPort, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1665,7 +1664,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeIP = "127.0.0.42" var hostnameConfig = v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostname, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1674,7 +1673,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }, } var ipAddressConfig = v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeIP, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1739,7 +1738,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeHostname = "fake.example.com" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostname, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1784,7 +1783,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeHostname = "fake.example.com" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostname, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1832,7 +1831,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { var caCrt []byte it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostname, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1904,7 +1903,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, }, }, pinnipedInformerClient) @@ -1929,7 +1928,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Update the CredentialIssuer. updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeDisabled, }, }, pinnipedInformers.Config().V1alpha1().CredentialIssuers()) @@ -1946,7 +1945,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Update the CredentialIssuer again. updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, }, }, pinnipedInformers.Config().V1alpha1().CredentialIssuers()) @@ -1964,7 +1963,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: localhostIP, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -1994,7 +1993,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Switch to "enabled" mode without an "endpoint", so a load balancer is needed now. updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, }, }, pinnipedInformers.Config().V1alpha1().CredentialIssuers()) @@ -2035,7 +2034,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Now switch back to having the "endpoint" specified and explicitly saying that we don't want the load balancer service. updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: localhostIP, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2059,7 +2058,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: localhostIP, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2093,7 +2092,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Add annotations to the spec. annotations := map[string]string{"my-annotation-key": "my-annotation-val"} updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: localhostIP, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2118,7 +2117,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addSecretToTrackers(signingCASecret, kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2236,7 +2235,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2263,7 +2262,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("getting the control plane nodes returns an error, e.g. when there are no nodes", func() { it("returns an error", func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2280,7 +2279,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addNodeWithRoleToTracker("worker", kubeAPIClient) impersonatorFuncReturnedFuncError = errors.New("some immediate impersonator startup error") addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2340,7 +2339,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addNodeWithRoleToTracker("worker", kubeAPIClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2393,10 +2392,27 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) }) - when("the CredentialIssuer is invalid", func() { + when("the CredentialIssuer has nil impersonation spec", func() { it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: nil, + }, pinnipedInformerClient) + }) + + it("returns an error", func() { + startInformersAndController() + errString := `could not load CredentialIssuer: spec.impersonationProxy is nil` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + requireTLSServerWasNeverStarted() + }) + }) + + when("the CredentialIssuer has invalid mode", func() { + it.Before(func() { + addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: "not-valid", }, }, pinnipedInformerClient) @@ -2404,7 +2420,71 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it("returns an error", func() { startInformersAndController() - errString := `invalid impersonator configuration: invalid impersonation proxy mode "not-valid", valid values are auto, disabled, or enabled` + errString := `could not load CredentialIssuer spec.impersonationProxy: invalid proxy mode "not-valid" (expected auto, disabled, or enabled)` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + requireTLSServerWasNeverStarted() + }) + }) + + when("the CredentialIssuer has invalid service type", func() { + it.Before(func() { + addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ + Mode: v1alpha1.ImpersonationProxyModeEnabled, + Service: v1alpha1.ImpersonationProxyServiceSpec{ + Type: "not-valid", + }, + }, + }, pinnipedInformerClient) + }) + + it("returns an error", func() { + startInformersAndController() + errString := `could not load CredentialIssuer spec.impersonationProxy: invalid service type "not-valid" (expected None, LoadBalancer, or ClusterIP)` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + requireTLSServerWasNeverStarted() + }) + }) + + when("the CredentialIssuer has invalid LoadBalancerIP", func() { + it.Before(func() { + addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ + Mode: v1alpha1.ImpersonationProxyModeEnabled, + Service: v1alpha1.ImpersonationProxyServiceSpec{ + LoadBalancerIP: "invalid-ip-address", + }, + }, + }, pinnipedInformerClient) + }) + + it("returns an error", func() { + startInformersAndController() + errString := `could not load CredentialIssuer spec.impersonationProxy: invalid LoadBalancerIP "invalid-ip-address"` + r.EqualError(runControllerSync(), errString) + requireCredentialIssuer(newErrorStrategy(errString)) + requireSigningCertProviderIsEmpty() + requireTLSServerWasNeverStarted() + }) + }) + + when("the CredentialIssuer has invalid ExternalEndpoint", func() { + it.Before(func() { + addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ + Mode: v1alpha1.ImpersonationProxyModeEnabled, + ExternalEndpoint: "[invalid", + }, + }, pinnipedInformerClient) + }) + + it("returns an error", func() { + startInformersAndController() + errString := `could not load CredentialIssuer spec.impersonationProxy: invalid ExternalEndpoint "[invalid": address [invalid:443: missing ']' in address` r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) requireSigningCertProviderIsEmpty() @@ -2419,7 +2499,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { return true, nil, fmt.Errorf("error on create") }) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2437,7 +2517,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("there is an error creating the tls secret", func() { it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: "example.com", Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2471,7 +2551,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { when("there is an error creating the CA secret", func() { it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: "example.com", Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2505,7 +2585,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addNodeWithRoleToTracker("control-plane", kubeAPIClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: "example.com", Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2535,7 +2615,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addLoadBalancerServiceToTracker(loadBalancerServiceName, kubeAPIClient) addSecretToTrackers(newEmptySecret(tlsSecretName), kubeAPIClient, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2562,7 +2642,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addNodeWithRoleToTracker("control-plane", kubeAPIClient) addSecretToTrackers(newEmptySecret(tlsSecretName), kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeDisabled, }, }, pinnipedInformerClient) @@ -2584,7 +2664,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: localhostIP, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2647,7 +2727,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, }, }, pinnipedInformerClient) @@ -2710,7 +2790,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addLoadBalancerServiceWithIngressToTracker(loadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: localhostIP}}, kubeInformerClient) addLoadBalancerServiceWithIngressToTracker(loadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: localhostIP}}, kubeAPIClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2755,7 +2835,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addSecretToTrackers(signingCASecret, kubeInformerClient) addNodeWithRoleToTracker("worker", kubeAPIClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeAuto, }, }, pinnipedInformerClient) @@ -2787,7 +2867,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { const fakeHostname = "foo.example.com" it.Before(func() { addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostname, Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2879,7 +2959,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addSecretToTrackers(signingCASecret, kubeInformerClient) addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: "", Service: v1alpha1.ImpersonationProxyServiceSpec{ @@ -2892,7 +2972,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it("returns a validation error", func() { startInformersAndController() - r.EqualError(runControllerSync(), "invalid impersonator configuration: invalid impersonation proxy configuration: must specify an external endpoint or set a service type") + r.EqualError(runControllerSync(), "could not load CredentialIssuer spec.impersonationProxy: externalEndpoint must be set when service.type is None") r.Len(kubeAPIClient.Actions(), 0) }) }) @@ -2921,3 +3001,53 @@ func (q *testQueue) AddRateLimited(key controllerlib.Key) { q.key = key } + +func TestValidateExternalEndpoint(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + input string + expectErr string + }{ + {input: ""}, + {input: "127.0.0.1"}, + {input: "127.0.0.1:8443"}, + {input: "[127.0.0.1]:8443"}, + {input: "2001:db8::ffff"}, + {input: "[2001:db8::ffff]:8443"}, + {input: "host.example.com"}, + {input: "host-dev.example.com"}, + {input: "host.example.com:8443"}, + {input: "[host.example.com]:8443"}, + { + input: "https://host.example.com", + expectErr: `invalid port "//host.example.com"`, + }, + { + input: "host.example.com/some/path", + expectErr: `host "host.example.com/some/path" is not a valid hostname or IP address`, + }, + { + input: "[host.example.com", + expectErr: "address [host.example.com:443: missing ']' in address", + }, + { + input: "___.example.com:1234", + expectErr: `host "___.example.com" is not a valid hostname or IP address`, + }, + { + input: "HOST.EXAMPLE.COM", + expectErr: `host "HOST.EXAMPLE.COM" is not a valid hostname or IP address`, + }, + } { + tt := tt + t.Run(fmt.Sprintf("parse %q", tt.input), func(t *testing.T) { + got := validateExternalEndpoint(tt.input) + if tt.expectErr == "" { + require.NoError(t, got) + } else { + require.EqualError(t, got, tt.expectErr) + } + }) + } +} diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index e3273cab..ff5ebd9e 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -193,7 +193,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl case impersonatorShouldHaveStartedAutomaticallyByDefault && clusterSupportsLoadBalancers: // configure the credential issuer spec to have the impersonation proxy in auto mode updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{ - ImpersonationProxy: conciergev1alpha.ImpersonationProxySpec{ + ImpersonationProxy: &conciergev1alpha.ImpersonationProxySpec{ Mode: conciergev1alpha.ImpersonationProxyModeAuto, Service: conciergev1alpha.ImpersonationProxyServiceSpec{ Type: conciergev1alpha.ImpersonationProxyServiceTypeLoadBalancer, @@ -223,7 +223,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // Create configuration to make the impersonation proxy turn on with no endpoint (i.e. automatically create a load balancer). updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{ - ImpersonationProxy: conciergev1alpha.ImpersonationProxySpec{ + ImpersonationProxy: &conciergev1alpha.ImpersonationProxySpec{ Mode: conciergev1alpha.ImpersonationProxyModeEnabled, }, }) @@ -249,7 +249,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // Create configuration to make the impersonation proxy turn on with a hard coded endpoint (without a load balancer). updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{ - ImpersonationProxy: conciergev1alpha.ImpersonationProxySpec{ + ImpersonationProxy: &conciergev1alpha.ImpersonationProxySpec{ Mode: conciergev1alpha.ImpersonationProxyModeEnabled, ExternalEndpoint: proxyServiceEndpoint, }, @@ -1185,7 +1185,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl t.Run("manually disabling the impersonation proxy feature", func(t *testing.T) { // Update configuration to force the proxy to disabled mode updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{ - ImpersonationProxy: conciergev1alpha.ImpersonationProxySpec{ + ImpersonationProxy: &conciergev1alpha.ImpersonationProxySpec{ Mode: conciergev1alpha.ImpersonationProxyModeDisabled, }, })