From ad0c83d2d8a4460df8cfa85329be213a581d2138 Mon Sep 17 00:00:00 2001 From: Cristian Cepeda <43882+pastuxso@users.noreply.github.com> Date: Tue, 31 Oct 2023 11:21:32 -0500 Subject: [PATCH 1/5] switches from xid to ulid --- go.mod | 2 +- go.sum | 5 +- internal/runner/command.go | 6 +-- internal/runner/service.go | 4 +- internal/runner/session.go | 4 +- internal/runner/session_test.go | 26 +++++++++- internal/runner/shell_session.go | 4 +- internal/utils/id.go | 60 ++++++++++++++++++++++ internal/utils/id_test.go | 85 ++++++++++++++++++++++++++++++++ 9 files changed, 182 insertions(+), 14 deletions(-) create mode 100644 internal/utils/id.go create mode 100644 internal/utils/id_test.go diff --git a/go.mod b/go.mod index e20281eaa..db6549f82 100644 --- a/go.mod +++ b/go.mod @@ -23,9 +23,9 @@ require ( github.com/mattn/go-isatty v0.0.20 github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d github.com/muesli/cancelreader v0.2.2 + github.com/oklog/ulid/v2 v2.1.0 github.com/rogpeppe/go-internal v1.11.0 github.com/rs/cors v1.10.1 - github.com/rs/xid v1.5.0 github.com/rwtodd/Go.Sed v0.0.0-20230610052213-ba3e9c186f0a github.com/vektah/gqlparser/v2 v2.5.10 github.com/yuin/goldmark v1.5.6 diff --git a/go.sum b/go.sum index 44bf88953..955c3e444 100644 --- a/go.sum +++ b/go.sum @@ -153,8 +153,11 @@ github.com/muesli/reflow v0.3.0 h1:IFsN6K9NfGtjeggFP+68I4chLZV2yIKsXJFNZ+eWh6s= github.com/muesli/reflow v0.3.0/go.mod h1:pbwTDkVPibjO2kyvBQRBxTWEEGDGq0FlB1BIKtnHY/8= github.com/muesli/termenv v0.15.2 h1:GohcuySI0QmI3wN8Ok9PtKGkgkFIk7y6Vpb5PvrY+Wo= github.com/muesli/termenv v0.15.2/go.mod h1:Epx+iuz8sNs7mNKhxzH4fWXGNpZwUaJKRS1noLXviQ8= +github.com/oklog/ulid/v2 v2.1.0 h1:+9lhoxAP56we25tyYETBBY1YLA2SaoLvUFgrP2miPJU= +github.com/oklog/ulid/v2 v2.1.0/go.mod h1:rcEKHmBBKfef9DhnvX7y1HZBYxjXb0cP5ExxNsTT1QQ= github.com/onsi/gomega v1.27.10 h1:naR28SdDFlqrG6kScpT8VWpu1xWY5nJRCF3XaYyBjhI= github.com/onsi/gomega v1.27.10/go.mod h1:RsS8tutOdbdgzbPtzzATp12yT7kM5I5aElG3evPbQ0M= +github.com/pborman/getopt v0.0.0-20170112200414-7148bc3a4c30/go.mod h1:85jBQOZwpVEaDAr341tbn15RS4fCAsIst0qp7i8ex1o= github.com/pelletier/go-toml/v2 v2.1.0 h1:FnwAJ4oYMvbT/34k9zzHuZNrhlz48GB3/s6at6/MHO4= github.com/pelletier/go-toml/v2 v2.1.0/go.mod h1:tJU2Z3ZkXwnxa4DPO899bsyIoywizdUvyaeZurnPPDc= github.com/pjbgf/sha1cd v0.3.0 h1:4D5XXmUUBUl/xQ6IjCkEAbqXskkq/4O7LmGn0AqMDs4= @@ -175,8 +178,6 @@ github.com/rogpeppe/go-internal v1.11.0 h1:cWPaGQEPrBb5/AsnsZesgZZ9yb1OQ+GOISoDN github.com/rogpeppe/go-internal v1.11.0/go.mod h1:ddIwULY96R17DhadqLgMfk9H9tvdUzkipdSkR5nkCZA= github.com/rs/cors v1.10.1 h1:L0uuZVXIKlI1SShY2nhFfo44TYvDPQ1w4oFkUJNfhyo= github.com/rs/cors v1.10.1/go.mod h1:XyqrcTp5zjWr1wsJ8PIRZssZ8b/WMcMf71DJnit4EMU= -github.com/rs/xid v1.5.0 h1:mKX4bl4iPYJtEIxp6CYiUuLQ/8DYMoz0PUdtGgMFRVc= -github.com/rs/xid v1.5.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/rwtodd/Go.Sed v0.0.0-20230610052213-ba3e9c186f0a h1:URwYffGNuBQkfwkcn+1CZhb8IE/mKSXxPXp/zzQsn80= github.com/rwtodd/Go.Sed v0.0.0-20230610052213-ba3e9c186f0a/go.mod h1:c6qgHcSUeSISur4+Kcf3WYTvpL07S8eAsoP40hDiQ1I= diff --git a/internal/runner/command.go b/internal/runner/command.go index 765c84727..a2bf44761 100644 --- a/internal/runner/command.go +++ b/internal/runner/command.go @@ -15,7 +15,7 @@ import ( "github.com/creack/pty" "github.com/pkg/errors" - "github.com/rs/xid" + "github.com/stateful/runme/internal/utils" "go.uber.org/multierr" "go.uber.org/zap" ) @@ -198,8 +198,8 @@ func newCommand(cfg *commandConfig) (*command, error) { extraArgs = append(extraArgs, "-c", script) case CommandModeTempFile: for { - id := xid.New() - tempScriptFile = filepath.Join(cfg.Directory, fmt.Sprintf(".runme-script-%s", id.String())) + id := utils.GenerateID() + tempScriptFile = filepath.Join(cfg.Directory, fmt.Sprintf(".runme-script-%s", id)) if fileExtension != "" { tempScriptFile += "." + fileExtension diff --git a/internal/runner/service.go b/internal/runner/service.go index 16ddcd3f1..8be36fecf 100644 --- a/internal/runner/service.go +++ b/internal/runner/service.go @@ -9,10 +9,10 @@ import ( "github.com/creack/pty" "github.com/pkg/errors" - "github.com/rs/xid" "github.com/stateful/runme/internal/env" runnerv1 "github.com/stateful/runme/internal/gen/proto/go/runme/runner/v1" "github.com/stateful/runme/internal/rbuffer" + "github.com/stateful/runme/internal/utils" "github.com/stateful/runme/pkg/project" "go.uber.org/zap" "golang.org/x/sync/errgroup" @@ -155,7 +155,7 @@ func ConvertRunnerProject(runnerProj *runnerv1.Project) (project.Project, error) } func (r *runnerService) Execute(srv runnerv1.RunnerService_ExecuteServer) error { - logger := r.logger.With(zap.String("_id", xid.New().String())) + logger := r.logger.With(zap.String("_id", utils.GenerateID())) logger.Info("running Execute in runnerService") diff --git a/internal/runner/session.go b/internal/runner/session.go index d553bf621..0fa44766c 100644 --- a/internal/runner/session.go +++ b/internal/runner/session.go @@ -5,7 +5,7 @@ import ( "sync" lru "github.com/hashicorp/golang-lru/v2" - "github.com/rs/xid" + "github.com/stateful/runme/internal/utils" "github.com/stateful/runme/pkg/project" "go.uber.org/zap" ) @@ -36,7 +36,7 @@ func NewSession(envs []string, proj project.Project, logger *zap.Logger) (*Sessi } s := &Session{ - ID: xid.New().String(), + ID: utils.GenerateID(), envStore: newEnvStore(sessionEnvs...), logger: logger, diff --git a/internal/runner/session_test.go b/internal/runner/session_test.go index 075fd8241..9afb5dcac 100644 --- a/internal/runner/session_test.go +++ b/internal/runner/session_test.go @@ -41,6 +41,9 @@ func Test_SessionList(t *testing.T) { session2, err := createSession() require.NoError(t, err) + + assert.NotEqual(t, session1.ID, session2.ID) + list.AddSession(session2) found, ok := list.GetSession(session1.ID) @@ -62,10 +65,19 @@ func Test_SessionList(t *testing.T) { session2, err := list.CreateAndAddSession(createSession) require.NoError(t, err) + assert.NotEqual(t, session1.ID, session2.ID) + sessions, err := list.ListSessions() require.NoError(t, err) - assert.Equal(t, []*Session{session1, session2}, sessions) + expected := []string{session1.ID, session2.ID} + actual := []string{} + + for _, session := range sessions { + actual = append(actual, session.ID) + } + + assert.Equal(t, expected, actual) }) t.Run("DeleteSession", func(t *testing.T) { @@ -78,6 +90,8 @@ func Test_SessionList(t *testing.T) { session2, err := list.CreateAndAddSession(createSession) require.NoError(t, err) + assert.NotEqual(t, session1.ID, session2.ID) + { sessionList, err := list.ListSessions() require.NoError(t, err) @@ -91,7 +105,15 @@ func Test_SessionList(t *testing.T) { sessionList, err := list.ListSessions() require.NoError(t, err) assert.Equal(t, 1, len(sessionList)) - assert.Equal(t, session1, sessionList[0]) + + expected := []string{session1.ID} + actual := []string{} + + for _, session := range sessionList { + actual = append(actual, session.ID) + } + + assert.Equal(t, expected, actual) } deleted = list.DeleteSession(session1.ID) diff --git a/internal/runner/shell_session.go b/internal/runner/shell_session.go index 6956a374a..646993ec6 100644 --- a/internal/runner/shell_session.go +++ b/internal/runner/shell_session.go @@ -8,8 +8,8 @@ import ( "github.com/creack/pty" "github.com/pkg/errors" - "github.com/rs/xid" xpty "github.com/stateful/runme/internal/pty" + "github.com/stateful/runme/internal/utils" "golang.org/x/term" ) @@ -25,7 +25,7 @@ type ShellSession struct { } func NewShellSession(command string) (*ShellSession, error) { - id := xid.New().String() + id := utils.GenerateID() cmd := exec.Command(command) cmd.Env = append(os.Environ(), "RUNMESHELL="+id) diff --git a/internal/utils/id.go b/internal/utils/id.go new file mode 100644 index 000000000..2fbac065c --- /dev/null +++ b/internal/utils/id.go @@ -0,0 +1,60 @@ +// Package utils contains utility functions for the application +package utils + +import ( + "io" + "regexp" + "sync" + "time" + + "github.com/oklog/ulid/v2" + "golang.org/x/exp/rand" +) + +var ( + entropy io.Reader + entropyOnce sync.Once +) + +func DefaultEntropy() io.Reader { + entropyOnce.Do(func() { + seed := uint64(time.Now().UnixNano()) + source := rand.NewSource(seed) + rng := rand.New(source) + + entropy = &ulid.LockedMonotonicReader{ + MonotonicReader: ulid.Monotonic(rng, 0), + } + }) + return entropy +} + +// IsULID checks if the given string is a valid ULID +// ULID pattern: +// +// 01AN4Z07BY 79KA1307SR9X4MV3 +// |----------| |----------------| +// Timestamp Randomness +// +// 10 characters 16 characters +// Crockford's Base32 is used (excludes I, L, O, and U to avoid confusion and abuse) +func isULID(s string) bool { + ulidRegex := `^[0123456789ABCDEFGHJKMNPQRSTVWXYZ]{26}$` + matched, _ := regexp.MatchString(ulidRegex, s) + return matched +} + +// ValidID checks if the given id is valid +func ValidID(id string) bool { + _, err := ulid.Parse(id) + + return err == nil && isULID(id) +} + +// GenerateID generates a new universal ID +func GenerateID() string { + entropy := DefaultEntropy() + now := time.Now() + ts := ulid.Timestamp(now) + return ulid.MustNew(ts, entropy).String() +} diff --git a/internal/utils/id_test.go b/internal/utils/id_test.go new file mode 100644 index 000000000..a0e625a7b --- /dev/null +++ b/internal/utils/id_test.go @@ -0,0 +1,85 @@ +package utils + +import ( + "strings" + "sync" + "testing" + + "github.com/go-playground/assert/v2" +) + +// TestValidID tests the ValidID function with both valid and invalid inputs. +func TestValidID(t *testing.T) { + // Generate a valid ULID for testing. + validULID := GenerateID() + + tests := []struct { + id string + expected bool + }{ + {validULID, true}, + {"0", false}, + {"invalidulid", false}, + {"invalidulid", false}, + {"01B4E6BXY0PRJ5G420D25MWQY!", false}, + } + + for _, tt := range tests { + t.Run(tt.id, func(t *testing.T) { + if got := ValidID(tt.id); got != tt.expected { + t.Errorf("ValidID(%s) = %v; want %v", tt.id, got, tt.expected) + } + }) + } +} + +func TestGenerateID(t *testing.T) { + id := GenerateID() + if !ValidID(id) { + t.Errorf("Generated ID is not a valid ULID: %s", id) + } + + if len(id) != 26 { + t.Errorf("Generated ID does not have the correct length: got %v want %v", len(id), 26) + } + + if strings.ContainsAny(id, "ilou") { + t.Errorf("Generated ID contains invalid characters: %s", id) + } +} + +func TestGenerateUniqueID(t *testing.T) { + // inline function to set the generator + Generator := func() string { return GenerateID() } + + t.Run("uniqueness", func(t *testing.T) { + id1 := Generator() + id2 := Generator() + assert.NotEqual(t, id1, id2) + }) + + t.Run("concurrent uniqueness", func(t *testing.T) { + var wg sync.WaitGroup + ids := make(map[string]struct{}) + mu := sync.Mutex{} + + generateAndStoreID := func() { + defer wg.Done() + id := Generator() + mu.Lock() + defer mu.Unlock() + ids[id] = struct{}{} + } + + numIDs := 10000 + + wg.Add(numIDs) + for i := 0; i < numIDs; i++ { + go generateAndStoreID() + } + + wg.Wait() + + assert.Equal(t, numIDs, len(ids)) + }) +} From f566f43f1715be60df8d518c0e33bee2c5554a73 Mon Sep 17 00:00:00 2001 From: Cristian Cepeda <43882+pastuxso@users.noreply.github.com> Date: Wed, 1 Nov 2023 07:24:19 -0500 Subject: [PATCH 2/5] tests cleanup --- internal/utils/id_test.go | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/internal/utils/id_test.go b/internal/utils/id_test.go index a0e625a7b..94694bc0d 100644 --- a/internal/utils/id_test.go +++ b/internal/utils/id_test.go @@ -1,11 +1,10 @@ package utils import ( - "strings" "sync" "testing" - "github.com/go-playground/assert/v2" + "github.com/stretchr/testify/assert" ) // TestValidID tests the ValidID function with both valid and invalid inputs. @@ -27,27 +26,12 @@ func TestValidID(t *testing.T) { for _, tt := range tests { t.Run(tt.id, func(t *testing.T) { if got := ValidID(tt.id); got != tt.expected { - t.Errorf("ValidID(%s) = %v; want %v", tt.id, got, tt.expected) + assert.Equal(t, tt.expected, got) } }) } } -func TestGenerateID(t *testing.T) { - id := GenerateID() - if !ValidID(id) { - t.Errorf("Generated ID is not a valid ULID: %s", id) - } - - if len(id) != 26 { - t.Errorf("Generated ID does not have the correct length: got %v want %v", len(id), 26) - } - - if strings.ContainsAny(id, "ilou") { - t.Errorf("Generated ID contains invalid characters: %s", id) - } -} - func TestGenerateUniqueID(t *testing.T) { // inline function to set the generator Generator := func() string { return GenerateID() } From 3791e7e2f2e4f3220ce6f5a1e30b90ff5e7ced0a Mon Sep 17 00:00:00 2001 From: Cristian Cepeda <43882+pastuxso@users.noreply.github.com> Date: Wed, 1 Nov 2023 07:35:55 -0500 Subject: [PATCH 3/5] adds DefaultEntropy docs --- internal/utils/id.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/utils/id.go b/internal/utils/id.go index 2fbac065c..29ca7d47c 100644 --- a/internal/utils/id.go +++ b/internal/utils/id.go @@ -16,6 +16,9 @@ var ( entropyOnce sync.Once ) +// DefaultEntropy returns a reader that generates ULID entropy. +// The default entropy function utilizes math/rand.Rand, which is not safe for concurrent use by multiple goroutines. +// Therefore, this function employs x/exp/rand, as recommended by the authors of the library. func DefaultEntropy() io.Reader { entropyOnce.Do(func() { seed := uint64(time.Now().UnixNano()) @@ -32,9 +35,9 @@ func DefaultEntropy() io.Reader { // IsULID checks if the given string is a valid ULID // ULID pattern: // -// 01AN4Z07BY 79KA1307SR9X4MV3 +// 01AN4Z07BY 79KA1307SR9X4MV3 // |----------| |----------------| -// Timestamp Randomness +// Timestamp Randomness // // 10 characters 16 characters // Crockford's Base32 is used (excludes I, L, O, and U to avoid confusion and abuse) From c6a3db67dfd05d613df5a7b0492f6ce2352daaf6 Mon Sep 17 00:00:00 2001 From: Cristian Cepeda <43882+pastuxso@users.noreply.github.com> Date: Wed, 1 Nov 2023 07:42:43 -0500 Subject: [PATCH 4/5] removes unnecessary comments --- internal/utils/id_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/utils/id_test.go b/internal/utils/id_test.go index 94694bc0d..c6c7dd25c 100644 --- a/internal/utils/id_test.go +++ b/internal/utils/id_test.go @@ -7,9 +7,7 @@ import ( "github.com/stretchr/testify/assert" ) -// TestValidID tests the ValidID function with both valid and invalid inputs. func TestValidID(t *testing.T) { - // Generate a valid ULID for testing. validULID := GenerateID() tests := []struct { @@ -33,7 +31,6 @@ func TestValidID(t *testing.T) { } func TestGenerateUniqueID(t *testing.T) { - // inline function to set the generator Generator := func() string { return GenerateID() } t.Run("uniqueness", func(t *testing.T) { From 4f8b0c19dc889ea59aecfe287727c2ac1432c959 Mon Sep 17 00:00:00 2001 From: Cristian Cepeda <43882+pastuxso@users.noreply.github.com> Date: Wed, 1 Nov 2023 14:46:57 -0500 Subject: [PATCH 5/5] addresses codereview comments --- internal/{utils => idgen}/id.go | 2 +- internal/{utils => idgen}/id_test.go | 2 +- internal/runner/command.go | 4 ++-- internal/runner/service.go | 4 ++-- internal/runner/session.go | 4 ++-- internal/runner/shell_session.go | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) rename internal/{utils => idgen}/id.go (99%) rename internal/{utils => idgen}/id_test.go (98%) diff --git a/internal/utils/id.go b/internal/idgen/id.go similarity index 99% rename from internal/utils/id.go rename to internal/idgen/id.go index 29ca7d47c..80d5cfe67 100644 --- a/internal/utils/id.go +++ b/internal/idgen/id.go @@ -1,5 +1,5 @@ // Package utils contains utility functions for the application -package utils +package idgen import ( "io" diff --git a/internal/utils/id_test.go b/internal/idgen/id_test.go similarity index 98% rename from internal/utils/id_test.go rename to internal/idgen/id_test.go index c6c7dd25c..999a00717 100644 --- a/internal/utils/id_test.go +++ b/internal/idgen/id_test.go @@ -1,4 +1,4 @@ -package utils +package idgen import ( "sync" diff --git a/internal/runner/command.go b/internal/runner/command.go index a2bf44761..c42d14284 100644 --- a/internal/runner/command.go +++ b/internal/runner/command.go @@ -15,7 +15,7 @@ import ( "github.com/creack/pty" "github.com/pkg/errors" - "github.com/stateful/runme/internal/utils" + "github.com/stateful/runme/internal/idgen" "go.uber.org/multierr" "go.uber.org/zap" ) @@ -198,7 +198,7 @@ func newCommand(cfg *commandConfig) (*command, error) { extraArgs = append(extraArgs, "-c", script) case CommandModeTempFile: for { - id := utils.GenerateID() + id := idgen.GenerateID() tempScriptFile = filepath.Join(cfg.Directory, fmt.Sprintf(".runme-script-%s", id)) if fileExtension != "" { diff --git a/internal/runner/service.go b/internal/runner/service.go index 8be36fecf..fa022921c 100644 --- a/internal/runner/service.go +++ b/internal/runner/service.go @@ -11,8 +11,8 @@ import ( "github.com/pkg/errors" "github.com/stateful/runme/internal/env" runnerv1 "github.com/stateful/runme/internal/gen/proto/go/runme/runner/v1" + "github.com/stateful/runme/internal/idgen" "github.com/stateful/runme/internal/rbuffer" - "github.com/stateful/runme/internal/utils" "github.com/stateful/runme/pkg/project" "go.uber.org/zap" "golang.org/x/sync/errgroup" @@ -155,7 +155,7 @@ func ConvertRunnerProject(runnerProj *runnerv1.Project) (project.Project, error) } func (r *runnerService) Execute(srv runnerv1.RunnerService_ExecuteServer) error { - logger := r.logger.With(zap.String("_id", utils.GenerateID())) + logger := r.logger.With(zap.String("_id", idgen.GenerateID())) logger.Info("running Execute in runnerService") diff --git a/internal/runner/session.go b/internal/runner/session.go index 0fa44766c..c35d7520f 100644 --- a/internal/runner/session.go +++ b/internal/runner/session.go @@ -5,7 +5,7 @@ import ( "sync" lru "github.com/hashicorp/golang-lru/v2" - "github.com/stateful/runme/internal/utils" + "github.com/stateful/runme/internal/idgen" "github.com/stateful/runme/pkg/project" "go.uber.org/zap" ) @@ -36,7 +36,7 @@ func NewSession(envs []string, proj project.Project, logger *zap.Logger) (*Sessi } s := &Session{ - ID: utils.GenerateID(), + ID: idgen.GenerateID(), envStore: newEnvStore(sessionEnvs...), logger: logger, diff --git a/internal/runner/shell_session.go b/internal/runner/shell_session.go index 646993ec6..f0354dfc8 100644 --- a/internal/runner/shell_session.go +++ b/internal/runner/shell_session.go @@ -8,8 +8,8 @@ import ( "github.com/creack/pty" "github.com/pkg/errors" + "github.com/stateful/runme/internal/idgen" xpty "github.com/stateful/runme/internal/pty" - "github.com/stateful/runme/internal/utils" "golang.org/x/term" ) @@ -25,7 +25,7 @@ type ShellSession struct { } func NewShellSession(command string) (*ShellSession, error) { - id := utils.GenerateID() + id := idgen.GenerateID() cmd := exec.Command(command) cmd.Env = append(os.Environ(), "RUNMESHELL="+id)