httputil: remove Transport constructors aware of environment

Each of the servers has slightly different needs for configuring their outbound HTTP transport. A little copying is better than a little dependency. Notably, the configuration logic in gddo-server is now consistent with the rest of the configuration-gathering logic. Change-Id: Ibe7d287a102e20e6e13d97e8a6483d9febab6442 Reviewed-on: https://go-review.googlesource.com/69291 Reviewed-by: Tuo Shan <shantuo@google.com>
golang · Oct 13, 2017 · 2fa0678 · 2fa0678
1 parent 7957f9b
commit 2fa0678
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 87 deletions.
diff --git a/gddo-server/client.go b/gddo-server/client.go
@@ -23,7 +23,7 @@ import (
 
 func newHTTPClient(v *viper.Viper) *http.Client {
 	requestTimeout := v.GetDuration(ConfigRequestTimeout)
-	t := &http.Transport{
+	var t http.RoundTripper = &http.Transport{
 		Proxy: http.ProxyFromEnvironment,
 		Dial: (&net.Dialer{
 			Timeout:   v.GetDuration(ConfigDialTimeout),
@@ -32,18 +32,22 @@ func newHTTPClient(v *viper.Viper) *http.Client {
 		ResponseHeaderTimeout: requestTimeout / 2,
 		TLSHandshakeTimeout:   requestTimeout / 2,
 	}
-
-	var rt http.RoundTripper
 	if addr := v.GetString(ConfigMemcacheAddr); addr != "" {
 		ct := httpcache.NewTransport(memcache.New(addr))
 		ct.Transport = t
-		rt = httputil.NewAuthTransport(ct)
-	} else {
-		rt = httputil.NewAuthTransport(t)
+		t = ct
+	}
+	t = &httputil.AuthTransport{
+		Base: t,
+
+		UserAgent:          v.GetString(ConfigUserAgent),
+		GithubToken:        v.GetString(ConfigGithubToken),
+		GithubClientID:     v.GetString(ConfigGithubClientID),
+		GithubClientSecret: v.GetString(ConfigGithubClientSecret),
 	}
+	t = trace.Transport{Base: t}
 	return &http.Client{
-		// Wrap the cached transport with GitHub authentication.
-		Transport: trace.Transport{Base: rt},
+		Transport: t,
 		Timeout:   requestTimeout,
 	}
 }
diff --git a/gddo-server/config.go b/gddo-server/config.go
@@ -18,6 +18,11 @@ import (
 const (
 	gaeProjectEnvVar = "GCLOUD_PROJECT"
 	gaAccountEnvVar  = "GA_ACCOUNT"
+
+	userAgentEnvVar          = "USER_AGENT"
+	githubTokenEnvVar        = "GITHUB_TOKEN"
+	githubClientIDEnvVar     = "GITHUB_CLIENT_ID"
+	githubClientSecretEnvVar = "GITHUB_CLIENT_SECRET"
 )
 
 const (
@@ -54,6 +59,12 @@ const (
 	// Trace Config
 	ConfigTraceSamplerFraction = "trace_fraction"
 	ConfigTraceSamplerMaxQPS   = "trace_max_qps"
+
+	// Outbound HTTP Config
+	ConfigUserAgent          = "user_agent"
+	ConfigGithubToken        = "github_token"
+	ConfigGithubClientID     = "github_client_id"
+	ConfigGithubClientSecret = "github_client_secret"
 )
 
 func loadConfig(ctx context.Context, args []string) (*viper.Viper, error) {
@@ -67,6 +78,10 @@ func loadConfig(ctx context.Context, args []string) (*viper.Viper, error) {
 	if metadata.OnGCE() {
 		gceProjectAttributeDefault(ctx, v, ConfigGAAccount, "ga-account")
 		gceProjectAttributeDefault(ctx, v, ConfigGCELogName, "gce-log-name")
+		gceProjectAttributeDefault(ctx, v, ConfigUserAgent, "user-agent")
+		gceProjectAttributeDefault(ctx, v, ConfigGithubToken, "github-token")
+		gceProjectAttributeDefault(ctx, v, ConfigGithubClientID, "github-client-id")
+		gceProjectAttributeDefault(ctx, v, ConfigGithubClientSecret, "github-client-secret")
 		if id, err := metadata.ProjectID(); err != nil {
 			log.Warn(ctx, "failed to retrieve project ID", "error", err)
 		} else {
@@ -89,6 +104,10 @@ func loadConfig(ctx context.Context, args []string) (*viper.Viper, error) {
 	v.AutomaticEnv()
 	v.BindEnv(ConfigProject, gaeProjectEnvVar)
 	v.BindEnv(ConfigGAAccount, gaAccountEnvVar)
+	v.BindEnv(ConfigUserAgent, userAgentEnvVar)
+	v.BindEnv(ConfigGithubToken, githubTokenEnvVar)
+	v.BindEnv(ConfigGithubClientID, githubClientIDEnvVar)
+	v.BindEnv(ConfigGithubClientSecret, githubClientSecretEnvVar)
 
 	// Read from config.
 	if err := readViperConfig(ctx, v); err != nil {

diff --git a/gosrc/print.go b/gosrc/print.go
@@ -16,6 +16,7 @@ import (
 	"fmt"
 	"log"
 	"net/http"
+	"os"
 	"strings"
 
 	"github.com/golang/gddo/gosrc"
@@ -45,7 +46,13 @@ func printDir(path string) {
 		gosrc.SetLocalDevMode(*local)
 	}
 	c := &http.Client{
-		Transport: httputil.NewAuthTransport(&http.Transport{}),
+		Transport: &httputil.AuthTransport{
+			Base:               http.DefaultTransport,
+			UserAgent:          os.Getenv("USER_AGENT"),
+			GithubToken:        os.Getenv("GITHUB_TOKEN"),
+			GithubClientID:     os.Getenv("GITHUB_CLIENT_ID"),
+			GithubClientSecret: os.Getenv("GITHUB_CLIENT_SECRET"),
+		},
 	}
 	dir, err := gosrc.Get(c, path, *etag)
 	if e, ok := err.(gosrc.NotFoundError); ok && e.Redirect != "" {

diff --git a/httputil/transport.go b/httputil/transport.go
@@ -10,58 +10,20 @@
 package httputil
 
 import (
-	"log"
 	"net/http"
 	"net/url"
-	"os"
-
-	"cloud.google.com/go/compute/metadata"
 )
 
 // AuthTransport is an implementation of http.RoundTripper that authenticates
 // with the GitHub API.
 //
 // When both a token and client credentials are set, the latter is preferred.
 type AuthTransport struct {
-	UserAgent    string
-	Token        string
-	ClientID     string
-	ClientSecret string
-	Base         http.RoundTripper
-}
-
-// NewAuthTransport gives new AuthTransport created with GitHub credentials
-// read from GCE metadata when the metadata server is accessible (we're on GCE)
-// or read from environment varialbes otherwise.
-func NewAuthTransport(base http.RoundTripper) *AuthTransport {
-	if metadata.OnGCE() {
-		return NewAuthTransportFromMetadata(base)
-	}
-	return NewAuthTransportFromEnvironment(base)
-}
-
-// NewAuthTransportFromEnvironment gives new AuthTransport created with GitHub
-// credentials read from environment variables.
-func NewAuthTransportFromEnvironment(base http.RoundTripper) *AuthTransport {
-	return &AuthTransport{
-		UserAgent:    os.Getenv("USER_AGENT"),
-		Token:        os.Getenv("GITHUB_TOKEN"),
-		ClientID:     os.Getenv("GITHUB_CLIENT_ID"),
-		ClientSecret: os.Getenv("GITHUB_CLIENT_SECRET"),
-		Base:         base,
-	}
-}
-
-// NewAuthTransportFromMetadata gives new AuthTransport created with GitHub
-// credentials read from GCE metadata.
-func NewAuthTransportFromMetadata(base http.RoundTripper) *AuthTransport {
-	return &AuthTransport{
-		UserAgent:    gceAttr("user-agent"),
-		Token:        gceAttr("github-token"),
-		ClientID:     gceAttr("github-client-id"),
-		ClientSecret: gceAttr("github-client-secret"),
-		Base:         base,
-	}
+	UserAgent          string
+	GithubToken        string
+	GithubClientID     string
+	GithubClientSecret string
+	Base               http.RoundTripper
 }
 
 // RoundTrip implements the http.RoundTripper interface.
@@ -73,20 +35,20 @@ func (t *AuthTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	}
 	if req.URL.Host == "api.github.com" && req.URL.Scheme == "https" {
 		switch {
-		case t.ClientID != "" && t.ClientSecret != "":
+		case t.GithubClientID != "" && t.GithubClientSecret != "":
 			if reqCopy == nil {
 				reqCopy = copyRequest(req)
 			}
 			if reqCopy.URL.RawQuery == "" {
-				reqCopy.URL.RawQuery = "client_id=" + t.ClientID + "&client_secret=" + t.ClientSecret
+				reqCopy.URL.RawQuery = "client_id=" + t.GithubClientID + "&client_secret=" + t.GithubClientSecret
 			} else {
-				reqCopy.URL.RawQuery += "&client_id=" + t.ClientID + "&client_secret=" + t.ClientSecret
+				reqCopy.URL.RawQuery += "&client_id=" + t.GithubClientID + "&client_secret=" + t.GithubClientSecret
 			}
-		case t.Token != "":
+		case t.GithubToken != "":
 			if reqCopy == nil {
 				reqCopy = copyRequest(req)
 			}
-			reqCopy.Header.Set("Authorization", "token "+t.Token)
+			reqCopy.Header.Set("Authorization", "token "+t.GithubToken)
 		}
 	}
 	if reqCopy != nil {
@@ -112,15 +74,6 @@ func (t *AuthTransport) base() http.RoundTripper {
 	return http.DefaultTransport
 }
 
-func gceAttr(name string) string {
-	s, err := metadata.ProjectAttributeValue(name)
-	if err != nil {
-		log.Printf("error querying metadata for %q: %s", name, err)
-		return ""
-	}
-	return s
-}
-
 func copyRequest(req *http.Request) *http.Request {
 	req2 := new(http.Request)
 	*req2 = *req

diff --git a/httputil/transport_test.go b/httputil/transport_test.go
@@ -87,9 +87,9 @@ func TestTransportGithubAuth(t *testing.T) {
 						query = r.URL.Query()
 						authHeader = r.Header.Get("Authorization")
 					}),
-					Token:        test.token,
-					ClientID:     test.clientID,
-					ClientSecret: test.clientSecret,
+					GithubToken:        test.token,
+					GithubClientID:     test.clientID,
+					GithubClientSecret: test.clientSecret,
 				},
 			}
 			_, err := client.Get(test.url)

diff --git a/lintapp/main.go b/lintapp/main.go
@@ -49,7 +49,6 @@ var (
 		"timeago":      timeagoFn,
 		"contactEmail": contactEmailFn,
 	}
-	github = httputil.NewAuthTransportFromEnvironment(nil)
 )
 
 func parseTemplate(fnames ...string) *template.Template {
@@ -114,11 +113,11 @@ func httpClient(r *http.Request) *http.Client {
 	c := appengine.NewContext(r)
 	return &http.Client{
 		Transport: &httputil.AuthTransport{
-			Token:        github.Token,
-			ClientID:     github.ClientID,
-			ClientSecret: github.ClientSecret,
-			Base:         &urlfetch.Transport{Context: c},
-			UserAgent:    fmt.Sprintf("%s (+http://%s/-/bot)", appengine.AppID(c), r.Host),
+			GithubToken:        os.Getenv("GITHUB_TOKEN"),
+			GithubClientID:     os.Getenv("GITHUB_CLIENT_ID"),
+			GithubClientSecret: os.Getenv("GITHUB_CLIENT_SECRET"),
+			Base:               &urlfetch.Transport{Context: c},
+			UserAgent:          fmt.Sprintf("%s (+http://%s/-/bot)", appengine.AppID(c), r.Host),
 		},
 	}
 }

diff --git a/talksapp/main.go b/talksapp/main.go
@@ -41,6 +41,10 @@ var (
 	// used for mocking in tests
 	getPresentation = gosrc.GetPresentation
 	playCompileURL  = "https://play.golang.org/compile"
+
+	githubToken        = os.Getenv("GITHUB_TOKEN")
+	githubClientID     = os.Getenv("GITHUB_CLIENT_ID")
+	githubClientSecret = os.Getenv("GITHUB_CLIENT_SECRET")
 )
 
 func init() {
@@ -52,12 +56,13 @@ func init() {
 		contactEmail = s
 	}
 
-	if appengine.IsDevAppServer() {
-		return
-	}
-	github := httputil.NewAuthTransportFromEnvironment(nil)
-	if github.Token == "" || github.ClientID == "" || github.ClientSecret == "" {
-		panic("missing GitHub metadata, follow the instructions on README.md")
+	if !appengine.IsDevAppServer() {
+		if githubToken == "" && githubClientID == "" && githubClientSecret == "" {
+			panic("missing GitHub metadata, follow the instructions on README.md")
+		}
+		if githubToken != "" && (githubClientID != "" || githubClientSecret != "") {
+			panic("GitHub token and client secret given, follow the instructions on README.md")
+		}
 	}
 }
 
@@ -126,15 +131,14 @@ func writeTextHeader(w http.ResponseWriter, status int) {
 
 func httpClient(r *http.Request) *http.Client {
 	ctx, _ := context.WithTimeout(appengine.NewContext(r), 10*time.Second)
-	github := httputil.NewAuthTransportFromEnvironment(nil)
 
 	return &http.Client{
 		Transport: &httputil.AuthTransport{
-			Token:        github.Token,
-			ClientID:     github.ClientID,
-			ClientSecret: github.ClientSecret,
-			Base:         &urlfetch.Transport{Context: ctx},
-			UserAgent:    fmt.Sprintf("%s (+http://%s/-/bot)", appengine.AppID(ctx), r.Host),
+			GithubToken:        githubToken,
+			GithubClientID:     githubClientID,
+			GithubClientSecret: githubClientSecret,
+			Base:               &urlfetch.Transport{Context: ctx},
+			UserAgent:          fmt.Sprintf("%s (+http://%s/-/bot)", appengine.AppID(ctx), r.Host),
 		},
 	}
 }