Skip to content

Commit

Permalink
dashboard,cmd/coordinator: unify and simplify GOPROXY setting behavior
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@prattmic
prattmic committed Apr 6, 2023
1 parent 7639850 commit bca91f5
Show file tree
Hide file tree
Showing 6 changed files with 220 additions and 180 deletions.
62 changes: 39 additions & 23 deletions cmd/coordinator/buildstatus.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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 == "" {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand Down
156 changes: 156 additions & 0 deletions cmd/coordinator/buildstatus_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
})
}
}
4 changes: 2 additions & 2 deletions cmd/debugnewvm/debugnewvm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit bca91f5

Please sign in to comment.