ui: various glue fixes #29692

benesch · 2018-09-07T03:17:12Z

This PR restores the "no UI installed" message in short binaries:

I came across a few minor nits that seemed worth fixing too.

cockroach-teamcity · 2018-09-07T03:17:21Z

This change is

benesch

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/authentication.go, line 365 at r5 (raw file):

	}

	newCtx := context.WithValue(req.Context(), webSessionUserKey{}, username)

This would install a username of "" if getSession returned an error. This seemed wrong and required a special case in serveUIAssets to convert "" usernames to nil.

pkg/server/server.go, line 1240 at r5 (raw file):

			LoginEnabled:         s.cfg.RequireWebSession(),
			GetUser: func(ctx context.Context) *string {
				if u, ok := ctx.Value(webSessionUserKey{}).(string); ok {

See the comment on the gzip response writer for why we no longer need to check for empty username.

pkg/server/server.go, line 1923 at r5 (raw file):

	if w.Header().Get("Content-Type") == "" {
		w.Header().Set("Content-Type", http.DetectContentType(b))
	}

This garbage was requiring us to explicitly specifyContent-Type: text/html when rendering the template, because it was otherwise getting detected as application/gzip. (And gosh that's a garbage MIME type. How is that ever useful?)

pkg/ui/ui.go, line 141 at r5 (raw file):

			Tag:                  buildInfo.Tag,
			Version:              build.VersionPrefix(),
		}); err != nil {

Note the lack of template.JS here. We actually want to be escaping this JSON. Otherwise I think you could XSS yourself by creating a username with special JS chars in it. Not very useful because only root can create users, but still silly.

benesch

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/ui/ui.go, line 141 at r5 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Note the lack of template.JS here. We actually want to be escaping this JSON. Otherwise I think you could XSS yourself by creating a username with special JS chars in it. Not very useful because only root can create users, but still silly.

Er scratch that, the old code was calling template.JS(json.Marshal(indexHTMLArgs{...})), which is exactly equivalent to and equally safe as passing indexHTMLArgs{...} directly.

benesch · 2018-09-12T17:39:26Z

Bump!

vilterp

Thanks!

vilterp · 2018-09-12T18:01:40Z

pkg/server/server.go

@@ -1964,6 +1964,7 @@ func serveUIAssets(fileServer http.Handler, cfg Config) http.Handler {
 		argsJSON, err := json.Marshal(tmplArgs)
 		if err != nil {
 			http.Error(writer, err.Error(), 500)
+			return


Release note: None

This function was missing a return in one place and had an extraneous return in another. Release note: None

Don't attempt to use non-error return values when the error is non-nil. That's not idiomatic Go. Release note: None

Release note: None

couchand

Reviewed 1 of 1 files at r8, 4 of 4 files at r10.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/server.go, line 1239 at r10 (raw file):

	maybeAuthMux := newAuthenticationMuxAllowAnonymous(
		s.authentication, ui.Handler(ui.Config{
			ExperimentalUseLogin: s.cfg.EnableWebSessionAuthentication,

why not just pass over the Config and let ui decide what it needs?

pkg/server/server.go, line 1241 at r10 (raw file):

			ExperimentalUseLogin: s.cfg.EnableWebSessionAuthentication,
			LoginEnabled:         s.cfg.RequireWebSession(),
			GetUser: func(ctx context.Context) *string {

regardless of the above, why not just inline this in ui.Handler?

pkg/server/server.go, line 1897 at r10 (raw file):

}

// TODO(benesch): Use https://github.com/NYTimes/gziphandler instead.

👍

pkg/server/server_test.go, line 867 at r10 (raw file):

		ui.AssetDir = func(name string) (_ []string, _ error) { return }
		ui.AssetInfo = func(name string) (_ os.FileInfo, _ error) { return }
		return func() {

I think this would be clearer as two distinct functions, so the usage is just:

linkInFakeUI()
defer unlinkFakeUI()

pkg/server/server_test.go, line 890 at r10 (raw file):

		}

		t.Run("short build", func(t *testing.T) {

yay a test!

Better separate concerns by making package ui responsible for providing an HTTP handler that can serve its assets. As a side effect of the refactor, fix rendering of the "no UI installed" page in short binaries, which was broken in cockroachdb#25195. This is not pure code movement, so ui.Handler should be reviewed as if it were new code. Release note: None

benesch

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/server.go, line 1239 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

why not just pass over the Config and let ui decide what it needs?

That would create a dependency cycle (server -> ui -> server), which is verboten in Go. We could extract a serverconfig package, but doesn't seem worth it for just this one struct.

pkg/server/server.go, line 1241 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

regardless of the above, why not just inline this in ui.Handler?

Same problem: accessing webSessionUserKey{} from ui would cause a dependency cycle.

pkg/server/server_test.go, line 867 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

I think this would be clearer as two distinct functions, so the usage is just:
linkInFakeUI()
defer unlinkFakeUI()

Done. FYI returning a cleanup closure is a Cockroach-ism (and maybe a Go-ism?) that we use all over the place:

cockroach/pkg/server/server_test.go

Line 76 in f565ab1

defer leaktest.AfterTest(t)()

cockroach/pkg/storage/replica.go

Lines 5300 to 5320 in f565ab1

    
           if splitMergeUnlock, err := r.maybeAcquireSplitMergeLock(ctx, raftCmd); err != nil { 
        
           	log.Eventf(ctx, "unable to acquire split lock: %s", err) 
        
           	// Send a crash report because a former bug in the error handling might have 
        
           	// been the root cause of #19172. 
        
           	_ = r.store.stopper.RunAsyncTask(ctx, "crash report", func(ctx context.Context) { 
        
           		log.SendCrashReport( 
        
           			ctx, 
        
           			&r.store.cfg.Settings.SV, 
        
           			0, // depth 
        
           			"while acquiring split lock: %s", 
        
           			[]interface{}{err}, 
        
           		) 
        
           	}) 
        
           	forcedErr = roachpb.NewError(err) 
        
           } else if splitMergeUnlock != nil { 
        
           	// Close over raftCmd to capture its value at execution time; we clear 
        
           	// ReplicatedEvalResult on certain errors. 
        
           	defer func() { 
        
           		splitMergeUnlock(raftCmd.ReplicatedEvalResult) 
        
           	}()

cockroach/pkg/ccl/backupccl/backup_test.go

Lines 209 to 210 in f565ab1

_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)

defer cleanupFn()
cockroach/pkg/storage/engine/temp_engine_test.go

Lines 29 to 30 in 5a013c1

tempDir, tempDirCleanup := testutils.TempDir(t)

defer tempDirCleanup()

I also find this pattern kind of gross, so happy to defer to you here, but I've learned to live with it.

couchand

LGTM

Reviewed 1 of 1 files at r11.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/server.go, line 1239 at r10 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

That would create a dependency cycle (server -> ui -> server), which is verboten in Go. We could extract a serverconfig package, but doesn't seem worth it for just this one struct.

Oh I was thinking that was in config. Okay.

pkg/server/server.go, line 1241 at r10 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Same problem: accessing webSessionUserKey{} from ui would cause a dependency cycle.

that's unpleasant.

pkg/server/server_test.go, line 867 at r10 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Done. FYI returning a cleanup closure is a Cockroach-ism (and maybe a Go-ism?) that we use all over the place:

cockroach/pkg/server/server_test.go

Line 76 in f565ab1

defer leaktest.AfterTest(t)()

cockroach/pkg/storage/replica.go

Lines 5300 to 5320 in f565ab1

if splitMergeUnlock, err := r.maybeAcquireSplitMergeLock(ctx, raftCmd); err != nil {

log.Eventf(ctx, "unable to acquire split lock: %s", err)

// Send a crash report because a former bug in the error handling might have

// been the root cause of #19172.

_ = r.store.stopper.RunAsyncTask(ctx, "crash report", func(ctx context.Context) {

log.SendCrashReport(

ctx,

&r.store.cfg.Settings.SV,

0, // depth

"while acquiring split lock: %s",

[]interface{}{err},

)

})

forcedErr = roachpb.NewError(err)

} else if splitMergeUnlock != nil {

// Close over raftCmd to capture its value at execution time; we clear

// ReplicatedEvalResult on certain errors.

defer func() {

splitMergeUnlock(raftCmd.ReplicatedEvalResult)

}()

cockroach/pkg/ccl/backupccl/backup_test.go

Lines 209 to 210 in f565ab1

_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)

defer cleanupFn()

cockroach/pkg/storage/engine/temp_engine_test.go

Lines 29 to 30 in 5a013c1

tempDir, tempDirCleanup := testutils.TempDir(t)

defer tempDirCleanup()

I also find this pattern kind of gross, so happy to defer to you here, but I've learned to live with it.

oh gross. it's at least less gross to name the return value.

benesch

Thanks!

bors r=vilterp,couchand

Dismissed @couchand from 5 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/server_test.go, line 867 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

oh gross. it's at least less gross to name the return value.

Yep, though having defer leaktest.AfterTest(t)() at the top of every test really makes you numb to the non-named pattern.

craig · 2018-09-13T18:53:50Z

Build failed

GitHub CI (Cockroach)

benesch · 2018-09-13T18:55:19Z

Seems like a spurious roachtest failure.

bors r=vilterp,couchand

29692: ui: various glue fixes r=vilterp,couchand a=benesch This PR restores the "no UI installed" message in short binaries: ![image](https://user-images.githubusercontent.com/882976/45196553-e713b880-b22a-11e8-928a-06c7a2da0f63.png) I came across a few minor nits that seemed worth fixing too. 30135: sql: add '2.0' setting for distsql r=jordanlewis a=jordanlewis The 2.0 setting for distsql (both a cluster setting and a session setting) instructs the executor to use the 2.0 method of determining how to execute a query: the query runs via local sql unless the query is both distributable and recommended to be distributed, in which case it runs via the distsql and is actually distributed. Release note (sql change): add the '2.0' value for both the distsql session setting and the sql.defaults.distsql cluster setting, which instructs the database to use the 2.0 'auto' behavior for determining whether queries run via distsql or not. 30148: storage: add new metrics for the RaftEntryCache r=nvanbenschoten a=nvanbenschoten Four new metrics are introduced: - `raft.entrycache.bytes` - `raft.entrycache.size` - `raft.entrycache.accesses` - `raft.entrycache.hits` 30163: kv: Don't evict from leaseholder cache on context cancellations r=a-robinson a=a-robinson This was a major contributor to the hundreds of NotLeaseHolderErrors per second that we see whenever we run tpc-c at scale. A non-essential batch request like a QueryTxn would get cancelled, causing the range to be evicted from the leaseholder cache and the next request to that range to have to guess at the leaseholder. This is effectively an extension of #26764 that we should have thought to inspect more closely at the time. Actually fixes #23543, which was not fully fixed before. Although I still haven't seen the errors drop all the way to 0, so I'm letting tpc-c 10k continue to run for a while longer to verify that they do. They are continuing to decrease about 15 minutes in. I don't think getting to 0 will be possible because there are still occasional splits and lease transfers), but it looks like it should be able to get down to single digit errors per second from the hundreds it was at before this change. Also, avoid doing unnecessary sorting by latency of replicas in the dist_sender in the common case when we know who the leaseholder is and plan on sending our request there. 30197: sql/parser: fix the action for empty rules r=knz a=knz Fixes #30141. Co-authored-by: Nikhil Benesch <[email protected]> Co-authored-by: Jordan Lewis <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Alex Robinson <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>

craig · 2018-09-13T20:18:47Z

Build succeeded

GitHub CI (Cockroach)

couchand · 2018-09-19T17:19:59Z

@benesch seems like we should backport this, no?

benesch · 2018-09-24T16:24:26Z

Yep, done!

benesch requested review from vilterp, couchand and a team September 7, 2018 03:17

benesch commented Sep 7, 2018

View reviewed changes

benesch force-pushed the ui-html branch from b498b9c to a0ee70d Compare September 7, 2018 03:52

benesch commented Sep 7, 2018

View reviewed changes

vilterp approved these changes Sep 12, 2018

View reviewed changes

benesch added 4 commits September 12, 2018 22:18

ui: simplify init with template.Must

1c03f33

Release note: None

server: fix control flow in serveUIAssets

2dc17ec

This function was missing a return in one place and had an extraneous return in another. Release note: None

server: better error handling in authenticationMux

f3f64b4

Don't attempt to use non-error return values when the error is non-nil. That's not idiomatic Go. Release note: None

server: don't break content type sniffing when gzipping

d1d666c

Release note: None

benesch force-pushed the ui-html branch from a0ee70d to 35f0d42 Compare September 13, 2018 02:19

couchand suggested changes Sep 13, 2018

View reviewed changes

benesch force-pushed the ui-html branch from 35f0d42 to 9333e4f Compare September 13, 2018 16:53

benesch commented Sep 13, 2018

View reviewed changes

couchand suggested changes Sep 13, 2018

View reviewed changes

benesch commented Sep 13, 2018

View reviewed changes

craig bot merged commit 9333e4f into cockroachdb:master Sep 13, 2018

benesch deleted the ui-html branch September 16, 2018 20:24

benesch mentioned this pull request Sep 24, 2018

release-2.1: ui: various glue fixes #30570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: various glue fixes #29692

ui: various glue fixes #29692

benesch commented Sep 7, 2018

cockroach-teamcity commented Sep 7, 2018

benesch left a comment

benesch left a comment

benesch commented Sep 12, 2018

vilterp left a comment

vilterp Sep 12, 2018

couchand left a comment

benesch left a comment

couchand left a comment

benesch left a comment

craig bot commented Sep 13, 2018

benesch commented Sep 13, 2018

craig bot commented Sep 13, 2018

couchand commented Sep 19, 2018

benesch commented Sep 24, 2018

	if splitMergeUnlock, err := r.maybeAcquireSplitMergeLock(ctx, raftCmd); err != nil {
	log.Eventf(ctx, "unable to acquire split lock: %s", err)
	// Send a crash report because a former bug in the error handling might have
	// been the root cause of #19172.
	_ = r.store.stopper.RunAsyncTask(ctx, "crash report", func(ctx context.Context) {
	log.SendCrashReport(
	ctx,
	&r.store.cfg.Settings.SV,
	0, // depth
	"while acquiring split lock: %s",
	[]interface{}{err},
	)
	})

	forcedErr = roachpb.NewError(err)
	} else if splitMergeUnlock != nil {
	// Close over raftCmd to capture its value at execution time; we clear
	// ReplicatedEvalResult on certain errors.
	defer func() {
	splitMergeUnlock(raftCmd.ReplicatedEvalResult)
	}()

	_, _, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, initNone)
	defer cleanupFn()

	tempDir, tempDirCleanup := testutils.TempDir(t)
	defer tempDirCleanup()

ui: various glue fixes #29692

ui: various glue fixes #29692

Conversation

benesch commented Sep 7, 2018

cockroach-teamcity commented Sep 7, 2018

benesch left a comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

benesch commented Sep 12, 2018

vilterp left a comment

Choose a reason for hiding this comment

vilterp Sep 12, 2018

Choose a reason for hiding this comment

couchand left a comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

couchand left a comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

craig bot commented Sep 13, 2018

Build failed

benesch commented Sep 13, 2018

craig bot commented Sep 13, 2018

Build succeeded

couchand commented Sep 19, 2018

benesch commented Sep 24, 2018