Merge pull request #208 from stripe/xieyuxi-configurable-proxy-addrs

Configurable http and https proxy addrs

.github/workflows/test.yml

@@ -1,5 +1,9 @@
 on: [push, pull_request]
 name: Test
+# Workaround for coveralls error "Can't add a job to a build that is already closed"
+# See https://github.com/lemurheavy/coveralls-public/issues/1716
+env:
+  COVERALLS_SERVICE_NUMBER: ${{ github.run_id }}-${{ github.run_attempt }}
 jobs:
   test:
     strategy:
@@ -23,6 +27,7 @@ jobs:
         go test -race -v -timeout 2m -failfast -covermode atomic -coverprofile=.covprofile ./... -tags=nointegration
         # Run integration tests hermetically to avoid nondeterministic races on environment variables
         go test -race -v -timeout 2m -failfast ./cmd/... -run TestSmokescreenIntegration
+        go test -race -v -timeout 2m -failfast ./cmd/... -run TestInvalidUpstreamProxyConfiguratedFromEnv
         go test -race -v -timeout 2m -failfast ./cmd/... -run TestInvalidUpstreamProxyConfiguration
         go test -race -v -timeout 2m -failfast ./cmd/... -run TestClientHalfCloseConnection
     - name: Install goveralls

cmd/integration_test.go

@@ -279,7 +279,7 @@ func TestSmokescreenIntegration(t *testing.T) {
 
 	for _, useTLS := range []bool{true, false} {
 		// Smokescreen instances
-		_, proxyServer, err := startSmokescreen(t, useTLS, &logHook)
+		_, proxyServer, err := startSmokescreen(t, useTLS, &logHook, "")
 		require.NoError(t, err)
 		defer proxyServer.Close()
 		proxyServers[useTLS] = proxyServer
@@ -449,13 +449,13 @@ func validateProxyResponseWithUpstream(t *testing.T, test *TestCase, resp *http.
 
 // This test must be run with a separate test command as the environment variables
 // required can race with the test above.
-func TestInvalidUpstreamProxyConfiguration(t *testing.T) {
+func TestInvalidUpstreamProxyConfiguratedFromEnv(t *testing.T) {
 	var logHook logrustest.Hook
 	servers := map[bool]*httptest.Server{}
 
 	// Create TLS and non-TLS instances of Smokescreen
 	for _, useTLS := range []bool{true, false} {
-		_, server, err := startSmokescreen(t, useTLS, &logHook)
+		_, server, err := startSmokescreen(t, useTLS, &logHook, "")
 		require.NoError(t, err)
 		defer server.Close()
 		servers[useTLS] = server
@@ -500,6 +500,58 @@ func TestInvalidUpstreamProxyConfiguration(t *testing.T) {
 	}
 }
 
+func TestInvalidUpstreamProxyConfiguration(t *testing.T) {
+	var logHook logrustest.Hook
+	servers := map[bool]*httptest.Server{}
+
+	// Create TLS and non-TLS instances of Smokescreen
+	for _, useTLS := range []bool{true, false} {
+		var httpProxyAddr string
+		if useTLS {
+			httpProxyAddr = "https://notaproxy.prxy.svc:443"
+		} else {
+			httpProxyAddr = "http://notaproxy.prxy.svc:80"
+		}
+		_, server, err := startSmokescreen(t, useTLS, &logHook, httpProxyAddr)
+		require.NoError(t, err)
+		defer server.Close()
+		servers[useTLS] = server
+	}
+
+	// Passing an illegal upstream proxy value is not designed to be an especially well
+	// handled error so it would fail many of the checks in our other tests. We really
+	// only care to ensure that these requests never succeed.
+	for _, overConnect := range []bool{true, false} {
+		t.Run(fmt.Sprintf("illegal proxy with CONNECT %t", overConnect), func(t *testing.T) {
+			var proxyTarget string
+			var upstreamProxy string
+
+			// These proxy targets don't actually matter as the requests won't be sent.
+			// because the resolution of the upstream proxy will fail.
+			if overConnect {
+				upstreamProxy = "https://notaproxy.prxy.svc:443"
+				proxyTarget = "https://api.stripe.com:443"
+			} else {
+				upstreamProxy = "http://notaproxy.prxy.svc:80"
+				proxyTarget = "http://checkip.amazonaws.com:80"
+			}
+
+			testCase := &TestCase{
+				OverConnect:   overConnect,
+				OverTLS:       overConnect,
+				ProxyURL:      servers[overConnect].URL,
+				TargetURL:     proxyTarget,
+				UpstreamProxy: upstreamProxy,
+				RoleName:      generateRoleForPolicy(acl.Open),
+				ExpectStatus:  http.StatusBadGateway,
+			}
+			resp, err := executeRequestForTest(t, testCase, &logHook)
+			validateProxyResponseWithUpstream(t, testCase, resp, err, logHook.AllEntries())
+
+		})
+	}
+}
+
 func TestClientHalfCloseConnection(t *testing.T) {
 	a := assert.New(t)
 
@@ -510,7 +562,7 @@ func TestClientHalfCloseConnection(t *testing.T) {
 
 	var logHook logrustest.Hook
 
-	conf, server, err := startSmokescreen(t, false, &logHook)
+	conf, server, err := startSmokescreen(t, false, &logHook, "")
 	require.NoError(t, err)
 	defer server.Close()
 
@@ -577,7 +629,7 @@ func findLogEntry(entries []*logrus.Entry, msg string) *logrus.Entry {
 	return nil
 }
 
-func startSmokescreen(t *testing.T, useTLS bool, logHook logrus.Hook) (*smokescreen.Config, *httptest.Server, error) {
+func startSmokescreen(t *testing.T, useTLS bool, logHook logrus.Hook, httpProxyAddr string) (*smokescreen.Config, *httptest.Server, error) {
 	args := []string{
 		"smokescreen",
 		"--listen-ip=127.0.0.1",
@@ -596,6 +648,11 @@ func startSmokescreen(t *testing.T, useTLS bool, logHook logrus.Hook) (*smokescr
 		)
 	}
 
+	if httpProxyAddr != ""{
+		args = append(args, fmt.Sprintf("--upstream-http-proxy-addr=%s", httpProxyAddr))
+		args = append(args, fmt.Sprintf("--upstream-https-proxy-addr=%s", httpProxyAddr))
+	}
+
 	conf, err := NewConfiguration(args, nil)
 	if err != nil {
 		t.Fatalf("Failed to create configuration: %v", err)

cmd/smokescreen.go

@@ -94,11 +94,11 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e
 			Value: "/metrics",
 			Usage: "Expose prometheus metrics on `ENDPOINT`. Requires --expose-prometheus-metrics to be set. Defaults to \"/metrics\"",
 		},
-    cli.StringFlag{
-      Name: "prometheus-listen-ip",
-      Value: "0.0.0.0",
-      Usage: "Listen for prometheus metrics on interface with address IP. Requires --expose-prometheus-metrics to be set. Defaults to \"0.0.0.0\"",
-    },
+		cli.StringFlag{
+			Name: "prometheus-listen-ip",
+			Value: "0.0.0.0",
+			Usage: "Listen for prometheus metrics on interface with address IP. Requires --expose-prometheus-metrics to be set. Defaults to \"0.0.0.0\"",
+		},
 		cli.StringFlag{
 			Name:  "prometheus-port",
 			Value: "9810",
@@ -146,6 +146,16 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e
 			Name:  "unsafe-allow-private-ranges",
 			Usage: "Allow private ip ranges by default",
 		},
+		cli.StringFlag{
+			Name:  "upstream-http-proxy-addr",
+			Value: "",
+			Usage: "Set Smokescreen's upstream HTTP proxy address",
+		},
+		cli.StringFlag{
+			Name:  "upstream-https-proxy-addr",
+			Value: "",
+			Usage: "Set Smokescreen's upstream HTTPS proxy address",
+		},
 	}
 
 	app.Action = func(c *cli.Context) error {
@@ -287,6 +297,14 @@ func NewConfiguration(args []string, logger *log.Logger) (*smokescreen.Config, e
 			}
 		}
 
+		if c.IsSet("upstream-http-proxy-addr") {
+			conf.UpstreamHttpProxyAddr = c.String("upstream-http-proxy-addr")
+		}
+
+		if c.IsSet("upstream-https-proxy-addr") {
+			conf.UpstreamHttpsProxyAddr = c.String("upstream-https-proxy-addr")
+		}
+
 		// Setup the connection tracker if there is not yet one in the config
 		if conf.ConnTracker == nil {
 			conf.ConnTracker = conntrack.NewTracker(conf.IdleTimeout, conf.MetricsClient, conf.Log, conf.ShuttingDown, nil)

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/rs/xid v1.2.1
 	github.com/sirupsen/logrus v1.9.0
 	github.com/stretchr/testify v1.8.0
-	github.com/stripe/goproxy v0.0.0-20230801191332-fabc3ecb7251
+	github.com/stripe/goproxy v0.0.0-20231113215313-dbbdf2f6d709
 	golang.org/x/net v0.17.0
 	gopkg.in/urfave/cli.v1 v1.20.0
 	gopkg.in/yaml.v2 v2.4.0

go.sum

@@ -218,6 +218,8 @@ github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PK
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/stripe/goproxy v0.0.0-20230801191332-fabc3ecb7251 h1:wR1exp7OglR0ctk8yWPVp1oTOuyaLUlJv3/Wlbvbw64=
 github.com/stripe/goproxy v0.0.0-20230801191332-fabc3ecb7251/go.mod h1:hF2CVgH4++5ijZiy9grGVP8Fsi4u+SMOtbnIKYbMUjY=
+github.com/stripe/goproxy v0.0.0-20231113215313-dbbdf2f6d709 h1:b0AttHAJ5f9rIK2frq9Q4WEeeBNQccr1j+cjQCmOl6s=
+github.com/stripe/goproxy v0.0.0-20231113215313-dbbdf2f6d709/go.mod h1:hF2CVgH4++5ijZiy9grGVP8Fsi4u+SMOtbnIKYbMUjY=
 github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=

pkg/smokescreen/config.go

@@ -73,6 +73,10 @@ type Config struct {
 	TransportMaxIdleConns        int
 	TransportMaxIdleConnsPerHost int
 
+	// These are the http and https address for the upstream proxy 
+	UpstreamHttpProxyAddr  string
+	UpstreamHttpsProxyAddr string
+
 	// Used for logging connection time
 	TimeConnect bool
 

pkg/smokescreen/smokescreen.go

@@ -433,7 +433,7 @@ func newContext(cfg *Config, proxyType string, req *http.Request) *SmokescreenCo
 }
 
 func BuildProxy(config *Config) *goproxy.ProxyHttpServer {
-	proxy := goproxy.NewProxyHttpServer()
+	proxy := goproxy.NewProxyHttpServer(goproxy.WithHttpProxyAddr(config.UpstreamHttpProxyAddr), goproxy.WithHttpsProxyAddr(config.UpstreamHttpsProxyAddr))
 	proxy.Verbose = false
 	configureTransport(proxy.Tr, config)
 

vendor/modules.txt

@@ -69,7 +69,7 @@ github.com/sirupsen/logrus/hooks/test
 ## explicit; go 1.13
 github.com/stretchr/testify/assert
 github.com/stretchr/testify/require
-# github.com/stripe/goproxy v0.0.0-20230801191332-fabc3ecb7251
+# github.com/stripe/goproxy v0.0.0-20231113215313-dbbdf2f6d709
 ## explicit; go 1.13
 github.com/stripe/goproxy
 # golang.org/x/mod v0.8.0