From e4db845246d2a9982ba5ac7e422e92a51886dd0a Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Wed, 23 Sep 2020 12:37:33 +0100 Subject: [PATCH] Refactor uiserver to separate package, cleaner Reloading --- GNUmakefile | 2 +- agent/agent.go | 45 ++-- agent/agent_test.go | 30 --- agent/http.go | 260 +++++++---------------- agent/http_oss.go | 2 - agent/http_test.go | 107 +++++----- agent/reload.go | 7 + agent/testagent.go | 2 +- agent/{ => uiserver}/bindata_assetfs.go | 26 +-- agent/uiserver/buf_index_fs.go | 21 ++ agent/uiserver/buffered_file.go | 67 ++++++ agent/uiserver/redirect_fs.go | 21 ++ agent/uiserver/ui_template_data.go | 52 +++++ agent/uiserver/ui_template_data_oss.go | 9 + agent/uiserver/uiserver.go | 176 +++++++++++++++ agent/uiserver/uiserver_test.go | 230 ++++++++++++++++++++ build-support/functions/20-build.sh | 2 +- logging/names.go | 1 + ui-v2/config/environment.js | 20 +- ui-v2/lib/startup/templates/head.html.js | 2 + 20 files changed, 767 insertions(+), 315 deletions(-) create mode 100644 agent/reload.go rename agent/{ => uiserver}/bindata_assetfs.go (97%) create mode 100644 agent/uiserver/buf_index_fs.go create mode 100644 agent/uiserver/buffered_file.go create mode 100644 agent/uiserver/redirect_fs.go create mode 100644 agent/uiserver/ui_template_data.go create mode 100644 agent/uiserver/ui_template_data_oss.go create mode 100644 agent/uiserver/uiserver.go create mode 100644 agent/uiserver/uiserver_test.go diff --git a/GNUmakefile b/GNUmakefile index b9540879ee..87758e688f 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -306,7 +306,7 @@ lint: # also run as part of the release build script when it verifies that there are no # changes to the UI assets that aren't checked in. static-assets: - @go-bindata-assetfs -modtime 1 -pkg agent -prefix pkg -o $(ASSETFS_PATH) ./pkg/web_ui/... + @go-bindata-assetfs -modtime 1 -pkg uiserver -prefix pkg -o $(ASSETFS_PATH) ./pkg/web_ui/... @go fmt $(ASSETFS_PATH) diff --git a/agent/agent.go b/agent/agent.go index 43135abc49..a74eae9ce4 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -255,6 +255,23 @@ type Agent struct { // fail, the agent will be shutdown. apiServers *apiServers + // httpHandlers provides direct access to (one of) the HTTPHandlers started by + // this agent. This is used in tests to test HTTP endpoints without overhead + // of TCP connections etc. + // + // TODO: this is a temporary re-introduction after we removed a list of + // HTTPServers in favour of apiServers abstraction. Now that HTTPHandlers is + // stateful and has config reloading though it's not OK to just use a + // different instance of handlers in tests to the ones that the agent is wired + // up to since then config reloads won't actually affect the handlers under + // test while plumbing the external handlers in the TestAgent through bypasses + // testing that the agent itself is actually reloading the state correctly. + // Once we move `apiServers` to be a passed-in dependency for NewAgent, we + // should be able to remove this and have the Test Agent create the + // HTTPHandlers and pass them in removing the need to pull them back out + // again. + httpHandlers *HTTPHandlers + // wgServers is the wait group for all HTTP and DNS servers // TODO: remove once dnsServers are handled by apiServers wgServers sync.WaitGroup @@ -290,6 +307,11 @@ type Agent struct { // IP. httpConnLimiter connlimit.Limiter + // configReloaders are subcomponents that need to be notified on a reload so + // they can update their internal state. + configReloaders []ConfigReloader + + // enterpriseAgent embeds fields that we only access in consul-enterprise builds enterpriseAgent } @@ -333,9 +355,6 @@ func New(bd BaseDeps) (*Agent, error) { cache: bd.Cache, } - // Initialize the UI Config - a.uiConfig.Store(a.config.UIConfig) - a.serviceManager = NewServiceManager(&a) // TODO: do this somewhere else, maybe move to newBaseDeps @@ -737,6 +756,8 @@ func (a *Agent) listenHTTP() ([]apiServer, error) { agent: a, denylist: NewDenylist(a.config.HTTPBlockEndpoints), } + a.configReloaders = append(a.configReloaders, srv.ReloadConfig) + a.httpHandlers = srv httpServer := &http.Server{ Addr: l.Addr().String(), TLSConfig: tlscfg, @@ -3573,8 +3594,11 @@ func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error { a.State.SetDiscardCheckOutput(newCfg.DiscardCheckOutput) - // Reload metrics config - a.uiConfig.Store(newCfg.UIConfig) + for _, r := range a.configReloaders { + if err := r(newCfg); err != nil { + return err + } + } return nil } @@ -3828,14 +3852,3 @@ func defaultIfEmpty(val, defaultVal string) string { } return defaultVal } - -// getUIConfig is the canonical way to read the value of the UIConfig at -// runtime. It is thread safe and returns the most recent configuration which -// may have changed since the agent started due to config reload. -func (a *Agent) getUIConfig() config.UIConfig { - if cfg, ok := a.uiConfig.Load().(config.UIConfig); ok { - return cfg - } - // Shouldn't happen but be defensive - return config.UIConfig{} -} diff --git a/agent/agent_test.go b/agent/agent_test.go index df3f0e75b2..283e90c147 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3503,36 +3503,6 @@ func TestAgent_ReloadConfigTLSConfigFailure(t *testing.T) { require.Len(t, tlsConf.RootCAs.Subjects(), 1) } -func TestAgent_ReloadConfigUIConfig(t *testing.T) { - t.Parallel() - dataDir := testutil.TempDir(t, "agent") // we manage the data dir - hcl := ` - data_dir = "` + dataDir + `" - ui_config { - enabled = true // note that this is _not_ reloadable - metrics_provider = "foo" - } - ` - a := NewTestAgent(t, hcl) - defer a.Shutdown() - - uiCfg := a.getUIConfig() - require.Equal(t, "foo", uiCfg.MetricsProvider) - - hcl = ` - data_dir = "` + dataDir + `" - ui_config { - enabled = true - metrics_provider = "bar" - } - ` - c := TestConfig(testutil.Logger(t), config.FileSource{Name: t.Name(), Format: "hcl", Data: hcl}) - require.NoError(t, a.reloadConfigInternal(c)) - - uiCfg = a.getUIConfig() - require.Equal(t, "bar", uiCfg.MetricsProvider) -} - func TestAgent_consulConfig_AutoEncryptAllowTLS(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir diff --git a/agent/http.go b/agent/http.go index ea66ba72c9..43f31f688e 100644 --- a/agent/http.go +++ b/agent/http.go @@ -1,16 +1,13 @@ package agent import ( - "bytes" "encoding/json" "fmt" "io" - "io/ioutil" "net" "net/http" "net/http/pprof" "net/url" - "os" "reflect" "regexp" "strconv" @@ -21,8 +18,10 @@ import ( "github.com/armon/go-metrics" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/cache" + "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/agent/uiserver" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/logging" @@ -78,135 +77,12 @@ func (e ForbiddenError) Error() string { return "Access is restricted" } -// HTTPHandlers provides http.Handler functions for the HTTP APi. +// HTTPHandlers provides an HTTP api for an agent. type HTTPHandlers struct { - agent *Agent - denylist *Denylist -} - -// bufferedFile implements os.File and allows us to modify a file from disk by -// writing out the new version into a buffer and then serving file reads from -// that. It assumes you are modifying a real file and presents the actual file's -// info when queried. -type bufferedFile struct { - templated *bytes.Reader - info os.FileInfo -} - -func newBufferedFile(buf *bytes.Buffer, raw http.File) *bufferedFile { - info, _ := raw.Stat() - return &bufferedFile{ - templated: bytes.NewReader(buf.Bytes()), - info: info, - } -} - -func (t *bufferedFile) Read(p []byte) (n int, err error) { - return t.templated.Read(p) -} - -func (t *bufferedFile) Seek(offset int64, whence int) (int64, error) { - return t.templated.Seek(offset, whence) -} - -func (t *bufferedFile) Close() error { - return nil -} - -func (t *bufferedFile) Readdir(count int) ([]os.FileInfo, error) { - return nil, errors.New("not a directory") -} - -func (t *bufferedFile) Stat() (os.FileInfo, error) { - return t, nil -} - -func (t *bufferedFile) Name() string { - return t.info.Name() -} - -func (t *bufferedFile) Size() int64 { - return int64(t.templated.Len()) -} - -func (t *bufferedFile) Mode() os.FileMode { - return t.info.Mode() -} - -func (t *bufferedFile) ModTime() time.Time { - return t.info.ModTime() -} - -func (t *bufferedFile) IsDir() bool { - return false -} - -func (t *bufferedFile) Sys() interface{} { - return nil -} - -type redirectFS struct { - fs http.FileSystem -} - -func (fs *redirectFS) Open(name string) (http.File, error) { - file, err := fs.fs.Open(name) - if err != nil { - file, err = fs.fs.Open("/index.html") - } - return file, err -} - -type settingsInjectedIndexFS struct { - fs http.FileSystem - UISettings map[string]interface{} -} - -func (fs *settingsInjectedIndexFS) Open(name string) (http.File, error) { - file, err := fs.fs.Open(name) - if err != nil || name != "/index.html" { - return file, err - } - - content, err := ioutil.ReadAll(file) - if err != nil { - return nil, fmt.Errorf("failed reading index.html: %s", err) - } - file.Seek(0, 0) - - // Replace the placeholder in the meta ENV with the actual UI config settings. - // Ember passes the ENV with URL encoded JSON in a meta tag. We are replacing - // a key and value that is the encoded version of - // `"CONSUL_UI_SETTINGS_PLACEHOLDER":"__CONSUL_UI_SETTINGS_GO_HERE__"` - // with a URL-encoded JSON blob representing the actual config. - - // First built an escaped, JSON blob from the settings passed. - bs, err := json.Marshal(fs.UISettings) - if err != nil { - return nil, fmt.Errorf("failed marshalling UI settings JSON: %s", err) - } - // We want to remove the first and last chars which will be the { and } since - // we are injecting these variabled into the middle of an existing object. - bs = bytes.Trim(bs, "{}") - - // We use PathEscape because we don't want spaces to be turned into "+" like - // QueryEscape does. - escaped := url.PathEscape(string(bs)) - - content = bytes.Replace(content, - []byte("%22CONSUL_UI_SETTINGS_PLACEHOLDER%22%3A%22__CONSUL_UI_SETTINGS_GO_HERE__%22"), - []byte(escaped), 1) - - // We also need to inject the content path. This used to be a go template - // hence the syntax but for now simple string replacement is fine esp. since - // all the other templated stuff above can't easily be done that was as we are - // replacing an entire placeholder element in an encoded JSON blob with - // multiple encoded JSON elements. - if path, ok := fs.UISettings["CONSUL_CONTENT_PATH"].(string); ok { - content = bytes.Replace(content, []byte("{{.ContentPath}}"), []byte(path), -1) - } - - return newBufferedFile(bytes.NewBuffer(content), file), nil + agent *Agent + denylist *Denylist + configReloaders []ConfigReloader + h http.Handler } // endpoint is a Consul-specific HTTP handler that takes the usual arguments in @@ -249,8 +125,45 @@ func (w *wrappedMux) ServeHTTP(resp http.ResponseWriter, req *http.Request) { w.handler.ServeHTTP(resp, req) } -// handler is used to attach our handlers to the mux +// ReloadConfig updates any internal state when the config is changed at +// runtime. +func (s *HTTPHandlers) ReloadConfig(newCfg *config.RuntimeConfig) error { + for _, r := range s.configReloaders { + if err := r(newCfg); err != nil { + return err + } + } + return nil +} + +// handler is used to initialize the Handler. In agent code we only ever call +// this once during agent initialization so it was always intended as a single +// pass init method. However many test rely on it as a cheaper way to get a +// handler to call ServeHTTP against and end up calling it multiple times on a +// single agent instance. Until this method had to manage state that might be +// affected by a reload or otherwise vary over time that was not problematic +// although it was wasteful to redo all this setup work multiple times in one +// test. +// +// Now uiserver and possibly other components need to handle reloadable state +// having test randomly clobber the state with the original config again for +// each call gets confusing fast. So handler will memoize it's response - it's +// allowed to call it multiple times on the same agent, but it will only do the +// work the first time and return the same handler on subsequent calls. +// +// The `enableDebug` argument used in the first call will be effective and a +// later change will not do anything. The same goes for the initial config. For +// example if config is reloaded with UI enabled but it was not originally, the +// http.Handler returned will still have it disabled. +// +// The first call must not be concurrent with any other call. Subsequent calls +// may be concurrent with HTTP requests since no state is modified. func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { + // Memoize multiple calls. + if s.h != nil { + return s.h + } + mux := http.NewServeMux() // handleFuncMetrics takes the given pattern and handler and wraps to produce @@ -347,38 +260,27 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { handlePProf("/debug/pprof/trace", pprof.Trace) if s.IsUIEnabled() { - // Note that we _don't_ support reloading ui_config.{enabled,content_dir} - // since this only runs at initial startup. - - var uifs http.FileSystem - // Use the custom UI dir if provided. - uiConfig := s.agent.getUIConfig() - if uiConfig.Dir != "" { - uifs = http.Dir(uiConfig.Dir) - } else { - fs := assetFS() - uifs = fs - } + // Note that we _don't_ support reloading ui_config.{enabled, content_dir, + // content_path} since this only runs at initial startup. - uifs = &redirectFS{fs: &settingsInjectedIndexFS{ - fs: uifs, - UISettings: s.GetUIENVFromConfig(), - }} - // create a http handler using the ui file system - // and the headers specified by the http_config.response_headers user config - uifsWithHeaders := serveHandlerWithHeaders( - http.FileServer(uifs), + uiHandler := uiserver.NewHandler(s.agent.config, s.agent.logger.Named(logging.HTTP)) + s.configReloaders = append(s.configReloaders, uiHandler.ReloadConfig) + + // Wrap it to add the headers specified by the http_config.response_headers + // user config + uiHandlerWithHeaders := serveHandlerWithHeaders( + uiHandler, s.agent.config.HTTPResponseHeaders, ) mux.Handle( "/robots.txt", - uifsWithHeaders, + uiHandlerWithHeaders, ) mux.Handle( - uiConfig.ContentPath, + s.agent.config.UIConfig.ContentPath, http.StripPrefix( - uiConfig.ContentPath, - uifsWithHeaders, + s.agent.config.UIConfig.ContentPath, + uiHandlerWithHeaders, ), ) } @@ -391,37 +293,11 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { h = mux } h = s.enterpriseHandler(h) - return &wrappedMux{ + s.h = &wrappedMux{ mux: mux, handler: h, } -} - -func (s *HTTPHandlers) GetUIENVFromConfig() map[string]interface{} { - uiCfg := s.agent.getUIConfig() - - vars := map[string]interface{}{ - "CONSUL_CONTENT_PATH": uiCfg.ContentPath, - "CONSUL_ACLS_ENABLED": s.agent.config.ACLsEnabled, - "CONSUL_METRICS_PROVIDER": uiCfg.MetricsProvider, - // We explicitly MUST NOT pass the metrics_proxy object since it might - // contain add_headers with secrets that the UI shouldn't know e.g. API - // tokens for the backend. The provider should either require the proxy to - // be configured and then use that or hit the backend directly from the - // browser. - "CONSUL_METRICS_PROXY_ENABLED": uiCfg.MetricsProxy.BaseURL != "", - "CONSUL_DASHBOARD_URL_TEMPLATES": uiCfg.DashboardURLTemplates, - } - - // Only set this if there is some actual JSON or we'll cause a JSON - // marshalling error later during serving which ends up being silent. - if uiCfg.MetricsProviderOptionsJSON != "" { - vars["CONSUL_METRICS_PROVIDER_OPTIONS"] = json.RawMessage(uiCfg.MetricsProviderOptionsJSON) - } - - s.addEnterpriseUIENVVars(vars) - - return vars + return s.h } // nodeName returns the node name of the agent @@ -659,11 +535,17 @@ func (s *HTTPHandlers) marshalJSON(req *http.Request, obj interface{}) ([]byte, // Returns true if the UI is enabled. func (s *HTTPHandlers) IsUIEnabled() bool { - return s.agent.config.UIDir != "" || s.agent.config.EnableUI + // Note that we _don't_ support reloading ui_config.{enabled,content_dir} + // since this only runs at initial startup. + return s.agent.config.UIConfig.Dir != "" || s.agent.config.UIConfig.Enabled } // Renders a simple index page func (s *HTTPHandlers) Index(resp http.ResponseWriter, req *http.Request) { + // Send special headers too since this endpoint isn't wrapped with something + // that sends them. + setHeaders(resp, s.agent.config.HTTPResponseHeaders) + // Check if this is a non-index path if req.URL.Path != "/" { resp.WriteHeader(http.StatusNotFound) @@ -678,8 +560,12 @@ func (s *HTTPHandlers) Index(resp http.ResponseWriter, req *http.Request) { } // Redirect to the UI endpoint - uiCfg := s.agent.getUIConfig() - http.Redirect(resp, req, uiCfg.ContentPath, http.StatusMovedPermanently) // 301 + http.Redirect( + resp, + req, + s.agent.config.UIConfig.ContentPath, + http.StatusMovedPermanently, + ) // 301 } func decodeBody(body io.Reader, out interface{}) error { diff --git a/agent/http_oss.go b/agent/http_oss.go index e9439d3ef2..42009e8ad7 100644 --- a/agent/http_oss.go +++ b/agent/http_oss.go @@ -55,8 +55,6 @@ func (s *HTTPHandlers) rewordUnknownEnterpriseFieldError(err error) error { return err } -func (s *HTTPHandlers) addEnterpriseUIENVVars(_ map[string]interface{}) {} - func parseACLAuthMethodEnterpriseMeta(req *http.Request, _ *structs.ACLAuthMethodEnterpriseMeta) error { if methodNS := req.URL.Query().Get("authmethod-ns"); methodNS != "" { return BadRequestError{Reason: "Invalid query parameter: \"authmethod-ns\" - Namespaces are a Consul Enterprise feature"} diff --git a/agent/http_test.go b/agent/http_test.go index d927b3ac80..883bf70c27 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -20,6 +20,7 @@ import ( "time" "github.com/NYTimes/gziphandler" + "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/structs" tokenStore "github.com/hashicorp/consul/agent/token" "github.com/hashicorp/consul/api" @@ -417,62 +418,56 @@ func TestHTTPAPI_TranslateAddrHeader(t *testing.T) { func TestHTTPAPIResponseHeaders(t *testing.T) { t.Parallel() a := NewTestAgent(t, ` + ui_config { + # Explicitly disable UI so we can ensure the index replacement gets headers too. + enabled = false + } http_config { response_headers = { "Access-Control-Allow-Origin" = "*" "X-XSS-Protection" = "1; mode=block" + "X-Frame-Options" = "SAMEORIGIN" } } `) defer a.Shutdown() - resp := httptest.NewRecorder() - handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - return nil, nil - } + requireHasHeadersSet(t, a, "/v1/agent/self") - req, _ := http.NewRequest("GET", "/v1/agent/self", nil) - a.srv.wrap(handler, []string{"GET"})(resp, req) + // Check the Index page that just renders a simple message with UI disabled + // also gets the right headers. + requireHasHeadersSet(t, a, "/") +} - origin := resp.Header().Get("Access-Control-Allow-Origin") - if origin != "*" { - t.Fatalf("bad Access-Control-Allow-Origin: expected %q, got %q", "*", origin) - } +func requireHasHeadersSet(t *testing.T, a *TestAgent, path string) { + t.Helper() - xss := resp.Header().Get("X-XSS-Protection") - if xss != "1; mode=block" { - t.Fatalf("bad X-XSS-Protection header: expected %q, got %q", "1; mode=block", xss) - } + resp := httptest.NewRecorder() + req, _ := http.NewRequest("GET", path, nil) + a.srv.handler(true).ServeHTTP(resp, req) + + hdrs := resp.Header() + require.Equal(t, "*", hdrs.Get("Access-Control-Allow-Origin"), + "Access-Control-Allow-Origin header value incorrect") + + require.Equal(t, "1; mode=block", hdrs.Get("X-XSS-Protection"), + "X-XSS-Protection header value incorrect") } + func TestUIResponseHeaders(t *testing.T) { t.Parallel() a := NewTestAgent(t, ` http_config { response_headers = { "Access-Control-Allow-Origin" = "*" + "X-XSS-Protection" = "1; mode=block" "X-Frame-Options" = "SAMEORIGIN" } } `) defer a.Shutdown() - resp := httptest.NewRecorder() - handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - return nil, nil - } - - req, _ := http.NewRequest("GET", "/ui", nil) - a.srv.wrap(handler, []string{"GET"})(resp, req) - - origin := resp.Header().Get("Access-Control-Allow-Origin") - if origin != "*" { - t.Fatalf("bad Access-Control-Allow-Origin: expected %q, got %q", "*", origin) - } - - frameOptions := resp.Header().Get("X-Frame-Options") - if frameOptions != "SAMEORIGIN" { - t.Fatalf("bad X-XSS-Protection header: expected %q, got %q", "SAMEORIGIN", frameOptions) - } + requireHasHeadersSet(t, a, "/ui") } func TestAcceptEncodingGzip(t *testing.T) { @@ -1210,34 +1205,36 @@ func TestEnableWebUI(t *testing.T) { require.Equal(t, http.StatusOK, resp.Code) // Validate that it actually sent the index page we expect since an error - // during serving the special intercepted index.html in - // settingsInjectedIndexFS.Open will actually result in http.FileServer just - // serving a plain directory listing instead which still passes the above HTTP - // status assertion. This comment is part of our index.html template + // during serving the special intercepted index.html can result in an empty + // response but a 200 status. require.Contains(t, resp.Body.String(), ` + +