Skip to content

Commit

Permalink
Add a rand parameter to the storage service
Browse files Browse the repository at this point in the history
This modification enables the use of a custom random number generator in our
tests. It eliminates the need to alter global state in the
`github.com/google/uuid` package, which is unsafe during parallel testing.

We continue to utilize `crypto/rand`, but now it must be explicitly passed to
the service. We maintain the assumption that the operation will succeed, and
will trigger a panic otherwise, to mirror the behavior of `uuid.New`. We have
implemented middleware to manage panics, thus mitigating concerns in the
unlikely event of such failures.
  • Loading branch information
sevein committed Nov 11, 2023
1 parent fa639ab commit 90cdc51
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 12 deletions.
17 changes: 15 additions & 2 deletions internal/storage/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package storage

import (
"context"
"crypto/rand"
"errors"
"fmt"
"io"
"net/http"
"time"

Expand Down Expand Up @@ -50,7 +52,11 @@ type serviceImpl struct {
// Persistence client.
storagePersistence persistence.Storage

// OAuth 2.0 token verifier.
tokenVerifier auth.TokenVerifier

// Random number generator
rander io.Reader
}

var _ Service = (*serviceImpl)(nil)
Expand All @@ -63,13 +69,19 @@ func NewService(
storagePersistence persistence.Storage,
tc temporalsdk_client.Client,
tokenVerifier auth.TokenVerifier,
rander io.Reader,
) (s *serviceImpl, err error) {
s = &serviceImpl{
logger: logger,
tc: tc,
config: config,
storagePersistence: storagePersistence,
tokenVerifier: tokenVerifier,
rander: rander,
}

if s.rander == nil {
s.rander = rand.Reader
}

l, err := NewInternalLocation(&config.Internal)
Expand Down Expand Up @@ -126,7 +138,8 @@ func (s *serviceImpl) Submit(ctx context.Context, payload *goastorage.SubmitPayl
return nil, goastorage.MakeNotAvailable(errors.New("cannot perform operation"))
}

objectKey := uuid.New()
objectKey := uuid.Must(uuid.NewRandomFromReader(s.rander))

_, err = s.storagePersistence.CreatePackage(ctx, &goastorage.Package{
Name: payload.Name,
AipID: aipID,
Expand Down Expand Up @@ -332,7 +345,7 @@ func (s *serviceImpl) PackageReader(ctx context.Context, pkg *goastorage.Package
func (s *serviceImpl) AddLocation(ctx context.Context, payload *goastorage.AddLocationPayload) (res *goastorage.AddLocationResult, err error) {
source := types.NewLocationSource(payload.Source)
purpose := types.NewLocationPurpose(payload.Purpose)
UUID := uuid.New()
UUID := uuid.Must(uuid.NewRandomFromReader(s.rander))

var config types.LocationConfig
switch c := payload.Config.(type) {
Expand Down
11 changes: 2 additions & 9 deletions internal/storage/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func setUpService(t *testing.T, attrs *setUpAttrs) storage.Service {
*params.persistence,
*params.temporalClient,
params.tokenVerifier,
staticRand{},
)
assert.NilError(t, err)

Expand Down Expand Up @@ -128,6 +129,7 @@ func TestNewService(t *testing.T) {
nil,
nil,
&auth.OIDCTokenVerifier{},
nil,
)

assert.ErrorContains(t, err, "invalid configuration")
Expand Down Expand Up @@ -1434,9 +1436,6 @@ func TestServiceAddLocation(t *testing.T) {
})

t.Run("Returns not_valid if cannot persist location", func(t *testing.T) {
t.Cleanup(func() { uuid.SetRand(nil) })

uuid.SetRand(staticRand{})
locationID := uuid.MustParse("00010203-0405-4607-8809-0a0b0c0d0e0f")
attrs := &setUpAttrs{}
svc := setUpService(t, attrs)
Expand Down Expand Up @@ -1480,9 +1479,6 @@ func TestServiceAddLocation(t *testing.T) {
})

t.Run("Returns result with location UUID", func(t *testing.T) {
t.Cleanup(func() { uuid.SetRand(nil) })

uuid.SetRand(staticRand{})
locationID := uuid.MustParse("00010203-0405-4607-8809-0a0b0c0d0e0f")
attrs := &setUpAttrs{}
svc := setUpService(t, attrs)
Expand Down Expand Up @@ -1525,9 +1521,6 @@ func TestServiceAddLocation(t *testing.T) {
})

t.Run("Returns location with URL config", func(t *testing.T) {
t.Cleanup(func() { uuid.SetRand(nil) })

uuid.SetRand(staticRand{})
locationID := uuid.MustParse("00010203-0405-4607-8809-0a0b0c0d0e0f")
attrs := &setUpAttrs{}
svc := setUpService(t, attrs)
Expand Down
2 changes: 1 addition & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func main() {
// Set up the storage service.
var storagesvc storage.Service
{
storagesvc, err = storage.NewService(logger.WithName("storage"), cfg.Storage, storagePersistence, temporalClient, tokenVerifier)
storagesvc, err = storage.NewService(logger.WithName("storage"), cfg.Storage, storagePersistence, temporalClient, tokenVerifier, rand.Reader)
if err != nil {
logger.Error(err, "Error setting up storage service.")
os.Exit(1)
Expand Down

0 comments on commit 90cdc51

Please sign in to comment.