cmd/coordinator: fix gomote create compatibility for older gomote clients

I broke it in CL 207841. That CL included a compatibility mode, but I
never tested it.

Fixes golang/go#35778

Change-Id: Ia0362af44c464ae2b0c786b93ed2732dfbbfe3a9
Reviewed-on: https://go-review.googlesource.com/c/build/+/208437
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
diff --git a/cmd/coordinator/remote.go b/cmd/coordinator/remote.go
index e1302df..d134d44 100644
--- a/cmd/coordinator/remote.go
+++ b/cmd/coordinator/remote.go
@@ -118,6 +118,8 @@
 	}
 }
 
+var timeNow = time.Now // for testing
+
 // always wrapped in requireBuildletProxyAuth.
 func handleBuildletCreate(w http.ResponseWriter, r *http.Request) {
 	if r.Method != "POST" {
@@ -179,9 +181,9 @@
 	}()
 	// One of these fields is set:
 	type msg struct {
-		Error    string                   `json:"error"`
-		Buildlet *remoteBuildlet          `json:"buildlet"`
-		Status   types.BuildletWaitStatus `json:"status"`
+		Error    string                    `json:"error,omitempty"`
+		Buildlet *remoteBuildlet           `json:"buildlet,omitempty"`
+		Status   *types.BuildletWaitStatus `json:"status,omitempty"`
 	}
 	sendJSONLine := func(v interface{}) {
 		jenc, err := json.Marshal(v)
@@ -195,26 +197,32 @@
 	for {
 		select {
 		case <-ticker:
-			sendJSONLine(msg{Status: sched.waiterState(si)})
+			st := sched.waiterState(si)
+			sendJSONLine(msg{Status: &st})
 		case bc := <-resc:
 			rb := &remoteBuildlet{
 				User:        user,
 				BuilderType: builderType,
 				HostType:    bconf.HostType,
 				buildlet:    bc,
-				Created:     time.Now(),
-				Expires:     time.Now().Add(remoteBuildletIdleTimeout),
+				Created:     timeNow(),
+				Expires:     timeNow().Add(remoteBuildletIdleTimeout),
 			}
 			rb.Name = addRemoteBuildlet(rb)
 			bc.SetName(rb.Name)
 			log.Printf("created buildlet %v for %v (%s)", rb.Name, rb.User, bc.String())
 			if wantStream {
-				// We already set a content-type above before we flushed, so don't
-				// set it again.
+				// We already sent the Content-Type
+				// (and perhaps status update JSON
+				// lines) earlier, so just send the
+				// final JSON update with the result:
+				sendJSONLine(msg{Buildlet: rb})
 			} else {
+				// Legacy client path.
+				// TODO: delete !wantStream support 3-6 months after 2019-11-19.
 				w.Header().Set("Content-Type", "application/json; charset=utf-8")
+				sendJSONLine(rb)
 			}
-			sendJSONLine(msg{Buildlet: rb})
 			return
 		case err := <-errc:
 			log.Printf("error creating gomote buildlet: %v", err)
diff --git a/cmd/coordinator/remote_test.go b/cmd/coordinator/remote_test.go
index d35614e..b4264f6 100644
--- a/cmd/coordinator/remote_test.go
+++ b/cmd/coordinator/remote_test.go
@@ -14,10 +14,10 @@
 	"net/http/httptest"
 	"net/url"
 	"os"
-	"runtime"
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"golang.org/x/build/buildlet"
 	"golang.org/x/build/dashboard"
@@ -109,7 +109,7 @@
 	testPool.Remove("test-host")
 }
 
-var buildName = runtime.GOOS + "-" + runtime.GOARCH + "-test"
+const buildName = "linux-amd64-test"
 
 type tlogger struct{ t *testing.T }
 
@@ -118,23 +118,60 @@
 	return len(p), nil
 }
 
-func TestHandleBuildletCreate(t *testing.T) {
+func TestHandleBuildletCreate_PreStream(t *testing.T) {
 	log.SetOutput(tlogger{t})
 	defer log.SetOutput(os.Stderr)
 	addBuilder(buildName)
+	remoteBuildlets.m = map[string]*remoteBuildlet{}
 	testPoolHook = func(_ *dashboard.HostConfig) BuildletPool { return testPool }
 	defer func() {
+		timeNow = time.Now
 		removeBuilder(buildName)
 		testPoolHook = nil
 	}()
+	timeNow = func() time.Time { return time.Unix(123, 0) }
 	data := url.Values{}
 	data.Set("version", "20160922")
 	data.Set("builderType", buildName)
 	req := httptest.NewRequest("POST", "/buildlet/create", strings.NewReader(data.Encode()))
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.SetBasicAuth("gopher", "fake-not-checked-password") // auth is handled in outer wrapper
 	w := httptest.NewRecorder()
 	handleBuildletCreate(w, req)
 	if w.Code != 200 {
 		t.Fatal("bad code", w.Code, w.Body.String())
 	}
+	want := `{"User":"gopher","Name":"gopher-linux-amd64-test-0","HostType":"test-host","BuilderType":"linux-amd64-test","Created":"1970-01-01T00:02:03Z","Expires":"1970-01-01T00:32:03Z"}`
+	if got := strings.TrimSpace(w.Body.String()); got != want {
+		t.Errorf("unexpected output.\n got: %s\nwant: %s\n", got, want)
+	}
+}
+
+func TestHandleBuildletCreate_Stream(t *testing.T) {
+	log.SetOutput(tlogger{t})
+	defer log.SetOutput(os.Stderr)
+	addBuilder(buildName)
+	remoteBuildlets.m = map[string]*remoteBuildlet{}
+	testPoolHook = func(_ *dashboard.HostConfig) BuildletPool { return testPool }
+	defer func() {
+		timeNow = time.Now
+		removeBuilder(buildName)
+		testPoolHook = nil
+	}()
+	timeNow = func() time.Time { return time.Unix(123, 0) }
+	data := url.Values{}
+	data.Set("version", buildlet.GomoteCreateStreamVersion)
+	data.Set("builderType", buildName)
+	req := httptest.NewRequest("POST", "/buildlet/create", strings.NewReader(data.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.SetBasicAuth("gopher", "fake-not-checked-password") // auth is handled in outer wrapper
+	w := httptest.NewRecorder()
+	handleBuildletCreate(w, req)
+	if w.Code != 200 {
+		t.Fatal("bad code", w.Code, w.Body.String())
+	}
+	want := `{"buildlet":{"User":"gopher","Name":"gopher-linux-amd64-test-0","HostType":"test-host","BuilderType":"linux-amd64-test","Created":"1970-01-01T00:02:03Z","Expires":"1970-01-01T00:32:03Z"}}`
+	if got := strings.TrimSpace(w.Body.String()); got != want {
+		t.Errorf("unexpected output.\n got: %s\nwant: %s\n", got, want)
+	}
 }