supervisor-oidc: checkpoint: controller watches OIDCProviderConfig

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

cmd/pinniped-supervisor/main.go

@@ -6,22 +6,20 @@ package main
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
 	"net"
 	"net/http"
 	"os"
 	"os/signal"
 	"time"
 
-	kubeinformers "k8s.io/client-go/informers"
-	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/pkg/version"
 	"k8s.io/client-go/rest"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/component-base/logs"
 	"k8s.io/klog/v2"
-	"sigs.k8s.io/yaml"
 
+	pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned"
+	pinnipedinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions"
 	"go.pinniped.dev/internal/controller/supervisorconfig"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/downward"
@@ -69,63 +67,57 @@ func waitForSignal() os.Signal {
 func startControllers(
 	ctx context.Context,
 	issuerProvider *issuerprovider.Provider,
-	kubeClient kubernetes.Interface,
+	pinnipedInformers pinnipedinformers.SharedInformerFactory,
-	kubeInformers kubeinformers.SharedInformerFactory,
-	serverInstallationNamespace string,
-	staticConfig StaticConfig,
 ) {
 	// Create controller manager.
 	controllerManager := controllerlib.
 		NewManager().
 		WithController(
 			supervisorconfig.NewDynamicConfigWatcherController(
-				serverInstallationNamespace,
-				staticConfig.NamesConfig.DynamicConfigMap,
 				issuerProvider,
-				kubeClient,
+				pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(),
-				kubeInformers.Core().V1().ConfigMaps(),
 				controllerlib.WithInformer,
 			),
 			singletonWorker,
 		)
 
-	kubeInformers.Start(ctx.Done())
+	pinnipedInformers.Start(ctx.Done())
 
 	go controllerManager.Start(ctx)
 }
 
-func newK8sClient() (kubernetes.Interface, error) {
+func newPinnipedClient() (pinnipedclientset.Interface, error) {
 	kubeConfig, err := restclient.InClusterConfig()
 	if err != nil {
 		return nil, fmt.Errorf("could not load in-cluster configuration: %w", err)
 	}
 
 	// Connect to the core Kubernetes API.
-	kubeClient, err := kubernetes.NewForConfig(kubeConfig)
+	pinnipedClient, err := pinnipedclientset.NewForConfig(kubeConfig)
 	if err != nil {
 		return nil, fmt.Errorf("could not load in-cluster configuration: %w", err)
 	}
 
-	return kubeClient, nil
+	return pinnipedClient, nil
 }
 
-func run(serverInstallationNamespace string, staticConfig StaticConfig) error {
+func run(serverInstallationNamespace string) error {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	kubeClient, err := newK8sClient()
+	pinnipedClient, err := newPinnipedClient()
 	if err != nil {
 		return fmt.Errorf("cannot create k8s client: %w", err)
 	}
 
-	kubeInformers := kubeinformers.NewSharedInformerFactoryWithOptions(
+	pinnipedInformers := pinnipedinformers.NewSharedInformerFactoryWithOptions(
-		kubeClient,
+		pinnipedClient,
 		defaultResyncInterval,
-		kubeinformers.WithNamespace(serverInstallationNamespace),
+		pinnipedinformers.WithNamespace(serverInstallationNamespace),
 	)
 
 	issuerProvider := issuerprovider.New()
-	startControllers(ctx, issuerProvider, kubeClient, kubeInformers, serverInstallationNamespace, staticConfig)
+	startControllers(ctx, issuerProvider, pinnipedInformers)
 
 	//nolint: gosec // Intentionally binding to all network interfaces.
 	l, err := net.Listen("tcp", ":80")
@@ -143,14 +135,6 @@ func run(serverInstallationNamespace string, staticConfig StaticConfig) error {
 	return nil
 }
 
-type StaticConfig struct {
-	NamesConfig NamesConfigSpec `json:"names"`
-}
-
-type NamesConfigSpec struct {
-	DynamicConfigMap string `json:"dynamicConfigMap"`
-}
-
 func main() {
 	logs.InitLogs()
 	defer logs.FlushLogs()
@@ -164,17 +148,7 @@ func main() {
 		klog.Fatal(fmt.Errorf("could not read pod metadata: %w", err))
 	}
 
-	// Read static config.
+	if err := run(podInfo.Namespace); err != nil {
-	data, err := ioutil.ReadFile(os.Args[2])
-	if err != nil {
-		klog.Fatal(fmt.Errorf("read file: %w", err))
-	}
-	var staticConfig StaticConfig
-	if err := yaml.Unmarshal(data, &staticConfig); err != nil {
-		klog.Fatal(fmt.Errorf("decode yaml: %w", err))
-	}
-
-	if err := run(podInfo.Namespace, staticConfig); err != nil {
 		klog.Fatal(err)
 	}
 }

deploy-supervisor/rbac.yaml

@@ -13,8 +13,8 @@ metadata:
   labels:
     app: #@ data.values.app_name
 rules:
-  - apiGroups: [""]
+  - apiGroups: [config.pinniped.dev]
-    resources: [configmaps]
+    resources: [oidcproviderconfigs]
     verbs: [get, list, watch]
 ---
 kind: RoleBinding

internal/controller/supervisorconfig/dynamic_config_watcher.go

@@ -5,12 +5,12 @@ package supervisorconfig
 
 import (
 	"fmt"
+	"net/url"
 
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	corev1informers "k8s.io/client-go/informers/core/v1"
-	"k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
 
+	configinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions/config/v1alpha1"
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
 	"go.pinniped.dev/internal/controllerlib"
 )
@@ -22,41 +22,36 @@ const (
 // IssuerSetter can be notified of a valid issuer with its SetIssuer function. If there is no
 // longer any valid issuer, then nil can be passed to this interface.
 //
+// If the IssuerSetter doesn't like the provided issuer, it can return an error.
+//
 // Implementations of this type should be thread-safe to support calls from multiple goroutines.
 type IssuerSetter interface {
-	SetIssuer(issuer *string)
+	SetIssuer(issuer *url.URL) error
 }
 
 type dynamicConfigWatcherController struct {
-	configMapName      string
-	configMapNamespace string
 	issuerSetter IssuerSetter
-	k8sClient          kubernetes.Interface
+	opcInformer  configinformers.OIDCProviderConfigInformer
-	configMapInformer  corev1informers.ConfigMapInformer
 }
 
+// NewDynamicConfigWatcherController creates a controllerlib.Controller that watches
+// OIDCProviderConfig objects and notifies a callback object of their creation or deletion.
 func NewDynamicConfigWatcherController(
-	serverInstallationNamespace string,
-	configMapName string,
 	issuerObserver IssuerSetter,
-	k8sClient kubernetes.Interface,
+	opcInformer configinformers.OIDCProviderConfigInformer,
-	configMapInformer corev1informers.ConfigMapInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 ) controllerlib.Controller {
 	return controllerlib.New(
 		controllerlib.Config{
 			Name: "DynamicConfigWatcherController",
 			Syncer: &dynamicConfigWatcherController{
-				configMapNamespace: serverInstallationNamespace,
-				configMapName:      configMapName,
 				issuerSetter: issuerObserver,
-				k8sClient:          k8sClient,
+				opcInformer:  opcInformer,
-				configMapInformer:  configMapInformer,
 			},
 		},
 		withInformer(
-			configMapInformer,
+			opcInformer,
-			pinnipedcontroller.NameAndNamespaceExactMatchFilterFactory(configMapName, serverInstallationNamespace),
+			pinnipedcontroller.NoOpFilter(),
 			controllerlib.InformerOption{},
 		),
 	)
@@ -69,44 +64,49 @@ func (c *dynamicConfigWatcherController) Sync(ctx controllerlib.Context) error {
 	// TODO The discovery endpoint would return an error until all missing configuration options are
 	// filled in.
 
-	configMap, err := c.configMapInformer.
+	opc, err := c.opcInformer.
 		Lister().
-		ConfigMaps(c.configMapNamespace).
+		OIDCProviderConfigs(ctx.Key.Namespace).
-		Get(c.configMapName)
+		Get(ctx.Key.Name)
 	notFound := k8serrors.IsNotFound(err)
 	if err != nil && !notFound {
-		return fmt.Errorf("failed to get %s/%s secret: %w", c.configMapNamespace, c.configMapName, err)
+		return fmt.Errorf("failed to get %s/%s oidcproviderconfig: %w", ctx.Key.Namespace, ctx.Key.Name, err)
 	}
 
 	if notFound {
 		klog.InfoS(
-			"dynamicConfigWatcherController Sync found no configmap",
+			"dynamicConfigWatcherController Sync found no oidcproviderconfig",
-			"configmap",
+			"oidcproviderconfig",
-			klog.KRef(c.configMapNamespace, c.configMapName),
+			klog.KRef(ctx.Key.Namespace, ctx.Key.Name),
 		)
 		c.issuerSetter.SetIssuer(nil)
 		return nil
 	}
 
-	issuer, ok := configMap.Data[issuerConfigMapKey]
+	url, err := url.Parse(opc.Spec.Issuer)
-	if !ok {
+	if err != nil {
 		klog.InfoS(
-			"dynamicConfigWatcherController Sync found no issuer",
+			"dynamicConfigWatcherController Sync failed to parse issuer",
-			"configmap",
+			"err",
-			klog.KObj(configMap),
+			err,
 		)
-		c.issuerSetter.SetIssuer(nil)
 		return nil
 	}
 
 	klog.InfoS(
 		"dynamicConfigWatcherController Sync issuer",
-		"configmap",
+		"oidcproviderconfig",
-		klog.KObj(configMap),
+		klog.KObj(opc),
 		"issuer",
-		issuer,
+		url,
 	)
-	c.issuerSetter.SetIssuer(&issuer)
+	if err := c.issuerSetter.SetIssuer(url); err != nil {
+		klog.InfoS(
+			"dynamicConfigWatcherController Sync failed to set issuer",
+			"err",
+			err,
+		)
+	}
 
 	return nil
 }

internal/oidc/discovery/discovery.go

@@ -8,6 +8,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"net/url"
 )
 
 // Metadata holds all fields (that we care about) from the OpenID Provider Metadata section in the
@@ -30,7 +31,7 @@ type Metadata struct {
 //
 // Implementations of this type should be thread-safe to support calls from multiple goroutines.
 type IssuerGetter interface {
-	GetIssuer() *string
+	GetIssuer() *url.URL
 }
 
 // New returns an http.Handler that will use information from the provided IssuerGetter to serve an
@@ -39,22 +40,23 @@ func New(ig IssuerGetter) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/json")
 
+		issuer := ig.GetIssuer()
+		if issuer == nil {
+			http.Error(w, `{"error": "OIDC discovery not available (unknown issuer)"}`, http.StatusNotFound)
+			return
+		}
+
 		if r.Method != http.MethodGet {
 			http.Error(w, `{"error": "Method not allowed (try GET)"}`, http.StatusMethodNotAllowed)
 			return
 		}
 
-		issuer := ig.GetIssuer()
+		issuerURL := issuer.String()
-		if issuer == nil {
-			http.Error(w, `{"error": "OIDC discovery not available (unknown issuer)"}`, http.StatusServiceUnavailable)
-			return
-		}
-
 		oidcConfig := Metadata{
-			Issuer:                           *issuer,
+			Issuer:                           issuerURL,
-			AuthorizationEndpoint:            fmt.Sprintf("%s/oauth2/v0/auth", *issuer),
+			AuthorizationEndpoint:            fmt.Sprintf("%s/oauth2/v0/auth", issuerURL),
-			TokenEndpoint:                    fmt.Sprintf("%s/oauth2/v0/token", *issuer),
+			TokenEndpoint:                    fmt.Sprintf("%s/oauth2/v0/token", issuerURL),
-			JWKSURL:                          fmt.Sprintf("%s/oauth2/v0/keys", *issuer),
+			JWKSURL:                          fmt.Sprintf("%s/oauth2/v0/keys", issuerURL),
 			ResponseTypesSupported:           []string{},
 			SubjectTypesSupported:            []string{},
 			IDTokenSigningAlgValuesSupported: []string{},

internal/oidc/discovery/discovery_test.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -18,7 +19,7 @@ func TestDiscovery(t *testing.T) {
 	tests := []struct {
 		name string
 
-		issuer string
+		issuer *url.URL
 		method string
 
 		wantStatus      int
@@ -26,16 +27,16 @@ func TestDiscovery(t *testing.T) {
 		wantBody        interface{}
 	}{
 		{
-			name:       "issuer returns nil issuer",
+			name:       "nil issuer",
 			method:     http.MethodGet,
-			wantStatus: http.StatusServiceUnavailable,
+			wantStatus: http.StatusNotFound,
 			wantBody: map[string]string{
 				"error": "OIDC discovery not available (unknown issuer)",
 			},
 		},
 		{
-			name:            "issuer returns non-nil issuer",
+			name:            "issuer without path",
-			issuer:          "https://some-issuer.com",
+			issuer:          must(url.Parse("https://some-issuer.com")),
 			method:          http.MethodGet,
 			wantStatus:      http.StatusOK,
 			wantContentType: "application/json",
@@ -49,9 +50,25 @@ func TestDiscovery(t *testing.T) {
 				IDTokenSigningAlgValuesSupported: []string{},
 			},
 		},
+		{
+			name:            "issuer with path",
+			issuer:          must(url.Parse("https://some-issuer.com/some/path")),
+			method:          http.MethodGet,
+			wantStatus:      http.StatusOK,
+			wantContentType: "application/json",
+			wantBody: &Metadata{
+				Issuer:                           "https://some-issuer.com/some/path",
+				AuthorizationEndpoint:            "https://some-issuer.com/some/path/oauth2/v0/auth",
+				TokenEndpoint:                    "https://some-issuer.com/some/path/oauth2/v0/token",
+				JWKSURL:                          "https://some-issuer.com/some/path/oauth2/v0/keys",
+				ResponseTypesSupported:           []string{},
+				SubjectTypesSupported:            []string{},
+				IDTokenSigningAlgValuesSupported: []string{},
+			},
+		},
 		{
 			name:       "bad method",
-			issuer:     "https://some-issuer.com",
+			issuer:     must(url.Parse("https://some-issuer.com")),
 			method:     http.MethodPost,
 			wantStatus: http.StatusMethodNotAllowed,
 			wantBody: map[string]string{
@@ -63,11 +80,7 @@ func TestDiscovery(t *testing.T) {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
 			p := issuerprovider.New()
-			if test.issuer != "" {
+			p.SetIssuer(test.issuer)
-				p.SetIssuer(&test.issuer)
-			} else {
-				p.SetIssuer(nil)
-			}
 
 			handler := New(p)
 			req := httptest.NewRequest(test.method, "/this/path/shouldnt/matter", nil)
@@ -88,3 +101,10 @@ func TestDiscovery(t *testing.T) {
 		})
 	}
 }
+
+func must(u *url.URL, err error) *url.URL {
+	if err != nil {
+		panic(err)
+	}
+	return u
+}

internal/oidc/issuerprovider/issuerprovider.go

@@ -4,14 +4,20 @@
 // Package issuerprovider provides a thread-safe type that can hold on to an OIDC issuer name.
 package issuerprovider
 
-import "sync"
+import (
+	"net/url"
+	"strings"
+	"sync"
+
+	"go.pinniped.dev/internal/constable"
+)
 
 // Provider is a type that can hold onto an issuer value, which may be nil.
 //
 // It is thread-safe.
 type Provider struct {
 	mu     sync.RWMutex
-	issuer *string
+	issuer *url.URL
 }
 
 // New returns an empty Provider, i.e., one that holds a nil issuer.
@@ -19,14 +25,56 @@ func New() *Provider {
 	return &Provider{}
 }
 
-func (p *Provider) SetIssuer(issuer *string) {
+// SetIssuer validates and sets the provided issuer. If validation fails, SetIssuer will return
+// an error.
+func (p *Provider) SetIssuer(issuer *url.URL) error {
+	if err := p.validateIssuer(issuer); err != nil {
+		return err
+	}
+	p.setIssuer(issuer)
+	return nil
+}
+
+func (p *Provider) validateIssuer(issuer *url.URL) error {
+	if issuer == nil {
+		return nil
+	}
+
+	if issuer.Scheme != "https" && removeMeAfterWeNoLongerNeedHTTPIssuerSupport(issuer.Scheme) {
+		return constable.Error(`issuer must have "https" scheme`)
+	}
+
+	if issuer.User != nil {
+		return constable.Error(`issuer must not have username or password`)
+	}
+
+	if strings.HasSuffix(issuer.Path, "/") {
+		return constable.Error(`issuer must not have trailing slash in path`)
+	}
+
+	if issuer.RawQuery != "" {
+		return constable.Error(`issuer must not have query`)
+	}
+
+	if issuer.Fragment != "" {
+		return constable.Error(`issuer must not have fragment`)
+	}
+
+	return nil
+}
+
+func (p *Provider) setIssuer(issuer *url.URL) {
 	p.mu.Lock()
 	defer p.mu.Unlock()
 	p.issuer = issuer
 }
 
-func (p *Provider) GetIssuer() *string {
+func (p *Provider) GetIssuer() *url.URL {
 	p.mu.RLock()
 	defer p.mu.RUnlock()
 	return p.issuer
 }
+
+func removeMeAfterWeNoLongerNeedHTTPIssuerSupport(scheme string) bool {
+	return scheme != "http"
+}

internal/oidc/issuerprovider/issuerprovider_test.go

@@ -0,0 +1,84 @@
+package issuerprovider
+
+import (
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestProvider(t *testing.T) {
+	tests := []struct {
+		name      string
+		issuer    *url.URL
+		wantError string
+	}{
+		{
+			name:   "nil issuer",
+			issuer: nil,
+		},
+		{
+			name:      "no scheme",
+			issuer:    must(url.Parse("tuna.com")),
+			wantError: `issuer must have "https" scheme`,
+		},
+		{
+			name:      "bad scheme",
+			issuer:    must(url.Parse("ftp://tuna.com")),
+			wantError: `issuer must have "https" scheme`,
+		},
+		{
+			name:      "fragment",
+			issuer:    must(url.Parse("https://tuna.com/fish#some-frag")),
+			wantError: `issuer must not have fragment`,
+		},
+		{
+			name:      "query",
+			issuer:    must(url.Parse("https://tuna.com?some=query")),
+			wantError: `issuer must not have query`,
+		},
+		{
+			name:      "username",
+			issuer:    must(url.Parse("https://username@tuna.com")),
+			wantError: `issuer must not have username or password`,
+		},
+		{
+			name:      "password",
+			issuer:    must(url.Parse("https://username:password@tuna.com")),
+			wantError: `issuer must not have username or password`,
+		},
+		{
+			name:   "without path",
+			issuer: must(url.Parse("https://tuna.com")),
+		},
+		{
+			name:   "with path",
+			issuer: must(url.Parse("https://tuna.com/fish/marlin")),
+		},
+		{
+			name:      "trailing slash in path",
+			issuer:    must(url.Parse("https://tuna.com/")),
+			wantError: `issuer must not have trailing slash in path`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p := New()
+			err := p.SetIssuer(tt.issuer)
+			if tt.wantError != "" {
+				require.EqualError(t, err, tt.wantError)
+				require.Nil(t, p.GetIssuer())
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.issuer, p.GetIssuer())
+			}
+		})
+	}
+}
+
+func must(u *url.URL, err error) *url.URL {
+	if err != nil {
+		panic(err)
+	}
+	return u
+}

test/integration/supervisor_discovery_test.go

@@ -40,9 +40,13 @@ func TestSupervisorOIDCDiscovery(t *testing.T) {
 
 	// When this test has finished, recreate any OIDCProviderConfigs that had existed on the cluster before this test.
 	t.Cleanup(func() {
+		cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+		defer cancel()
+
 		for _, config := range originalConfigList.Items {
 			thisConfig := config
-			_, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(ctx, &thisConfig, metav1.CreateOptions{})
+			thisConfig.ResourceVersion = "" // Get rid of resource version since we can't create an object with one.
+			_, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(cleanupCtx, &thisConfig, metav1.CreateOptions{})
 			require.NoError(t, err)
 		}
 	})
@@ -64,6 +68,10 @@ func TestSupervisorOIDCDiscovery(t *testing.T) {
 	// Create a new OIDCProviderConfig with a known issuer.
 	issuer := fmt.Sprintf("http://%s/nested/issuer", env.SupervisorAddress)
 	newOIDCProviderConfig := v1alpha1.OIDCProviderConfig{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "OIDCProviderConfig",
+			APIVersion: v1alpha1.SchemeGroupVersion.String(),
+		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "nested-issuser-config-from-integration-test",
 			Namespace: ns,
@@ -77,7 +85,10 @@ func TestSupervisorOIDCDiscovery(t *testing.T) {
 
 	// When this test has finished, clean up the new OIDCProviderConfig.
 	t.Cleanup(func() {
-		err = client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(ctx, newOIDCProviderConfig.Name, metav1.DeleteOptions{})
+		cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+		defer cancel()
+
+		err = client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(cleanupCtx, newOIDCProviderConfig.Name, metav1.DeleteOptions{})
 		require.NoError(t, err)
 	})
 
@@ -94,9 +105,11 @@ func TestSupervisorOIDCDiscovery(t *testing.T) {
 	var response *http.Response
 	assert.Eventually(t, func() bool {
 		response, err = httpClient.Do(requestDiscoveryEndpoint) //nolint:bodyclose // the body is closed below after it is read
-		return err == nil
+		return err == nil && response.StatusCode == http.StatusOK
 	}, 10*time.Second, 200*time.Millisecond)
 	require.NoError(t, err)
+	require.Equal(t, http.StatusOK, response.StatusCode)
+
 	responseBody, err := ioutil.ReadAll(response.Body)
 	require.NoError(t, err)
 	err = response.Body.Close()
@@ -116,7 +129,6 @@ func TestSupervisorOIDCDiscovery(t *testing.T) {
     }`)
 	expectedJSON := fmt.Sprintf(expectedResultTemplate, issuer, issuer, issuer, issuer)
 
-	require.Equal(t, 200, response.StatusCode)
 	require.Equal(t, "application/json", response.Header.Get("content-type"))
 	require.JSONEq(t, expectedJSON, string(responseBody))
 }