From d62c0e050c4933d6d07929c59b4fbb0ecde20130 Mon Sep 17 00:00:00 2001 From: wangxiaoxuan273 Date: Tue, 4 Jul 2023 00:21:54 +0800 Subject: [PATCH 01/10] feat: support trace with executables Signed-off-by: Xiaoxuan Wang --- internal/executer/executer.go | 9 +++ native_store_test.go | 101 +++++++++++++++++++++++++++++++++- trace/trace.go | 61 ++++++++++++++++++++ 3 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 trace/trace.go diff --git a/internal/executer/executer.go b/internal/executer/executer.go index 4e9ee42..c9220ee 100644 --- a/internal/executer/executer.go +++ b/internal/executer/executer.go @@ -24,6 +24,8 @@ import ( "io" "os" "os/exec" + + "github.com/oras-project/oras-credentials-go/trace" ) // dockerDesktopHelperName is the name of the docker credentials helper @@ -49,10 +51,17 @@ func New(name string) Executer { // Execute operates on an executable binary and supports context. func (c *executable) Execute(ctx context.Context, input io.Reader, action string) ([]byte, error) { + trace := trace.ContextExecutableTrace(ctx) cmd := exec.CommandContext(ctx, c.name, action) cmd.Stdin = input cmd.Stderr = os.Stderr + if trace != nil && trace.ExecuteStart != nil { + trace.ExecuteStart(c.name, action) + } output, err := cmd.Output() + if trace != nil && trace.ExecuteDone != nil { + trace.ExecuteDone(c.name, action, err) + } if err != nil { switch execErr := err.(type) { case *exec.ExitError: diff --git a/native_store_test.go b/native_store_test.go index b2e2b25..3faab1e 100644 --- a/native_store_test.go +++ b/native_store_test.go @@ -16,6 +16,7 @@ limitations under the License. package credentials import ( + "bytes" "context" "encoding/json" "fmt" @@ -23,15 +24,18 @@ import ( "strings" "testing" + "github.com/oras-project/oras-credentials-go/trace" + "oras.land/oras-go/v2/registry/remote/auth" ) const ( basicAuthHost = "localhost:2333" - bearerAuthHost = "localhost:6666" + bearerAuthHost = "localhost:666" exeErrorHost = "localhost:500/exeError" jsonErrorHost = "localhost:500/jsonError" noCredentialsHost = "localhost:404" + traceHost = "localhost:808" testUsername = "test_username" testPassword = "test_password" testRefreshToken = "test_token" @@ -69,6 +73,13 @@ func (e *testExecuter) Execute(ctx context.Context, input io.Reader, action stri return []byte("json.Unmarshal failed"), nil case noCredentialsHost: return []byte("credentials not found"), errCredentialsNotFound + case traceHost: + traceHook := trace.ContextExecutableTrace(ctx) + if traceHook != nil { + traceHook.ExecuteStart("testExecuter", "get") + traceHook.ExecuteDone("testExecuter", "get", nil) + } + return []byte(`{"Username": "test_username", "Secret": "test_password"}`), nil default: return []byte("program failed"), errCommandExited } @@ -81,6 +92,13 @@ func (e *testExecuter) Execute(ctx context.Context, input io.Reader, action stri switch c.ServerURL { case basicAuthHost, bearerAuthHost, exeErrorHost: return nil, nil + case traceHost: + traceHook := trace.ContextExecutableTrace(ctx) + if traceHook != nil { + traceHook.ExecuteStart("testExecuter", "store") + traceHook.ExecuteDone("testExecuter", "store", nil) + } + return nil, nil default: return []byte("program failed"), errCommandExited } @@ -88,6 +106,13 @@ func (e *testExecuter) Execute(ctx context.Context, input io.Reader, action stri switch inS { case basicAuthHost, bearerAuthHost: return nil, nil + case traceHost: + traceHook := trace.ContextExecutableTrace(ctx) + if traceHook != nil { + traceHook.ExecuteStart("testExecuter", "erase") + traceHook.ExecuteDone("testExecuter", "erase", nil) + } + return nil, nil default: return []byte("program failed"), errCommandExited } @@ -185,3 +210,77 @@ func TestNewDefaultNativeStore(t *testing.T) { t.Errorf("NewDefaultNativeStore() = %v, want %v", ok, wantOK) } } + +func TestNativeStore_trace(t *testing.T) { + ns := &nativeStore{ + &testExecuter{}, + } + // create trace hooks that write to buffer + buffer := bytes.Buffer{} + traceHook := &trace.ExecutableTrace{ + ExecuteStart: func(executableName string, action string) { + buffer.WriteString(fmt.Sprintf("test trace, start the execution of executable %s with action %s ", executableName, action)) + }, + ExecuteDone: func(executableName string, action string, err error) { + buffer.WriteString(fmt.Sprintf("test trace, completed the execution of executable %s with action %s and got err %v", executableName, action, err)) + }, + } + ctx := trace.WithExecutableTrace(context.Background(), traceHook) + // Test ns.Put trace + err := ns.Put(ctx, traceHost, auth.Credential{Username: testUsername, Password: testPassword}) + if err != nil { + t.Fatalf("trace test ns.Put fails: %v", err) + } + bufferContent := buffer.String() + if bufferContent != "test trace, start the execution of executable testExecuter with action store test trace, completed the execution of executable testExecuter with action store and got err " { + t.Fatalf("incorrect buffer content: %s", bufferContent) + } + buffer.Reset() + // Test ns.Get trace + _, err = ns.Get(ctx, traceHost) + if err != nil { + t.Fatalf("trace test ns.Get fails: %v", err) + } + bufferContent = buffer.String() + if bufferContent != "test trace, start the execution of executable testExecuter with action get test trace, completed the execution of executable testExecuter with action get and got err " { + t.Fatalf("incorrect buffer content: %s", bufferContent) + } + buffer.Reset() + // Test ns.Delete trace + err = ns.Delete(ctx, traceHost) + if err != nil { + t.Fatalf("trace test ns.Delete fails: %v", err) + } + bufferContent = buffer.String() + if bufferContent != "test trace, start the execution of executable testExecuter with action erase test trace, completed the execution of executable testExecuter with action erase and got err " { + t.Fatalf("incorrect buffer content: %s", bufferContent) + } +} + +// This test ensures that a nil trace will not cause an error. +func TestNativeStore_noTrace(t *testing.T) { + ns := &nativeStore{ + &testExecuter{}, + } + // Put + err := ns.Put(context.Background(), traceHost, auth.Credential{Username: testUsername, Password: testPassword}) + if err != nil { + t.Fatalf("basic auth test ns.Put fails: %v", err) + } + // Get + cred, err := ns.Get(context.Background(), traceHost) + if err != nil { + t.Fatalf("basic auth test ns.Get fails: %v", err) + } + if cred.Username != testUsername { + t.Fatal("incorrect username") + } + if cred.Password != testPassword { + t.Fatal("incorrect password") + } + // Delete + err = ns.Delete(context.Background(), traceHost) + if err != nil { + t.Fatalf("basic auth test ns.Delete fails: %v", err) + } +} diff --git a/trace/trace.go b/trace/trace.go new file mode 100644 index 0000000..cbf9324 --- /dev/null +++ b/trace/trace.go @@ -0,0 +1,61 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package trace + +import ( + "context" +) + +// executableTraceHookKey is a value key used to retrieve the ExecutableTrace +// from Context. +type executableTraceContextKey struct{} + +// ExecutableTrace is a set of hooks used to trace the execution of the binary +// executable used with NativeStore. Any particular hook may be nil. +type ExecutableTrace struct { + // ExecuteStart is called before the execution of the executable. The + // executableName parameter is the name of the credential helper executable + // used with NativeStore. The action parameter is one of "store", "get" and + // "erase". + // + // Reference: + // - https://docs.docker.com/engine/reference/commandline/login#credentials-store + ExecuteStart func(executableName string, action string) + // ExecuteEnd is called after the execution of an executable completes. + // The executableName parameter is the name of the credential helper executable + // used with NativeStore. The action parameter is one of "store", "get" and + // "erase". The err parameter is the error (if any) returned from the execution. + // + // Reference: + // - https://docs.docker.com/engine/reference/commandline/login#credentials-store + ExecuteDone func(executableName string, action string, err error) +} + +// GetTraceHooksFromContext returns the ExecutableTrace associated with the context. +// If none, it returns nil. +func ContextExecutableTrace(ctx context.Context) *ExecutableTrace { + trace, _ := ctx.Value(executableTraceContextKey{}).(*ExecutableTrace) + return trace +} + +// WithTraceHooks takes a Context and an ExecutableTrace, and returns a Context with +// the ExecutableTrace added as a Value. +func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Context { + if trace != nil { + ctx = context.WithValue(ctx, executableTraceContextKey{}, trace) + } + return ctx +} From b36938b121c4d71065bb2af340e5f6d8ea8bd0df Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Tue, 11 Jul 2023 06:21:40 +0000 Subject: [PATCH 02/10] changed my terrible renaming typos Signed-off-by: Xiaoxuan Wang --- trace/trace.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/trace/trace.go b/trace/trace.go index cbf9324..bb50b38 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -19,7 +19,7 @@ import ( "context" ) -// executableTraceHookKey is a value key used to retrieve the ExecutableTrace +// executableTraceContextKey is a value key used to retrieve the ExecutableTrace // from Context. type executableTraceContextKey struct{} @@ -34,7 +34,8 @@ type ExecutableTrace struct { // Reference: // - https://docs.docker.com/engine/reference/commandline/login#credentials-store ExecuteStart func(executableName string, action string) - // ExecuteEnd is called after the execution of an executable completes. + + // ExecuteDone is called after the execution of an executable completes. // The executableName parameter is the name of the credential helper executable // used with NativeStore. The action parameter is one of "store", "get" and // "erase". The err parameter is the error (if any) returned from the execution. @@ -44,14 +45,14 @@ type ExecutableTrace struct { ExecuteDone func(executableName string, action string, err error) } -// GetTraceHooksFromContext returns the ExecutableTrace associated with the context. +// ContextExecutableTrace returns the ExecutableTrace associated with the context. // If none, it returns nil. func ContextExecutableTrace(ctx context.Context) *ExecutableTrace { trace, _ := ctx.Value(executableTraceContextKey{}).(*ExecutableTrace) return trace } -// WithTraceHooks takes a Context and an ExecutableTrace, and returns a Context with +// WithExecutableTrace takes a Context and an ExecutableTrace, and returns a Context with // the ExecutableTrace added as a Value. func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Context { if trace != nil { From 77450580cc1767c0cb39a9c374952a1b62ad7bfa Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 12 Jul 2023 05:27:25 +0000 Subject: [PATCH 03/10] resolved the comments Signed-off-by: Xiaoxuan Wang --- internal/executer/executer.go | 2 +- trace/trace.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/executer/executer.go b/internal/executer/executer.go index c9220ee..fc872c9 100644 --- a/internal/executer/executer.go +++ b/internal/executer/executer.go @@ -51,10 +51,10 @@ func New(name string) Executer { // Execute operates on an executable binary and supports context. func (c *executable) Execute(ctx context.Context, input io.Reader, action string) ([]byte, error) { - trace := trace.ContextExecutableTrace(ctx) cmd := exec.CommandContext(ctx, c.name, action) cmd.Stdin = input cmd.Stderr = os.Stderr + trace := trace.ContextExecutableTrace(ctx) if trace != nil && trace.ExecuteStart != nil { trace.ExecuteStart(c.name, action) } diff --git a/trace/trace.go b/trace/trace.go index bb50b38..2eeffa2 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -23,8 +23,8 @@ import ( // from Context. type executableTraceContextKey struct{} -// ExecutableTrace is a set of hooks used to trace the execution of the binary -// executable used with NativeStore. Any particular hook may be nil. +// ExecutableTrace is a set of hooks used to trace the execution of binary +// executables. Any particular hook may be nil. type ExecutableTrace struct { // ExecuteStart is called before the execution of the executable. The // executableName parameter is the name of the credential helper executable @@ -55,8 +55,8 @@ func ContextExecutableTrace(ctx context.Context) *ExecutableTrace { // WithExecutableTrace takes a Context and an ExecutableTrace, and returns a Context with // the ExecutableTrace added as a Value. func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Context { - if trace != nil { - ctx = context.WithValue(ctx, executableTraceContextKey{}, trace) + if trace == nil { + return ctx } - return ctx + return context.WithValue(ctx, executableTraceContextKey{}, trace) } From 4a45e6e39fdb8ab4860da3d3fd67542d1da7b4d6 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 12 Jul 2023 08:26:51 +0000 Subject: [PATCH 04/10] add chaining feature of the trace Signed-off-by: Xiaoxuan Wang --- native_store_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++ trace/trace.go | 25 ++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/native_store_test.go b/native_store_test.go index 3faab1e..469bf66 100644 --- a/native_store_test.go +++ b/native_store_test.go @@ -284,3 +284,59 @@ func TestNativeStore_noTrace(t *testing.T) { t.Fatalf("basic auth test ns.Delete fails: %v", err) } } + +func TestNativeStore_multipleTrace(t *testing.T) { + ns := &nativeStore{ + &testExecuter{}, + } + // create trace hooks that write to buffer + buffer := bytes.Buffer{} + trace1 := &trace.ExecutableTrace{ + ExecuteStart: func(executableName string, action string) { + buffer.WriteString(fmt.Sprintf("trace 1 start %s, %s ", executableName, action)) + }, + ExecuteDone: func(executableName string, action string, err error) { + buffer.WriteString(fmt.Sprintf("trace 1 done %s, %s ", executableName, action)) + }, + } + ctx := context.Background() + ctx = trace.WithExecutableTrace(ctx, trace1) + trace2 := &trace.ExecutableTrace{ + ExecuteStart: func(executableName string, action string) { + buffer.WriteString(fmt.Sprintf("trace 2 start %s, %s ", executableName, action)) + }, + ExecuteDone: func(executableName string, action string, err error) { + buffer.WriteString(fmt.Sprintf("trace 2 done %s, %s ", executableName, action)) + }, + } + ctx = trace.WithExecutableTrace(ctx, trace2) + // Test ns.Put trace + err := ns.Put(ctx, traceHost, auth.Credential{Username: testUsername, Password: testPassword}) + if err != nil { + t.Fatalf("trace test ns.Put fails: %v", err) + } + bufferContent := buffer.String() + if bufferContent != "trace 1 start testExecuter, store trace 2 start testExecuter, store trace 1 done testExecuter, store trace 2 done testExecuter, store " { + t.Fatalf("incorrect buffer content: %s", bufferContent) + } + buffer.Reset() + // Test ns.Get trace + _, err = ns.Get(ctx, traceHost) + if err != nil { + t.Fatalf("trace test ns.Get fails: %v", err) + } + bufferContent = buffer.String() + if bufferContent != "trace 1 start testExecuter, get trace 2 start testExecuter, get trace 1 done testExecuter, get trace 2 done testExecuter, get " { + t.Fatalf("incorrect buffer content: %s", bufferContent) + } + buffer.Reset() + // Test ns.Delete trace + err = ns.Delete(ctx, traceHost) + if err != nil { + t.Fatalf("trace test ns.Delete fails: %v", err) + } + bufferContent = buffer.String() + if bufferContent != "trace 1 start testExecuter, erase trace 2 start testExecuter, erase trace 1 done testExecuter, erase trace 2 done testExecuter, erase " { + t.Fatalf("incorrect buffer content: %s", bufferContent) + } +} diff --git a/trace/trace.go b/trace/trace.go index 2eeffa2..6cc4d21 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -58,5 +58,30 @@ func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Co if trace == nil { return ctx } + oldTrace, _ := ctx.Value(executableTraceContextKey{}).(*ExecutableTrace) + if oldTrace != nil { + trace.compose(oldTrace) + } return context.WithValue(ctx, executableTraceContextKey{}, trace) } + +// compose takes an oldTrace and modifies the existing trace to include +// the hooks defined in the oldTrace. +func (trace *ExecutableTrace) compose(oldTrace *ExecutableTrace) { + oldStart := oldTrace.ExecuteStart + if oldStart != nil { + start := trace.ExecuteStart + trace.ExecuteStart = func(executableName, action string) { + oldStart(executableName, action) + start(executableName, action) + } + } + oldDone := oldTrace.ExecuteDone + if oldDone != nil { + done := trace.ExecuteDone + trace.ExecuteDone = func(executableName, action string, err error) { + oldDone(executableName, action, err) + done(executableName, action, err) + } + } +} From 23945dea270d8ec4fa5a902b2767780a3a0c63c6 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 12 Jul 2023 08:55:07 +0000 Subject: [PATCH 05/10] used LIFO order Signed-off-by: Xiaoxuan Wang --- native_store_test.go | 6 +++--- trace/trace.go | 8 +++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/native_store_test.go b/native_store_test.go index 469bf66..31516dc 100644 --- a/native_store_test.go +++ b/native_store_test.go @@ -316,7 +316,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { t.Fatalf("trace test ns.Put fails: %v", err) } bufferContent := buffer.String() - if bufferContent != "trace 1 start testExecuter, store trace 2 start testExecuter, store trace 1 done testExecuter, store trace 2 done testExecuter, store " { + if bufferContent != "trace 2 start testExecuter, store trace 1 start testExecuter, store trace 2 done testExecuter, store trace 1 done testExecuter, store " { t.Fatalf("incorrect buffer content: %s", bufferContent) } buffer.Reset() @@ -326,7 +326,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { t.Fatalf("trace test ns.Get fails: %v", err) } bufferContent = buffer.String() - if bufferContent != "trace 1 start testExecuter, get trace 2 start testExecuter, get trace 1 done testExecuter, get trace 2 done testExecuter, get " { + if bufferContent != "trace 2 start testExecuter, get trace 1 start testExecuter, get trace 2 done testExecuter, get trace 1 done testExecuter, get " { t.Fatalf("incorrect buffer content: %s", bufferContent) } buffer.Reset() @@ -336,7 +336,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { t.Fatalf("trace test ns.Delete fails: %v", err) } bufferContent = buffer.String() - if bufferContent != "trace 1 start testExecuter, erase trace 2 start testExecuter, erase trace 1 done testExecuter, erase trace 2 done testExecuter, erase " { + if bufferContent != "trace 2 start testExecuter, erase trace 1 start testExecuter, erase trace 2 done testExecuter, erase trace 1 done testExecuter, erase " { t.Fatalf("incorrect buffer content: %s", bufferContent) } } diff --git a/trace/trace.go b/trace/trace.go index 6cc4d21..11d59cb 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -53,7 +53,9 @@ func ContextExecutableTrace(ctx context.Context) *ExecutableTrace { } // WithExecutableTrace takes a Context and an ExecutableTrace, and returns a Context with -// the ExecutableTrace added as a Value. +// the ExecutableTrace added as a Value. If the Context has a previously added trace, +// the hooks defined in the new trace will be added in addition to the previous ones. +// The recent hooks will be called first. func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Context { if trace == nil { return ctx @@ -72,16 +74,16 @@ func (trace *ExecutableTrace) compose(oldTrace *ExecutableTrace) { if oldStart != nil { start := trace.ExecuteStart trace.ExecuteStart = func(executableName, action string) { - oldStart(executableName, action) start(executableName, action) + oldStart(executableName, action) } } oldDone := oldTrace.ExecuteDone if oldDone != nil { done := trace.ExecuteDone trace.ExecuteDone = func(executableName, action string, err error) { - oldDone(executableName, action, err) done(executableName, action, err) + oldDone(executableName, action, err) } } } From 21c00238be4345b9571e0ae22520df24967de116 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 12 Jul 2023 09:04:26 +0000 Subject: [PATCH 06/10] added another doc comment Signed-off-by: Xiaoxuan Wang --- trace/trace.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/trace/trace.go b/trace/trace.go index 11d59cb..810344d 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -68,7 +68,8 @@ func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Co } // compose takes an oldTrace and modifies the existing trace to include -// the hooks defined in the oldTrace. +// the hooks defined in the oldTrace. The hooks in the existing trace will +// be called first. func (trace *ExecutableTrace) compose(oldTrace *ExecutableTrace) { oldStart := oldTrace.ExecuteStart if oldStart != nil { From f995ab22f4fc17d8309797ab8d8afd3ce4a37d7c Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 12 Jul 2023 14:05:54 +0000 Subject: [PATCH 07/10] resolved comments Signed-off-by: Xiaoxuan Wang --- native_store_test.go | 10 +++++----- trace/trace.go | 22 +++++++++++++++------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/native_store_test.go b/native_store_test.go index 31516dc..b94af44 100644 --- a/native_store_test.go +++ b/native_store_test.go @@ -296,7 +296,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { buffer.WriteString(fmt.Sprintf("trace 1 start %s, %s ", executableName, action)) }, ExecuteDone: func(executableName string, action string, err error) { - buffer.WriteString(fmt.Sprintf("trace 1 done %s, %s ", executableName, action)) + buffer.WriteString(fmt.Sprintf("trace 1 done %s, %s, %v ", executableName, action, err)) }, } ctx := context.Background() @@ -306,7 +306,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { buffer.WriteString(fmt.Sprintf("trace 2 start %s, %s ", executableName, action)) }, ExecuteDone: func(executableName string, action string, err error) { - buffer.WriteString(fmt.Sprintf("trace 2 done %s, %s ", executableName, action)) + buffer.WriteString(fmt.Sprintf("trace 2 done %s, %s, %v ", executableName, action, err)) }, } ctx = trace.WithExecutableTrace(ctx, trace2) @@ -316,7 +316,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { t.Fatalf("trace test ns.Put fails: %v", err) } bufferContent := buffer.String() - if bufferContent != "trace 2 start testExecuter, store trace 1 start testExecuter, store trace 2 done testExecuter, store trace 1 done testExecuter, store " { + if bufferContent != "trace 2 start testExecuter, store trace 1 start testExecuter, store trace 2 done testExecuter, store, trace 1 done testExecuter, store, " { t.Fatalf("incorrect buffer content: %s", bufferContent) } buffer.Reset() @@ -326,7 +326,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { t.Fatalf("trace test ns.Get fails: %v", err) } bufferContent = buffer.String() - if bufferContent != "trace 2 start testExecuter, get trace 1 start testExecuter, get trace 2 done testExecuter, get trace 1 done testExecuter, get " { + if bufferContent != "trace 2 start testExecuter, get trace 1 start testExecuter, get trace 2 done testExecuter, get, trace 1 done testExecuter, get, " { t.Fatalf("incorrect buffer content: %s", bufferContent) } buffer.Reset() @@ -336,7 +336,7 @@ func TestNativeStore_multipleTrace(t *testing.T) { t.Fatalf("trace test ns.Delete fails: %v", err) } bufferContent = buffer.String() - if bufferContent != "trace 2 start testExecuter, erase trace 1 start testExecuter, erase trace 2 done testExecuter, erase trace 1 done testExecuter, erase " { + if bufferContent != "trace 2 start testExecuter, erase trace 1 start testExecuter, erase trace 2 done testExecuter, erase, trace 1 done testExecuter, erase, " { t.Fatalf("incorrect buffer content: %s", bufferContent) } } diff --git a/trace/trace.go b/trace/trace.go index 810344d..2997c87 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -60,7 +60,7 @@ func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Co if trace == nil { return ctx } - oldTrace, _ := ctx.Value(executableTraceContextKey{}).(*ExecutableTrace) + oldTrace := ContextExecutableTrace(ctx) if oldTrace != nil { trace.compose(oldTrace) } @@ -74,17 +74,25 @@ func (trace *ExecutableTrace) compose(oldTrace *ExecutableTrace) { oldStart := oldTrace.ExecuteStart if oldStart != nil { start := trace.ExecuteStart - trace.ExecuteStart = func(executableName, action string) { - start(executableName, action) - oldStart(executableName, action) + if start != nil { + trace.ExecuteStart = func(executableName, action string) { + start(executableName, action) + oldStart(executableName, action) + } + } else { + trace.ExecuteStart = oldStart } } oldDone := oldTrace.ExecuteDone if oldDone != nil { done := trace.ExecuteDone - trace.ExecuteDone = func(executableName, action string, err error) { - done(executableName, action, err) - oldDone(executableName, action, err) + if done != nil { + trace.ExecuteDone = func(executableName, action string, err error) { + done(executableName, action, err) + oldDone(executableName, action, err) + } + } else { + trace.ExecuteDone = oldDone } } } From 6dff8714e2f76d4e3d87fc27392e645b0e7f80eb Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 13 Jul 2023 07:26:51 +0000 Subject: [PATCH 08/10] refined Signed-off-by: Xiaoxuan Wang --- native_store_test.go | 1 - trace/trace.go | 9 +++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/native_store_test.go b/native_store_test.go index b94af44..d6722ff 100644 --- a/native_store_test.go +++ b/native_store_test.go @@ -25,7 +25,6 @@ import ( "testing" "github.com/oras-project/oras-credentials-go/trace" - "oras.land/oras-go/v2/registry/remote/auth" ) diff --git a/trace/trace.go b/trace/trace.go index 2997c87..41cf1d8 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -60,8 +60,7 @@ func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Co if trace == nil { return ctx } - oldTrace := ContextExecutableTrace(ctx) - if oldTrace != nil { + if oldTrace := ContextExecutableTrace(ctx); oldTrace != nil { trace.compose(oldTrace) } return context.WithValue(ctx, executableTraceContextKey{}, trace) @@ -71,8 +70,7 @@ func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Co // the hooks defined in the oldTrace. The hooks in the existing trace will // be called first. func (trace *ExecutableTrace) compose(oldTrace *ExecutableTrace) { - oldStart := oldTrace.ExecuteStart - if oldStart != nil { + if oldStart := oldTrace.ExecuteStart; oldStart != nil { start := trace.ExecuteStart if start != nil { trace.ExecuteStart = func(executableName, action string) { @@ -83,8 +81,7 @@ func (trace *ExecutableTrace) compose(oldTrace *ExecutableTrace) { trace.ExecuteStart = oldStart } } - oldDone := oldTrace.ExecuteDone - if oldDone != nil { + if oldDone := oldTrace.ExecuteDone; oldDone != nil { done := trace.ExecuteDone if done != nil { trace.ExecuteDone = func(executableName, action string, err error) { From a6027403567e35f3bb933ce49b5547eba4a1bc26 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 13 Jul 2023 08:24:49 +0000 Subject: [PATCH 09/10] resolved comments Signed-off-by: Xiaoxuan Wang --- native_store_test.go | 56 +++++++++++++++++++++++++++++++++++++++----- trace/trace.go | 8 +++---- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/native_store_test.go b/native_store_test.go index d6722ff..6ee0059 100644 --- a/native_store_test.go +++ b/native_store_test.go @@ -75,8 +75,12 @@ func (e *testExecuter) Execute(ctx context.Context, input io.Reader, action stri case traceHost: traceHook := trace.ContextExecutableTrace(ctx) if traceHook != nil { - traceHook.ExecuteStart("testExecuter", "get") - traceHook.ExecuteDone("testExecuter", "get", nil) + if traceHook.ExecuteStart != nil { + traceHook.ExecuteStart("testExecuter", "get") + } + if traceHook.ExecuteDone != nil { + traceHook.ExecuteDone("testExecuter", "get", nil) + } } return []byte(`{"Username": "test_username", "Secret": "test_password"}`), nil default: @@ -94,8 +98,12 @@ func (e *testExecuter) Execute(ctx context.Context, input io.Reader, action stri case traceHost: traceHook := trace.ContextExecutableTrace(ctx) if traceHook != nil { - traceHook.ExecuteStart("testExecuter", "store") - traceHook.ExecuteDone("testExecuter", "store", nil) + if traceHook.ExecuteStart != nil { + traceHook.ExecuteStart("testExecuter", "store") + } + if traceHook.ExecuteDone != nil { + traceHook.ExecuteDone("testExecuter", "store", nil) + } } return nil, nil default: @@ -108,8 +116,12 @@ func (e *testExecuter) Execute(ctx context.Context, input io.Reader, action stri case traceHost: traceHook := trace.ContextExecutableTrace(ctx) if traceHook != nil { - traceHook.ExecuteStart("testExecuter", "erase") - traceHook.ExecuteDone("testExecuter", "erase", nil) + if traceHook.ExecuteStart != nil { + traceHook.ExecuteStart("testExecuter", "erase") + } + if traceHook.ExecuteDone != nil { + traceHook.ExecuteDone("testExecuter", "erase", nil) + } } return nil, nil default: @@ -284,6 +296,36 @@ func TestNativeStore_noTrace(t *testing.T) { } } +// This test ensures that an empty trace will not cause an error. +func TestNativeStore_emptyTrace(t *testing.T) { + ns := &nativeStore{ + &testExecuter{}, + } + traceHook := &trace.ExecutableTrace{} + ctx := trace.WithExecutableTrace(context.Background(), traceHook) + // Put + err := ns.Put(ctx, traceHost, auth.Credential{Username: testUsername, Password: testPassword}) + if err != nil { + t.Fatalf("basic auth test ns.Put fails: %v", err) + } + // Get + cred, err := ns.Get(ctx, traceHost) + if err != nil { + t.Fatalf("basic auth test ns.Get fails: %v", err) + } + if cred.Username != testUsername { + t.Fatal("incorrect username") + } + if cred.Password != testPassword { + t.Fatal("incorrect password") + } + // Delete + err = ns.Delete(ctx, traceHost) + if err != nil { + t.Fatalf("basic auth test ns.Delete fails: %v", err) + } +} + func TestNativeStore_multipleTrace(t *testing.T) { ns := &nativeStore{ &testExecuter{}, @@ -309,6 +351,8 @@ func TestNativeStore_multipleTrace(t *testing.T) { }, } ctx = trace.WithExecutableTrace(ctx, trace2) + trace3 := &trace.ExecutableTrace{} + ctx = trace.WithExecutableTrace(ctx, trace3) // Test ns.Put trace err := ns.Put(ctx, traceHost, auth.Credential{Username: testUsername, Password: testPassword}) if err != nil { diff --git a/trace/trace.go b/trace/trace.go index 41cf1d8..57e88a8 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -52,10 +52,10 @@ func ContextExecutableTrace(ctx context.Context) *ExecutableTrace { return trace } -// WithExecutableTrace takes a Context and an ExecutableTrace, and returns a Context with -// the ExecutableTrace added as a Value. If the Context has a previously added trace, -// the hooks defined in the new trace will be added in addition to the previous ones. -// The recent hooks will be called first. +// WithExecutableTrace takes a Context and an ExecutableTrace, and returns a +// Context with the ExecutableTrace added as a Value. If the Context has a +// previously added trace, the hooks defined in the new trace will be added +// in addition to the previous ones. The recent hooks will be called first. func WithExecutableTrace(ctx context.Context, trace *ExecutableTrace) context.Context { if trace == nil { return ctx From 14e008a7707c080d05ba62c2e1bcfa344ca980e7 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 13 Jul 2023 09:03:48 +0000 Subject: [PATCH 10/10] shrink line length Signed-off-by: Xiaoxuan Wang --- trace/trace.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/trace/trace.go b/trace/trace.go index 57e88a8..244a3f1 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -36,17 +36,18 @@ type ExecutableTrace struct { ExecuteStart func(executableName string, action string) // ExecuteDone is called after the execution of an executable completes. - // The executableName parameter is the name of the credential helper executable - // used with NativeStore. The action parameter is one of "store", "get" and - // "erase". The err parameter is the error (if any) returned from the execution. + // The executableName parameter is the name of the credential helper + // executable used with NativeStore. The action parameter is one of "store", + // "get" and "erase". The err parameter is the error (if any) returned from + // the execution. // // Reference: // - https://docs.docker.com/engine/reference/commandline/login#credentials-store ExecuteDone func(executableName string, action string, err error) } -// ContextExecutableTrace returns the ExecutableTrace associated with the context. -// If none, it returns nil. +// ContextExecutableTrace returns the ExecutableTrace associated with the +// context. If none, it returns nil. func ContextExecutableTrace(ctx context.Context) *ExecutableTrace { trace, _ := ctx.Value(executableTraceContextKey{}).(*ExecutableTrace) return trace