Skip to content

Commit efa540e

Browse files
dmitshurgopherbot
authored andcommitted
cmd/coordinator/internal/legacydash: refactor package-scoped variables
I was on the of fence whether to take this on so late in the game; it seems to favor slightly towards doing it anyway. This makes it easier to see where the variables are used, and will make the future changes easier to reason about. For golang/go#65913. Change-Id: I3c274d2aa7174a9fbd6be91869ce4b1da0dfecaa Reviewed-on: https://go-review.googlesource.com/c/build/+/567499 Reviewed-by: Michael Knyszek <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
1 parent f15cdea commit efa540e

File tree

5 files changed

+66
-75
lines changed

5 files changed

+66
-75
lines changed

cmd/coordinator/internal/legacydash/build.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
// Use of this source code is governed by a BSD-style
33
// license that can be found in the LICENSE file.
44

5-
//go:build go1.16
6-
75
package legacydash
86

97
import (
@@ -449,13 +447,13 @@ func (l *Log) Text() ([]byte, error) {
449447
return b, nil
450448
}
451449

452-
func PutLog(c context.Context, text string) (hash string, err error) {
450+
func (h handler) putLog(c context.Context, text string) (hash string, err error) {
453451
b := new(bytes.Buffer)
454452
z, _ := gzip.NewWriterLevel(b, gzip.BestCompression)
455453
io.WriteString(z, text)
456454
z.Close()
457455
hash = loghash.New(text)
458456
key := dsKey("Log", hash, nil)
459-
_, err = datastoreClient.Put(c, key, &Log{b.Bytes()})
457+
_, err = h.datastoreCl.Put(c, key, &Log{b.Bytes()})
460458
return
461459
}

cmd/coordinator/internal/legacydash/dash.go

Lines changed: 19 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
// Use of this source code is governed by a BSD-style
33
// license that can be found in the LICENSE file.
44

5-
//go:build go1.16
6-
75
// Package legacydash holds the serving code for the build dashboard
86
// (build.golang.org) and its remaining HTTP API endpoints.
97
//
@@ -26,48 +24,46 @@ import (
2624
"google.golang.org/grpc"
2725
)
2826

29-
var (
27+
type handler struct {
28+
mux *http.ServeMux
29+
3030
// Datastore client to a GCP project where build results are stored.
3131
// Typically this is the golang-org GCP project.
32-
datastoreClient *datastore.Client
32+
datastoreCl *datastore.Client
3333

3434
// Maintner client for the maintner service.
3535
// Typically the one at maintner.golang.org.
36-
maintnerClient apipb.MaintnerServiceClient
36+
maintnerCl apipb.MaintnerServiceClient
37+
}
3738

38-
// The builder master key.
39-
masterKey string
40-
41-
// TODO(golang.org/issue/38337): Keep moving away from package scope
42-
// variables during future refactors.
43-
)
39+
func (h handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { h.mux.ServeHTTP(w, req) }
4440

4541
// fakeResults controls whether to make up fake random results. If true, datastore is not used.
4642
const fakeResults = false
4743

4844
// Handler sets a datastore client, maintner client, builder master key and
4945
// GRPC server at the package scope, and returns an HTTP mux for the legacy dashboard.
5046
func Handler(dc *datastore.Client, mc apipb.MaintnerServiceClient, key string, grpcServer *grpc.Server) http.Handler {
51-
datastoreClient = dc
52-
maintnerClient = mc
53-
masterKey = key
54-
grpcServer = grpcServer
55-
56-
mux := http.NewServeMux()
47+
h := handler{
48+
mux: http.NewServeMux(),
49+
datastoreCl: dc,
50+
maintnerCl: mc,
51+
}
52+
kc := keyCheck{masterKey: key}
5753

5854
// authenticated handlers
59-
mux.Handle("/clear-results", hstsGzip(AuthHandler(clearResultsHandler))) // called by coordinator for x/build/cmd/retrybuilds
60-
mux.Handle("/result", hstsGzip(AuthHandler(resultHandler))) // called by coordinator after build
55+
h.mux.Handle("/clear-results", hstsGzip(authHandler{kc, h.clearResultsHandler})) // called by coordinator for x/build/cmd/retrybuilds
56+
h.mux.Handle("/result", hstsGzip(authHandler{kc, h.resultHandler})) // called by coordinator after build
6157

6258
// public handlers
63-
mux.Handle("/", GRPCHandler(grpcServer, hstsGzip(http.HandlerFunc(uiHandler)))) // enables GRPC server for build.golang.org
64-
mux.Handle("/log/", hstsGzip(http.HandlerFunc(logHandler)))
59+
h.mux.Handle("/", GRPCHandler(grpcServer, hstsGzip(http.HandlerFunc(h.uiHandler)))) // enables GRPC server for build.golang.org
60+
h.mux.Handle("/log/", hstsGzip(http.HandlerFunc(h.logHandler)))
6561

6662
// static handler
6763
fs := http.FileServer(http.FS(static))
68-
mux.Handle("/static/", hstsGzip(fs))
64+
h.mux.Handle("/static/", hstsGzip(fs))
6965

70-
return mux
66+
return h
7167
}
7268

7369
//go:embed static

cmd/coordinator/internal/legacydash/handler.go

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,9 @@
22
// Use of this source code is governed by a BSD-style
33
// license that can be found in the LICENSE file.
44

5-
//go:build go1.16
6-
75
package legacydash
86

97
import (
10-
"context"
118
"crypto/hmac"
129
"crypto/md5"
1310
"encoding/json"
@@ -32,7 +29,7 @@ const (
3229
// creates a new Result entity, and creates or updates the relevant Commit entity.
3330
// If the Log field is not empty, resultHandler creates a new Log entity
3431
// and updates the LogHash field before putting the Commit entity.
35-
func resultHandler(r *http.Request) (interface{}, error) {
32+
func (h handler) resultHandler(r *http.Request) (interface{}, error) {
3633
if r.Method != "POST" {
3734
return nil, errBadMethod(r.Method)
3835
}
@@ -54,7 +51,7 @@ func resultHandler(r *http.Request) (interface{}, error) {
5451
}
5552
// store the Log text if supplied
5653
if len(res.Log) > 0 {
57-
hash, err := PutLog(ctx, res.Log)
54+
hash, err := h.putLog(ctx, res.Log)
5855
if err != nil {
5956
return nil, fmt.Errorf("putting Log: %v", err)
6057
}
@@ -75,23 +72,27 @@ func resultHandler(r *http.Request) (interface{}, error) {
7572
}
7673
return nil
7774
}
78-
_, err := datastoreClient.RunInTransaction(ctx, tx)
75+
_, err := h.datastoreCl.RunInTransaction(ctx, tx)
7976
return nil, err
8077
}
8178

8279
// logHandler displays log text for a given hash.
8380
// It handles paths like "/log/hash".
84-
func logHandler(w http.ResponseWriter, r *http.Request) {
81+
func (h handler) logHandler(w http.ResponseWriter, r *http.Request) {
82+
if h.datastoreCl == nil {
83+
http.Error(w, "no datastore client", http.StatusNotFound)
84+
return
85+
}
8586
c := r.Context()
8687
hash := r.URL.Path[strings.LastIndex(r.URL.Path, "/")+1:]
8788
key := dsKey("Log", hash, nil)
8889
l := new(Log)
89-
if err := datastoreClient.Get(c, key, l); err != nil {
90+
if err := h.datastoreCl.Get(c, key, l); err != nil {
9091
if err == datastore.ErrNoSuchEntity {
9192
// Fall back to default namespace;
9293
// maybe this was on the old dashboard.
9394
key.Namespace = ""
94-
err = datastoreClient.Get(c, key, l)
95+
err = h.datastoreCl.Get(c, key, l)
9596
}
9697
if err != nil {
9798
log.Printf("Error: %v", err)
@@ -111,7 +112,7 @@ func logHandler(w http.ResponseWriter, r *http.Request) {
111112

112113
// clearResultsHandler purge a single build failure from the dashboard.
113114
// It currently only supports the main Go repo.
114-
func clearResultsHandler(r *http.Request) (interface{}, error) {
115+
func (h handler) clearResultsHandler(r *http.Request) (interface{}, error) {
115116
if r.Method != "POST" {
116117
return nil, errBadMethod(r.Method)
117118
}
@@ -126,7 +127,7 @@ func clearResultsHandler(r *http.Request) (interface{}, error) {
126127

127128
ctx := r.Context()
128129

129-
_, err := datastoreClient.RunInTransaction(ctx, func(tx *datastore.Transaction) error {
130+
_, err := h.datastoreCl.RunInTransaction(ctx, func(tx *datastore.Transaction) error {
130131
c := &Commit{
131132
PackagePath: "", // TODO(adg): support clearing sub-repos
132133
Hash: hash,
@@ -186,12 +187,15 @@ func builderKeyRevoked(builder string) bool {
186187
return false
187188
}
188189

189-
// AuthHandler wraps an http.HandlerFunc with a handler that validates the
190-
// supplied key and builder query parameters.
191-
func AuthHandler(h dashHandler) http.Handler {
192-
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
193-
c := r.Context()
190+
// authHandler wraps an http.HandlerFunc with a handler that validates the
191+
// supplied key and builder query parameters with the provided key checker.
192+
type authHandler struct {
193+
kc keyCheck
194+
h dashHandler
195+
}
194196

197+
func (a authHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
198+
{ // Block to improve diff readability. Can be unnested later.
195199
// Put the URL Query values into r.Form to avoid parsing the
196200
// request body when calling r.FormValue.
197201
r.Form = r.URL.Query()
@@ -202,13 +206,13 @@ func AuthHandler(h dashHandler) http.Handler {
202206
// Validate key query parameter for POST requests only.
203207
key := r.FormValue("key")
204208
builder := r.FormValue("builder")
205-
if r.Method == "POST" && !validKey(c, key, builder) {
209+
if r.Method == "POST" && !a.kc.ValidKey(key, builder) {
206210
err = fmt.Errorf("invalid key %q for builder %q", key, builder)
207211
}
208212

209213
// Call the original HandlerFunc and return the response.
210214
if err == nil {
211-
resp, err = h(r)
215+
resp, err = a.h(r)
212216
}
213217

214218
// Write JSON response.
@@ -221,7 +225,7 @@ func AuthHandler(h dashHandler) http.Handler {
221225
if err = json.NewEncoder(w).Encode(dashResp); err != nil {
222226
log.Printf("encoding response: %v", err)
223227
}
224-
})
228+
}
225229
}
226230

227231
// validHash reports whether hash looks like a valid git commit hash.
@@ -231,22 +235,27 @@ func validHash(hash string) bool {
231235
return hash != ""
232236
}
233237

234-
func validKey(c context.Context, key, builder string) bool {
235-
if isMasterKey(c, key) {
238+
type keyCheck struct {
239+
// The builder master key.
240+
masterKey string
241+
}
242+
243+
func (kc keyCheck) ValidKey(key, builder string) bool {
244+
if kc.isMasterKey(key) {
236245
return true
237246
}
238247
if builderKeyRevoked(builder) {
239248
return false
240249
}
241-
return key == builderKey(c, builder)
250+
return key == kc.builderKey(builder)
242251
}
243252

244-
func isMasterKey(ctx context.Context, k string) bool {
245-
return k == masterKey
253+
func (kc keyCheck) isMasterKey(k string) bool {
254+
return k == kc.masterKey
246255
}
247256

248-
func builderKey(ctx context.Context, builder string) string {
249-
h := hmac.New(md5.New, []byte(masterKey))
257+
func (kc keyCheck) builderKey(builder string) string {
258+
h := hmac.New(md5.New, []byte(kc.masterKey))
250259
h.Write([]byte(builder))
251260
return fmt.Sprintf("%x", h.Sum(nil))
252261
}

cmd/coordinator/internal/legacydash/ui.go

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
// Use of this source code is governed by a BSD-style
33
// license that can be found in the LICENSE file.
44

5-
//go:build go1.16
6-
75
package legacydash
86

97
import (
@@ -34,7 +32,7 @@ import (
3432
)
3533

3634
// uiHandler is the HTTP handler for the https://build.golang.org/.
37-
func uiHandler(w http.ResponseWriter, r *http.Request) {
35+
func (h handler) uiHandler(w http.ResponseWriter, r *http.Request) {
3836
view, err := viewForRequest(r)
3937
if err != nil {
4038
http.Error(w, err.Error(), http.StatusBadRequest)
@@ -55,7 +53,7 @@ func uiHandler(w http.ResponseWriter, r *http.Request) {
5553
var rpcs errgroup.Group
5654
rpcs.Go(func() error {
5755
var err error
58-
tb.res, err = maintnerClient.GetDashboard(ctx, dashReq)
56+
tb.res, err = h.maintnerCl.GetDashboard(ctx, dashReq)
5957
return err
6058
})
6159
if view.ShowsActiveBuilds() {
@@ -68,7 +66,7 @@ func uiHandler(w http.ResponseWriter, r *http.Request) {
6866
http.Error(w, "maintner.GetDashboard: "+err.Error(), httpStatusOfErr(err))
6967
return
7068
}
71-
data, err := tb.buildTemplateData(ctx)
69+
data, err := tb.buildTemplateData(ctx, h.datastoreCl)
7270
if err != nil {
7371
http.Error(w, err.Error(), http.StatusInternalServerError)
7472
return
@@ -152,7 +150,7 @@ func (tb *uiTemplateDataBuilder) getCommitsToLoad() map[commitInPackage]bool {
152150
// want map. The returned map is keyed by the git hash and may not
153151
// contain items that didn't exist in the datastore. (It is not an
154152
// error if 1 or all don't exist.)
155-
func (tb *uiTemplateDataBuilder) loadDatastoreCommits(ctx context.Context, want map[commitInPackage]bool) (map[string]*Commit, error) {
153+
func (tb *uiTemplateDataBuilder) loadDatastoreCommits(ctx context.Context, cl *datastore.Client, want map[commitInPackage]bool) (map[string]*Commit, error) {
156154
ret := map[string]*Commit{}
157155

158156
// Allow tests to fake what the datastore would've loaded, and
@@ -174,7 +172,7 @@ func (tb *uiTemplateDataBuilder) loadDatastoreCommits(ctx context.Context, want
174172
}).Key()
175173
keys = append(keys, key)
176174
}
177-
commits, err := fetchCommits(ctx, keys)
175+
commits, err := fetchCommits(ctx, cl, keys)
178176
if err != nil {
179177
return nil, fmt.Errorf("fetchCommits: %v", err)
180178
}
@@ -277,8 +275,8 @@ func repoImportPath(rh *apipb.DashRepoHead) string {
277275
return ri.ImportPath
278276
}
279277

280-
func (tb *uiTemplateDataBuilder) buildTemplateData(ctx context.Context) (*uiTemplateData, error) {
281-
dsCommits, err := tb.loadDatastoreCommits(ctx, tb.getCommitsToLoad())
278+
func (tb *uiTemplateDataBuilder) buildTemplateData(ctx context.Context, datastoreCl *datastore.Client) (*uiTemplateData, error) {
279+
dsCommits, err := tb.loadDatastoreCommits(ctx, datastoreCl, tb.getCommitsToLoad())
282280
if err != nil {
283281
return nil, err
284282
}
@@ -325,14 +323,6 @@ func (tb *uiTemplateDataBuilder) buildTemplateData(ctx context.Context) (*uiTemp
325323
}
326324
}
327325

328-
// Release Branches
329-
var releaseBranches []string
330-
for _, gr := range tb.res.Releases {
331-
if gr.BranchName != "master" {
332-
releaseBranches = append(releaseBranches, gr.BranchName)
333-
}
334-
}
335-
336326
gerritProject := "go"
337327
if repo := repos.ByImportPath[tb.req.Repo]; repo != nil {
338328
gerritProject = repo.GoGerritProject
@@ -561,7 +551,7 @@ type Pagination struct {
561551
// It is not an error if a commit doesn't exist.
562552
// Only commits that were found in datastore are returned,
563553
// in an unspecified order.
564-
func fetchCommits(ctx context.Context, keys []*datastore.Key) ([]*Commit, error) {
554+
func fetchCommits(ctx context.Context, cl *datastore.Client, keys []*datastore.Key) ([]*Commit, error) {
565555
if len(keys) == 0 {
566556
return nil, nil
567557
}
@@ -570,7 +560,7 @@ func fetchCommits(ctx context.Context, keys []*datastore.Key) ([]*Commit, error)
570560
out[i] = new(Commit)
571561
}
572562

573-
err := datastoreClient.GetMulti(ctx, keys, out)
563+
err := cl.GetMulti(ctx, keys, out)
574564
err = filterDatastoreError(err)
575565
err = filterNoSuchEntity(err)
576566
if err != nil {

cmd/coordinator/internal/legacydash/ui_test.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
// Use of this source code is governed by a BSD-style
33
// license that can be found in the LICENSE file.
44

5-
//go:build go1.16
6-
75
package legacydash
86

97
import (
@@ -327,7 +325,7 @@ func TestUITemplateDataBuilder(t *testing.T) {
327325
activeBuilds: tt.activeBuilds,
328326
testCommitData: tt.testCommitData,
329327
}
330-
data, err := tb.buildTemplateData(context.Background())
328+
data, err := tb.buildTemplateData(context.Background(), nil)
331329
if err != nil {
332330
t.Fatal(err)
333331
}

0 commit comments

Comments
 (0)