Repository: tetratelabs/getenvoy
Pull request #257 (2021-06-02 06:57:04)

Buckets user-agent only to release or non-release without an internal flag
This also captures that netlify won't help us know which user-agents are in use without custom edge handlers.


Patch:
diff --git a/internal/cmd/app.go b/internal/cmd/app.go
index 3588460..3afd88c 100644
--- a/internal/cmd/app.go
+++ b/internal/cmd/app.go
@@ -28,7 +28,7 @@ import (
 // NewApp create a new root command. The globals.GlobalOpts parameter allows tests to scope overrides, which avoids
 // having to define a flag for everything needed in tests.
 func NewApp(o *globals.GlobalOpts) *cli.App {
-	var homeDir, envoyVersionsURL, userAgent string
+	var homeDir, envoyVersionsURL string
 
 	app := cli.NewApp()
 	app.Name = "getenvoy"
@@ -49,15 +49,8 @@ func NewApp(o *globals.GlobalOpts) *cli.App {
 			DefaultText: globals.DefaultEnvoyVersionsURL,
 			Destination: &envoyVersionsURL,
 			EnvVars:     []string{"ENVOY_VERSIONS_URL"},
-		},
-		&cli.StringFlag{
-			Name:        "internal-user-agent", // Hidden, but settable for release e2e tests
-			Hidden:      true,                  // Hidden, but settable for release e2e tests
-			Value:       globals.DefaultUserAgent,
-			Destination: &userAgent,
 		}}
 	app.Before = func(c *cli.Context) error {
-		o.UserAgent = userAgent
 		if err := setHomeDir(o, homeDir); err != nil {
 			return err
 		}
diff --git a/internal/cmd/run.go b/internal/cmd/run.go
index 430e949..9d5ceff 100644
--- a/internal/cmd/run.go
+++ b/internal/cmd/run.go
@@ -26,6 +26,7 @@ import (
 	"github.com/tetratelabs/getenvoy/internal/envoy"
 	"github.com/tetratelabs/getenvoy/internal/envoy/debug"
 	"github.com/tetratelabs/getenvoy/internal/globals"
+	"github.com/tetratelabs/getenvoy/internal/version"
 )
 
 // NewRunCmd create a command responsible for starting an Envoy process
@@ -87,10 +88,10 @@ $ getenvoy run -c ./bootstrap.yaml`, envoy.VersionUsageList()),
 // initializeRunOpts allows us to default values when not overridden for tests.
 // The version parameter correlates with the globals.GlobalOpts EnvoyPath which is installed if needed.
 // Notably, this creates and sets a globals.GlobalOpts WorkingDirectory for Envoy, and any files that precede it.
-func initializeRunOpts(o *globals.GlobalOpts, platform, version string) error {
+func initializeRunOpts(o *globals.GlobalOpts, p, v string) error {
 	runOpts := &o.RunOpts
 	if o.EnvoyPath == "" { // not overridden for tests
-		envoyPath, err := envoy.InstallIfNeeded(o, platform, version)
+		envoyPath, err := envoy.InstallIfNeeded(o, p, v)
 		if err != nil {
 			return err
 		}
@@ -121,7 +122,7 @@ func setHomeEnvoyVersion(o *globals.GlobalOpts) error {
 
 	// First time install: look up the latest version, which may be newer than version.LastKnownEnvoy!
 	fmt.Fprintln(o.Out, "looking up latest version") //nolint
-	m, err := envoy.GetEnvoyVersions(o.EnvoyVersionsURL, o.UserAgent)
+	m, err := envoy.GetEnvoyVersions(o.EnvoyVersionsURL, globals.CurrentPlatform, version.GetEnvoy)
 	if err != nil {
 		return NewValidationError(`couldn't read latest version from %s: %s`, o.EnvoyVersionsURL, err)
 	}
diff --git a/internal/cmd/versions.go b/internal/cmd/versions.go
index 7653106..3146474 100644
--- a/internal/cmd/versions.go
+++ b/internal/cmd/versions.go
@@ -50,7 +50,7 @@ func NewVersionsCmd(o *globals.GlobalOpts) *cli.Command {
 			currentVersion, currentVersionSource, _ := envoy.CurrentVersion(o.HomeDir)
 
 			if c.Bool("all") {
-				if ev, err := envoy.GetEnvoyVersions(o.EnvoyVersionsURL, o.UserAgent); err != nil {
+				if ev, err := envoy.GetEnvoyVersions(o.EnvoyVersionsURL, globals.CurrentPlatform, version.GetEnvoy); err != nil {
 					return err
 				} else if err := addAvailableVersions(&rows, ev.Versions, globals.CurrentPlatform); err != nil {
 					return err
diff --git a/internal/envoy/http.go b/internal/envoy/http.go
index b2dd81b..a0b0b53 100644
--- a/internal/envoy/http.go
+++ b/internal/envoy/http.go
@@ -15,16 +15,32 @@
 package envoy
 
 import (
+	"fmt"
 	"net/http"
+	"strings"
 )
 
-// httpGet adds the "User-Agent" header to the request, so that we can tell what is a dev build vs release.
-func httpGet(url, userAgent string) (*http.Response, error) {
+// httpGet adds the userAgent header to the request, so that we can tell what is a dev build vs release.
+func httpGet(url, platform, version string) (*http.Response, error) {
 	// #nosec -> url can be anywhere by design
 	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
 		return nil, err
 	}
-	req.Header.Add("User-Agent", userAgent)
+	req.Header.Add("User-Agent", userAgent(platform, version))
 	return http.DefaultClient.Do(req)
 }
+
+// userAgent returns the 'User-Agent' header value used in HTTP requests. This is useful in log, metrics, analytics, or
+// request filtering. As this is a CLI, the best 'User-Agent' is the binary version including platform.
+//
+// The returned value limits cardinality to formal release * platform or one value for all non-releases.
+//
+// Note: Analytics may not work out-of-box. For example, Netlify does not support server-side analytics on 'User-Agent',
+// and even its 'Referer' analytics are limited to requests to HTML resources.
+func userAgent(platform, version string) string {
+	if !strings.HasPrefix(version, "v") || strings.Contains(version, "SNAPSHOT") {
+		return "getenvoy/dev"
+	}
+	return fmt.Sprintf("getenvoy/%s (%s)", version, platform)
+}
diff --git a/internal/envoy/install.go b/internal/envoy/install.go
index 1d40d49..2aa67c9 100644
--- a/internal/envoy/install.go
+++ b/internal/envoy/install.go
@@ -36,7 +36,7 @@ func InstallIfNeeded(o *globals.GlobalOpts, p, v string) (string, error) {
 	switch {
 	case os.IsNotExist(err):
 		var ev version.EnvoyVersions // Get version metadata for what we will install
-		ev, err = GetEnvoyVersions(o.EnvoyVersionsURL, o.UserAgent)
+		ev, err = GetEnvoyVersions(o.EnvoyVersionsURL, p, v)
 		if err != nil {
 			return "", err
 		}
@@ -54,8 +54,8 @@ func InstallIfNeeded(o *globals.GlobalOpts, p, v string) (string, error) {
 			return "", fmt.Errorf("unable to create directory %q: %w", installPath, err)
 		}
 
-		fmt.Fprintln(o.Out, "downloading", tarballURL) //nolint
-		if err = untarEnvoy(installPath, tarballURL, o.UserAgent); err != nil {
+		fmt.Fprintln(o.Out, "downloading", tarballURL)                   //nolint
+		if err = untarEnvoy(installPath, tarballURL, p, v); err != nil { //nolint
 			return "", err
 		}
 		if err = os.Chtimes(installPath, mtime, mtime); err != nil { // overwrite the mtime to preserve it in the list
@@ -82,8 +82,8 @@ func verifyEnvoy(installPath string) (string, error) {
 	return envoyPath, nil
 }
 
-func untarEnvoy(dst, url, userAgent string) error { // dst, src order like io.Copy
-	resp, err := httpGet(url, userAgent)
+func untarEnvoy(dst, url, p, v string) error { // dst, src order like io.Copy
+	resp, err := httpGet(url, p, v)
 	if err != nil {
 		return err
 	}
diff --git a/internal/envoy/versions.go b/internal/envoy/versions.go
index 747c2cf..8ccb840 100644
--- a/internal/envoy/versions.go
+++ b/internal/envoy/versions.go
@@ -24,10 +24,10 @@ import (
 )
 
 // GetEnvoyVersions returns a version map from a remote URL. eg globals.DefaultEnvoyVersionsURL.
-func GetEnvoyVersions(envoyVersionsURL, userAgent string) (version.EnvoyVersions, error) {
+func GetEnvoyVersions(envoyVersionsURL, p, v string) (version.EnvoyVersions, error) {
 	result := version.EnvoyVersions{}
 	// #nosec => This is by design, users can call out to wherever they like!
-	resp, err := httpGet(envoyVersionsURL, userAgent)
+	resp, err := httpGet(envoyVersionsURL, p, v)
 	if err != nil {
 		return result, err
 	}
diff --git a/internal/globals/globals.go b/internal/globals/globals.go
index 6ac5b34..38b8bec 100644
--- a/internal/globals/globals.go
+++ b/internal/globals/globals.go
@@ -15,12 +15,9 @@
 package globals
 
 import (
-	"fmt"
 	"io"
 	"regexp"
 	"runtime"
-
-	"github.com/tetratelabs/getenvoy/internal/version"
 )
 
 // RunOpts support invocations of "getenvoy run"
@@ -52,8 +49,6 @@ type GlobalOpts struct {
 	EnvoyVersion string
 	// HomeDir is an absolute path which most importantly contains "versions" installed from EnvoyVersionsURL. Defaults to DefaultHomeDir
 	HomeDir string
-	// UserAgent is the "User-Agent" header added to all HTTP requests. Defaults to DefaultUserAgent
-	UserAgent string
 	// Out is where status messages are written. Defaults to os.Stdout
 	Out io.Writer
 }
@@ -70,8 +65,4 @@ var (
 	EnvoyVersionPattern = regexp.MustCompile(`^[1-9][0-9]*\.[0-9]+\.[0-9]+$`)
 	// CurrentPlatform is the platform of the current process. This is used as a key in EnvoyVersion.Tarballs.
 	CurrentPlatform = runtime.GOOS + "/" + runtime.GOARCH
-	// DefaultUserAgent is the default value for GlobalOpts.UserAgent.
-	// This includes the platform to help differentiate installations in site analytics.
-	// This doesn't normalize the platform value like browsers: we use this to track usage of CurrentPlatform.
-	DefaultUserAgent = fmt.Sprintf("GetEnvoy/%s (%s)", version.GetEnvoy, CurrentPlatform)
 )


Test patch:
diff --git a/e2e/command_test.go b/e2e/command_test.go
index 5449b2b..1a54352 100644
--- a/e2e/command_test.go
+++ b/e2e/command_test.go
@@ -31,17 +31,12 @@ import (
 // getEnvoyPath holds a path to a 'getenvoy' binary under test.
 var getEnvoyPath = "getenvoy"
 
-// cmdBuilder represents a command builder.
+// cmdBuilder's main function is providing a nice String
 type cmdBuilder struct {
 	cmd *exec.Cmd
 }
 
-// getEnvoy returns a new command builder.
-// Hard-code "User-Agent" HTTP header so that it doesn't interfere with site analytics.
 func getEnvoy(args ...string) *cmdBuilder { //nolint:golint
-	if args[0] != "--version" {
-		args = append([]string{"--internal-user-agent", userAgent}, args...)
-	}
 	return &cmdBuilder{exec.Command(getEnvoyPath, args...)} //nolint:gosec
 }
 
diff --git a/e2e/main_test.go b/e2e/main_test.go
index af82444..8b20278 100644
--- a/e2e/main_test.go
+++ b/e2e/main_test.go
@@ -31,9 +31,10 @@ const (
 	getenvoyBinaryEnvKey   = "E2E_GETENVOY_BINARY"
 	envoyVersionsURLEnvKey = "ENVOY_VERSIONS_URL"
 	envoyVersionsJSON      = "../site/envoy-versions.json"
-	userAgent              = "GetEnvoy/dev"
 )
 
+var expectedMockHeaders = map[string]string{"User-Agent": "getenvoy/dev"}
+
 // TestMain ensures the "getenvoy" binary is valid.
 func TestMain(m *testing.M) {
 	// As this is an e2e test, we execute all tests with a binary compiled earlier.
@@ -80,10 +81,13 @@ func mockEnvoyVersionsServer() (*httptest.Server, error) {
 
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		// Ensure e2e tests won't eventually interfere with analytics when run against a release version
-		if r.UserAgent() != userAgent {
-			w.WriteHeader(500)
-			w.Write([]byte(fmt.Sprintln(`invalid "User-Agent":`, r.UserAgent()))) //nolint
-			return
+		for k, v := range expectedMockHeaders {
+			h := r.Header.Get(k)
+			if h != v {
+				w.WriteHeader(500)
+				w.Write([]byte(fmt.Sprintf(`invalid %q: %s != %s\n`, k, h, v))) //nolint
+				return
+			}
 		}
 		w.Header().Add("Content-Type", "application/json")
 		w.WriteHeader(200)
diff --git a/internal/cmd/app_test.go b/internal/cmd/app_test.go
index 530a439..2f7aebb 100644
--- a/internal/cmd/app_test.go
+++ b/internal/cmd/app_test.go
@@ -170,40 +170,6 @@ func TestEnvoyVersionsURL(t *testing.T) {
 	}
 }
 
-// TestGetEnvoyUserAgent tests we can re-assign the user-agent, even if this ability is internal.
-func TestGetEnvoyUserAgent(t *testing.T) {
-	type testCase struct {
-		name     string
-		args     []string
-		expected string
-	}
-
-	tests := []testCase{ // we don't test default as that depends on the runtime env
-		{
-			name:     `default is "GetEnvoy/$version ($platform)"`,
-			args:     []string{"getenvoy"},
-			expected: globals.DefaultUserAgent,
-		},
-		{
-			name:     "--internal-user-agent",
-			args:     []string{"getenvoy", "--internal-user-agent", "GetEnvoy/dev"},
-			expected: "GetEnvoy/dev",
-		},
-	}
-
-	for _, tc := range tests {
-		tc := tc // pin! see https://github.com/kyoh86/scopelint for why
-
-		t.Run(tc.name, func(t *testing.T) {
-			o := &globals.GlobalOpts{}
-			err := runTestCommand(t, o, tc.args)
-
-			require.NoError(t, err)
-			require.Equal(t, tc.expected, o.UserAgent)
-		})
-	}
-}
-
 // newApp initializes a command with buffers for stdout and stderr.
 func newApp(o *globals.GlobalOpts) (c *cli.App, stdout, stderr *bytes.Buffer) {
 	stdout = new(bytes.Buffer)
diff --git a/internal/envoy/http_test.go b/internal/envoy/http_test.go
index 276c60c..5c45db0 100644
--- a/internal/envoy/http_test.go
+++ b/internal/envoy/http_test.go
@@ -20,17 +20,20 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-)
 
-func TestHttpGet_AddsUserAgent(t *testing.T) {
-	userAgent := "foo"
+	"github.com/tetratelabs/getenvoy/internal/globals"
+	"github.com/tetratelabs/getenvoy/internal/version"
+)
 
+func TestHttpGet_AddsDefaultHeaders(t *testing.T) {
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		require.Equal(t, userAgent, r.UserAgent())
+		for k, v := range map[string]string{"User-Agent": "getenvoy/dev"} {
+			require.Equal(t, v, r.Header.Get(k))
+		}
 	}))
 	defer ts.Close()
 
-	res, err := httpGet(ts.URL, userAgent)
+	res, err := httpGet(ts.URL, globals.CurrentPlatform, version.GetEnvoy)
 	require.NoError(t, err)
 
 	defer res.Body.Close()
diff --git a/internal/envoy/install_test.go b/internal/envoy/install_test.go
index 2d49d34..5fba8b5 100644
--- a/internal/envoy/install_test.go
+++ b/internal/envoy/install_test.go
@@ -35,7 +35,6 @@ import (
 )
 
 func TestUntarEnvoyError(t *testing.T) {
-	userAgent := "test"
 	tempDir, removeTempDir := morerequire.RequireNewTempDir(t)
 	dst := filepath.Join(tempDir, "dst")
 	defer removeTempDir()
@@ -52,7 +51,7 @@ func TestUntarEnvoyError(t *testing.T) {
 
 	url := server.URL + "/file.tar.gz"
 	t.Run("error on incorrect URL", func(t *testing.T) {
-		err := untarEnvoy(dst, url, userAgent)
+		err := untarEnvoy(dst, url, globals.CurrentPlatform, version.GetEnvoy)
 		require.EqualError(t, err, fmt.Sprintf(`received 404 status code from %s`, url))
 	})
 
@@ -60,7 +59,7 @@ func TestUntarEnvoyError(t *testing.T) {
 		w.WriteHeader(200)
 	}
 	t.Run("error on empty", func(t *testing.T) {
-		err := untarEnvoy(dst, url, userAgent)
+		err := untarEnvoy(dst, url, globals.CurrentPlatform, version.GetEnvoy)
 		require.EqualError(t, err, fmt.Sprintf(`error untarring %s: EOF`, url))
 	})
 
@@ -69,7 +68,7 @@ func TestUntarEnvoyError(t *testing.T) {
 		w.Write([]byte("mary had a little lamb")) //nolint
 	}
 	t.Run("error on not a tar", func(t *testing.T) {
-		err := untarEnvoy(dst, url, userAgent)
+		err := untarEnvoy(dst, url, globals.CurrentPlatform, version.GetEnvoy)
 		require.EqualError(t, err, fmt.Sprintf(`error untarring %s: gzip: invalid header`, url))
 	})
 }
@@ -79,7 +78,7 @@ func TestUntarEnvoy(t *testing.T) {
 	o, cleanup := setupInstallTest(t)
 	defer cleanup()
 
-	err := untarEnvoy(o.tempDir, o.tarballURL, o.UserAgent)
+	err := untarEnvoy(o.tempDir, o.tarballURL, globals.CurrentPlatform, version.GetEnvoy)
 	require.NoError(t, err)
 	require.FileExists(t, filepath.Join(o.tempDir, binEnvoy))
 }
@@ -228,7 +227,6 @@ func setupInstallTest(t *testing.T) (*installTest, func()) {
 			GlobalOpts: globals.GlobalOpts{
 				HomeDir:          tempDir,
 				EnvoyVersionsURL: versionsServer.URL + "/envoy-versions.json",
-				UserAgent:        globals.DefaultUserAgent,
 				Out:              new(bytes.Buffer),
 				RunOpts: globals.RunOpts{
 					EnvoyPath: filepath.Join(tempDir, "versions", version.LastKnownEnvoy, "bin", "envoy"),
diff --git a/internal/test/server.go b/internal/test/server.go
index 0b5dd25..3c0fa77 100644
--- a/internal/test/server.go
+++ b/internal/test/server.go
@@ -28,7 +28,6 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	"github.com/tetratelabs/getenvoy/internal/globals"
 	"github.com/tetratelabs/getenvoy/internal/tar"
 	"github.com/tetratelabs/getenvoy/internal/test/morerequire"
 	"github.com/tetratelabs/getenvoy/internal/version"
@@ -76,7 +75,6 @@ type server struct {
 func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	switch {
 	case r.RequestURI == "/envoy-versions.json":
-		require.Equal(s.t, globals.DefaultUserAgent, r.UserAgent())
 		w.WriteHeader(http.StatusOK)
 		_, err := w.Write(s.getEnvoyVersions())
 		require.NoError(s.t, err)