From ae6503e972431fbbd6c55d323f888d233e4cbb27 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 4 Feb 2021 20:02:38 -0500 Subject: [PATCH 1/2] internal/plog: add KObj() and KRef() Signed-off-by: Andrew Keesler --- internal/plog/klog.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/plog/klog.go b/internal/plog/klog.go index 52759814..9544d886 100644 --- a/internal/plog/klog.go +++ b/internal/plog/klog.go @@ -4,6 +4,7 @@ package plog import ( + "fmt" "sync" "github.com/spf13/pflag" @@ -32,6 +33,17 @@ func RemoveKlogGlobalFlags() { } } +// KRef is (mostly) copied from klog - it is a standard way to represent a a metav1.Object in logs +// when you only have access to the namespace and name of the object. +func KRef(namespace, name string) string { + return fmt.Sprintf("%s/%s", namespace, name) +} + +// KObj is (mostly) copied from klog - it is a standard way to represent a metav1.Object in logs. +func KObj(obj klog.KMetadata) string { + return fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName()) +} + func klogLevelForPlogLevel(plogLevel LogLevel) (klogLevel klog.Level) { switch plogLevel { case LevelWarning: From 0fc1f1786662eb6a01f9124572837362e8e54d94 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 4 Feb 2021 20:02:59 -0500 Subject: [PATCH 2/2] internal/groupsuffix: mutate TokenCredentialRequest's Authenticator This is a partial revert of 288d9c999efe. For some reason it didn't occur to me that we could do it this way earlier. Whoops. This also contains a middleware update: mutation funcs can return an error now and short-circuit the rest of the request/response flow. The idea here is that if someone is configuring their kubeclient to use middleware, they are agreeing to a narrow-er client contract by doing so (e.g., their TokenCredentialRequest's must have an Spec.Authenticator.APIGroup set). I also updated some internal/groupsuffix tests to be more realistic. Signed-off-by: Andrew Keesler --- internal/groupsuffix/groupsuffix.go | 59 ++++- internal/groupsuffix/groupsuffix_test.go | 232 ++++++++++++++---- internal/kubeclient/kubeclient_test.go | 117 +++++++-- internal/kubeclient/middleware.go | 27 +- internal/kubeclient/middleware_test.go | 7 +- internal/kubeclient/verb_test.go | 2 +- internal/ownerref/ownerref.go | 6 +- internal/ownerref/ownerref_test.go | 2 +- .../testutil/{round_trip.go => roundtrip.go} | 6 +- pkg/conciergeclient/conciergeclient.go | 33 ++- pkg/conciergeclient/conciergeclient_test.go | 117 ++++----- .../concierge_api_serving_certs_test.go | 8 +- .../concierge_credentialrequest_test.go | 22 +- test/library/client.go | 10 +- 14 files changed, 453 insertions(+), 195 deletions(-) rename internal/testutil/{round_trip.go => roundtrip.go} (87%) diff --git a/internal/groupsuffix/groupsuffix.go b/internal/groupsuffix/groupsuffix.go index 39f556a7..d080662a 100644 --- a/internal/groupsuffix/groupsuffix.go +++ b/internal/groupsuffix/groupsuffix.go @@ -5,6 +5,7 @@ package groupsuffix import ( "context" + "fmt" "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,8 +13,10 @@ import ( "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/validation" + loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" "go.pinniped.dev/internal/constable" "go.pinniped.dev/internal/kubeclient" + "go.pinniped.dev/internal/plog" ) const ( @@ -35,7 +38,7 @@ func New(apiGroupSuffix string) kubeclient.Middleware { return // ignore APIs that do not have our group } - rt.MutateRequest(func(obj kubeclient.Object) { + rt.MutateRequest(func(obj kubeclient.Object) error { typeMeta := obj.GetObjectKind() origGVK := typeMeta.GroupVersionKind() newGVK := schema.GroupVersionKind{ @@ -44,6 +47,7 @@ func New(apiGroupSuffix string) kubeclient.Middleware { Kind: origGVK.Kind, } typeMeta.SetGroupVersionKind(newGVK) + return nil }) }), @@ -61,6 +65,49 @@ func New(apiGroupSuffix string) kubeclient.Middleware { rt.MutateRequest(mutateOwnerRefs(Replace, apiGroupSuffix)) }), + kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) { + // we only care if this is a create on a TokenCredentialRequest without a subresource + if rt.Resource() != loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests") || + rt.Verb() != kubeclient.VerbCreate || + rt.Subresource() != "" { + return + } + + // we only do this on the way out, since on the way back in we don't set a spec in our + // TokenCredentialRequest + rt.MutateRequest(func(obj kubeclient.Object) error { + tokenCredentialRequest, ok := obj.(*loginv1alpha1.TokenCredentialRequest) + if !ok { + return fmt.Errorf("cannot cast obj of type %T to *loginv1alpha1.TokenCredentialRequest", obj) + } + + if tokenCredentialRequest.Spec.Authenticator.APIGroup == nil { + // technically, the APIGroup field is optional, so clients are free to do this, but we + // want our middleware to be opinionated so that it can be really good at a specific task + // and give us specific feedback when it can't do that specific task + return fmt.Errorf( + "cannot replace token credential request %q without authenticator API group", + plog.KObj(obj), + ) + } + + mutatedAuthenticatorAPIGroup, ok := Replace(*tokenCredentialRequest.Spec.Authenticator.APIGroup, apiGroupSuffix) + if !ok { + // see comment above about specificity of middleware + return fmt.Errorf( + "cannot replace token credential request %q authenticator API group %q with group suffix %q", + plog.KObj(obj), + *tokenCredentialRequest.Spec.Authenticator.APIGroup, + apiGroupSuffix, + ) + } + + tokenCredentialRequest.Spec.Authenticator.APIGroup = &mutatedAuthenticatorAPIGroup + + return nil + }) + }), + kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) { // always unreplace owner refs with apiGroupSuffix because we can consume those objects across all verbs rt.MutateResponse(mutateOwnerRefs(Unreplace, apiGroupSuffix)) @@ -68,12 +115,12 @@ func New(apiGroupSuffix string) kubeclient.Middleware { } } -func mutateOwnerRefs(replaceFunc func(baseAPIGroup, apiGroupSuffix string) (string, bool), apiGroupSuffix string) func(kubeclient.Object) { - return func(obj kubeclient.Object) { +func mutateOwnerRefs(replaceFunc func(baseAPIGroup, apiGroupSuffix string) (string, bool), apiGroupSuffix string) func(kubeclient.Object) error { + return func(obj kubeclient.Object) error { // fix up owner refs because they are consumed by external and internal actors oldRefs := obj.GetOwnerReferences() if len(oldRefs) == 0 { - return + return nil } var changedGroup bool @@ -94,10 +141,12 @@ func mutateOwnerRefs(replaceFunc func(baseAPIGroup, apiGroupSuffix string) (stri } if !changedGroup { - return + return nil } obj.SetOwnerReferences(newRefs) + + return nil } } diff --git a/internal/groupsuffix/groupsuffix_test.go b/internal/groupsuffix/groupsuffix_test.go index f9c599e0..e9e2036b 100644 --- a/internal/groupsuffix/groupsuffix_test.go +++ b/internal/groupsuffix/groupsuffix_test.go @@ -14,6 +14,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + authv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/authentication/v1alpha1" loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" configv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/config/v1alpha1" "go.pinniped.dev/internal/kubeclient" @@ -31,6 +32,7 @@ func ExampleReplace_string() { fmt.Println(s) // Output: idp.supervisor.marlin.io } + func TestMiddlware(t *testing.T) { const newSuffix = "some.suffix.com" @@ -117,14 +119,14 @@ func TestMiddlware(t *testing.T) { }, }, } - federationDomainWithPinnipedOwnerWithNewGroup := &configv1alpha1.FederationDomain{ + federationDomainWithNewGroupAndPinnipedOwner := &configv1alpha1.FederationDomain{ TypeMeta: metav1.TypeMeta{ - APIVersion: configv1alpha1.SchemeGroupVersion.String(), + APIVersion: replaceGV(t, configv1alpha1.SchemeGroupVersion, newSuffix).String(), Kind: "FederationDomain", }, ObjectMeta: metav1.ObjectMeta{ OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(pinnipedOwner, pinnipedOwnerWithNewGroupGVK), + *metav1.NewControllerRef(pinnipedOwner, pinnipedOwnerGVK), // make sure we don't update the non-pinniped owner *metav1.NewControllerRef(nonPinnipedOwner, nonPinnipedOwnerGVK), @@ -133,7 +135,7 @@ func TestMiddlware(t *testing.T) { } federationDomainWithNewGroupAndPinnipedOwnerWithNewGroup := &configv1alpha1.FederationDomain{ TypeMeta: metav1.TypeMeta{ - APIVersion: replaceGV(t, configv1alpha1.SchemeGroupVersion, newSuffix), + APIVersion: replaceGV(t, configv1alpha1.SchemeGroupVersion, newSuffix).String(), Kind: "FederationDomain", }, ObjectMeta: metav1.ObjectMeta{ @@ -146,29 +148,40 @@ func TestMiddlware(t *testing.T) { }, } - tokenCredentialRequest := &loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{ - APIVersion: loginv1alpha1.SchemeGroupVersion.String(), - Kind: "TokenCredentialRequest", - }, - } - tokenCredentialRequestWithNewGroup := &loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{ - APIVersion: replaceGV(t, loginv1alpha1.SchemeGroupVersion, newSuffix), - Kind: "TokenCredentialRequest", - }, - } + tokenCredentialRequest := with( + &loginv1alpha1.TokenCredentialRequest{}, + gvk(replaceGV(t, loginv1alpha1.SchemeGroupVersion, newSuffix).WithKind("TokenCredentialRequest")), + ) + tokenCredentialRequestWithPinnipedAuthenticator := with( + tokenCredentialRequest, + authenticatorAPIGroup(authv1alpha1.SchemeGroupVersion.Group), + ) + tokenCredentialRequestWithCustomAPIGroupAuthenticator := with( + tokenCredentialRequest, + authenticatorAPIGroup(replaceGV(t, authv1alpha1.SchemeGroupVersion, newSuffix).Group), + ) + tokenCredentialRequestWithNewGroup := with( + tokenCredentialRequest, + gvk(replaceGV(t, loginv1alpha1.SchemeGroupVersion, newSuffix).WithKind("TokenCredentialRequest")), + ) + tokenCredentialRequestWithNewGroupAndPinnipedAuthenticator := with( + tokenCredentialRequestWithNewGroup, + authenticatorAPIGroup(authv1alpha1.SchemeGroupVersion.Group), + ) + tokenCredentialRequestWithNewGroupAndCustomAPIGroupAuthenticator := with( + tokenCredentialRequestWithNewGroup, + authenticatorAPIGroup(replaceGV(t, authv1alpha1.SchemeGroupVersion, newSuffix).Group), + ) tests := []struct { - name string - apiGroupSuffix string - rt *testutil.RoundTrip - requestObj, responseObj kubeclient.Object - wantNilMiddleware bool - wantMutateRequests, wantMutateResponses int - wantRequestObj, wantResponseObj kubeclient.Object - - skip bool + name string + apiGroupSuffix string + rt *testutil.RoundTrip + requestObj, responseObj kubeclient.Object + wantNilMiddleware bool + wantMutateRequests, wantMutateResponses int + wantMutateRequestErrors, wantMutateResponseErrors []string + wantRequestObj, wantResponseObj kubeclient.Object }{ { name: "api group suffix is empty", @@ -227,12 +240,12 @@ func TestMiddlware(t *testing.T) { rt: (&testutil.RoundTrip{}). WithVerb(kubeclient.VerbGet). WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), - requestObj: tokenCredentialRequest, - responseObj: tokenCredentialRequest, + requestObj: with(&metav1.PartialObjectMetadata{}, gvk(loginv1alpha1.SchemeGroupVersion.WithKind("TokenCredentialRequest"))), + responseObj: tokenCredentialRequestWithNewGroup, wantMutateRequests: 1, wantMutateResponses: 1, - wantRequestObj: tokenCredentialRequestWithNewGroup, - wantResponseObj: tokenCredentialRequest, + wantRequestObj: with(&metav1.PartialObjectMetadata{}, gvk(replaceGV(t, loginv1alpha1.SchemeGroupVersion, newSuffix).WithKind("TokenCredentialRequest"))), + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us }, { name: "create resource without pinniped.dev and without owner ref", @@ -289,7 +302,7 @@ func TestMiddlware(t *testing.T) { wantResponseObj: podWithPinnipedOwner, }, { - // test that both of our middleware request mutations play nicely with each other + // test that multiple of our middleware request mutations play nicely with each other name: "create resource with pinniped.dev and with owner ref that has pinniped.dev owner", apiGroupSuffix: newSuffix, rt: (&testutil.RoundTrip{}). @@ -297,11 +310,11 @@ func TestMiddlware(t *testing.T) { WithNamespace("some-namespace"). WithResource(configv1alpha1.SchemeGroupVersion.WithResource("federationdomains")), requestObj: federationDomainWithPinnipedOwner, - responseObj: federationDomainWithPinnipedOwnerWithNewGroup, + responseObj: federationDomainWithNewGroupAndPinnipedOwnerWithNewGroup, wantMutateRequests: 2, wantMutateResponses: 1, wantRequestObj: federationDomainWithNewGroupAndPinnipedOwnerWithNewGroup, - wantResponseObj: federationDomainWithPinnipedOwner, + wantResponseObj: federationDomainWithNewGroupAndPinnipedOwner, // the middleware will reset object GVK for us }, { name: "update resource without pinniped.dev", @@ -321,11 +334,11 @@ func TestMiddlware(t *testing.T) { WithVerb(kubeclient.VerbUpdate). WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), requestObj: tokenCredentialRequest, - responseObj: tokenCredentialRequest, + responseObj: tokenCredentialRequestWithNewGroup, wantMutateRequests: 1, wantMutateResponses: 1, wantRequestObj: tokenCredentialRequestWithNewGroup, - wantResponseObj: tokenCredentialRequest, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us }, { name: "list resource without pinniped.dev", @@ -345,11 +358,11 @@ func TestMiddlware(t *testing.T) { WithVerb(kubeclient.VerbList). WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), requestObj: tokenCredentialRequest, - responseObj: tokenCredentialRequest, + responseObj: tokenCredentialRequestWithNewGroup, wantMutateRequests: 1, wantMutateResponses: 1, wantRequestObj: tokenCredentialRequestWithNewGroup, - wantResponseObj: tokenCredentialRequest, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us }, { name: "watch resource without pinniped.dev", @@ -369,11 +382,11 @@ func TestMiddlware(t *testing.T) { WithVerb(kubeclient.VerbList). WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), requestObj: tokenCredentialRequest, - responseObj: tokenCredentialRequest, + responseObj: tokenCredentialRequestWithNewGroup, wantMutateRequests: 1, wantMutateResponses: 1, wantRequestObj: tokenCredentialRequestWithNewGroup, - wantResponseObj: tokenCredentialRequest, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us }, { name: "patch resource without pinniped.dev", @@ -390,23 +403,98 @@ func TestMiddlware(t *testing.T) { name: "patch resource with pinniped.dev", apiGroupSuffix: newSuffix, rt: (&testutil.RoundTrip{}). - WithVerb(kubeclient.VerbList). + WithVerb(kubeclient.VerbPatch). WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), requestObj: tokenCredentialRequest, - responseObj: tokenCredentialRequest, + responseObj: tokenCredentialRequestWithNewGroup, wantMutateRequests: 1, wantMutateResponses: 1, wantRequestObj: tokenCredentialRequestWithNewGroup, - wantResponseObj: tokenCredentialRequest, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us + }, + { + name: "create tokencredentialrequest with pinniped.dev authenticator api group", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequestWithPinnipedAuthenticator, + responseObj: tokenCredentialRequestWithNewGroup, // a token credential response does not contain a spec + wantMutateRequests: 3, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroupAndCustomAPIGroupAuthenticator, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us + }, + { + name: "create tokencredentialrequest with custom authenticator api group fails because api group is expected to be pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequestWithCustomAPIGroupAuthenticator, + responseObj: tokenCredentialRequestWithNewGroup, // a token credential response does not contain a spec + wantMutateRequests: 3, + wantMutateResponses: 1, + wantMutateRequestErrors: []string{`cannot replace token credential request "/" authenticator API group "authentication.concierge.some.suffix.com" with group suffix "some.suffix.com"`}, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us + }, + { + name: "create tokencredentialrequest with pinniped.dev authenticator api group and subresource", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")). + WithSubresource("some-subresource"), + requestObj: tokenCredentialRequestWithPinnipedAuthenticator, + responseObj: tokenCredentialRequestWithNewGroup, // a token credential response does not contain a spec + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroupAndPinnipedAuthenticator, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us + }, + { + name: "non-create tokencredentialrequest with pinniped.dev authenticator api group", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbList). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequestWithPinnipedAuthenticator, + responseObj: tokenCredentialRequestWithNewGroup, // a token credential response does not contain a spec + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroupAndPinnipedAuthenticator, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us + }, + { + name: "create tokencredentialrequest with nil authenticator api group", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequest, + responseObj: tokenCredentialRequestWithNewGroup, // a token credential response does not contain a spec + wantMutateRequests: 3, + wantMutateResponses: 1, + wantMutateRequestErrors: []string{`cannot replace token credential request "/" without authenticator API group`}, + wantResponseObj: tokenCredentialRequestWithNewGroup, // the middleware will reset object GVK for us + }, + { + name: "create tokencredentialrequest with non-*loginv1alpha1.TokenCredentialRequest", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: podWithoutOwner, + responseObj: podWithoutOwner, + wantMutateRequests: 3, + wantMutateResponses: 1, + wantMutateRequestErrors: []string{`cannot cast obj of type *v1.Pod to *loginv1alpha1.TokenCredentialRequest`}, + wantResponseObj: podWithoutOwner, }, } for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - if test.skip { - t.Skip() - } - m := New(test.apiGroupSuffix) if test.wantNilMiddleware { require.Nil(t, m, "wanted nil middleware") @@ -414,27 +502,41 @@ func TestMiddlware(t *testing.T) { } m.Handle(context.Background(), test.rt) - require.Len(t, test.rt.MutateRequests, test.wantMutateRequests) - require.Len(t, test.rt.MutateResponses, test.wantMutateResponses) + require.Len(t, test.rt.MutateRequests, test.wantMutateRequests, "undesired request mutation count") + require.Len(t, test.rt.MutateResponses, test.wantMutateResponses, "undesired response mutation count") if test.wantMutateRequests != 0 { require.NotNil(t, test.requestObj, "expected test.requestObj to be set") objMutated := test.requestObj.DeepCopyObject().(kubeclient.Object) + var mutateRequestErrors []string for _, mutateRequest := range test.rt.MutateRequests { mutateRequest := mutateRequest - mutateRequest(objMutated) + if err := mutateRequest(objMutated); err != nil { + mutateRequestErrors = append(mutateRequestErrors, err.Error()) + } + } + if len(test.wantMutateRequestErrors) > 0 { + require.Equal(t, test.wantMutateRequestErrors, mutateRequestErrors, "mutate request errors did not match") + } else { + require.Equal(t, test.wantRequestObj, objMutated, "request obj did not match") } - require.Equal(t, test.wantRequestObj, objMutated, "request obj did not match") } if test.wantMutateResponses != 0 { require.NotNil(t, test.responseObj, "expected test.responseObj to be set") objMutated := test.responseObj.DeepCopyObject().(kubeclient.Object) + var mutateResponseErrors []string for _, mutateResponse := range test.rt.MutateResponses { mutateResponse := mutateResponse - mutateResponse(objMutated) + if err := mutateResponse(objMutated); err != nil { + mutateResponseErrors = append(mutateResponseErrors, err.Error()) + } + } + if len(test.wantMutateRequestErrors) > 0 { + require.Equal(t, test.wantMutateResponseErrors, mutateResponseErrors, "mutate response errors did not match") + } else { + require.Equal(t, test.wantResponseObj, objMutated, "response obj did not match") } - require.Equal(t, test.wantResponseObj, objMutated, "response obj did not match") } }) } @@ -519,11 +621,35 @@ func TestValidate(t *testing.T) { } } -func replaceGV(t *testing.T, baseGV schema.GroupVersion, apiGroupSuffix string) string { +type withFunc func(obj kubeclient.Object) + +func with(obj kubeclient.Object, withFuncs ...withFunc) kubeclient.Object { + obj = obj.DeepCopyObject().(kubeclient.Object) + for _, withFunc := range withFuncs { + withFunc(obj) + } + return obj +} + +func gvk(gvk schema.GroupVersionKind) withFunc { + return func(obj kubeclient.Object) { + obj.GetObjectKind().SetGroupVersionKind(gvk) + } +} + +func authenticatorAPIGroup(apiGroup string) withFunc { + return func(obj kubeclient.Object) { + tokenCredentialRequest := obj.(*loginv1alpha1.TokenCredentialRequest) + tokenCredentialRequest.Spec.Authenticator.APIGroup = &apiGroup + } +} + +//nolint:unparam // the apiGroupSuffix parameter might always be the same, but this is nice for test readability +func replaceGV(t *testing.T, baseGV schema.GroupVersion, apiGroupSuffix string) schema.GroupVersion { t.Helper() groupName, ok := Replace(baseGV.Group, apiGroupSuffix) require.True(t, ok, "expected to be able to replace %q's suffix with %q", baseGV.Group, apiGroupSuffix) - return schema.GroupVersion{Group: groupName, Version: baseGV.Version}.String() + return schema.GroupVersion{Group: groupName, Version: baseGV.Version} } func chars(count int) string { diff --git a/internal/kubeclient/kubeclient_test.go b/internal/kubeclient/kubeclient_test.go index 2bb4aaee..cd79a920 100644 --- a/internal/kubeclient/kubeclient_test.go +++ b/internal/kubeclient/kubeclient_test.go @@ -492,8 +492,9 @@ func TestKubeclient(t *testing.T) { return []*spyMiddleware{{ name: "non-pertinent mutater", t: t, - mutateReq: func(rt RoundTrip, obj Object) { + mutateReq: func(rt RoundTrip, obj Object) error { clusterName()(obj) + return nil }, }} }, @@ -529,6 +530,64 @@ func TestKubeclient(t *testing.T) { wantMiddlewareReqs: [][]Object{{with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK))}}, wantMiddlewareResps: [][]Object{nil}, }, + { + name: "when there are request middleware failures, we return an error and don't send the request", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{ + // use 3 middleware to ensure that we collect all errors from all middlewares + newFailingMiddleware(t, "aaa", true, false), + newFailingMiddleware(t, "bbb", false, false), + newFailingMiddleware(t, "ccc", true, false), + } + }, + reallyRunTest: func(t *testing.T, c *Client) { + _, err := c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(goodFederationDomain.Namespace). + Get(context.Background(), goodFederationDomain.Name, metav1.GetOptions{}) + require.Error(t, err) + require.Contains(t, err.Error(), ": request mutation failed: [aaa: request error, ccc: request error]") + }, + wantMiddlewareReqs: [][]Object{ + {with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK))}, + {with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK))}, + {with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK))}, + }, + wantMiddlewareResps: [][]Object{ + nil, + nil, + nil, + }, + }, + { + name: "when there are response middleware failures, we return an error", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{ + // use 3 middleware to ensure that we collect all errors from all middlewares + newFailingMiddleware(t, "aaa", false, true), + newFailingMiddleware(t, "bbb", false, false), + newFailingMiddleware(t, "ccc", false, true), + } + }, + reallyRunTest: func(t *testing.T, c *Client) { + _, err := c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(goodFederationDomain.Namespace). + Create(context.Background(), goodFederationDomain, metav1.CreateOptions{}) + require.Error(t, err) + require.Contains(t, err.Error(), ": response mutation failed: [aaa: response error, ccc: response error]") + }, + wantMiddlewareReqs: [][]Object{ + {with(goodFederationDomain, gvk(federationDomainGVK))}, + {with(goodFederationDomain, gvk(federationDomainGVK))}, + {with(goodFederationDomain, gvk(federationDomainGVK))}, + }, + wantMiddlewareResps: [][]Object{ + {with(goodFederationDomain, gvk(federationDomainGVK))}, + {with(goodFederationDomain, gvk(federationDomainGVK))}, + {with(goodFederationDomain, gvk(federationDomainGVK))}, + }, + }, } for _, test := range tests { test := test @@ -568,8 +627,8 @@ func TestKubeclient(t *testing.T) { type spyMiddleware struct { name string t *testing.T - mutateReq func(RoundTrip, Object) - mutateResp func(RoundTrip, Object) + mutateReq func(RoundTrip, Object) error + mutateResp func(RoundTrip, Object) error reqObjs []Object respObjs []Object } @@ -578,18 +637,18 @@ func (s *spyMiddleware) Handle(_ context.Context, rt RoundTrip) { s.t.Log(s.name, "handling", reqStr(rt, nil)) if s.mutateReq != nil { - rt.MutateRequest(func(obj Object) { + rt.MutateRequest(func(obj Object) error { s.t.Log(s.name, "mutating request", reqStr(rt, obj)) s.reqObjs = append(s.reqObjs, obj.DeepCopyObject().(Object)) - s.mutateReq(rt, obj) + return s.mutateReq(rt, obj) }) } if s.mutateResp != nil { - rt.MutateResponse(func(obj Object) { + rt.MutateResponse(func(obj Object) error { s.t.Log(s.name, "mutating response", reqStr(rt, obj)) s.respObjs = append(s.respObjs, obj.DeepCopyObject().(Object)) - s.mutateResp(rt, obj) + return s.mutateResp(rt, obj) }) } } @@ -611,17 +670,19 @@ func newAnnotationMiddleware(t *testing.T) *spyMiddleware { return &spyMiddleware{ name: "annotater", t: t, - mutateReq: func(rt RoundTrip, obj Object) { + mutateReq: func(rt RoundTrip, obj Object) error { if rt.Verb() == VerbCreate { annotations()(obj) } + return nil }, - mutateResp: func(rt RoundTrip, obj Object) { + mutateResp: func(rt RoundTrip, obj Object) error { if rt.Verb() == VerbCreate { for key := range middlewareAnnotations { delete(obj.GetAnnotations(), key) } } + return nil }, } } @@ -630,41 +691,69 @@ func newLabelMiddleware(t *testing.T) *spyMiddleware { return &spyMiddleware{ name: "labeler", t: t, - mutateReq: func(rt RoundTrip, obj Object) { + mutateReq: func(rt RoundTrip, obj Object) error { if rt.Verb() == VerbCreate { labels()(obj) } + return nil }, - mutateResp: func(rt RoundTrip, obj Object) { + mutateResp: func(rt RoundTrip, obj Object) error { if rt.Verb() == VerbCreate { for key := range middlewareLabels { delete(obj.GetLabels(), key) } } + return nil }, } } func newSimpleMiddleware(t *testing.T, hasMutateReqFunc, mutatedReq, hasMutateRespFunc bool) *spyMiddleware { m := &spyMiddleware{ - name: "nop", + name: "simple", t: t, } if hasMutateReqFunc { - m.mutateReq = func(rt RoundTrip, obj Object) { + m.mutateReq = func(rt RoundTrip, obj Object) error { if mutatedReq { if rt.Verb() == VerbCreate { obj.SetClusterName(someClusterName) } } + return nil } } if hasMutateRespFunc { - m.mutateResp = func(rt RoundTrip, obj Object) {} + m.mutateResp = func(rt RoundTrip, obj Object) error { + return nil + } } return m } +func newFailingMiddleware(t *testing.T, name string, mutateReqFails, mutateRespFails bool) *spyMiddleware { + m := &spyMiddleware{ + name: "failing-middleware-" + name, + t: t, + } + + m.mutateReq = func(rt RoundTrip, obj Object) error { + if mutateReqFails { + return fmt.Errorf("%s: request error", name) + } + return nil + } + + m.mutateResp = func(rt RoundTrip, obj Object) error { + if mutateRespFails { + return fmt.Errorf("%s: response error", name) + } + return nil + } + + return m +} + type wantCloser struct { io.ReadCloser closeCount int diff --git a/internal/kubeclient/middleware.go b/internal/kubeclient/middleware.go index 4c4a02be..f35b3532 100644 --- a/internal/kubeclient/middleware.go +++ b/internal/kubeclient/middleware.go @@ -12,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/errors" ) type Middleware interface { @@ -43,8 +44,8 @@ type RoundTrip interface { NamespaceScoped() bool Resource() schema.GroupVersionResource Subresource() string - MutateRequest(f func(obj Object)) - MutateResponse(f func(obj Object)) + MutateRequest(f func(obj Object) error) + MutateResponse(f func(obj Object) error) } type Object interface { @@ -58,7 +59,7 @@ type request struct { verb Verb namespace string resource schema.GroupVersionResource - reqFuncs, respFuncs []func(obj Object) + reqFuncs, respFuncs []func(obj Object) error subresource string } @@ -89,11 +90,11 @@ func (r *request) Subresource() string { return r.subresource } -func (r *request) MutateRequest(f func(obj Object)) { +func (r *request) MutateRequest(f func(obj Object) error) { r.reqFuncs = append(r.reqFuncs, f) } -func (r *request) MutateResponse(f func(obj Object)) { +func (r *request) MutateResponse(f func(obj Object) error) { r.respFuncs = append(r.respFuncs, f) } @@ -113,9 +114,15 @@ func (r *request) mutateRequest(obj Object) (*mutationResult, error) { return nil, fmt.Errorf("invalid deep copy semantics for %T: %#v", obj, r) } + var errs []error for _, reqFunc := range r.reqFuncs { reqFunc := reqFunc - reqFunc(obj) + if err := reqFunc(obj); err != nil { + errs = append(errs, err) + } + } + if err := errors.NewAggregate(errs); err != nil { + return nil, fmt.Errorf("request mutation failed: %w", err) } newGVK := obj.GetObjectKind().GroupVersionKind() @@ -137,9 +144,15 @@ func (r *request) mutateResponse(obj Object) (bool, error) { return false, fmt.Errorf("invalid deep copy semantics for %T: %#v", obj, r) } + var errs []error for _, respFunc := range r.respFuncs { respFunc := respFunc - respFunc(obj) + if err := respFunc(obj); err != nil { + errs = append(errs, err) + } + } + if err := errors.NewAggregate(errs); err != nil { + return false, fmt.Errorf("response mutation failed: %w", err) } mutated := !apiequality.Semantic.DeepEqual(origObj, obj) diff --git a/internal/kubeclient/middleware_test.go b/internal/kubeclient/middleware_test.go index 4c94e8df..583b3f74 100644 --- a/internal/kubeclient/middleware_test.go +++ b/internal/kubeclient/middleware_test.go @@ -15,7 +15,7 @@ import ( func Test_request_mutate(t *testing.T) { tests := []struct { name string - reqFuncs []func(Object) + reqFuncs []func(Object) error obj Object want *mutationResult wantObj Object @@ -23,10 +23,11 @@ func Test_request_mutate(t *testing.T) { }{ { name: "mutate config map data", - reqFuncs: []func(Object){ - func(obj Object) { + reqFuncs: []func(Object) error{ + func(obj Object) error { cm := obj.(*corev1.ConfigMap) cm.Data = map[string]string{"new": "stuff"} + return nil }, }, obj: &corev1.ConfigMap{ diff --git a/internal/kubeclient/verb_test.go b/internal/kubeclient/verb_test.go index 7b2bf61b..c9df4f89 100644 --- a/internal/kubeclient/verb_test.go +++ b/internal/kubeclient/verb_test.go @@ -42,7 +42,7 @@ func Test_verb(t *testing.T) { f: func() string { return fmt.Errorf("%#v", request{verb: VerbPatch}).Error() }, - want: `kubeclient.request{verb:"patch", namespace:"", resource:schema.GroupVersionResource{Group:"", Version:"", Resource:""}, reqFuncs:[]func(kubeclient.Object)(nil), respFuncs:[]func(kubeclient.Object)(nil), subresource:""}`, + want: `kubeclient.request{verb:"patch", namespace:"", resource:schema.GroupVersionResource{Group:"", Version:"", Resource:""}, reqFuncs:[]func(kubeclient.Object) error(nil), respFuncs:[]func(kubeclient.Object) error(nil), subresource:""}`, }, } for _, tt := range tests { diff --git a/internal/ownerref/ownerref.go b/internal/ownerref/ownerref.go index b036d5d2..854f7547 100644 --- a/internal/ownerref/ownerref.go +++ b/internal/ownerref/ownerref.go @@ -51,13 +51,15 @@ func New(refObj kubeclient.Object) kubeclient.Middleware { return } - rt.MutateRequest(func(obj kubeclient.Object) { + rt.MutateRequest(func(obj kubeclient.Object) error { // we only want to set the owner ref on create and when one is not already present if len(obj.GetOwnerReferences()) != 0 { - return + return nil } obj.SetOwnerReferences([]metav1.OwnerReference{ref}) + + return nil }) }) } diff --git a/internal/ownerref/ownerref_test.go b/internal/ownerref/ownerref_test.go index 0875ea7c..7f98ff94 100644 --- a/internal/ownerref/ownerref_test.go +++ b/internal/ownerref/ownerref_test.go @@ -213,7 +213,7 @@ func TestOwnerReferenceMiddleware(t *testing.T) { orig := tt.args.obj.DeepCopyObject().(kubeclient.Object) for _, mutateRequest := range rt.MutateRequests { mutateRequest := mutateRequest - mutateRequest(tt.args.obj) + require.NoError(t, mutateRequest(tt.args.obj)) } if !tt.wantMutates { require.Equal(t, orig, tt.args.obj) diff --git a/internal/testutil/round_trip.go b/internal/testutil/roundtrip.go similarity index 87% rename from internal/testutil/round_trip.go rename to internal/testutil/roundtrip.go index 9b12b0e6..8c04e7c4 100644 --- a/internal/testutil/round_trip.go +++ b/internal/testutil/roundtrip.go @@ -17,7 +17,7 @@ type RoundTrip struct { resource schema.GroupVersionResource subresource string - MutateRequests, MutateResponses []func(kubeclient.Object) + MutateRequests, MutateResponses []func(kubeclient.Object) error } func (rt *RoundTrip) WithVerb(verb kubeclient.Verb) *RoundTrip { @@ -61,10 +61,10 @@ func (rt *RoundTrip) Subresource() string { return rt.subresource } -func (rt *RoundTrip) MutateRequest(fn func(kubeclient.Object)) { +func (rt *RoundTrip) MutateRequest(fn func(kubeclient.Object) error) { rt.MutateRequests = append(rt.MutateRequests, fn) } -func (rt *RoundTrip) MutateResponse(fn func(kubeclient.Object)) { +func (rt *RoundTrip) MutateResponse(fn func(kubeclient.Object) error) { rt.MutateResponses = append(rt.MutateResponses, fn) } diff --git a/pkg/conciergeclient/conciergeclient.go b/pkg/conciergeclient/conciergeclient.go index 6f4c704e..0bebd3d2 100644 --- a/pkg/conciergeclient/conciergeclient.go +++ b/pkg/conciergeclient/conciergeclient.go @@ -12,7 +12,7 @@ import ( "net/url" "strings" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" "k8s.io/client-go/tools/clientcmd" @@ -34,12 +34,11 @@ type Option func(*Client) error // Client is a configuration for talking to the Pinniped concierge. type Client struct { - namespace string - authenticatorName string - authenticatorKind string - caBundle string - endpoint *url.URL - apiGroupSuffix string + namespace string + authenticator *corev1.TypedLocalObjectReference + caBundle string + endpoint *url.URL + apiGroupSuffix string } // WithNamespace configures the namespace where the TokenCredentialRequest is to be sent. @@ -56,15 +55,18 @@ func WithAuthenticator(authType, authName string) Option { if authName == "" { return fmt.Errorf("authenticator name must not be empty") } - c.authenticatorName = authName + authenticator := corev1.TypedLocalObjectReference{Name: authName} switch strings.ToLower(authType) { case "webhook": - c.authenticatorKind = "WebhookAuthenticator" + authenticator.APIGroup = &auth1alpha1.SchemeGroupVersion.Group + authenticator.Kind = "WebhookAuthenticator" case "jwt": - c.authenticatorKind = "JWTAuthenticator" + authenticator.APIGroup = &auth1alpha1.SchemeGroupVersion.Group + authenticator.Kind = "JWTAuthenticator" default: return fmt.Errorf(`invalid authenticator type: %q, supported values are "webhook" and "jwt"`, authType) } + c.authenticator = &authenticator return nil } } @@ -131,7 +133,7 @@ func New(opts ...Option) (*Client, error) { return nil, err } } - if c.authenticatorName == "" { + if c.authenticator == nil { return nil, fmt.Errorf("WithAuthenticator must be specified") } if c.endpoint == nil { @@ -178,18 +180,13 @@ func (c *Client) ExchangeToken(ctx context.Context, token string) (*clientauthen if err != nil { return nil, err } - replacedAPIGroupName, _ := groupsuffix.Replace(auth1alpha1.SchemeGroupVersion.Group, c.apiGroupSuffix) resp, err := clientset.LoginV1alpha1().TokenCredentialRequests(c.namespace).Create(ctx, &loginv1alpha1.TokenCredentialRequest{ ObjectMeta: metav1.ObjectMeta{ Namespace: c.namespace, }, Spec: loginv1alpha1.TokenCredentialRequestSpec{ - Token: token, - Authenticator: v1.TypedLocalObjectReference{ - APIGroup: &replacedAPIGroupName, - Kind: c.authenticatorKind, - Name: c.authenticatorName, - }, + Token: token, + Authenticator: *c.authenticator, }, }, metav1.CreateOptions{}) if err != nil { diff --git a/pkg/conciergeclient/conciergeclient_test.go b/pkg/conciergeclient/conciergeclient_test.go index bdf27364..c4f6702d 100644 --- a/pkg/conciergeclient/conciergeclient_test.go +++ b/pkg/conciergeclient/conciergeclient_test.go @@ -21,7 +21,6 @@ import ( loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" "go.pinniped.dev/internal/certauthority" - "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/testutil" ) @@ -221,7 +220,47 @@ func TestExchangeToken(t *testing.T) { t.Parallel() expires := metav1.NewTime(time.Now().Truncate(time.Second)) - caBundle, endpoint := runFakeServer(t, expires, "pinniped.dev") + // Start a test server that returns successfully and asserts various properties of the request. + caBundle, endpoint := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPost, r.Method) + require.Equal(t, "/apis/login.concierge.pinniped.dev/v1alpha1/namespaces/test-namespace/tokencredentialrequests", r.URL.Path) + require.Equal(t, "application/json", r.Header.Get("content-type")) + + body, err := ioutil.ReadAll(r.Body) + require.NoError(t, err) + require.JSONEq(t, + `{ + "kind": "TokenCredentialRequest", + "apiVersion": "login.concierge.pinniped.dev/v1alpha1", + "metadata": { + "creationTimestamp": null, + "namespace": "test-namespace" + }, + "spec": { + "token": "test-token", + "authenticator": { + "apiGroup": "authentication.concierge.pinniped.dev", + "kind": "WebhookAuthenticator", + "name": "test-webhook" + } + }, + "status": {} + }`, + string(body), + ) + + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&loginv1alpha1.TokenCredentialRequest{ + TypeMeta: metav1.TypeMeta{APIVersion: "login.concierge.pinniped.dev/v1alpha1", Kind: "TokenCredentialRequest"}, + Status: loginv1alpha1.TokenCredentialRequestStatus{ + Credential: &loginv1alpha1.ClusterCredential{ + ExpirationTimestamp: expires, + ClientCertificateData: "test-certificate", + ClientKeyData: "test-key", + }, + }, + }) + }) client, err := New(WithNamespace("test-namespace"), WithEndpoint(endpoint), WithCABundle(caBundle), WithAuthenticator("webhook", "test-webhook")) require.NoError(t, err) @@ -240,78 +279,4 @@ func TestExchangeToken(t *testing.T) { }, }, got) }) - - t.Run("changing the API group suffix for the client sends the custom suffix on the CredentialRequest's APIGroup and on its spec.Authenticator.APIGroup", func(t *testing.T) { - t.Parallel() - expires := metav1.NewTime(time.Now().Truncate(time.Second)) - - caBundle, endpoint := runFakeServer(t, expires, "suffix.com") - - client, err := New(WithAPIGroupSuffix("suffix.com"), WithNamespace("test-namespace"), WithEndpoint(endpoint), WithCABundle(caBundle), WithAuthenticator("webhook", "test-webhook")) - require.NoError(t, err) - - got, err := client.ExchangeToken(ctx, "test-token") - require.NoError(t, err) - require.Equal(t, &clientauthenticationv1beta1.ExecCredential{ - TypeMeta: metav1.TypeMeta{ - Kind: "ExecCredential", - APIVersion: "client.authentication.k8s.io/v1beta1", - }, - Status: &clientauthenticationv1beta1.ExecCredentialStatus{ - ClientCertificateData: "test-certificate", - ClientKeyData: "test-key", - ExpirationTimestamp: &expires, - }, - }, got) - }) -} - -// Start a test server that returns successfully and asserts various properties of the request. -func runFakeServer(t *testing.T, expires metav1.Time, pinnipedAPIGroupSuffix string) (string, string) { - caBundle, endpoint := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, http.MethodPost, r.Method) - require.Equal(t, - fmt.Sprintf("/apis/login.concierge.%s/v1alpha1/namespaces/test-namespace/tokencredentialrequests", pinnipedAPIGroupSuffix), - r.URL.Path) - require.Equal(t, "application/json", r.Header.Get("content-type")) - - body, err := ioutil.ReadAll(r.Body) - require.NoError(t, err) - require.JSONEq(t, here.Docf( - `{ - "kind": "TokenCredentialRequest", - "apiVersion": "login.concierge.%s/v1alpha1", - "metadata": { - "creationTimestamp": null, - "namespace": "test-namespace" - }, - "spec": { - "token": "test-token", - "authenticator": { - "apiGroup": "authentication.concierge.%s", - "kind": "WebhookAuthenticator", - "name": "test-webhook" - } - }, - "status": {} - }`, pinnipedAPIGroupSuffix, pinnipedAPIGroupSuffix), - string(body), - ) - - w.Header().Set("content-type", "application/json") - _ = json.NewEncoder(w).Encode(&loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{ - APIVersion: fmt.Sprintf("login.concierge.%s/v1alpha1", pinnipedAPIGroupSuffix), - Kind: "TokenCredentialRequest", - }, - Status: loginv1alpha1.TokenCredentialRequestStatus{ - Credential: &loginv1alpha1.ClusterCredential{ - ExpirationTimestamp: expires, - ClientCertificateData: "test-certificate", - ClientKeyData: "test-key", - }, - }, - }) - }) - return caBundle, endpoint } diff --git a/test/integration/concierge_api_serving_certs_test.go b/test/integration/concierge_api_serving_certs_test.go index 2aabe315..a0a67acf 100644 --- a/test/integration/concierge_api_serving_certs_test.go +++ b/test/integration/concierge_api_serving_certs_test.go @@ -78,11 +78,15 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { kubeClient := library.NewKubernetesClientset(t) aggregatedClient := library.NewAggregatedClientset(t) conciergeClient := library.NewConciergeClientset(t) - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() apiServiceName := "v1alpha1.login.concierge." + env.APIGroupSuffix + // Create a testWebhook so we have a legitimate authenticator to pass to the + // TokenCredentialRequest API. + testWebhook := library.CreateTestWebhookAuthenticator(ctx, t) + // Get the initial auto-generated version of the Secret. secret, err := kubeClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, defaultServingCertResourceName, metav1.GetOptions{}) require.NoError(t, err) @@ -145,7 +149,7 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { _, err = conciergeClient.LoginV1alpha1().TokenCredentialRequests(env.ConciergeNamespace).Create(ctx, &loginv1alpha1.TokenCredentialRequest{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{}, - Spec: loginv1alpha1.TokenCredentialRequestSpec{Token: "not a good token"}, + Spec: loginv1alpha1.TokenCredentialRequestSpec{Token: "not a good token", Authenticator: testWebhook}, }, metav1.CreateOptions{}) if err != nil { break diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index 29878b9f..46ad2357 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -135,7 +135,16 @@ func TestFailedCredentialRequestWhenTheRequestIsValidButTheTokenDoesNotAuthentic library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - response, err := makeRequest(context.Background(), t, loginv1alpha1.TokenCredentialRequestSpec{Token: "not a good token"}) + // Create a testWebhook so we have a legitimate authenticator to pass to the + // TokenCredentialRequest API. + ctx, cancel := context.WithTimeout(context.Background(), time.Second*30) + defer cancel() + testWebhook := library.CreateTestWebhookAuthenticator(ctx, t) + + response, err := makeRequest(context.Background(), t, loginv1alpha1.TokenCredentialRequestSpec{ + Token: "not a good token", + Authenticator: testWebhook, + }) require.NoError(t, err) @@ -149,7 +158,16 @@ func TestCredentialRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T library.AssertNoRestartsDuringTest(t, env.ConciergeNamespace, "") - response, err := makeRequest(context.Background(), t, loginv1alpha1.TokenCredentialRequestSpec{Token: ""}) + // Create a testWebhook so we have a legitimate authenticator to pass to the + // TokenCredentialRequest API. + ctx, cancel := context.WithTimeout(context.Background(), time.Second*30) + defer cancel() + testWebhook := library.CreateTestWebhookAuthenticator(ctx, t) + + response, err := makeRequest(context.Background(), t, loginv1alpha1.TokenCredentialRequestSpec{ + Token: "", + Authenticator: testWebhook, + }) require.Error(t, err) statusError, isStatus := err.(*errors.StatusError) diff --git a/test/library/client.go b/test/library/client.go index 392c153e..011f2b77 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -180,11 +180,8 @@ func CreateTestWebhookAuthenticator(ctx context.Context, t *testing.T) corev1.Ty require.NoErrorf(t, err, "could not cleanup test WebhookAuthenticator %s/%s", webhook.Namespace, webhook.Name) }) - apiGroup, replacedSuffix := groupsuffix.Replace(auth1alpha1.SchemeGroupVersion.Group, testEnv.APIGroupSuffix) - require.True(t, replacedSuffix) - return corev1.TypedLocalObjectReference{ - APIGroup: &apiGroup, + APIGroup: &auth1alpha1.SchemeGroupVersion.Group, Kind: "WebhookAuthenticator", Name: webhook.Name, } @@ -253,11 +250,8 @@ func CreateTestJWTAuthenticator(ctx context.Context, t *testing.T, spec auth1alp require.NoErrorf(t, err, "could not cleanup test JWTAuthenticator %s/%s", jwtAuthenticator.Namespace, jwtAuthenticator.Name) }) - apiGroup, replacedSuffix := groupsuffix.Replace(auth1alpha1.SchemeGroupVersion.Group, testEnv.APIGroupSuffix) - require.True(t, replacedSuffix) - return corev1.TypedLocalObjectReference{ - APIGroup: &apiGroup, + APIGroup: &auth1alpha1.SchemeGroupVersion.Group, Kind: "JWTAuthenticator", Name: jwtAuthenticator.Name, }