logpolicy: fix log target override with a custom HTTP client

This makes sure that the log target override is respected even if a
custom HTTP client is passed to logpolicy.

Updates tailscale/maple#29

Signed-off-by: Anton Tolchanov <anton@tailscale.com>

logpolicy/logpolicy.go

@@ -518,8 +518,9 @@ type Options struct {
 	MaxUploadSize int
 }
 
-// New returns a new log policy (a logger and its instance ID).
+// init initializes the log policy and returns a logtail.Config and the
-func (opts Options) New() *Policy {
+// Policy.
+func (opts Options) init(disableLogging bool) (*logtail.Config, *Policy) {
 	if hostinfo.IsNATLabGuestVM() {
 		// In NATLab Gokrazy instances, tailscaled comes up concurently with
 		// DHCP and the doesn't have DNS for a while. Wait for DHCP first.
@@ -628,7 +629,7 @@ func (opts Options) New() *Policy {
 		conf.IncludeProcSequence = true
 	}
 
-	if envknob.NoLogsNoSupport() || testenv.InTest() || runtime.GOOS == "plan9" {
+	if disableLogging {
 		opts.Logf("You have disabled logging. Tailscale will not be able to provide support.")
 		conf.HTTPC = &http.Client{Transport: noopPretendSuccessTransport{}}
 	} else {
@@ -637,14 +638,15 @@ func (opts Options) New() *Policy {
 		attachFilchBuffer(&conf, opts.Dir, opts.CmdName, opts.MaxBufferSize, opts.Logf)
 		conf.HTTPC = opts.HTTPC
 
+		logHost := logtail.DefaultHost
+		if val := getLogTarget(); val != "" {
+			opts.Logf("You have enabled a non-default log target. Doing without being told to by Tailscale staff or your network administrator will make getting support difficult.")
+			conf.BaseURL = val
+			u, _ := url.Parse(val)
+			logHost = u.Host
+		}
+
 		if conf.HTTPC == nil {
-			logHost := logtail.DefaultHost
-			if val := getLogTarget(); val != "" {
-				opts.Logf("You have enabled a non-default log target. Doing without being told to by Tailscale staff or your network administrator will make getting support difficult.")
-				conf.BaseURL = val
-				u, _ := url.Parse(val)
-				logHost = u.Host
-			}
 			conf.HTTPC = &http.Client{Transport: TransportOptions{
 				Host:   logHost,
 				NetMon: opts.NetMon,
@@ -680,13 +682,20 @@ func (opts Options) New() *Policy {
 		opts.Logf("%s", earlyErrBuf.Bytes())
 	}
 
-	return &Policy{
+	return &conf, &Policy{
 		Logtail:  lw,
 		PublicID: newc.PublicID,
 		Logf:     opts.Logf,
 	}
 }
 
+// New returns a new log policy (a logger and its instance ID).
+func (opts Options) New() *Policy {
+	disableLogging := envknob.NoLogsNoSupport() || testenv.InTest() || runtime.GOOS == "plan9"
+	_, policy := opts.init(disableLogging)
+	return policy
+}
+
 // attachFilchBuffer creates an on-disk ring buffer using filch and attaches
 // it to the logtail config. Note that this is optional; if no buffer is set,
 // logtail will use an in-memory buffer.

logpolicy/logpolicy_test.go

@@ -4,6 +4,7 @@
 package logpolicy
 
 import (
+	"net/http"
 	"os"
 	"reflect"
 	"testing"
@@ -11,12 +12,14 @@ import (
 	"tailscale.com/logtail"
 )
 
-func TestLogHost(t *testing.T) {
+func resetLogTarget() {
+	os.Unsetenv("TS_LOG_TARGET")
 	v := reflect.ValueOf(&getLogTargetOnce).Elem()
-	reset := func() {
+	v.Set(reflect.Zero(v.Type()))
-		v.Set(reflect.Zero(v.Type()))
+}
-	}
+
-	defer reset()
+func TestLogHost(t *testing.T) {
+	defer resetLogTarget()
 
 	tests := []struct {
 		env  string
@@ -29,10 +32,55 @@ func TestLogHost(t *testing.T) {
 		{"https://foo.com:123/", "foo.com"},
 	}
 	for _, tt := range tests {
-		reset()
+		resetLogTarget()
 		os.Setenv("TS_LOG_TARGET", tt.env)
 		if got := LogHost(); got != tt.want {
 			t.Errorf("for env %q, got %q, want %q", tt.env, got, tt.want)
 		}
 	}
 }
+func TestOptions(t *testing.T) {
+	defer resetLogTarget()
+
+	tests := []struct {
+		name        string
+		opts        func() Options
+		wantBaseURL string
+	}{
+		{
+			name:        "default",
+			opts:        func() Options { return Options{} },
+			wantBaseURL: "",
+		},
+		{
+			name: "custom_baseurl",
+			opts: func() Options {
+				os.Setenv("TS_LOG_TARGET", "http://localhost:1234")
+				return Options{}
+			},
+			wantBaseURL: "http://localhost:1234",
+		},
+		{
+			name: "custom_httpc_and_baseurl",
+			opts: func() Options {
+				os.Setenv("TS_LOG_TARGET", "http://localhost:12345")
+				return Options{HTTPC: &http.Client{Transport: noopPretendSuccessTransport{}}}
+			},
+			wantBaseURL: "http://localhost:12345",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			resetLogTarget()
+			config, policy := tt.opts().init(false)
+			if policy == nil {
+				t.Fatal("unexpected nil policy")
+			}
+			if config.BaseURL != tt.wantBaseURL {
+				t.Errorf("got %q, want %q", config.BaseURL, tt.wantBaseURL)
+			}
+			policy.Close()
+		})
+	}
+}