From 488296a4804d39d3df4f153a0be5b250adcef518 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 13 Sep 2022 15:43:16 -0400 Subject: [PATCH] adding tests Co-authored-by: Ryan Richard Co-authored-by: Benjamin A. Petersen --- internal/crud/crud.go | 9 +- internal/crud/crud_test.go | 249 ++++++++++- internal/registry/clientsecretrequest/rest.go | 57 +-- .../registry/clientsecretrequest/rest_test.go | 404 ++++++++++++++++++ internal/supervisor/apiserver/apiserver.go | 2 +- 5 files changed, 657 insertions(+), 64 deletions(-) create mode 100644 internal/registry/clientsecretrequest/rest_test.go diff --git a/internal/crud/crud.go b/internal/crud/crud.go index e59b4652..e3212fd5 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -14,10 +14,8 @@ import ( "time" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/runtime/schema" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "go.pinniped.dev/internal/constable" @@ -95,6 +93,8 @@ func (s *secretsStorage) Get(ctx context.Context, signature string, data JSON) ( return secret.ResourceVersion, nil } +// Update takes a resourceVersion because it assumes Get has been recently called to obtain the latest resource version. +// This is to ensure that concurrent edits are treated as conflict errors (only one will win). func (s *secretsStorage) Update(ctx context.Context, signature, resourceVersion string, data JSON) (string, error) { secret, err := s.toSecret(signature, resourceVersion, data, nil, nil) if err != nil { @@ -105,11 +105,6 @@ func (s *secretsStorage) Update(ctx context.Context, signature, resourceVersion if err != nil { return "", fmt.Errorf("failed to get %s for signature %s: %w", s.resource, signature, err) } - // do not assume that our secret client does live reads - if oldSecret.ResourceVersion != resourceVersion { - return "", errors.NewConflict(schema.GroupResource{Resource: "Secret"}, secret.Name, - fmt.Errorf("resource version %s does not match expected value: %s", oldSecret.ResourceVersion, resourceVersion)) - } // preserve these fields - they are effectively immutable on update secret.Labels = oldSecret.Labels diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 33817c44..286e7280 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -13,6 +13,7 @@ import ( "github.com/ory/fosite/compose" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -64,6 +65,7 @@ func TestStorage(t *testing.T) { mocks func(*testing.T, mocker) lifetime func() time.Duration run func(*testing.T, Storage, *clocktesting.FakeClock) error + useNilClock bool wantActions []coretesting.Action wantSecrets []corev1.Secret wantErr string @@ -270,69 +272,129 @@ func TestStorage(t *testing.T) { wantErr: "", }, { - name: "create and get with additional labels", - resource: "access-tokens", - mocks: nil, + name: "create and get and update with additional labels, annotations, and ownerRefs", + resource: "kittens", + mocks: func(t *testing.T, mock mocker) { + mock.PrependReactor("create", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + secret := action.(coretesting.UpdateAction).GetObject().(*corev1.Secret) + secret.ResourceVersion = "1" + return false, nil, nil + }) + + mock.PrependReactor("update", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + secret := action.(coretesting.UpdateAction).GetObject().(*corev1.Secret) + secret.ResourceVersion = "45" + return false, nil, nil + }) + }, run: func(t *testing.T, storage Storage, fakeClock *clocktesting.FakeClock) error { signature := hmac.AuthorizeCodeSignature(authorizationCode1) require.NotEmpty(t, signature) require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is data := &testJSON{Data: "create-and-get"} - rv1, err := storage.Create(ctx, signature, data, map[string]string{"label1": "value1", "label2": "value2"}, nil) - require.Empty(t, rv1) // fake client does not set this + rv1, err := storage.Create(ctx, signature, data, map[string]string{"label1": "value1", "label2": "value2"}, []metav1.OwnerReference{{ + APIVersion: "some-api-version", + Kind: "some-kind", + Name: "some-owner", + UID: "123", + }}) + require.Equal(t, "1", rv1) require.NoError(t, err) out := &testJSON{} rv2, err := storage.Get(ctx, signature, out) - require.Empty(t, rv2) // fake client does not set this + require.Equal(t, "1", rv2) require.NoError(t, err) require.Equal(t, data, out) + newData := &testJSON{Data: "performed-an-update"} + rv3, err := storage.Update(ctx, signature, rv2, newData) + require.Equal(t, "45", rv3) + require.NoError(t, err) + return nil }, wantActions: []coretesting.Action{ coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + Name: "pinniped-storage-kittens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev/type": "access-tokens", + "storage.pinniped.dev/type": "kittens", "label1": "value1", "label2": "value2", }, Annotations: map[string]string{ "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "some-api-version", + Kind: "some-kind", + Name: "some-owner", + UID: "123", + }}, }, Data: map[string][]byte{ "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), "pinniped-storage-version": []byte("1"), }, - Type: "storage.pinniped.dev/access-tokens", + Type: "storage.pinniped.dev/kittens", + }), + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-kittens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq"), + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-kittens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq"), + coretesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-kittens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + ResourceVersion: "1", + Labels: map[string]string{ + "storage.pinniped.dev/type": "kittens", + "label1": "value1", + "label2": "value2", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "some-api-version", + Kind: "some-kind", + Name: "some-owner", + UID: "123", + }}, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"performed-an-update"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/kittens", }), - coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq"), }, wantSecrets: []corev1.Secret{ { ObjectMeta: metav1.ObjectMeta{ - Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + Name: "pinniped-storage-kittens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", Namespace: namespace, - ResourceVersion: "", + ResourceVersion: "45", Labels: map[string]string{ - "storage.pinniped.dev/type": "access-tokens", + "storage.pinniped.dev/type": "kittens", "label1": "value1", "label2": "value2", }, Annotations: map[string]string{ "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "some-api-version", + Kind: "some-kind", + Name: "some-owner", + UID: "123", + }}, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), + "pinniped-storage-data": []byte(`{"Data":"performed-an-update"}`), "pinniped-storage-version": []byte("1"), }, - Type: "storage.pinniped.dev/access-tokens", + Type: "storage.pinniped.dev/kittens", }, }, wantErr: "", @@ -499,6 +561,92 @@ func TestStorage(t *testing.T) { }, wantErr: "", }, + { + name: "update failed, correctly wrap kubernetes conflict error", + resource: "stores", + mocks: func(t *testing.T, mock mocker) { + err := mock.Tracker().Add(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba", + Namespace: namespace, + ResourceVersion: "35", + Labels: map[string]string{ + "storage.pinniped.dev/type": "stores", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"pants"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/stores", + }) + require.NoError(t, err) + + mock.PrependReactor("update", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, apierrors.NewConflict(schema.GroupResource{ + Group: corev1.GroupName, + Resource: "secrets", + }, "v1.", fmt.Errorf("there was a conflict")) + }) + }, + run: func(t *testing.T, storage Storage, fakeClock *clocktesting.FakeClock) error { + signature := hmac.AuthorizeCodeSignature(authorizationCode3) + require.NotEmpty(t, signature) + require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is + + newData := &testJSON{Data: "shirts"} + rv2, err := storage.Update(ctx, signature, "35", newData) + require.Empty(t, rv2) + require.EqualError(t, err, "failed to update stores for signature 5aUhdNmfWLW3yKX8Zfz5ztS5IiiWBgu36Gja-o2xl0I at resource version 35: Operation cannot be fulfilled on secrets \"v1.\": there was a conflict") + require.True(t, apierrors.IsConflict(err)) + + return nil + }, + wantActions: []coretesting.Action{ + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba"), + coretesting.NewUpdateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba", + ResourceVersion: "35", // update at initial RV + Labels: map[string]string{ + "storage.pinniped.dev/type": "stores", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"shirts"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/stores", + }), + }, + wantSecrets: []corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba", + Namespace: namespace, + ResourceVersion: "35", + Labels: map[string]string{ + "storage.pinniped.dev/type": "stores", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"pants"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/stores", + }, + }, + wantErr: "", + }, { name: "delete existing", resource: "seals", @@ -1077,6 +1225,68 @@ func TestStorage(t *testing.T) { }, wantErr: "", }, + { + name: "create and get with infinite lifetime when lifetime is specified as zero and clock is specified as nil", + resource: "access-tokens", + useNilClock: true, + mocks: nil, + lifetime: func() time.Duration { return 0 }, // 0 == infinity + run: func(t *testing.T, storage Storage, fakeClock *clocktesting.FakeClock) error { + signature := hmac.AuthorizeCodeSignature(authorizationCode1) + require.NotEmpty(t, signature) + require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is + + data := &testJSON{Data: "create-and-get"} + rv1, err := storage.Create(ctx, signature, data, nil, nil) + require.Empty(t, rv1) // fake client does not set this + require.NoError(t, err) + + out := &testJSON{} + rv2, err := storage.Get(ctx, signature, out) + require.Empty(t, rv2) // fake client does not set this + require.NoError(t, err) + require.Equal(t, data, out) + + return nil + }, + wantActions: []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + ResourceVersion: "", + // No garbage collection annotation was added. + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }), + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq"), + }, + wantSecrets: []corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + Namespace: namespace, + ResourceVersion: "", + // No garbage collection annotation was added. + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-tokens", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }, + }, + wantErr: "", + }, } for _, tt := range tests { @@ -1093,8 +1303,15 @@ func TestStorage(t *testing.T) { useLifetime = tt.lifetime() } secrets := client.CoreV1().Secrets(namespace) + fakeClock := clocktesting.NewFakeClock(fakeNow) - storage := New(tt.resource, secrets, fakeClock.Now, useLifetime) + clock := fakeClock.Now + if tt.useNilClock { + fakeClock = nil + clock = nil + } + + storage := New(tt.resource, secrets, clock, useLifetime) err := tt.run(t, storage, fakeClock) diff --git a/internal/registry/clientsecretrequest/rest.go b/internal/registry/clientsecretrequest/rest.go index 92236300..1df7f045 100644 --- a/internal/registry/clientsecretrequest/rest.go +++ b/internal/registry/clientsecretrequest/rest.go @@ -6,15 +6,12 @@ package clientsecretrequest import ( "context" - "crypto/rand" "encoding/hex" "fmt" "io" "strings" "golang.org/x/crypto/bcrypt" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apiextensions-apiserver/pkg/registry/customresource/tableconvertor" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" genericvalidation "k8s.io/apimachinery/pkg/api/validation" @@ -34,48 +31,27 @@ import ( "go.pinniped.dev/internal/oidcclientsecretstorage" ) -// cost is a good bcrypt cost for 2022, should take about 250 ms to validate. +// Cost is a good bcrypt cost for 2022, should take about 250 ms to validate. // This value is expected to be increased over time to match CPU improvements. -const cost = 12 +const Cost = 12 -//nolint:gochecknoglobals -var tableConvertor = func() rest.TableConvertor { - // sadly this is not useful at the moment because `kubectl create` does not support table output - columns := []apiextensionsv1.CustomResourceColumnDefinition{ - { - Name: "Secret", - Type: "string", - Description: "", // TODO generate SwaggerDoc() method to fill this field - JSONPath: ".status.generatedSecret", - }, - { - Name: "Total", - Type: "integer", - Description: "", // TODO generate SwaggerDoc() method to fill this field - JSONPath: ".status.totalClientSecrets", - }, - } - tc, err := tableconvertor.New(columns) // just re-use the CRD table code so we do not have to implement the interface ourselves - if err != nil { - panic(err) // inputs are static so this should never happen - } - return tc -}() - -func NewREST(secrets corev1client.SecretInterface, clients configv1alpha1clientset.OIDCClientInterface, namespace string) *REST { +func NewREST(resource schema.GroupResource, secrets corev1client.SecretInterface, clients configv1alpha1clientset.OIDCClientInterface, namespace string, cost int) *REST { return &REST{ - secretStorage: oidcclientsecretstorage.New(secrets), - clients: clients, - namespace: namespace, - rand: rand.Reader, + secretStorage: oidcclientsecretstorage.New(secrets), + clients: clients, + namespace: namespace, + cost: cost, + tableConvertor: rest.NewDefaultTableConvertor(resource), } } type REST struct { - secretStorage *oidcclientsecretstorage.OIDCClientSecretStorage - clients configv1alpha1clientset.OIDCClientInterface - namespace string - rand io.Reader + secretStorage *oidcclientsecretstorage.OIDCClientSecretStorage + clients configv1alpha1clientset.OIDCClientInterface + namespace string + rand io.Reader + cost int + tableConvertor rest.TableConvertor } // Assert that our *REST implements all the optional interfaces that we expect it to implement. @@ -111,7 +87,7 @@ func (*REST) List(_ context.Context, _ *metainternalversion.ListOptions) (runtim } func (r *REST) ConvertToTable(ctx context.Context, obj runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { - return tableConvertor.ConvertToTable(ctx, obj, tableOptions) + return r.tableConvertor.ConvertToTable(ctx, obj, tableOptions) } func (*REST) NamespaceScoped() bool { @@ -130,6 +106,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation defer t.Log() // Validate the create request before honoring it. + // This function is provided from kube kube-api server calling validating admission webhooks if there are any registered. req, err := r.validateRequest(ctx, obj, createValidation, options, t) if err != nil { return nil, err @@ -168,7 +145,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation } t.Step("generateSecret") - hash, err := bcrypt.GenerateFromPassword([]byte(secret), cost) + hash, err := bcrypt.GenerateFromPassword([]byte(secret), r.cost) if err != nil { traceFailureWithError(t, "bcrypt.GenerateFromPassword", err) return nil, apierrors.NewInternalError(fmt.Errorf("hash generation failed")) diff --git a/internal/registry/clientsecretrequest/rest_test.go b/internal/registry/clientsecretrequest/rest_test.go new file mode 100644 index 00000000..be0f87c5 --- /dev/null +++ b/internal/registry/clientsecretrequest/rest_test.go @@ -0,0 +1,404 @@ +// NOTES: +// Take a look at the unit tests from the sibling files as we already have some there. +// whoamirequest is super simple, zero side effects... +// - add bits similar to what we did for crud.go to seed kube with secrets & prove your updates +// - perhaps look at oidcclientsecretstorage.go also for the seeding code we wrote +// - the things we are seeding here IS the storage of client secrets + hashes +// - this is the specific secret formats that we need +// - but instead of hello world hashes we will want to put real BCrypt hashes in the seeds +// - there is a test helper with some real BCrypt hashes we can use in oidcclient.go -- HashedPassword1AtGoMinCost - 04 for unit tests for speed +// - look for a test using HashedPassword1AtGoMinCost to see how we step around the min value of 11 tho production code must do that +// - NewKubeStorage, bcrypt.MinCost will show this +// // Use lower minimum required bcrypt cost than we would use in production to keep unit the tests fast. +// oauthStore := oidc.NewKubeStorage(secrets, oidcClientsClient, timeoutsConfiguration, bcrypt.MinCost) +// credentialrequest also has no side effects... +// +// +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +package clientsecretrequest + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + 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" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/registry/rest" + + clientsecretapi "go.pinniped.dev/generated/latest/apis/supervisor/clientsecret" +) + +func TestNew(t *testing.T) { + + r := NewREST(schema.GroupResource{Group: "bears", Resource: "panda"}, nil, nil, "foobar", 4) + + require.NotNil(t, r) + require.True(t, r.NamespaceScoped()) + require.Equal(t, []string{"pinniped"}, r.Categories()) + + require.IsType(t, &clientsecretapi.OIDCClientSecretRequest{}, r.New()) + require.IsType(t, &clientsecretapi.OIDCClientSecretRequestList{}, 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, &clientsecretapi.OIDCClientSecretRequestList{}, list) + require.Equal(t, "0", list.(*clientsecretapi.OIDCClientSecretRequestList).ResourceVersion) + require.NotNil(t, list.(*clientsecretapi.OIDCClientSecretRequestList).Items) + require.Len(t, list.(*clientsecretapi.OIDCClientSecretRequestList).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) { + type args struct { + ctx context.Context + obj runtime.Object + createValidation rest.ValidateObjectFunc + options *metav1.CreateOptions + } + namespace := "some-namespace" + + tests := []struct { + name string + args args + want runtime.Object + wantErrStatus metav1.Status + }{ + { + name: "wrong type", + args: args{ + ctx: genericapirequest.NewContext(), + obj: &metav1.Status{}, + createValidation: nil, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: `not an OIDCClientSecretRequest: &v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"", Message:"", Reason:"", Details:(*v1.StatusDetails)(nil), Code:0}`, + Reason: metav1.StatusReasonBadRequest, + Code: http.StatusBadRequest, + }, + }, + { + name: "bad options for dry run", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client-name", + }, + }, + createValidation: nil, + options: &metav1.CreateOptions{DryRun: []string{"stuff"}}, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-some-client-name" is invalid: dryRun: Unsupported value: []string{"stuff"}`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "OIDCClientSecretRequest", + Name: "client.oauth.pinniped.dev-some-client-name", + Causes: []metav1.StatusCause{{ + Type: "FieldValueNotSupported", + Message: "Unsupported value: []string{\"stuff\"}", + Field: "dryRun", + }}, + }, + }, + }, + { + name: "incorrect namespace", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), "wrong-namespace"), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client-name", + }, + }, + createValidation: nil, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: `namespace must be some-namespace on OIDCClientSecretRequest, was wrong-namespace`, + Reason: metav1.StatusReasonBadRequest, + Code: http.StatusBadRequest, + }, + }, + { + name: "create validation: failure from kube api-server rest.ValidateObjectFunc", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client-name", + }, + }, + createValidation: func(ctx context.Context, obj runtime.Object) error { + return apierrors.NewInternalError(errors.New("some-error-here")) + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: "Internal error occurred: some-error-here", + Reason: metav1.StatusReasonInternalError, + Code: http.StatusInternalServerError, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Message: "some-error-here", + }}, + }, + }, + }, + { + name: "create validation: no namespace on the request context", + args: args{ + ctx: genericapirequest.NewContext(), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client-name", + }, + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: "Internal error occurred: no namespace information found in request context", + Reason: metav1.StatusReasonInternalError, + Code: http.StatusInternalServerError, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{{ + Message: "no namespace information found in request context", + }}, + }, + }, + }, + { + name: "create validation: namespace on object does not match namespace on request", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-some-client-name", + Namespace: "not-a-matching-namespace", + }, + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: "the namespace of the provided object does not match the namespace sent on the request", + Reason: metav1.StatusReasonBadRequest, + Code: http.StatusBadRequest, + }, + }, + { + name: "create validation: generateName is unsupported", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "foo", + }, + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: "OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev \"\" is invalid: [metadata.generateName: Invalid value: \"foo\": generateName is not supported, metadata.name: Required value: name or generateName is required]", + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "OIDCClientSecretRequest", + Name: "", + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: "Invalid value: \"foo\": generateName is not supported", + Field: "metadata.generateName", + }, { + Type: metav1.CauseTypeFieldValueRequired, + Message: "Required value: name or generateName is required", + Field: "metadata.name", + }}, + }, + }, + }, + { + name: "create validation: name cannot exactly match client.oauth.pinniped.dev-", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-", + }, + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: "OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev \"client.oauth.pinniped.dev-\" is invalid: metadata.name: Invalid value: \"client.oauth.pinniped.dev-\": must not equal 'client.oauth.pinniped.dev-'", + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "OIDCClientSecretRequest", + Name: "client.oauth.pinniped.dev-", + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: "Invalid value: \"client.oauth.pinniped.dev-\": must not equal 'client.oauth.pinniped.dev-'", + Field: "metadata.name", + }}, + }, + }, + }, + { + name: "create validation: name must contain prefix client.oauth.pinniped.dev-", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-contain-the-prefix", + }, + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: "OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev \"does-not-contain-the-prefix\" is invalid: metadata.name: Invalid value: \"does-not-contain-the-prefix\": must start with 'client.oauth.pinniped.dev-'", + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "OIDCClientSecretRequest", + Name: "does-not-contain-the-prefix", + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: "Invalid value: \"does-not-contain-the-prefix\": must start with 'client.oauth.pinniped.dev-'", + Field: "metadata.name", + }}, + }, + }, + }, + { + name: "create validation: name with invalid characters should error", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client.oauth.pinniped.dev-contains/invalid/characters", + }, + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "client.oauth.pinniped.dev-contains/invalid/characters" is invalid: metadata.name: Invalid value: "client.oauth.pinniped.dev-contains/invalid/characters": may not contain '/'`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "OIDCClientSecretRequest", + Name: "client.oauth.pinniped.dev-contains/invalid/characters", + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: `Invalid value: "client.oauth.pinniped.dev-contains/invalid/characters": may not contain '/'`, + Field: "metadata.name", + }}, + }, + }, + }, + { + name: "create validation: name validation may return multiple errors", + args: args{ + ctx: genericapirequest.WithNamespace(genericapirequest.NewContext(), namespace), + obj: &clientsecretapi.OIDCClientSecretRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "multiple/errors/aggregated", + GenerateName: "no-generate-allowed", + }, + }, + options: nil, + }, + want: nil, + wantErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Message: `OIDCClientSecretRequest.clientsecret.supervisor.pinniped.dev "multiple/errors/aggregated" is invalid: [metadata.generateName: Invalid value: "no-generate-allowed": generateName is not supported, metadata.name: Invalid value: "multiple/errors/aggregated": must start with 'client.oauth.pinniped.dev-', metadata.name: Invalid value: "multiple/errors/aggregated": may not contain '/']`, + Reason: metav1.StatusReasonInvalid, + Code: http.StatusUnprocessableEntity, + Details: &metav1.StatusDetails{ + Group: "clientsecret.supervisor.pinniped.dev", + Kind: "OIDCClientSecretRequest", + Name: "multiple/errors/aggregated", + Causes: []metav1.StatusCause{{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: `Invalid value: "no-generate-allowed": generateName is not supported`, + Field: "metadata.generateName", + }, { + Type: metav1.CauseTypeFieldValueInvalid, + Message: `Invalid value: "multiple/errors/aggregated": must start with 'client.oauth.pinniped.dev-'`, + Field: "metadata.name", + }, { + Type: metav1.CauseTypeFieldValueInvalid, + Message: `Invalid value: "multiple/errors/aggregated": may not contain '/'`, + Field: "metadata.name", + }}, + }, + }, + }, + // {name: ""}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + // TODO: update and fill these in with actual values, not nil! + r := NewREST(schema.GroupResource{Group: "bears", Resource: "panda"}, nil, nil, namespace, 4) + got, err := r.Create(tt.args.ctx, tt.args.obj, tt.args.createValidation, tt.args.options) + require.Equal(t, &apierrors.StatusError{ + ErrStatus: tt.wantErrStatus, + }, err) + require.Equal(t, tt.want, got) + }) + } +} + +func errString(err error) string { + if err == nil { + return "" + } + + return err.Error() +} diff --git a/internal/supervisor/apiserver/apiserver.go b/internal/supervisor/apiserver/apiserver.go index 9f0cbc52..a6deec6d 100644 --- a/internal/supervisor/apiserver/apiserver.go +++ b/internal/supervisor/apiserver/apiserver.go @@ -80,7 +80,7 @@ func (c completedConfig) New() (*PinnipedServer, error) { for _, f := range []func() (schema.GroupVersionResource, rest.Storage){ func() (schema.GroupVersionResource, rest.Storage) { clientSecretReqGVR := c.ExtraConfig.ClientSecretSupervisorGroupVersion.WithResource("oidcclientsecretrequests") - clientSecretReqStorage := clientsecretrequest.NewREST(c.ExtraConfig.Secrets, c.ExtraConfig.OIDCClients, c.ExtraConfig.Namespace) + clientSecretReqStorage := clientsecretrequest.NewREST(clientSecretReqGVR.GroupResource(), c.ExtraConfig.Secrets, c.ExtraConfig.OIDCClients, c.ExtraConfig.Namespace, clientsecretrequest.Cost) return clientSecretReqGVR, clientSecretReqStorage }, } {