WIP: not using impersonator.config just credentialissuer directly

Signed-off-by: Matt Moyer <moyerm@vmware.com>

internal/concierge/impersonator/config.go

@@ -1,83 +0,0 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
-// SPDX-License-Identifier: Apache-2.0
-
-package impersonator
-
-import (
-	"fmt"
-
-	v1 "k8s.io/api/core/v1"
-	"sigs.k8s.io/yaml"
-
-	"go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
-)
-
-type Mode string
-
-const (
-	// Explicitly enable the impersonation proxy.
-	ModeEnabled Mode = "enabled"
-
-	// Explicitly disable the impersonation proxy.
-	ModeDisabled Mode = "disabled"
-
-	// Allow the proxy to decide if it should be enabled or disabled based upon the cluster in which it is running.
-	ModeAuto Mode = "auto"
-)
-
-const (
-	ConfigMapDataKey = "config.yaml"
-)
-
-type Config struct {
-	// Enable or disable the impersonation proxy. Optional. Defaults to ModeAuto.
-	Mode Mode `json:"mode,omitempty"`
-
-	// Used when creating TLS certificates and for clients to discover the endpoint. Optional. When not specified, if the
-	// impersonation proxy is started, then it will automatically create a LoadBalancer Service and use its ingress as the
-	// endpoint.
-	//
-	// When specified, it may be a hostname or IP address, optionally with a port number, of the impersonation proxy
-	// for clients to use from outside the cluster. E.g. myhost.mycompany.com:8443. Clients should assume that they should
-	// connect via HTTPS to this service.
-	Endpoint string `json:"endpoint,omitempty"`
-}
-
-func (c *Config) HasEndpoint() bool {
-	return c.Endpoint != ""
-}
-
-func NewConfig() *Config {
-	return &Config{Mode: ModeAuto}
-}
-
-func ConfigFromConfigMap(configMap *v1.ConfigMap) (*Config, error) {
-	stringConfig, ok := configMap.Data[ConfigMapDataKey]
-	if !ok {
-		return nil, fmt.Errorf(`ConfigMap is missing expected key "%s"`, ConfigMapDataKey)
-	}
-	config := NewConfig()
-	if err := yaml.Unmarshal([]byte(stringConfig), config); err != nil {
-		return nil, fmt.Errorf("decode yaml: %w", err)
-	}
-	if config.Mode != ModeAuto && config.Mode != ModeEnabled && config.Mode != ModeDisabled {
-		return nil, fmt.Errorf(`illegal value for "mode": %s`, config.Mode)
-	}
-	return config, nil
-}
-
-func ConfigFromCredentialIssuer(credIssuer *v1alpha1.CredentialIssuer) (*Config, error) {
-	config := NewConfig()
-	switch mode := credIssuer.Spec.ImpersonationProxy.Mode; mode {
-	case v1alpha1.ImpersonationProxyModeAuto:
-		config.Mode = ModeAuto
-	case v1alpha1.ImpersonationProxyModeDisabled:
-		config.Mode = ModeDisabled
-	case v1alpha1.ImpersonationProxyModeEnabled:
-		config.Mode = ModeEnabled
-	default:
-		return nil, fmt.Errorf("invalid impersonation proxy mode %q, valid values are auto, disabled, or enabled", mode)
-	}
-	config.Endpoint = credIssuer.Spec.ImpersonationProxy.ExternalEndpoint
-	return config, nil
-}

internal/concierge/impersonator/config_test.go

@@ -1,155 +0,0 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
-// SPDX-License-Identifier: Apache-2.0
-
-package impersonator
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/require"
-	v1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	"go.pinniped.dev/internal/here"
-)
-
-func TestNewConfig(t *testing.T) {
-	// It defaults the mode.
-	require.Equal(t, &Config{Mode: ModeAuto}, NewConfig())
-}
-
-func TestHasEndpoint(t *testing.T) {
-	configWithoutEndpoint := Config{}
-	configWithEndpoint := Config{Endpoint: "something"}
-	require.False(t, configWithoutEndpoint.HasEndpoint())
-	require.True(t, configWithEndpoint.HasEndpoint())
-}
-
-func TestConfigFromConfigMap(t *testing.T) {
-	tests := []struct {
-		name       string
-		configMap  *v1.ConfigMap
-		wantConfig *Config
-		wantError  string
-	}{
-		{
-			name: "fully configured, valid config",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"config.yaml": here.Doc(`
-						mode: enabled
-						endpoint: proxy.example.com:8443
-					`),
-				},
-			},
-			wantConfig: &Config{
-				Mode:     "enabled",
-				Endpoint: "proxy.example.com:8443",
-			},
-		},
-		{
-			name: "empty, valid config",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"config.yaml": "",
-				},
-			},
-			wantConfig: &Config{
-				Mode:     "auto",
-				Endpoint: "",
-			},
-		},
-		{
-			name: "valid config with mode enabled",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"config.yaml": "mode: enabled",
-				},
-			},
-			wantConfig: &Config{
-				Mode:     "enabled",
-				Endpoint: "",
-			},
-		},
-		{
-			name: "valid config with mode disabled",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"config.yaml": "mode: disabled",
-				},
-			},
-			wantConfig: &Config{
-				Mode:     "disabled",
-				Endpoint: "",
-			},
-		},
-		{
-			name: "valid config with mode auto",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"config.yaml": "mode: auto",
-				},
-			},
-			wantConfig: &Config{
-				Mode:     "auto",
-				Endpoint: "",
-			},
-		},
-		{
-			name: "wrong key in configmap",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"wrong-key": "",
-				},
-			},
-			wantError: `ConfigMap is missing expected key "config.yaml"`,
-		},
-		{
-			name: "illegal yaml in configmap",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"config.yaml": "this is not yaml",
-				},
-			},
-			wantError: "decode yaml: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type impersonator.Config",
-		},
-		{
-			name: "illegal value for mode in configmap",
-			configMap: &v1.ConfigMap{
-				TypeMeta:   metav1.TypeMeta{},
-				ObjectMeta: metav1.ObjectMeta{},
-				Data: map[string]string{
-					"config.yaml": "mode: unexpected-value",
-				},
-			},
-			wantError: `illegal value for "mode": unexpected-value`,
-		},
-	}
-
-	for _, tt := range tests {
-		test := tt
-		t.Run(test.name, func(t *testing.T) {
-			config, err := ConfigFromConfigMap(test.configMap)
-			require.Equal(t, test.wantConfig, config)
-			if test.wantError != "" {
-				require.EqualError(t, err, test.wantError)
-			} else {
-				require.NoError(t, err)
-			}
-		})
-	}
-}

internal/controller/impersonatorconfig/impersonator_config.go

@@ -259,55 +259,53 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context) (*v
 	return credentialIssuerStrategyResult, nil
 }
 
-func (c *impersonatorConfigController) loadImpersonationProxyConfiguration() (*impersonator.Config, error) {
+func (c *impersonatorConfigController) loadImpersonationProxyConfiguration() (*v1alpha1.ImpersonationProxySpec, error) {
 	credIssuer, err := c.credIssuerInformer.Lister().Get(c.credentialIssuerResourceName)
 
-	if k8serrors.IsNotFound(err) {
-		plog.Info("Did not find impersonation proxy config: using default config values",
-			"credentialIssuer", c.credentialIssuerResourceName,
-		)
-		return impersonator.NewConfig(), nil
-	}
-
 	if err != nil {
 		return nil, fmt.Errorf("failed to get %s CredentialIssuer: %w", c.credentialIssuerResourceName, err)
 	}
 
-	config, err := impersonator.ConfigFromCredentialIssuer(credIssuer)
+	credIssuer = credIssuer.DeepCopy()
+	err = validateCredentialIssuerSpec(credIssuer)
 	if err != nil {
 		return nil, fmt.Errorf("invalid impersonator configuration: %v", err)
 	}
 	plog.Info("Read impersonation proxy config",
 		"credentialIssuer", c.credentialIssuerResourceName,
 	)
-	return config, nil
+	if credIssuer.Spec.ImpersonationProxy.Service.Type == "" {
+		credIssuer.Spec.ImpersonationProxy.Service.Type = v1alpha1.ImpersonationProxyServiceTypeLoadBalancer
+	}
+	return &credIssuer.Spec.ImpersonationProxy, nil
 }
 
-func (c *impersonatorConfigController) shouldHaveImpersonator(config *impersonator.Config) bool {
+func (c *impersonatorConfigController) shouldHaveImpersonator(config *v1alpha1.ImpersonationProxySpec) bool {
-	return c.enabledByAutoMode(config) || config.Mode == impersonator.ModeEnabled
+	return c.enabledByAutoMode(config) || config.Mode == v1alpha1.ImpersonationProxyModeEnabled
 }
 
-func (c *impersonatorConfigController) enabledByAutoMode(config *impersonator.Config) bool {
+func (c *impersonatorConfigController) enabledByAutoMode(config *v1alpha1.ImpersonationProxySpec) bool {
-	return config.Mode == impersonator.ModeAuto && !*c.hasControlPlaneNodes
+	return config.Mode == v1alpha1.ImpersonationProxyModeAuto && !*c.hasControlPlaneNodes
 }
 
-func (c *impersonatorConfigController) disabledByAutoMode(config *impersonator.Config) bool {
+func (c *impersonatorConfigController) disabledByAutoMode(config *v1alpha1.ImpersonationProxySpec) bool {
-	return config.Mode == impersonator.ModeAuto && *c.hasControlPlaneNodes
+	return config.Mode == v1alpha1.ImpersonationProxyModeAuto && *c.hasControlPlaneNodes
 }
 
-func (c *impersonatorConfigController) disabledExplicitly(config *impersonator.Config) bool {
+func (c *impersonatorConfigController) disabledExplicitly(config *v1alpha1.ImpersonationProxySpec) bool {
-	return config.Mode == impersonator.ModeDisabled
+	return config.Mode == v1alpha1.ImpersonationProxyModeDisabled
 }
 
-func (c *impersonatorConfigController) shouldHaveLoadBalancer(config *impersonator.Config) bool {
+func (c *impersonatorConfigController) shouldHaveLoadBalancer(config *v1alpha1.ImpersonationProxySpec) bool {
-	return c.shouldHaveImpersonator(config) && !config.HasEndpoint()
+	return c.shouldHaveImpersonator(config) && config.Service.Type == v1alpha1.ImpersonationProxyServiceTypeLoadBalancer
 }
 
-func (c *impersonatorConfigController) shouldHaveTLSSecret(config *impersonator.Config) bool {
+func (c *impersonatorConfigController) shouldHaveTLSSecret(config *v1alpha1.ImpersonationProxySpec) bool {
 	return c.shouldHaveImpersonator(config)
 }
 
 func (c *impersonatorConfigController) updateStrategy(ctx context.Context, strategy *v1alpha1.CredentialIssuerStrategy) error {
+	// TODO use informer client rather than api client for reading
 	return issuerconfig.UpdateStrategy(ctx, c.credentialIssuerResourceName, c.labels, c.pinnipedAPIClient, *strategy)
 }
 
@@ -652,15 +650,25 @@ func (c *impersonatorConfigController) createCASecret(ctx context.Context) (*cer
 	return impersonationCA, nil
 }
 
-func (c *impersonatorConfigController) findDesiredTLSCertificateName(config *impersonator.Config) (*certNameInfo, error) {
+func (c *impersonatorConfigController) findDesiredTLSCertificateName(config *v1alpha1.ImpersonationProxySpec) (*certNameInfo, error) {
-	if config.HasEndpoint() {
+	// possible valid options:
+	// - you have a loadbalancer and are autoconfiguring the endpoint -> get cert info based on load balancer ip/hostnome
+	// - you have a loadbalancer AND an external endpoint -> either should work since they should be the same
+	// - external endpoint no loadbalancer or other service -> use the endpoint config
+	// - external endpoint and ClusterIP -> use external endpoint?
+	//
+	// - is it legal to have a clusterip and no external endpoint???
+	if config.ExternalEndpoint != "" {
 		return c.findTLSCertificateNameFromEndpointConfig(config), nil
+	} else if config.Service.Type == v1alpha1.ImpersonationProxyServiceTypeClusterIP {
+		// c.findTLSCertificateNameFromClusterIPService()
+		// TODO implement this
 	}
 	return c.findTLSCertificateNameFromLoadBalancer()
 }
 
-func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *impersonator.Config) *certNameInfo {
+func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *v1alpha1.ImpersonationProxySpec) *certNameInfo {
-	endpointMaybeWithPort := config.Endpoint
+	endpointMaybeWithPort := config.ExternalEndpoint
 	endpointWithoutPort := strings.Split(endpointMaybeWithPort, ":")[0]
 	parsedAsIP := net.ParseIP(endpointWithoutPort)
 	if parsedAsIP != nil {
@@ -820,7 +828,7 @@ func (c *impersonatorConfigController) clearSignerCA() {
 	c.impersonationSigningCertProvider.UnsetCertKeyContent()
 }
 
-func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *impersonator.Config, ca *certauthority.CA) *v1alpha1.CredentialIssuerStrategy {
+func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *v1alpha1.ImpersonationProxySpec, ca *certauthority.CA) *v1alpha1.CredentialIssuerStrategy {
 	switch {
 	case c.disabledExplicitly(config):
 		return &v1alpha1.CredentialIssuerStrategy{
@@ -863,3 +871,16 @@ func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, conf
 		}
 	}
 }
+
+func validateCredentialIssuerSpec(credIssuer *v1alpha1.CredentialIssuer) error {
+	// TODO check external endpoint for valid ip or hostname
+	// TODO if service type is none and externalendpoint is "" return error
+	switch mode := credIssuer.Spec.ImpersonationProxy.Mode; mode {
+	case v1alpha1.ImpersonationProxyModeAuto:
+	case v1alpha1.ImpersonationProxyModeDisabled:
+	case v1alpha1.ImpersonationProxyModeEnabled:
+	default:
+		return fmt.Errorf("invalid impersonation proxy mode %q, valid values are auto, disabled, or enabled", mode)
+	}
+	return nil
+}