From 3551b4c77dc6e51ced4b21ff91e28e955a914803 Mon Sep 17 00:00:00 2001 From: Marc Sanmiquel Date: Mon, 9 Sep 2024 20:08:07 +0200 Subject: [PATCH] refactor: deprecate AuthToken for non-OG pyroscope cloud (#125) * refactor: deprecate AuthToken for non-OG pyroscope cloud * chore: rewrite log message * test: add test for uploadProfile --- api.go | 4 +- collector_test.go | 29 +++------- go.mod | 13 ++++- go.sum | 20 +++++++ internal/testutil/logging.go | 32 +++++++++++ upstream/remote/remote.go | 18 ++++-- upstream/remote/remote_test.go | 102 +++++++++++++++++++++++++++++++++ 7 files changed, 189 insertions(+), 29 deletions(-) create mode 100644 internal/testutil/logging.go create mode 100644 upstream/remote/remote_test.go diff --git a/api.go b/api.go index 4eb95840..d37f147c 100644 --- a/api.go +++ b/api.go @@ -14,7 +14,6 @@ type Config struct { ApplicationName string // e.g backend.purchases Tags map[string]string ServerAddress string // e.g http://pyroscope.services.internal:4040 - AuthToken string // specify this token when using pyroscope cloud BasicAuthUser string // http basic auth user BasicAuthPassword string // http basic auth password TenantID string // specify TenantId when using phlare multi-tenancy @@ -24,6 +23,9 @@ type Config struct { DisableGCRuns bool // this will disable automatic runtime.GC runs between getting the heap profiles HTTPHeaders map[string]string + // Deprecated: the field will be removed in future releases. + // Use BasicAuthUser and BasicAuthPassword instead. + AuthToken string // specify this token when using pyroscope cloud // Deprecated: the field will be removed in future releases. // Use UploadRate instead. DisableAutomaticResets bool diff --git a/collector_test.go b/collector_test.go index 5b13d418..aa2c8017 100644 --- a/collector_test.go +++ b/collector_test.go @@ -1,18 +1,18 @@ package pyroscope import ( - "fmt" "io" "reflect" "sync" "testing" "time" + "github.com/grafana/pyroscope-go/internal/testutil" "github.com/grafana/pyroscope-go/upstream" ) func Test_StartCPUProfile_interrupts_background_profiling(t *testing.T) { - logger := new(testLogger) + logger := testutil.NewTestLogger() collector := new(mockCollector) c := newCPUProfileCollector( "test", @@ -45,14 +45,14 @@ func Test_StartCPUProfile_interrupts_background_profiling(t *testing.T) { c.Stop() - if !reflect.DeepEqual(logger.lines, []string{ + if !reflect.DeepEqual(logger.Lines(), []string{ "starting cpu profile collector", "cpu profile collector interrupted with StartCPUProfile", "cpu profile collector restored", "stopping cpu profile collector", "stopping cpu profile collector stopped", }) { - for _, line := range logger.lines { + for _, line := range logger.Lines() { t.Log(line) } t.Fatal("^ unexpected even sequence") @@ -60,7 +60,7 @@ func Test_StartCPUProfile_interrupts_background_profiling(t *testing.T) { } func Test_StartCPUProfile_blocks_Stop(t *testing.T) { - logger := new(testLogger) + logger := testutil.NewTestLogger() collector := new(mockCollector) c := newCPUProfileCollector( "test", @@ -86,14 +86,14 @@ func Test_StartCPUProfile_blocks_Stop(t *testing.T) { go c.StopCPUProfile() c.Stop() - if !reflect.DeepEqual(logger.lines, []string{ + if !reflect.DeepEqual(logger.Lines(), []string{ "starting cpu profile collector", "cpu profile collector interrupted with StartCPUProfile", "stopping cpu profile collector", "cpu profile collector restored", "stopping cpu profile collector stopped", }) { - for _, line := range logger.lines { + for _, line := range logger.Lines() { t.Log(line) } t.Fatal("^ unexpected even sequence") @@ -146,18 +146,3 @@ type mockUpstream struct{ uploaded []*upstream.UploadJob } func (m *mockUpstream) Upload(j *upstream.UploadJob) { m.uploaded = append(m.uploaded, j) } func (*mockUpstream) Flush() {} - -type testLogger struct { - sync.Mutex - lines []string -} - -func (t *testLogger) Debugf(format string, args ...interface{}) { t.put(format, args...) } -func (t *testLogger) Infof(format string, args ...interface{}) { t.put(format, args...) } -func (t *testLogger) Errorf(format string, args ...interface{}) { t.put(format, args...) } - -func (t *testLogger) put(format string, args ...interface{}) { - t.Lock() - t.lines = append(t.lines, fmt.Sprintf(format, args...)) - t.Unlock() -} diff --git a/go.mod b/go.mod index 41685acb..a1b5661d 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,15 @@ go 1.17 replace github.com/grafana/pyroscope-go/godeltaprof => ./godeltaprof -require github.com/grafana/pyroscope-go/godeltaprof v0.1.8 +require ( + github.com/grafana/pyroscope-go/godeltaprof v0.1.8 + github.com/stretchr/testify v1.9.0 +) -require github.com/klauspost/compress v1.17.8 // indirect +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/klauspost/compress v1.17.8 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/stretchr/objx v0.5.2 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) diff --git a/go.sum b/go.sum index 734f917e..558a0077 100644 --- a/go.sum +++ b/go.sum @@ -1,2 +1,22 @@ +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/klauspost/compress v1.17.8 h1:YcnTYrq7MikUT7k0Yb5eceMmALQPYBW/Xltxn0NAMnU= github.com/klauspost/compress v1.17.8/go.mod h1:Di0epgTjJY877eYKx5yC51cX2A2Vl2ibi7bDH9ttBbw= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= +github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/testutil/logging.go b/internal/testutil/logging.go new file mode 100644 index 00000000..911707c6 --- /dev/null +++ b/internal/testutil/logging.go @@ -0,0 +1,32 @@ +package testutil + +import ( + "fmt" + "sync" +) + +type TestLogger struct { + sync.Mutex + lines []string +} + +func NewTestLogger() *TestLogger { + return &TestLogger{lines: make([]string, 0)} +} + +func (t *TestLogger) Debugf(format string, args ...interface{}) { t.put(format, args...) } +func (t *TestLogger) Infof(format string, args ...interface{}) { t.put(format, args...) } +func (t *TestLogger) Errorf(format string, args ...interface{}) { t.put(format, args...) } + +func (t *TestLogger) put(format string, args ...interface{}) { + t.Lock() + t.lines = append(t.lines, fmt.Sprintf(format, args...)) + t.Unlock() +} + +// Lines returns a copy of the logged lines +func (t *TestLogger) Lines() []string { + t.Lock() + defer t.Unlock() + return append([]string(nil), t.lines...) +} diff --git a/upstream/remote/remote.go b/upstream/remote/remote.go index 287ff759..dd126315 100644 --- a/upstream/remote/remote.go +++ b/upstream/remote/remote.go @@ -21,7 +21,13 @@ import ( var errCloudTokenRequired = errors.New("please provide an authentication token. You can find it here: https://pyroscope.io/cloud") -const cloudHostnameSuffix = "pyroscope.cloud" +const ( + authTokenDeprecationWarning = "Authtoken is specified, but deprecated and ignored. " + + "Please switch to BasicAuthUser and BasicAuthPassword. " + + "If you need to use Bearer token authentication for a custom setup, " + + "you can use the HTTPHeaders option to set the Authorization header manually." + cloudHostnameSuffix = "pyroscope.cloud" +) type Remote struct { cfg Config @@ -40,6 +46,8 @@ type HTTPClient interface { } type Config struct { + // Deprecated: AuthToken will be removed in future releases. + // Use BasicAuthUser and BasicAuthPassword instead. AuthToken string BasicAuthUser string // http basic auth user BasicAuthPassword string // http basic auth password @@ -90,7 +98,7 @@ func NewRemote(cfg Config) (*Remote, error) { } // authorize the token first - if cfg.AuthToken == "" && requiresAuthToken(u) { + if cfg.AuthToken == "" && isOGPyroscopeCloud(u) { return nil, errCloudTokenRequired } @@ -188,10 +196,12 @@ func (r *Remote) uploadProfile(j *upstream.UploadJob) error { request.Header.Set("Content-Type", contentType) // request.Header.Set("Content-Type", "binary/octet-stream+"+string(j.Format)) - if r.cfg.AuthToken != "" { + if r.cfg.AuthToken != "" && isOGPyroscopeCloud(u) { request.Header.Set("Authorization", "Bearer "+r.cfg.AuthToken) } else if r.cfg.BasicAuthUser != "" && r.cfg.BasicAuthPassword != "" { request.SetBasicAuth(r.cfg.BasicAuthUser, r.cfg.BasicAuthPassword) + } else if r.cfg.AuthToken != "" { + r.logger.Infof(authTokenDeprecationWarning) } if r.cfg.TenantID != "" { request.Header.Set("X-Scope-OrgID", r.cfg.TenantID) @@ -234,7 +244,7 @@ func (r *Remote) handleJobs() { } } -func requiresAuthToken(u *url.URL) bool { +func isOGPyroscopeCloud(u *url.URL) bool { return strings.HasSuffix(u.Host, cloudHostnameSuffix) } diff --git a/upstream/remote/remote_test.go b/upstream/remote/remote_test.go new file mode 100644 index 00000000..f1e74fce --- /dev/null +++ b/upstream/remote/remote_test.go @@ -0,0 +1,102 @@ +package remote + +import ( + "bytes" + "io" + "net/http" + "testing" + "time" + + "github.com/grafana/pyroscope-go/internal/testutil" + "github.com/grafana/pyroscope-go/upstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func TestUploadProfile(t *testing.T) { + tests := []struct { + name string + cfg Config + serverAddress string + expectedAuthHeader string + expectWarning bool + }{ + { + name: "OG Pyroscope Cloud with AuthToken", + cfg: Config{ + AuthToken: "test-token", + Address: "https://example.pyroscope.cloud", + }, + expectedAuthHeader: "Bearer test-token", + expectWarning: false, + }, + { + name: "Non-OG Server with BasicAuth", + cfg: Config{ + BasicAuthUser: "user", + BasicAuthPassword: "pass", + Address: "https://example.com", + }, + expectedAuthHeader: "Basic dXNlcjpwYXNz", // Base64 encoded "user:pass" + expectWarning: false, + }, + { + name: "Non-OG Server with AuthToken (Deprecated)", + cfg: Config{ + AuthToken: "deprecated-token", + Address: "https://example.com", + }, + expectedAuthHeader: "", + expectWarning: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logger := testutil.NewTestLogger() + mockClient := new(MockHTTPClient) + + mockClient.On("Do", mock.Anything).Return(&http.Response{ + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString("OK")), + }, nil) + + r := &Remote{ + cfg: tt.cfg, + client: mockClient, + logger: logger, + } + + err := r.uploadProfile(&upstream.UploadJob{ + Name: "test-profile", + StartTime: time.Now(), + EndTime: time.Now().Add(time.Minute), + SpyName: "test-spy", + SampleRate: 100, + Units: "samples", + }) + assert.NoError(t, err) + + if tt.expectWarning { + assert.Contains(t, logger.Lines(), authTokenDeprecationWarning) + } else { + assert.NotContains(t, logger.Lines(), authTokenDeprecationWarning) + } + + mockClient.AssertCalled(t, "Do", mock.MatchedBy(func(req *http.Request) bool { + return req.Header.Get("Authorization") == tt.expectedAuthHeader + })) + + mockClient.AssertExpectations(t) + }) + } +} + +type MockHTTPClient struct { + mock.Mock +} + +func (m *MockHTTPClient) Do(req *http.Request) (*http.Response, error) { + args := m.Called(req) + return args.Get(0).(*http.Response), args.Error(1) +}