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

@@ -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

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,22 +45,22 @@ 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")
 	}
 }
 
 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()
-}

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
+)

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=

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...)
+}

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)
 }
 

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)
+}