diff --git a/generated/latest/apis/concierge/authentication/v1alpha1/types_webhookauthenticator.go b/generated/latest/apis/concierge/authentication/v1alpha1/types_webhookauthenticator.go index 207249b28..a78e7a7e5 100644 --- a/generated/latest/apis/concierge/authentication/v1alpha1/types_webhookauthenticator.go +++ b/generated/latest/apis/concierge/authentication/v1alpha1/types_webhookauthenticator.go @@ -5,6 +5,19 @@ package v1alpha1 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +type WebhookAuthenticatorPhase string + +const ( + // WebhookAuthenticatorPhasePending is the default phase for newly-created WebhookAuthenticator resources. + WebhookAuthenticatorPhasePending WebhookAuthenticatorPhase = "Pending" + + // WebhookAuthenticatorPhaseReady is the phase for an WebhookAuthenticator resource in a healthy state. + WebhookAuthenticatorPhaseReady WebhookAuthenticatorPhase = "Ready" + + // WebhookAuthenticatorPhaseError is the phase for an WebhookAuthenticator in an unhealthy state. + WebhookAuthenticatorPhaseError WebhookAuthenticatorPhase = "Error" +) + // Status of a webhook authenticator. type WebhookAuthenticatorStatus struct { // Represents the observations of the authenticator's current state. @@ -13,6 +26,10 @@ type WebhookAuthenticatorStatus struct { // +listType=map // +listMapKey=type Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` + // Phase summarizes the overall status of the WebhookAuthenticator. + // +kubebuilder:default=Pending + // +kubebuilder:validation:Enum=Pending;Ready;Error + Phase WebhookAuthenticatorPhase `json:"phase,omitempty"` } // Spec for configuring a webhook authenticator. diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index c7b24cc6f..191f7c77b 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -5,12 +5,17 @@ package webhookcachefiller import ( + "context" + "crypto/x509" "fmt" + "net/url" "os" - "github.com/go-logr/logr" k8sauthv1beta1 "k8s.io/api/authentication/v1beta1" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + errorsutil "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/net" "k8s.io/apiserver/pkg/authentication/authenticator" webhookutil "k8s.io/apiserver/pkg/util/webhook" @@ -18,24 +23,56 @@ import ( "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "k8s.io/klog/v2" + "k8s.io/utils/clock" auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" + conciergeclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned" authinformers "go.pinniped.dev/generated/latest/client/concierge/informers/externalversions/authentication/v1alpha1" pinnipedcontroller "go.pinniped.dev/internal/controller" pinnipedauthenticator "go.pinniped.dev/internal/controller/authenticator" "go.pinniped.dev/internal/controller/authenticator/authncache" + "go.pinniped.dev/internal/controller/conditionsutil" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/plog" +) + +const ( + controllerName = "webhookcachefiller-controller" + typeReady = "Ready" + typeTLSConfigurationValid = "TLSConfigurationValid" + typeEndpointURLValid = "EndpointURLValid" + typeEndpointPOSTValid = "EndpointPOSTValid" + typeAuthenticatorValid = "AuthenticatorValid" + reasonSuccess = "Success" + reasonNotReady = "NotReady" + reasonUnableToValidate = "UnableToValidate" + reasonUnableToCreateTempFile = "UnableToCreateTempFile" + reasonUnableToMarshallKubeconfig = "UnableToMarshallKubeconfig" + reasonUnableToLoadKubeconfig = "UnableToLoadKubeconfig" + reasonUnableToInstantiateWebhook = "UnableToInstantiateWebhook" + reasonInvalidTLSConfiguration = "InvalidTLSConfiguration" + reasonInvalidEndpointURL = "InvalidEndpointURL" + reasonInvalidEndpointURLScheme = "InvalidEndpointURLScheme" + msgUnableToValidate = "unable to validate; other issues present" ) // New instantiates a new controllerlib.Controller which will populate the provided authncache.Cache. -func New(cache *authncache.Cache, webhooks authinformers.WebhookAuthenticatorInformer, log logr.Logger) controllerlib.Controller { +func New( + cache *authncache.Cache, + client conciergeclientset.Interface, + webhooks authinformers.WebhookAuthenticatorInformer, + clock clock.Clock, + log plog.Logger, +) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ - Name: "webhookcachefiller-controller", - Syncer: &controller{ + Name: controllerName, + Syncer: &webhookCacheFillerController{ cache: cache, + client: client, webhooks: webhooks, - log: log.WithName("webhookcachefiller-controller"), + clock: clock, + log: log.WithName(controllerName), }, }, controllerlib.WithInformer( @@ -46,14 +83,16 @@ func New(cache *authncache.Cache, webhooks authinformers.WebhookAuthenticatorInf ) } -type controller struct { +type webhookCacheFillerController struct { cache *authncache.Cache webhooks authinformers.WebhookAuthenticatorInformer - log logr.Logger + client conciergeclientset.Interface + clock clock.Clock + log plog.Logger } // Sync implements controllerlib.Syncer. -func (c *controller) Sync(ctx controllerlib.Context) error { +func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { obj, err := c.webhooks.Lister().Get(ctx.Key.Name) if err != nil && errors.IsNotFound(err) { c.log.Info("Sync() found that the WebhookAuthenticator does not exist yet or was deleted") @@ -63,10 +102,22 @@ func (c *controller) Sync(ctx controllerlib.Context) error { return fmt.Errorf("failed to get WebhookAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) } - webhookAuthenticator, err := newWebhookAuthenticator(&obj.Spec, os.CreateTemp, clientcmd.WriteToFile) - if err != nil { - return fmt.Errorf("failed to build webhook config: %w", err) - } + conditions := make([]*metav1.Condition, 0) + specCopy := obj.Spec.DeepCopy() + var errs []error + + _, conditions, tlsOk := c.validateTLS(specCopy.TLS, conditions) + _, conditions, endpointOk := c.validateEndpoint(specCopy.Endpoint, conditions) + conditions, endpointPOSTOk := c.validateEndpointPOST(specCopy.Endpoint, conditions, tlsOk && endpointOk) + + webhookAuthenticator, conditions, err := newWebhookAuthenticator( + &obj.Spec, + os.CreateTemp, + clientcmd.WriteToFile, + conditions, + tlsOk && endpointOk && endpointPOSTOk, + ) + errs = append(errs, err) c.cache.Store(authncache.Key{ APIGroup: auth1alpha1.GroupName, @@ -74,7 +125,15 @@ func (c *controller) Sync(ctx controllerlib.Context) error { Name: ctx.Key.Name, }, webhookAuthenticator) c.log.WithValues("webhook", klog.KObj(obj), "endpoint", obj.Spec.Endpoint).Info("added new webhook authenticator") - return nil + err = c.updateStatus(ctx.Context, obj, conditions) + errs = append(errs, err) + + // sync loop errors: + // - should not be configuration errors. config errors a user must correct belong on the .Status + // object. The controller simply must wait for a user to correct before running again. + // - other errors, such as networking errors, etc. are the types of errors that should return here + // and signal the controller to retry the sync loop. These may be corrected by machines. + return errorsutil.NewAggregate(errs) } // newWebhookAuthenticator creates a webhook from the provided API server url and caBundle @@ -83,17 +142,44 @@ func newWebhookAuthenticator( spec *auth1alpha1.WebhookAuthenticatorSpec, tempfileFunc func(string, string) (*os.File, error), marshalFunc func(clientcmdapi.Config, string) error, -) (*webhook.WebhookTokenAuthenticator, error) { + conditions []*metav1.Condition, + prereqOk bool, +) (*webhook.WebhookTokenAuthenticator, []*metav1.Condition, error) { + if !prereqOk { + conditions = append(conditions, &metav1.Condition{ + Type: typeAuthenticatorValid, + Status: metav1.ConditionUnknown, + Reason: reasonUnableToValidate, + Message: msgUnableToValidate, + }) + return nil, conditions, nil + } temp, err := tempfileFunc("", "pinniped-webhook-kubeconfig-*") if err != nil { - return nil, fmt.Errorf("unable to create temporary file: %w", err) + errText := "unable to create temporary file" + msg := fmt.Sprintf("%s: %s", errText, err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeAuthenticatorValid, + Status: metav1.ConditionFalse, + Reason: reasonUnableToCreateTempFile, + Message: msg, + }) + return nil, conditions, fmt.Errorf("%s: %w", errText, err) } defer func() { _ = os.Remove(temp.Name()) }() cluster := &clientcmdapi.Cluster{Server: spec.Endpoint} _, cluster.CertificateAuthorityData, err = pinnipedauthenticator.CABundle(spec.TLS) if err != nil { - return nil, fmt.Errorf("invalid TLS configuration: %w", err) + errText := "invalid TLS configuration" + msg := fmt.Sprintf("%s: %s", errText, err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeAuthenticatorValid, + Status: metav1.ConditionFalse, + Reason: reasonInvalidTLSConfiguration, + Message: msg, + }) + return nil, conditions, fmt.Errorf("%s: %w", errText, err) } kubeconfig := clientcmdapi.NewConfig() @@ -102,7 +188,15 @@ func newWebhookAuthenticator( kubeconfig.CurrentContext = "anonymous" if err := marshalFunc(*kubeconfig, temp.Name()); err != nil { - return nil, fmt.Errorf("unable to marshal kubeconfig: %w", err) + errText := "unable to marshal kubeconfig" + msg := fmt.Sprintf("%s: %s", errText, err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeAuthenticatorValid, + Status: metav1.ConditionFalse, + Reason: reasonUnableToMarshallKubeconfig, + Message: msg, + }) + return nil, conditions, fmt.Errorf("%s: %w", errText, err) } // We use v1beta1 instead of v1 since v1beta1 is more prevalent in our desired @@ -136,10 +230,166 @@ func newWebhookAuthenticator( // then use client.JSONConfig as clientConfig clientConfig, err := webhookutil.LoadKubeconfig(temp.Name(), customDial) if err != nil { - return nil, err + errText := "unable to load kubeconfig" + msg := fmt.Sprintf("%s: %s", errText, err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeAuthenticatorValid, + Status: metav1.ConditionFalse, + Reason: reasonUnableToLoadKubeconfig, + Message: msg, + }) + return nil, conditions, fmt.Errorf("%s: %w", errText, err) } // this uses a http client that does not honor our TLS config - // TODO fix when we pick up https://github.com/kubernetes/kubernetes/pull/106155 - return webhook.New(clientConfig, version, implicitAuds, *webhook.DefaultRetryBackoff()) + // TODO: fix when we pick up https://github.com/kubernetes/kubernetes/pull/106155 + // NOTE: looks like the above was merged on Mar 18, 2022 + webhookA, err := webhook.New(clientConfig, version, implicitAuds, *webhook.DefaultRetryBackoff()) + if err != nil { + errText := "unable to instantiate webhook" + msg := fmt.Sprintf("%s: %s", errText, err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeAuthenticatorValid, + Status: metav1.ConditionFalse, + Reason: reasonUnableToInstantiateWebhook, + Message: msg, + }) + return nil, conditions, fmt.Errorf("%s: %w", errText, err) + } + msg := "authenticator initialized" + conditions = append(conditions, &metav1.Condition{ + Type: typeAuthenticatorValid, + Status: metav1.ConditionTrue, + Reason: reasonSuccess, + Message: msg, + }) + return webhookA, conditions, nil +} + +func (c *webhookCacheFillerController) validateTLS(tlsSpec *auth1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []*metav1.Condition, bool) { + rootCAs, _, err := pinnipedauthenticator.CABundle(tlsSpec) + if err != nil { + msg := fmt.Sprintf("%s: %s", "invalid TLS configuration", err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeTLSConfigurationValid, + Status: metav1.ConditionFalse, + Reason: reasonInvalidTLSConfiguration, + Message: msg, + }) + return rootCAs, conditions, false + } + msg := "valid TLS configuration" + conditions = append(conditions, &metav1.Condition{ + Type: typeTLSConfigurationValid, + Status: metav1.ConditionTrue, + Reason: reasonSuccess, + Message: msg, + }) + return rootCAs, conditions, true +} + +func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditions []*metav1.Condition) (*url.URL, []*metav1.Condition, bool) { + endpointURL, err := url.Parse(endpoint) + if err != nil { + msg := fmt.Sprintf("%s: %s", "spec.endpoint URL is invalid", err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeEndpointURLValid, + Status: metav1.ConditionFalse, + Reason: reasonInvalidEndpointURL, + Message: msg, + }) + return nil, conditions, false + } + + if endpointURL.Scheme != "https" { + msg := fmt.Sprintf("spec.issuer %s has invalid scheme, require 'https'", endpoint) + conditions = append(conditions, &metav1.Condition{ + Type: typeEndpointURLValid, + Status: metav1.ConditionFalse, + Reason: reasonInvalidEndpointURLScheme, + Message: msg, + }) + return nil, conditions, false + } + + conditions = append(conditions, &metav1.Condition{ + Type: typeEndpointURLValid, + Status: metav1.ConditionTrue, + Reason: reasonSuccess, + Message: "endpoint is a valid URL", + }) + return endpointURL, conditions, true +} + +func (c *webhookCacheFillerController) validateEndpointPOST(endpoint string, conditions []*metav1.Condition, prereqOk bool) ([]*metav1.Condition, bool) { + if endpoint == "" { + // TODO(BEN): do something with this. time to validate the endpoint will receive a POST + fmt.Println("FIX THIS") + } + if !prereqOk { + conditions = append(conditions, &metav1.Condition{ + Type: typeEndpointPOSTValid, + Status: metav1.ConditionUnknown, + Reason: reasonUnableToValidate, + Message: msgUnableToValidate, + }) + return conditions, false + } + + // TODO: do some things here so this func makes sense. + return conditions, false +} + +func (c *webhookCacheFillerController) updateStatus( + ctx context.Context, + original *auth1alpha1.WebhookAuthenticator, + conditions []*metav1.Condition, +) error { + + updated := original.DeepCopy() + + if hadErrorCondition(conditions) { + updated.Status.Phase = auth1alpha1.WebhookAuthenticatorPhaseError + conditions = append(conditions, &metav1.Condition{ + Type: typeReady, + Status: metav1.ConditionFalse, + Reason: reasonNotReady, + Message: "the WebhookAuthenticator is not ready: see other conditions for details", + }) + } else { + updated.Status.Phase = auth1alpha1.WebhookAuthenticatorPhaseReady + conditions = append(conditions, &metav1.Condition{ + Type: typeReady, + Status: metav1.ConditionTrue, + Reason: reasonSuccess, + Message: "the WebhookAuthenticator is ready", + }) + } + + _ = conditionsutil.MergeConfigConditions( + conditions, + original.Generation, + &updated.Status.Conditions, + plog.New().WithName(controllerName), + metav1.NewTime(c.clock.Now()), + ) + + if equality.Semantic.DeepEqual(original, updated) { + return nil + } + + _, err := c.client.AuthenticationV1alpha1().WebhookAuthenticators().UpdateStatus(ctx, updated, metav1.UpdateOptions{}) + if err != nil { + c.log.Info(fmt.Sprintf("ERROR: %v", err)) + } + return err +} + +func hadErrorCondition(conditions []*metav1.Condition) bool { + for _, c := range conditions { + if c.Status != metav1.ConditionTrue { + return true + } + } + return false } diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 291358003..be5e369dd 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -4,6 +4,7 @@ package webhookcachefiller import ( + "bytes" "context" "encoding/base64" "fmt" @@ -11,42 +12,154 @@ import ( "net/http" "os" "testing" + "time" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + clocktesting "k8s.io/utils/clock/testing" auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" pinnipedfake "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/fake" pinnipedinformers "go.pinniped.dev/generated/latest/client/concierge/informers/externalversions" "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" + "go.pinniped.dev/internal/testutil/conditionstestutil" "go.pinniped.dev/internal/testutil/testlogger" ) func TestController(t *testing.T) { t.Parallel() + goodEndpoint := "https://example.com" + + nowDoesntMatter := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local) + frozenMetav1Now := metav1.NewTime(nowDoesntMatter) + frozenClock := clocktesting.NewFakeClock(nowDoesntMatter) + + happyReadyCondition := func(time metav1.Time, observedGeneration int64) metav1.Condition { + return metav1.Condition{ + Type: "Ready", + Status: "True", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "Success", + Message: "the WebhookAuthenticator is ready", + } + } + // sadReadyCondition := func(time metav1.Time, observedGeneration int64) metav1.Condition { + // return metav1.Condition{ + // Type: "Ready", + // Status: "False", + // ObservedGeneration: observedGeneration, + // LastTransitionTime: time, + // Reason: "NotReady", + // Message: "the WebhookAuthenticator is not ready: see other conditions for details", + // } + // } + happyAuthenticatorValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { + return metav1.Condition{ + Type: "AuthenticatorValid", + Status: "True", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "Success", + Message: "authenticator initialized", + } + } + // unknownAuthenticatorValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { + // return metav1.Condition{ + // Type: "AuthenticatorValid", + // Status: "Unknown", + // ObservedGeneration: observedGeneration, + // LastTransitionTime: time, + // Reason: "UnableToValidate", + // Message: "unable to validate; other issues present", + // } + // } + // sadAuthenticatorValid := func() metav1.Condition {} + + happyTLSConfigurationValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { + return metav1.Condition{ + Type: "TLSConfigurationValid", + Status: "True", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "Success", + Message: "valid TLS configuration", + } + } + // sadTLSConfigurationValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { + // return metav1.Condition{ + // Type: "TLSConfigurationValid", + // Status: "False", + // ObservedGeneration: observedGeneration, + // LastTransitionTime: time, + // Reason: "InvalidTLSConfiguration", + // Message: "invalid TLS configuration: illegal base64 data at input byte 7", + // } + // } + + happyEndpointURLValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { + return metav1.Condition{ + Type: "EndpointURLValid", + Status: "True", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "Success", + Message: "endpoint is a valid URL", + } + } + // happyEndpointURLValidInvalid := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition { + // return metav1.Condition{ + // Type: "EndpointURLValid", + // Status: "False", + // ObservedGeneration: observedGeneration, + // LastTransitionTime: time, + // Reason: "InvalidIssuerURL", + // Message: fmt.Sprintf(`spec.endpoint URL is invalid: parse "%s": invalid character " " in host name`, issuer), + // } + // } + + allHappyConditionsSuccess := func(endpoint string, someTime metav1.Time, observedGeneration int64) []metav1.Condition { + + return conditionstestutil.SortByType([]metav1.Condition{ + happyEndpointURLValid(someTime, observedGeneration), + happyAuthenticatorValid(someTime, observedGeneration), + happyReadyCondition(someTime, observedGeneration), + happyTLSConfigurationValid(someTime, observedGeneration), + }) + } + tests := []struct { - name string - syncKey controllerlib.Key - webhooks []runtime.Object - wantErr string - wantLogs []string - wantCacheEntries int + name string + syncKey controllerlib.Key + webhooks []runtime.Object + wantErr string + wantLogs []string + wantStatusConditions []metav1.Condition + wantStatusPhase auth1alpha1.WebhookAuthenticatorPhase + wantCacheEntries int }{ { - name: "not found", + name: "404: webhook authenticator not found will abort sync loop and not write status", syncKey: controllerlib.Key{Name: "test-name"}, - wantLogs: []string{ - `webhookcachefiller-controller "level"=0 "msg"="Sync() found that the WebhookAuthenticator does not exist yet or was deleted"`, - }, + // TODO(BEN): we lost this line when swapping loggers. Is that ok? + // did the JWTAuthenticator also lose it? Should we ensure something exists otherwise? + // wantLogs: []string{ + // `webhookcachefiller-controller "level"=0 "msg"="Sync() found that the WebhookAuthenticator does not exist yet or was deleted"`, + // }, }, + // Existing code that was never tested. We would likely have to create a server with bad clients to + // simulate this. + // { name: "non-404 `failed to get webhook authenticator` for other API server reasons" } { - name: "invalid webhook", + // will fail sync loop and will report failed and unknown conditions and Error phase, but will not enqueue a resync due to user config error + name: "invalid webhook will fail the sync loop and........????", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -60,8 +173,14 @@ func TestController(t *testing.T) { }, wantErr: `failed to build webhook config: parse "http://invalid url": invalid character " " in host name`, }, + // TODO (BEN): add valid without CA? { - name: "valid webhook", + name: "valid webhook without CA...", + }, { + name: "", + }, + { + name: "valid webhook will complete sync loop successfully with success conditions and ready phase", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -69,15 +188,18 @@ func TestController(t *testing.T) { Name: "test-name", }, Spec: auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: "https://example.com", + Endpoint: goodEndpoint, TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: ""}, }, }, }, - wantLogs: []string{ - `webhookcachefiller-controller "level"=0 "msg"="added new webhook authenticator" "endpoint"="https://example.com" "webhook"={"name":"test-name"}`, - }, - wantCacheEntries: 1, + // TODO(BEN): we lost this changing loggers, make sure its captured in conditions + // wantLogs: []string{ + // `webhookcachefiller-controller "level"=0 "msg"="added new webhook authenticator" "endpoint"="https://example.com" "webhook"={"name":"test-name"}`, + // }, + wantStatusConditions: allHappyConditionsSuccess(goodEndpoint, frozenMetav1Now, 0), + wantStatusPhase: "Ready", + wantCacheEntries: 1, }, } for _, tt := range tests { @@ -85,12 +207,20 @@ func TestController(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - fakeClient := pinnipedfake.NewSimpleClientset(tt.webhooks...) - informers := pinnipedinformers.NewSharedInformerFactory(fakeClient, 0) + pinnipedAPIClient := pinnipedfake.NewSimpleClientset(tt.webhooks...) + informers := pinnipedinformers.NewSharedInformerFactory(pinnipedAPIClient, 0) cache := authncache.New() testLog := testlogger.NewLegacy(t) //nolint:staticcheck // old test with lots of log statements - controller := New(cache, informers.Authentication().V1alpha1().WebhookAuthenticators(), testLog.Logger) + var log bytes.Buffer + logger := plog.TestLogger(t, &log) + + controller := New( + cache, + pinnipedAPIClient, + informers.Authentication().V1alpha1().WebhookAuthenticators(), + frozenClock, + logger) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -105,54 +235,133 @@ func TestController(t *testing.T) { } else { require.NoError(t, err) } - require.Equal(t, tt.wantLogs, testLog.Lines()) - require.Equal(t, tt.wantCacheEntries, len(cache.Keys())) + require.Equal(t, tt.wantLogs, testLog.Lines(), "log lines should be correct") + + if tt.webhooks != nil { + var webhookAuthSubject *auth1alpha1.WebhookAuthenticator + getCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + webhookAuthSubject, getErr := pinnipedAPIClient.AuthenticationV1alpha1().WebhookAuthenticators().Get(getCtx, "test-name", metav1.GetOptions{}) + require.NoError(t, getErr) + require.Equal(t, tt.wantStatusConditions, webhookAuthSubject.Status.Conditions, "status.conditions must be correct") + require.Equal(t, tt.wantStatusPhase, webhookAuthSubject.Status.Phase, "status.phase should be correct") + } + + require.Equal(t, tt.wantCacheEntries, len(cache.Keys()), fmt.Sprintf("expected cache entries is incorrect. wanted:%d, got: %d, keys: %v", tt.wantCacheEntries, len(cache.Keys()), cache.Keys())) }) } } func TestNewWebhookAuthenticator(t *testing.T) { - t.Run("temp file failure", func(t *testing.T) { + goodEndpoint := "https://example.com" + + t.Run("prerequisites not ready, cannot create webhook authenticator", func(t *testing.T) { + conditions := []*metav1.Condition{} + res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{}, os.CreateTemp, clientcmd.WriteToFile, conditions, false) + require.Equal(t, []*metav1.Condition{ + { + Type: "AuthenticatorValid", + Status: "Unknown", + Reason: "UnableToValidate", + Message: "unable to validate; other issues present", + }, + }, conditions) + require.Nil(t, res) + require.Nil(t, err) + }) + + t.Run("temp file failure, cannot create webhook authenticator", func(t *testing.T) { brokenTempFile := func(_ string, _ string) (*os.File, error) { return nil, fmt.Errorf("some temp file error") } - res, err := newWebhookAuthenticator(nil, brokenTempFile, clientcmd.WriteToFile) + conditions := []*metav1.Condition{} + res, conditions, err := newWebhookAuthenticator(nil, brokenTempFile, clientcmd.WriteToFile, conditions, true) + require.Equal(t, []*metav1.Condition{ + { + Type: "AuthenticatorValid", + Status: "False", + Reason: "UnableToCreateTempFile", + Message: "unable to create temporary file: some temp file error", + }, + }, conditions) require.Nil(t, res) require.EqualError(t, err, "unable to create temporary file: some temp file error") }) - t.Run("marshal failure", func(t *testing.T) { + t.Run("marshal failure, cannot create webhook authenticator", func(t *testing.T) { marshalError := func(_ clientcmdapi.Config, _ string) error { return fmt.Errorf("some marshal error") } - res, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{}, os.CreateTemp, marshalError) + conditions := []*metav1.Condition{} + res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{}, os.CreateTemp, marshalError, conditions, true) + require.Equal(t, []*metav1.Condition{ + { + Type: "AuthenticatorValid", + Status: "False", + Reason: "UnableToMarshallKubeconfig", + Message: "unable to marshal kubeconfig: some marshal error", + }, + }, conditions) require.Nil(t, res) require.EqualError(t, err, "unable to marshal kubeconfig: some marshal error") }) - t.Run("invalid base64", func(t *testing.T) { - res, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: "https://example.com", + // t.Run("load kubeconfig err, not currently tested, may not be reasonable to test?") + + t.Run("invalid TLS config, base64 encoding err, cannot create webhook authenticator", func(t *testing.T) { + conditions := []*metav1.Condition{} + res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: goodEndpoint, TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "invalid-base64"}, - }, os.CreateTemp, clientcmd.WriteToFile) + }, os.CreateTemp, clientcmd.WriteToFile, conditions, true) + require.Equal(t, []*metav1.Condition{ + { + Type: "AuthenticatorValid", + Status: "False", + Reason: "InvalidTLSConfiguration", + Message: "invalid TLS configuration: illegal base64 data at input byte 7", + }, + }, conditions) require.Nil(t, res) + // TODO: should this trigger the sync loop again with an error, or should this have been only + // status and log, indicating user must correct? require.EqualError(t, err, "invalid TLS configuration: illegal base64 data at input byte 7") }) - t.Run("invalid pem data", func(t *testing.T) { - res, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: "https://example.com", + t.Run("invalid pem data, cannot create webhook authenticator", func(t *testing.T) { + conditions := []*metav1.Condition{} + res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: goodEndpoint, TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte("bad data"))}, - }, os.CreateTemp, clientcmd.WriteToFile) + }, os.CreateTemp, clientcmd.WriteToFile, conditions, true) + require.Equal(t, []*metav1.Condition{ + { + Type: "AuthenticatorValid", + Status: "False", + Reason: "InvalidTLSConfiguration", + Message: "invalid TLS configuration: certificateAuthorityData is not valid PEM: data does not contain any valid RSA or ECDSA certificates", + }, + }, conditions) require.Nil(t, res) require.EqualError(t, err, "invalid TLS configuration: certificateAuthorityData is not valid PEM: data does not contain any valid RSA or ECDSA certificates") }) - t.Run("valid config with no TLS spec", func(t *testing.T) { - res, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: "https://example.com", - }, os.CreateTemp, clientcmd.WriteToFile) + t.Run("valid config with no TLS spec, webhook authenticator created", func(t *testing.T) { + conditions := []*metav1.Condition{} + res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: goodEndpoint, + }, os.CreateTemp, clientcmd.WriteToFile, conditions, true) + require.Equal(t, []*metav1.Condition{ + { + Type: "AuthenticatorValid", + Status: "True", + Reason: "Success", + Message: "authenticator initialized", + }, + }, conditions) require.NotNil(t, res) require.NoError(t, err) }) - t.Run("success", func(t *testing.T) { + t.Run("success, webhook authenticator created", func(t *testing.T) { + // TODO(BEN): when enhancing webhook authenticator integration test, can prob + // steal this and create a super simpler server caBundle, url := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { body, err := io.ReadAll(r.Body) require.NoError(t, err) @@ -166,10 +375,18 @@ func TestNewWebhookAuthenticator(t *testing.T) { CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(caBundle)), }, } - res, err := newWebhookAuthenticator(spec, os.CreateTemp, clientcmd.WriteToFile) + conditions := []*metav1.Condition{} + res, conditions, err := newWebhookAuthenticator(spec, os.CreateTemp, clientcmd.WriteToFile, conditions, true) require.NoError(t, err) require.NotNil(t, res) - + require.Equal(t, []*metav1.Condition{ + { + Type: "AuthenticatorValid", + Status: "True", + Reason: "Success", + Message: "authenticator initialized", + }, + }, conditions) resp, authenticated, err := res.AuthenticateToken(context.Background(), "test-token") require.NoError(t, err) require.Nil(t, resp) diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 1a8e195d6..379db68a4 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -236,8 +236,10 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol WithController( webhookcachefiller.New( c.AuthenticatorCache, + client.PinnipedConcierge, informers.pinniped.Authentication().V1alpha1().WebhookAuthenticators(), - plog.Logr(), //nolint:staticcheck // old controller with lots of log statements + clock.RealClock{}, + plog.New(), ), singletonWorker, ). diff --git a/test/integration/concierge_webhookauthenticator_status_test.go b/test/integration/concierge_webhookauthenticator_status_test.go new file mode 100644 index 000000000..099848b22 --- /dev/null +++ b/test/integration/concierge_webhookauthenticator_status_test.go @@ -0,0 +1,4 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration