dashboard,cmd/coordinator: unify and simplify GOPROXY setting behavior

The existing behavior for setting GOPROXY is rather hard to follow, and doesn't work correctly in many cases. For example, longtest on a reverse builder gets the GKE proxy. Before CL 479837 there were no longtest builders outside of GCE, so this case was never covered. Fixing this is the motivation of this CL. They way configuration works today is: 1. buildstatus.go unconditionally sets GOPROXY to the GKE proxy [1]. 2. st.conf.ModuleEnv potentially overrides GOPROXY with a more reasonable setting, with a bunch of complex conditions. Unify and simplify this process by moving it into buildstatus.go, where their is now a strict ordering of possible GOPROXY values. Notable changes: * The GKE proxy is never used outside of GCE. * There is a consistent default/fallback of proxy.golang.org. I initially tried to split this into two CLs: one unifying the implementation and the next changing the behavior, but the old behavior is so mind-boggling that the first CL doesn't really make much sense. The annoying part of this CL is that tests move from dashboard to cmd/coordinator, requiring us to export additional fields so cmd/coordinator tests can configure the builders. The test cases themselves are unchanged except for the addition of a non-GCE longtest builder case. [1] Except in runSubrepoTests, which avoids doing so for reverse builders. This was a workaround for private proxy builders in CL 275412, but wasn't extended to other callers because only subrepo tests were seeing a regression. More strangeness. For golang/go#35678. Change-Id: I6090c8c5e91ce6be9bfc07c16f36ed339c9d27ae Reviewed-on: https://go-review.googlesource.com/c/build/+/482339 Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Carlos Amedee <carlos@golang.org> Run-TryBot: Michael Pratt <mpratt@google.com>
2023-04-04 17:50:46 -04:00 · 2023-04-04 17:50:46 -04:00 · bca91f5710
--- a/cmd/coordinator/buildstatus.go
+++ b/cmd/coordinator/buildstatus.go
@ -1042,11 +1042,7 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
 		"GOROOT="+goroot,
 		"GOPATH="+gopath,
 	)
-	if !st.conf.IsReverse() {
-		// GKE value but will be ignored/overwritten by reverse buildlets
-		env = append(env, "GOPROXY="+moduleProxy())
-	}
-	env = append(env, st.conf.ModulesEnv(st.SubName)...)
+	env = append(env, st.modulesEnv()...)

 	args := []string{"test"}
 	if st.conf.CompileOnly {
@ -1141,24 +1137,17 @@ func (m multiError) Error() string {
 	return b.String()
 }

-// moduleProxy returns the GOPROXY environment value to use for module-enabled
-// tests.
+// internalModuleProxy returns the GOPROXY environment value to use for
+// most module-enabled tests.
 //
 // We go through an internal (10.0.0.0/8) proxy that then hits
 // https://proxy.golang.org/ so we're still able to firewall
 // non-internal outbound connections on builder nodes.
 //
-// This moduleProxy func in prod mode (when running on GKE) returns an http
-// URL to the current GKE pod's IP with a Kubernetes NodePort service
-// port that forwards back to the coordinator's 8123. See comment below.
-//
-// In localhost dev mode it just returns the value of GOPROXY.
-func moduleProxy() string {
-	// If we're running on localhost, just use the current environment's value.
-	if pool.NewGCEConfiguration().BuildEnv() == nil || !pool.NewGCEConfiguration().BuildEnv().IsProd {
-		// If empty, use installed VCS tools as usual to fetch modules.
-		return os.Getenv("GOPROXY")
-	}
+// This internalModuleProxy func in prod mode (when running on GKE) returns an
+// http URL to the current GKE pod's IP with a Kubernetes NodePort service port
+// that forwards back to the coordinator's 8123. See comment below.
+func internalModuleProxy() string {
 	// We run a NodePort service on each GKE node
 	// (cmd/coordinator/module-proxy-service.yaml) on port 30157
 	// that maps back the coordinator's port 8123. (We could round
@ -1171,6 +1160,35 @@ func moduleProxy() string {
 	return "http://" + pool.NewGCEConfiguration().GKENodeHostname() + ":30157"
 }

+// modulesEnv returns the extra module-specific environment variables
+// to append to tests.
+func (st *buildStatus) modulesEnv() (env []string) {
+	// GOPROXY
+	switch {
+	case st.SubName == "" && !st.conf.OutboundNetworkAllowed():
+		env = append(env, "GOPROXY=off")
+	case st.conf.PrivateGoProxy():
+		// Don't add GOPROXY, the builder is pre-configured.
+	case pool.NewGCEConfiguration().BuildEnv() == nil || !pool.NewGCEConfiguration().BuildEnv().IsProd:
+		// Dev mode; use the system default.
+		env = append(env, "GOPROXY="+os.Getenv("GOPROXY"))
+	case st.conf.IsGCE():
+		// On GCE; the internal proxy is accessible, prefer that.
+		env = append(env, "GOPROXY="+internalModuleProxy())
+	default:
+		// Everything else uses the public proxy.
+		env = append(env, "GOPROXY=https://proxy.golang.org")
+	}
+
+	// GO111MODULE
+	switch st.SubName {
+	case "oauth2", "build", "perf", "website":
+		env = append(env, "GO111MODULE=on")
+	}
+
+	return env
+}
+
 // runBenchmarkTests runs benchmarks from x/benchmarks when RunBench is set.
 func (st *buildStatus) runBenchmarkTests() (remoteErr, err error) {
 	if st.SubName == "" {
@ -1275,10 +1293,9 @@ func (st *buildStatus) runBenchmarkTests() (remoteErr, err error) {
 		"BENCH_BRANCH="+st.RevBranch,
 		"BENCH_REPOSITORY="+repo,
 		"GOROOT="+goroot,
-		"GOPATH="+gopath,         // For module cache storage
-		"GOPROXY="+moduleProxy(), // GKE value but will be ignored/overwritten by reverse buildlets
+		"GOPATH="+gopath, // For module cache storage
 	)
-	env = append(env, st.conf.ModulesEnv("benchmarks")...)
+	env = append(env, st.modulesEnv()...)
 	if repo != "go" {
 		env = append(env, "BENCH_SUBREPO_PATH="+st.conf.FilePathJoin(workDir, subrepoDir))
 		env = append(env, "BENCH_SUBREPO_BASELINE_PATH="+st.conf.FilePathJoin(workDir, subrepoBaselineDir))
@ -1748,9 +1765,8 @@ func (st *buildStatus) runTestsOnBuildlet(bc buildlet.Client, tis []*testItem, g
 	env := append(st.conf.Env(),
 		"GOROOT="+goroot,
 		"GOPATH="+gopath,
-		"GOPROXY="+moduleProxy(),
 	)
-	env = append(env, st.conf.ModulesEnv("go")...)
+	env = append(env, st.modulesEnv()...)

 	remoteErr, err := bc.Exec(ctx, "./go/bin/go", buildlet.ExecOpts{
 		// We set Dir to "." instead of the default ("go/bin") so when the dist tests
--- a/cmd/coordinator/buildstatus_test.go
+++ b/cmd/coordinator/buildstatus_test.go
@ -10,6 +10,12 @@ package main

 import (
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"golang.org/x/build/buildenv"
+	"golang.org/x/build/dashboard"
+	"golang.org/x/build/internal/buildgo"
+	"golang.org/x/build/internal/coordinator/pool"
 )

 // TestParseOutputAndHeader tests header parsing by parseOutputAndHeader.
@ -198,3 +204,153 @@ XXXBANNERXXX:Testing packages.`),
 		})
 	}
 }
+
+func TestModulesEnv(t *testing.T) {
+	// modulesEnv looks at pool.NewGCEConfiguration().BuildEnv().IsProd for
+	// special behavior in dev mode. Temporarily override the environment
+	// to force testing of the prod configuration.
+	old := pool.NewGCEConfiguration().BuildEnv()
+	defer pool.NewGCEConfiguration().SetBuildEnv(old)
+	pool.NewGCEConfiguration().SetBuildEnv(&buildenv.Environment{
+		IsProd: true,
+	})
+
+	// In testing we never initialize
+	// pool.NewGCEConfiguration().GKENodeHostname(), so we get this odd
+	// concatenation back.
+	const gkeModuleProxy = "http://:30157"
+
+	testCases := []struct {
+		desc string
+		st   *buildStatus
+		want []string
+	}{
+		{
+			desc: "ec2-builder-repo-non-go",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: "bar"},
+				conf: &dashboard.BuildConfig{
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: false,
+						IsEC2:     true,
+					},
+				},
+			},
+			want: []string{"GOPROXY=https://proxy.golang.org"},
+		},
+		{
+			desc: "reverse-builder-repo-non-go",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: "bar"},
+				conf: &dashboard.BuildConfig{
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: true,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=https://proxy.golang.org"},
+		},
+		{
+			desc: "reverse-builder-repo-go",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: ""}, // go
+				conf: &dashboard.BuildConfig{
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: true,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=off"},
+		},
+		{
+			desc: "builder-repo-go",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: ""}, // go
+				conf: &dashboard.BuildConfig{
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: false,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=off"},
+		},
+		{
+			desc: "builder-repo-go-outbound-network-allowed",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: ""}, // go
+				conf: &dashboard.BuildConfig{
+					Name: "test-longtest",
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: false,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=" + gkeModuleProxy},
+		},
+		{
+			desc: "reverse-builder-repo-go-outbound-network-allowed",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: ""}, // go
+				conf: &dashboard.BuildConfig{
+					Name: "test-longtest",
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: true,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=https://proxy.golang.org"},
+		},
+		{
+			desc: "builder-repo-special-case",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: "build"},
+				conf: &dashboard.BuildConfig{
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: false,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=" + gkeModuleProxy, "GO111MODULE=on"},
+		},
+		{
+			desc: "reverse-builder-repo-special-case",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: "build"},
+				conf: &dashboard.BuildConfig{
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: true,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=https://proxy.golang.org", "GO111MODULE=on"},
+		},
+		{
+			desc: "builder-repo-non-special-case",
+			st: &buildStatus{
+				BuilderRev: buildgo.BuilderRev{SubName: "bar"},
+				conf: &dashboard.BuildConfig{
+					TestHostConf: &dashboard.HostConfig{
+						IsReverse: false,
+						IsEC2:     false,
+					},
+				},
+			},
+			want: []string{"GOPROXY=" + gkeModuleProxy},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			got := tc.st.modulesEnv()
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("buildStatus.modulesEnv() mismatch (-want, +got)\n%s", diff)
+			}
+		})
+	}
+}
--- a/cmd/debugnewvm/debugnewvm.go
+++ b/cmd/debugnewvm/debugnewvm.go
@ -84,7 +84,7 @@ func main() {
 	if !hconf.IsVM() && !hconf.IsContainer() {
 		log.Fatalf("host type %q is type %q; want a VM or container host type", *hostType, hconf.PoolName())
 	}
-	if hconf.IsEC2() && (*awsKeyID == "" || *awsAccessKey == "") {
+	if hconf.IsEC2 && (*awsKeyID == "" || *awsAccessKey == "") {
 		if !metadata.OnGCE() {
 			log.Fatal("missing -aws-key-id and -aws-access-key params are required for builders on AWS")
 		}
@ -116,7 +116,7 @@ func main() {

 	log.Printf("Creating %s (with VM image %s)", name, vmImageSummary)
 	var bc buildlet.Client
-	if hconf.IsEC2() {
+	if hconf.IsEC2 {
 		region := env.AWSRegion
 		if *awsRegion != "" {
 			region = *awsRegion
--- a/dashboard/builders.go
+++ b/dashboard/builders.go
@ -347,7 +347,7 @@ var Hosts = map[string]*HostConfig{
 		VMImage:        "ami-07409163bccd5ac4d",
 		ContainerImage: "gobuilder-arm-aws:latest",
 		machineType:    "m6g.xlarge",
-		isEC2:          true,
+		IsEC2:          true,
 		SSHUsername:    "root",
 	},
 	"host-linux-arm64-bullseye": {
@ -621,7 +621,7 @@ func init() {
 		if c.VMImage != "" {
 			nSet++
 		}
-		if c.ContainerImage != "" && !c.isEC2 {
+		if c.ContainerImage != "" && !c.IsEC2 {
 			nSet++
 		}
 		if c.IsReverse {
@ -692,7 +692,7 @@ type HostConfig struct {
 	cosArchitecture CosArch // optional. GCE instances which use COS need the architecture set. Default: CosArchAMD64

 	// EC2 options
-	isEC2 bool // if true, the instance is configured to run on EC2
+	IsEC2 bool // if true, the instance is configured to run on EC2

 	// GCE or EC2 options:
 	//
@ -830,8 +830,7 @@ type BuildConfig struct {
 	StopAfterMake bool

 	// privateGoProxy for builder has it's own Go proxy instead of
-	// proxy.golang.org, after setting this builder will respect
-	// GOPROXY environment value.
+	// proxy.golang.org, pre-set in GOPROXY on the builder.
 	privateGoProxy bool

 	// InstallRacePackages controls which packages to "go install
@ -876,7 +875,7 @@ type BuildConfig struct {
 	makeScriptArgs []string // extra args to pass to the make.bash-equivalent script
 	allScriptArgs  []string // extra args to pass to the all.bash-equivalent script

-	testHostConf *HostConfig // override HostConfig for testing, at least for now
+	TestHostConf *HostConfig // override HostConfig for testing, at least for now

 	// isRestricted marks if a builder should be restricted to a subset of users.
 	isRestricted bool
@ -897,28 +896,10 @@ func (c *BuildConfig) Env() []string {
 	return append(env, c.env...)
 }

-// ModulesEnv returns the extra module-specific environment variables
-// to append to this builder as a function of the repo being built
-// ("go", "oauth2", "net", etc).
-func (c *BuildConfig) ModulesEnv(repo string) (env []string) {
-	// EC2 and reverse builders should set the public module proxy
-	// address instead of the internal proxy.
-	if (c.HostConfig().isEC2 || c.IsReverse()) && repo != "go" && !c.PrivateGoProxy() {
-		env = append(env, "GOPROXY=https://proxy.golang.org")
-	}
-	switch repo {
-	case "go":
-		if !c.OutboundNetworkAllowed() {
-			env = append(env, "GOPROXY=off")
-		}
-	case "oauth2", "build", "perf", "website":
-		env = append(env, "GO111MODULE=on")
-	}
-	return env
-}
-
 func (c *BuildConfig) IsReverse() bool { return c.HostConfig().IsReverse }

+func (c *BuildConfig) IsGCE() bool { return !c.HostConfig().IsReverse && !c.HostConfig().IsEC2 }
+
 func (c *BuildConfig) IsContainer() bool { return c.HostConfig().IsContainer() }
 func (c *HostConfig) IsContainer() bool  { return c.ContainerImage != "" }

@ -929,7 +910,7 @@ func (c *BuildConfig) IsVM() bool { return c.HostConfig().IsVM() }
 // EC2 instances may be configured to run in containers that are running
 // on custom AMIs.
 func (c *HostConfig) IsVM() bool {
-	if c.isEC2 {
+	if c.IsEC2 {
 		return c.VMImage != "" && c.ContainerImage == ""
 	}
 	return c.VMImage != ""
@ -1000,8 +981,8 @@ func (c *BuildConfig) GoTestTimeoutScale() int {

 // HostConfig returns the host configuration of c.
 func (c *BuildConfig) HostConfig() *HostConfig {
-	if c.testHostConf != nil {
-		return c.testHostConf
+	if c.TestHostConf != nil {
+		return c.TestHostConf
 	}
 	if c, ok := Hosts[c.HostType]; ok {
 		return c
@ -1333,7 +1314,7 @@ func (c *BuildConfig) GorootFinal() string {

 // MachineType returns the AWS or GCE machine type to use for this builder.
 func (c *HostConfig) MachineType() string {
-	if c.IsEC2() {
+	if c.IsEC2 {
 		return c.machineType
 	}
 	typ := c.machineType
@ -1360,17 +1341,13 @@ func (c *HostConfig) MachineType() string {
 }

 // IsEC2 returns true if the machine type is an EC2 arm64 type.
-func (c *HostConfig) IsEC2() bool {
-	return c.isEC2
-}
-
 // PoolName returns a short summary of the builder's host type for the
 // https://farmer.golang.org/builders page.
 func (c *HostConfig) PoolName() string {
 	switch {
 	case c.IsReverse:
 		return "Reverse (dedicated machine/VM)"
-	case c.IsEC2():
+	case c.IsEC2:
 		return "EC2 VM Container"
 	case c.IsVM():
 		return "GCE VM"
@ -1394,7 +1371,7 @@ func (c *HostConfig) ContainerVMImage() string {
 	if c.NestedVirt {
 		return "debian-bullseye-vmx"
 	}
-	if c.isEC2 && c.ContainerImage != "" {
+	if c.IsEC2 && c.ContainerImage != "" {
 		return fmt.Sprintf("gcr.io/%s/%s", buildenv.Production.ProjectName, c.ContainerImage)
 	}
 	return ""
@ -1408,7 +1385,7 @@ func (c *HostConfig) IsHermetic() bool {
 	switch {
 	case c.IsReverse:
 		return c.HermeticReverse
-	case c.IsEC2():
+	case c.IsEC2:
 		return true
 	case c.IsVM():
 		return true
--- a/dashboard/builders_test.go
+++ b/dashboard/builders_test.go
@ -16,8 +16,6 @@ import (
 	"strings"
 	"testing"
 	"time"
-
-	"github.com/google/go-cmp/cmp"
 )

 func TestOSARCHAccessors(t *testing.T) {
@ -40,21 +38,21 @@ func TestDistTestsExecTimeout(t *testing.T) {
 		{
 			&BuildConfig{
 				env:          []string{},
-				testHostConf: &HostConfig{},
+				TestHostConf: &HostConfig{},
 			},
 			20 * time.Minute,
 		},
 		{
 			&BuildConfig{
 				env:          []string{"GO_TEST_TIMEOUT_SCALE=2"},
-				testHostConf: &HostConfig{},
+				TestHostConf: &HostConfig{},
 			},
 			40 * time.Minute,
 		},
 		{
 			&BuildConfig{
 				env: []string{},
-				testHostConf: &HostConfig{
+				TestHostConf: &HostConfig{
 					env: []string{"GO_TEST_TIMEOUT_SCALE=3"},
 				},
 			},
@ -64,7 +62,7 @@ func TestDistTestsExecTimeout(t *testing.T) {
 		{
 			&BuildConfig{
 				env: []string{"GO_TEST_TIMEOUT_SCALE=2"},
-				testHostConf: &HostConfig{
+				TestHostConf: &HostConfig{
 					env: []string{"GO_TEST_TIMEOUT_SCALE=3"},
 				},
 			},
@ -1152,7 +1150,7 @@ func TestHostConfigIsVM(t *testing.T) {
 			config: &HostConfig{
 				VMImage:        "image-x",
 				ContainerImage: "",
-				isEC2:          false,
+				IsEC2:          false,
 			},
 			want: true,
 		},
@ -1161,7 +1159,7 @@ func TestHostConfigIsVM(t *testing.T) {
 			config: &HostConfig{
 				VMImage:        "",
 				ContainerImage: "container-image-x",
-				isEC2:          false,
+				IsEC2:          false,
 			},
 			want: false,
 		},
@ -1170,7 +1168,7 @@ func TestHostConfigIsVM(t *testing.T) {
 			config: &HostConfig{
 				VMImage:        "image-x",
 				ContainerImage: "container-image-x",
-				isEC2:          true,
+				IsEC2:          true,
 			},
 			want: false,
 		},
@ -1179,7 +1177,7 @@ func TestHostConfigIsVM(t *testing.T) {
 			config: &HostConfig{
 				VMImage:        "image-x",
 				ContainerImage: "",
-				isEC2:          true,
+				IsEC2:          true,
 			},
 			want: true,
 		},
@ -1193,113 +1191,6 @@ func TestHostConfigIsVM(t *testing.T) {
 	}
 }

-func TestModulesEnv(t *testing.T) {
-	testCases := []struct {
-		desc        string
-		buildConfig *BuildConfig
-		repo        string
-		want        []string
-	}{
-		{
-			desc: "ec2-builder-repo-non-go",
-			buildConfig: &BuildConfig{
-				testHostConf: &HostConfig{
-					IsReverse: false,
-					isEC2:     true,
-				},
-			},
-			repo: "bar",
-			want: []string{"GOPROXY=https://proxy.golang.org"},
-		},
-		{
-			desc: "reverse-builder-repo-non-go",
-			buildConfig: &BuildConfig{
-				testHostConf: &HostConfig{
-					IsReverse: true,
-					isEC2:     false,
-				},
-			},
-			repo: "bar",
-			want: []string{"GOPROXY=https://proxy.golang.org"},
-		},
-		{
-			desc: "reverse-builder-repo-go",
-			buildConfig: &BuildConfig{
-				testHostConf: &HostConfig{
-					IsReverse: true,
-					isEC2:     false,
-				},
-			},
-			repo: "go",
-			want: []string{"GOPROXY=off"},
-		},
-		{
-			desc: "builder-repo-go",
-			buildConfig: &BuildConfig{
-				testHostConf: &HostConfig{
-					IsReverse: false,
-					isEC2:     false,
-				},
-			},
-			repo: "go",
-			want: []string{"GOPROXY=off"},
-		},
-		{
-			desc: "builder-repo-go-outbound-network-allowed",
-			buildConfig: &BuildConfig{
-				Name: "test-longtest",
-				testHostConf: &HostConfig{
-					IsReverse: false,
-					isEC2:     false,
-				},
-			},
-			repo: "go",
-			want: nil,
-		},
-		{
-			desc: "builder-repo-special-case",
-			buildConfig: &BuildConfig{
-				testHostConf: &HostConfig{
-					IsReverse: false,
-					isEC2:     false,
-				},
-			},
-			repo: "build",
-			want: []string{"GO111MODULE=on"},
-		},
-		{
-			desc: "reverse-builder-repo-special-case",
-			buildConfig: &BuildConfig{
-				testHostConf: &HostConfig{
-					IsReverse: true,
-					isEC2:     false,
-				},
-			},
-			repo: "build",
-			want: []string{"GOPROXY=https://proxy.golang.org", "GO111MODULE=on"},
-		},
-		{
-			desc: "builder-repo-non-special-case",
-			buildConfig: &BuildConfig{
-				testHostConf: &HostConfig{
-					IsReverse: false,
-					isEC2:     false,
-				},
-			},
-			repo: "bar",
-			want: nil,
-		},
-	}
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			got := tc.buildConfig.ModulesEnv(tc.repo)
-			if diff := cmp.Diff(tc.want, got); diff != "" {
-				t.Errorf("BuildConfig.ModulesEnv(%q) mismatch (-want, +got)\n%s", tc.repo, diff)
-			}
-		})
-	}
-}
-
 func TestDefaultPlusExpBuild(t *testing.T) {
 	for _, tc := range []struct {
 		repo string
--- a/internal/coordinator/pool/pool.go
+++ b/internal/coordinator/pool/pool.go
@ -124,7 +124,7 @@ func ForHost(conf *dashboard.HostConfig) Buildlet {
 		panic("nil conf")
 	}
 	switch {
-	case conf.IsEC2():
+	case conf.IsEC2:
 		return EC2BuildetPool()
 	case conf.IsVM(), conf.IsContainer():
 		return NewGCEConfiguration().BuildletPool()