From f7958ae75b2dda50b407e1bd8f18741bcc7a5c9e Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 5 Feb 2021 10:55:19 -0500 Subject: [PATCH] Add no-op list support to token credential request This allows us to keep all of our resources in the pinniped category while not having kubectl return errors for calls such as: kubectl get pinniped -A Signed-off-by: Monis Khan --- internal/concierge/apiserver/apiserver.go | 2 +- internal/registry/credentialrequest/rest.go | 32 ++++++- .../registry/credentialrequest/rest_test.go | 48 +++++++--- test/integration/category_test.go | 96 +++++++++++++++++++ test/integration/kube_api_discovery_test.go | 2 +- 5 files changed, 161 insertions(+), 19 deletions(-) create mode 100644 test/integration/category_test.go diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index af794115..51017066 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -71,7 +71,7 @@ func (c completedConfig) New() (*PinnipedServer, error) { } gvr := c.ExtraConfig.GroupVersion.WithResource("tokencredentialrequests") - storage := credentialrequest.NewREST(c.ExtraConfig.Authenticator, c.ExtraConfig.Issuer) + storage := credentialrequest.NewREST(c.ExtraConfig.Authenticator, c.ExtraConfig.Issuer, gvr.GroupResource()) if err := s.GenericAPIServer.InstallAPIGroup(&genericapiserver.APIGroupInfo{ PrioritizedVersions: []schema.GroupVersion{gvr.GroupVersion()}, VersionedResourcesStorageMap: map[string]map[string]rest.Storage{gvr.Version: {gvr.Resource: storage}}, diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index 3ecc88e2..dbe6821a 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -11,8 +11,10 @@ import ( "time" apierrors "k8s.io/apimachinery/pkg/api/errors" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/registry/rest" @@ -32,16 +34,18 @@ type TokenCredentialRequestAuthenticator interface { AuthenticateTokenCredentialRequest(ctx context.Context, req *loginapi.TokenCredentialRequest) (user.Info, error) } -func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer CertIssuer) *REST { +func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer CertIssuer, resource schema.GroupResource) *REST { return &REST{ - authenticator: authenticator, - issuer: issuer, + authenticator: authenticator, + issuer: issuer, + tableConvertor: rest.NewDefaultTableConvertor(resource), } } type REST struct { - authenticator TokenCredentialRequestAuthenticator - issuer CertIssuer + authenticator TokenCredentialRequestAuthenticator + issuer CertIssuer + tableConvertor rest.TableConvertor } // Assert that our *REST implements all the optional interfaces that we expect it to implement. @@ -51,12 +55,30 @@ var _ interface { rest.Scoper rest.Storage rest.CategoriesProvider + rest.Lister } = (*REST)(nil) func (*REST) New() runtime.Object { return &loginapi.TokenCredentialRequest{} } +func (*REST) NewList() runtime.Object { + return &loginapi.TokenCredentialRequestList{} +} + +func (*REST) List(_ context.Context, _ *metainternalversion.ListOptions) (runtime.Object, error) { + return &loginapi.TokenCredentialRequestList{ + ListMeta: metav1.ListMeta{ + ResourceVersion: "0", // this resource version means "from the API server cache" + }, + Items: []loginapi.TokenCredentialRequest{}, // avoid sending nil items list + }, nil +} + +func (r *REST) ConvertToTable(ctx context.Context, obj runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { + return r.tableConvertor.ConvertToTable(ctx, obj, tableOptions) +} + func (*REST) NamespaceScoped() bool { return true } diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 40a750f3..b72af025 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -17,6 +17,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/authentication/user" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" @@ -28,11 +29,34 @@ import ( ) func TestNew(t *testing.T) { - r := NewREST(nil, nil) + r := NewREST(nil, nil, schema.GroupResource{Group: "bears", Resource: "panda"}) require.NotNil(t, r) require.True(t, r.NamespaceScoped()) require.Equal(t, []string{"pinniped"}, r.Categories()) require.IsType(t, &loginapi.TokenCredentialRequest{}, r.New()) + require.IsType(t, &loginapi.TokenCredentialRequestList{}, r.NewList()) + + ctx := context.Background() + + // check the simple invariants of our no-op list + list, err := r.List(ctx, nil) + require.NoError(t, err) + require.NotNil(t, list) + require.IsType(t, &loginapi.TokenCredentialRequestList{}, list) + require.Equal(t, "0", list.(*loginapi.TokenCredentialRequestList).ResourceVersion) + require.NotNil(t, list.(*loginapi.TokenCredentialRequestList).Items) + require.Len(t, list.(*loginapi.TokenCredentialRequestList).Items, 0) + + // make sure we can turn lists into tables if needed + table, err := r.ConvertToTable(ctx, list, nil) + require.NoError(t, err) + require.NotNil(t, table) + require.Equal(t, "0", table.ResourceVersion) + require.Nil(t, table.Rows) + + // exercise group resource - force error by passing a runtime.Object that does not have an embedded object meta + _, err = r.ConvertToTable(ctx, &metav1.APIGroup{}, nil) + require.Error(t, err, "the resource panda.bears does not support being converted to a Table") } func TestCreate(t *testing.T) { @@ -73,7 +97,7 @@ func TestCreate(t *testing.T) { 5*time.Minute, ).Return([]byte("test-cert"), []byte("test-key"), nil) - storage := NewREST(requestAuthenticator, issuer) + storage := NewREST(requestAuthenticator, issuer, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, req) @@ -112,7 +136,7 @@ func TestCreate(t *testing.T) { IssuePEM(gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, nil, fmt.Errorf("some certificate authority error")) - storage := NewREST(requestAuthenticator, issuer) + storage := NewREST(requestAuthenticator, issuer, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) @@ -125,7 +149,7 @@ func TestCreate(t *testing.T) { requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req).Return(nil, nil) - storage := NewREST(requestAuthenticator, nil) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, req) @@ -140,7 +164,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). Return(nil, errors.New("some webhook error")) - storage := NewREST(requestAuthenticator, nil) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, req) @@ -155,7 +179,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). Return(&user.DefaultInfo{Name: ""}, nil) - storage := NewREST(requestAuthenticator, nil) + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, req) @@ -165,7 +189,7 @@ func TestCreate(t *testing.T) { it("CreateFailsWhenGivenTheWrongInputType", func() { notACredentialRequest := runtime.Unknown{} - response, err := NewREST(nil, nil).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}).Create( genericapirequest.NewContext(), ¬ACredentialRequest, rest.ValidateAllObjectFunc, @@ -176,7 +200,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenTokenValueIsEmptyInRequest", func() { - storage := NewREST(nil, nil) + storage := NewREST(nil, nil, schema.GroupResource{}) response, err := callCreate(context.Background(), storage, credentialRequest(loginapi.TokenCredentialRequestSpec{ Token: "", })) @@ -187,7 +211,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenValidationFails", func() { - storage := NewREST(nil, nil) + storage := NewREST(nil, nil, schema.GroupResource{}) response, err := storage.Create( context.Background(), validCredentialRequest(), @@ -207,7 +231,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req.DeepCopy()). Return(&user.DefaultInfo{Name: "test-user"}, nil) - storage := NewREST(requestAuthenticator, successfulIssuer(ctrl)) + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl), schema.GroupResource{}) response, err := storage.Create( context.Background(), req, @@ -228,7 +252,7 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req.DeepCopy()). Return(&user.DefaultInfo{Name: "test-user"}, nil) - storage := NewREST(requestAuthenticator, successfulIssuer(ctrl)) + storage := NewREST(requestAuthenticator, successfulIssuer(ctrl), schema.GroupResource{}) validationFunctionWasCalled := false var validationFunctionSawTokenValue string response, err := storage.Create( @@ -248,7 +272,7 @@ func TestCreate(t *testing.T) { }) it("CreateFailsWhenRequestOptionsDryRunIsNotEmpty", func() { - response, err := NewREST(nil, nil).Create( + response, err := NewREST(nil, nil, schema.GroupResource{}).Create( genericapirequest.NewContext(), validCredentialRequest(), rest.ValidateAllObjectFunc, diff --git a/test/integration/category_test.go b/test/integration/category_test.go new file mode 100644 index 00000000..a6957567 --- /dev/null +++ b/test/integration/category_test.go @@ -0,0 +1,96 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "bytes" + "os/exec" + "testing" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/test/library" +) + +func TestGetPinnipedCategory(t *testing.T) { + env := library.IntegrationEnv(t) + dotSuffix := "." + env.APIGroupSuffix + + t.Run("category, no special params", func(t *testing.T) { + var stdOut, stdErr bytes.Buffer + + cmd := exec.Command("kubectl", "get", "pinniped", "-A") + cmd.Stdout = &stdOut + cmd.Stderr = &stdErr + err := cmd.Run() + require.NoError(t, err, stdErr.String(), stdOut.String()) + require.Empty(t, stdErr.String()) + + require.NotContains(t, stdOut.String(), "MethodNotAllowed") + require.Contains(t, stdOut.String(), dotSuffix) + }) + + t.Run("category, table params", func(t *testing.T) { + var stdOut, stdErr bytes.Buffer + + cmd := exec.Command("kubectl", "get", "pinniped", "-A", "-o", "wide", "-v", "10") + cmd.Stdout = &stdOut + cmd.Stderr = &stdErr + err := cmd.Run() + require.NoError(t, err, stdErr.String(), stdOut.String()) + + require.NotContains(t, stdOut.String(), "MethodNotAllowed") + require.Contains(t, stdOut.String(), dotSuffix) + + require.Contains(t, stdErr.String(), `"kind":"Table"`) + require.Contains(t, stdErr.String(), `"resourceVersion":"0"`) + }) + + t.Run("list, no special params", func(t *testing.T) { + var stdOut, stdErr bytes.Buffer + + //nolint: gosec // input is part of test env + cmd := exec.Command("kubectl", "get", "tokencredentialrequests.login.concierge"+dotSuffix, "-A") + cmd.Stdout = &stdOut + cmd.Stderr = &stdErr + err := cmd.Run() + require.NoError(t, err, stdErr.String(), stdOut.String()) + require.Empty(t, stdOut.String()) + + require.NotContains(t, stdErr.String(), "MethodNotAllowed") + require.Contains(t, stdErr.String(), `No resources found`) + }) + + t.Run("list, table params", func(t *testing.T) { + var stdOut, stdErr bytes.Buffer + + //nolint: gosec // input is part of test env + cmd := exec.Command("kubectl", "get", "tokencredentialrequests.login.concierge"+dotSuffix, "-A", "-o", "wide", "-v", "10") + cmd.Stdout = &stdOut + cmd.Stderr = &stdErr + err := cmd.Run() + require.NoError(t, err, stdErr.String(), stdOut.String()) + require.Empty(t, stdOut.String()) + + require.NotContains(t, stdErr.String(), "MethodNotAllowed") + require.Contains(t, stdErr.String(), `"kind":"Table"`) + require.Contains(t, stdErr.String(), `"resourceVersion":"0"`) + }) + + t.Run("raw request to see body", func(t *testing.T) { + var stdOut, stdErr bytes.Buffer + + //nolint: gosec // input is part of test env + cmd := exec.Command("kubectl", "get", "--raw", "/apis/login.concierge"+dotSuffix+"/v1alpha1/tokencredentialrequests") + cmd.Stdout = &stdOut + cmd.Stderr = &stdErr + err := cmd.Run() + require.NoError(t, err, stdErr.String(), stdOut.String()) + require.Empty(t, stdErr.String()) + + require.NotContains(t, stdOut.String(), "MethodNotAllowed") + require.Contains(t, stdOut.String(), `{"kind":"TokenCredentialRequestList","apiVersion":"login.concierge`+ + dotSuffix+`/v1alpha1","metadata":{"resourceVersion":"0"},"items":[]}`) + }) +} diff --git a/test/integration/kube_api_discovery_test.go b/test/integration/kube_api_discovery_test.go index 4ab3bfaa..dba382bc 100644 --- a/test/integration/kube_api_discovery_test.go +++ b/test/integration/kube_api_discovery_test.go @@ -72,7 +72,7 @@ func TestGetAPIResourceList(t *testing.T) { { Name: "tokencredentialrequests", Kind: "TokenCredentialRequest", - Verbs: []string{"create"}, + Verbs: []string{"create", "list"}, Namespaced: true, Categories: []string{"pinniped"}, },