Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow overriding base path for UI/API routes; rm --query.prefix #748

Merged
merged 7 commits into from
Mar 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/query/app/fixture/index.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<!DOCTYPE html>
<html lang="en">
<meta charset="UTF-8">
<base href="/" data-inject-target="BASE_URL"/>
Copy link
Member Author

@yurishkuro yurishkuro Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffon this models the compiled index.html from UI assets.

Q: is it possible that babel/webpack would reorder the href and data-inject-target attributes? Because if it does, the search&replace in the query service won't work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro It should not change the order of the attributes.

<title>Test Page</title>
<!-- JAEGER_CONFIG=DEFAULT_CONFIG; -->
</html>
1 change: 1 addition & 0 deletions cmd/query/app/fixture/static/asset.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
some asset
12 changes: 6 additions & 6 deletions cmd/query/app/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

const (
queryPort = "query.port"
queryPrefix = "query.prefix"
queryBasePath = "query.base-path"
queryStaticFiles = "query.static-files"
queryUIConfig = "query.ui-config"
queryHealthCheckHTTPPort = "query.health-check-http-port"
Expand All @@ -32,8 +32,8 @@ const (
type QueryOptions struct {
// Port is the port that the query service listens in on
Port int
// Prefix is the prefix of the query service api
Prefix string
// BasePath is the prefix for all UI and API HTTP routes
BasePath string
// StaticAssets is the path for the static assets for the UI (https://github.com/uber/jaeger-ui)
StaticAssets string
// UIConfig is the path to a configuration file for the UI
Expand All @@ -45,16 +45,16 @@ type QueryOptions struct {
// AddFlags adds flags for QueryOptions
func AddFlags(flagSet *flag.FlagSet) {
flagSet.Int(queryPort, 16686, "The port for the query service")
flagSet.String(queryPrefix, "api", "The prefix for the url of the query service")
flagSet.String(queryStaticFiles, "jaeger-ui-build/build/", "The path for the static assets for the UI")
flagSet.String(queryBasePath, "/", "The base path for all HTTP routes, e.g. /jaeger; useful when running behind a reverse proxy")
flagSet.String(queryStaticFiles, "jaeger-ui-build/build/", "The directory path for the static assets for the UI")
flagSet.String(queryUIConfig, "", "The path to the UI configuration file in JSON format")
flagSet.Int(queryHealthCheckHTTPPort, 16687, "The http port for the health check service")
}

// InitFromViper initializes QueryOptions with properties from viper
func (qOpts *QueryOptions) InitFromViper(v *viper.Viper) *QueryOptions {
qOpts.Port = v.GetInt(queryPort)
qOpts.Prefix = v.GetString(queryPrefix)
qOpts.BasePath = v.GetString(queryBasePath)
qOpts.StaticAssets = v.GetString(queryStaticFiles)
qOpts.UIConfig = v.GetString(queryUIConfig)
qOpts.HealthCheckHTTPPort = v.GetInt(queryHealthCheckHTTPPort)
Expand Down
4 changes: 2 additions & 2 deletions cmd/query/app/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ func TestQueryBuilderFlags(t *testing.T) {
command.ParseFlags([]string{
"--query.static-files=/dev/null",
"--query.ui-config=some.json",
"--query.prefix=api",
"--query.base-path=/jaeger",
"--query.port=80",
})
qOpts := new(QueryOptions).InitFromViper(v)
assert.Equal(t, "/dev/null", qOpts.StaticAssets)
assert.Equal(t, "some.json", qOpts.UIConfig)
assert.Equal(t, "api", qOpts.Prefix)
assert.Equal(t, "/jaeger", qOpts.BasePath)
assert.Equal(t, 80, qOpts.Port)
}
11 changes: 6 additions & 5 deletions cmd/query/app/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const (

defaultDependencyLookbackDuration = time.Hour * 24
defaultTraceQueryLookbackDuration = time.Hour * 24 * 2
defaultHTTPPrefix = "api"
defaultAPIPrefix = "api"
)

var (
Expand Down Expand Up @@ -78,7 +78,8 @@ type APIHandler struct {
adjuster adjuster.Adjuster
logger *zap.Logger
queryParser queryParser
httpPrefix string
basePath string
apiPrefix string
tracer opentracing.Tracer
}

Expand All @@ -96,8 +97,8 @@ func NewAPIHandler(spanReader spanstore.Reader, dependencyReader dependencystore
for _, option := range options {
option(aH)
}
if aH.httpPrefix == "" {
aH.httpPrefix = defaultHTTPPrefix
if aH.apiPrefix == "" {
aH.apiPrefix = defaultAPIPrefix
}
if aH.adjuster == nil {
aH.adjuster = adjuster.Sequence(StandardAdjusters...)
Expand Down Expand Up @@ -141,7 +142,7 @@ func (aH *APIHandler) handleFunc(
}

func (aH *APIHandler) route(route string, args ...interface{}) string {
args = append([]interface{}{aH.httpPrefix}, args...)
args = append([]interface{}{aH.apiPrefix}, args...)
return fmt.Sprintf("/%s"+route, args...)
}

Expand Down
11 changes: 9 additions & 2 deletions cmd/query/app/handler_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,17 @@ func (handlerOptions) Adjusters(adjusters ...adjuster.Adjuster) HandlerOption {
}
}

// Prefix creates a HandlerOption that initializes prefix HTTP prefix of the API
// BasePath creates a HandlerOption that initializes the base path for all HTTP routes
func (handlerOptions) BasePath(prefix string) HandlerOption {
return func(apiHandler *APIHandler) {
apiHandler.basePath = prefix
}
}

// Prefix creates a HandlerOption that initializes the HTTP prefix for API routes
func (handlerOptions) Prefix(prefix string) HandlerOption {
return func(apiHandler *APIHandler) {
apiHandler.httpPrefix = prefix
apiHandler.apiPrefix = prefix
}
}

Expand Down
4 changes: 3 additions & 1 deletion cmd/query/app/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ func initializeTestServerWithHandler(options ...HandlerOption) (*httptest.Server
append(
[]HandlerOption{
HandlerOptions.Logger(zap.NewNop()),
HandlerOptions.Prefix(defaultHTTPPrefix),
// add options for test coverage
HandlerOptions.Prefix(defaultAPIPrefix),
HandlerOptions.BasePath("/"),
HandlerOptions.QueryLookbackDuration(defaultTraceQueryLookbackDuration),
},
options...,
Expand Down
53 changes: 47 additions & 6 deletions cmd/query/app/static_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,20 @@ import (
)

var (
staticRootFiles = []string{"favicon.ico"}
favoriteIcon = "favicon.ico"
staticRootFiles = []string{favoriteIcon}
configPattern = regexp.MustCompile("JAEGER_CONFIG *= *DEFAULT_CONFIG;")
basePathPattern = regexp.MustCompile(`<base href="/"`)
basePathReplace = `<base href="%s/"`
errBadBasePath = "Invalid base path '%s'. Must start but not end with a slash '/', e.g. '/jaeger/ui'"
)

// RegisterStaticHandler adds handler for static assets to the router.
func RegisterStaticHandler(r *mux.Router, logger *zap.Logger, qOpts *QueryOptions) {
staticHandler, err := NewStaticAssetsHandler(qOpts.StaticAssets, qOpts.UIConfig)
staticHandler, err := NewStaticAssetsHandler(qOpts.StaticAssets, StaticAssetsHandlerOptions{
BasePath: qOpts.BasePath,
UIConfigPath: qOpts.UIConfig,
})
if err != nil {
logger.Panic("Could not create static assets handler", zap.Error(err))
}
Expand All @@ -48,12 +55,19 @@ func RegisterStaticHandler(r *mux.Router, logger *zap.Logger, qOpts *QueryOption

// StaticAssetsHandler handles static assets
type StaticAssetsHandler struct {
options StaticAssetsHandlerOptions
staticAssetsRoot string
indexHTML []byte
}

// StaticAssetsHandlerOptions defines options for NewStaticAssetsHandler
type StaticAssetsHandlerOptions struct {
BasePath string
UIConfigPath string
}

// NewStaticAssetsHandler returns a StaticAssetsHandler
func NewStaticAssetsHandler(staticAssetsRoot string, uiConfig string) (*StaticAssetsHandler, error) {
func NewStaticAssetsHandler(staticAssetsRoot string, options StaticAssetsHandlerOptions) (*StaticAssetsHandler, error) {
if staticAssetsRoot == "" {
return nil, nil
}
Expand All @@ -65,7 +79,7 @@ func NewStaticAssetsHandler(staticAssetsRoot string, uiConfig string) (*StaticAs
return nil, errors.Wrap(err, "Cannot read UI static assets")
}
configString := "JAEGER_CONFIG = DEFAULT_CONFIG"
if config, err := loadUIConfig(uiConfig); err != nil {
if config, err := loadUIConfig(options.UIConfigPath); err != nil {
return nil, err
} else if config != nil {
// TODO if we want to support other config formats like YAML, we need to normalize `config` to be
Expand All @@ -74,9 +88,20 @@ func NewStaticAssetsHandler(staticAssetsRoot string, uiConfig string) (*StaticAs
bytes, _ := json.Marshal(config)
configString = fmt.Sprintf("JAEGER_CONFIG = %v", string(bytes))
}
indexBytes = configPattern.ReplaceAll(indexBytes, []byte(configString+";"))
if options.BasePath == "" {
options.BasePath = "/"
}
if options.BasePath != "/" {
if !strings.HasPrefix(options.BasePath, "/") || strings.HasSuffix(options.BasePath, "/") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a log statement notifying that what people see in the HTML is not what will be served to clients?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you elaborate why you think this would be useful? We're modifying the internal file of the app, not something that users provide.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly because it's deviating from what is "expected": if I had a problem with the static resources (like the original problem this PR is solving) and were to check the HTML, I would wonder why it's not the same as what my browser is seeing. A log entry at debug level would suffice, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, still not convinced why someone needs to know what happens internally, e.g. we also override the config, and in both cases we only do that when requested via cli arguments.

Let's do it separately, because the constructor doesn't take a logger and the unit tests explicitly verify that nothing is logged, so it's not as trivial a change as it sounds for a feature that seems rather marginal.

return nil, fmt.Errorf(errBadBasePath, options.BasePath)
}
indexBytes = basePathPattern.ReplaceAll(indexBytes, []byte(fmt.Sprintf(basePathReplace, options.BasePath)))
}
return &StaticAssetsHandler{
options: options,
staticAssetsRoot: staticAssetsRoot,
indexHTML: configPattern.ReplaceAll(indexBytes, []byte(configString+";")),
indexHTML: indexBytes,
}, nil
}

Expand Down Expand Up @@ -108,7 +133,7 @@ func loadUIConfig(uiConfig string) (map[string]interface{}, error) {

// RegisterRoutes registers routes for this handler on the given router
func (sH *StaticAssetsHandler) RegisterRoutes(router *mux.Router) {
router.PathPrefix("/static").Handler(http.FileServer(http.Dir(sH.staticAssetsRoot)))
router.PathPrefix("/static").Handler(sH.fileHandler())
for _, file := range staticRootFiles {
router.Path("/" + file).HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
http.ServeFile(w, r, sH.staticAssetsRoot+file)
Expand All @@ -117,6 +142,22 @@ func (sH *StaticAssetsHandler) RegisterRoutes(router *mux.Router) {
router.NotFoundHandler = http.HandlerFunc(sH.notFound)
}

func (sH *StaticAssetsHandler) fileHandler() http.Handler {
fs := http.FileServer(http.Dir(sH.staticAssetsRoot))
base := sH.options.BasePath
if base == "/" {
return fs
}
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if strings.HasPrefix(r.URL.Path, base) {
// gorilla Subroute() is a bit odd, it keeps the base path in the URL,
// which prevents the FileServer from locating the files, so we strip the prefix.
r.URL.Path = r.URL.Path[len(base):]
}
fs.ServeHTTP(w, r)
})
}

func (sH *StaticAssetsHandler) notFound(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "text/html; charset=utf-8")
w.Write(sH.indexHTML)
Expand Down
100 changes: 55 additions & 45 deletions cmd/query/app/static_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"io/ioutil"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"

Expand All @@ -30,32 +29,14 @@ import (
"github.com/jaegertracing/jaeger/pkg/testutils"
)

func TestStaticAssetsHandler(t *testing.T) {
r := mux.NewRouter()
handler, err := NewStaticAssetsHandler("fixture", "")
require.NoError(t, err)
handler.RegisterRoutes(r)
server := httptest.NewServer(r)
defer server.Close()

httpClient = &http.Client{
Timeout: 2 * time.Second,
}

resp, err := httpClient.Get(server.URL)
assert.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
}

func TestDefaultStaticAssetsRoot(t *testing.T) {
handler, err := NewStaticAssetsHandler("", "")
handler, err := NewStaticAssetsHandler("", StaticAssetsHandlerOptions{})
assert.Nil(t, handler)
assert.Nil(t, err)
}

func TestNotExistingUiConfig(t *testing.T) {
handler, err := NewStaticAssetsHandler("/foo/bar", "")
handler, err := NewStaticAssetsHandler("/foo/bar", StaticAssetsHandlerOptions{})
require.Error(t, err)
assert.Contains(t, err.Error(), "Cannot read UI static assets")
assert.Nil(t, handler)
Expand All @@ -77,40 +58,69 @@ func TestRegisterStaticHandlerNotCreated(t *testing.T) {
}

func TestRegisterStaticHandler(t *testing.T) {
logger, buf := testutils.NewLogger()
r := mux.NewRouter()
RegisterStaticHandler(r, logger, &QueryOptions{StaticAssets: "fixture/"})
assert.Empty(t, buf.String())

server := httptest.NewServer(r)
defer server.Close()
expectedRespString := "Test Favicon\n"

testCases := []struct {
basePath string // input to the test
subroute bool // should we create a subroute?
baseURL string // expected URL prefix
expectedBaseHTML string // substring to match in the home page
}{
{basePath: "", baseURL: "/", expectedBaseHTML: `<base href="/"`},
{basePath: "/", baseURL: "/", expectedBaseHTML: `<base href="/"`},
{basePath: "/jaeger", baseURL: "/jaeger/", expectedBaseHTML: `<base href="/jaeger/"`, subroute: true},
}
httpClient = &http.Client{
Timeout: 2 * time.Second,
}
for _, testCase := range testCases {
t.Run("basePath="+testCase.basePath, func(t *testing.T) {
logger, buf := testutils.NewLogger()
r := mux.NewRouter()
if testCase.subroute {
r = r.PathPrefix(testCase.basePath).Subrouter()
}
RegisterStaticHandler(r, logger, &QueryOptions{
StaticAssets: "fixture/",
BasePath: testCase.basePath,
UIConfig: "fixture/ui-config.json",
})
assert.Empty(t, buf.String(), "no logs during construction")

server := httptest.NewServer(r)
defer server.Close()

httpGet := func(path string) string {
resp, err := httpClient.Get(fmt.Sprintf("%s%s%s", server.URL, testCase.baseURL, path))
require.NoError(t, err)
defer resp.Body.Close()

respByteArray, err := ioutil.ReadAll(resp.Body)
require.NoError(t, err)
require.Equal(t, http.StatusOK, resp.StatusCode)
return string(respByteArray)
}

resp, err := httpClient.Get(fmt.Sprintf("%s/favicon.ico", server.URL))
require.NoError(t, err)
defer resp.Body.Close()
respString := httpGet(favoriteIcon)
assert.Contains(t, respString, "Test Favicon") // this text is present in fixtures/favicon.ico

respByteArray, _ := ioutil.ReadAll(resp.Body)
respString := string(respByteArray)
html := httpGet("") // get home page
assert.Contains(t, html, `JAEGER_CONFIG = {"x":"y"};`, "actual: %v", html)
assert.Contains(t, html, testCase.expectedBaseHTML, "actual: %v", html)

assert.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)
assert.Equal(t, expectedRespString, respString)
asset := httpGet("static/asset.txt")
assert.Contains(t, asset, "some asset", "actual: %v", asset)
})
}
}

func TestNewStaticAssetsHandlerWithConfig(t *testing.T) {
_, err := NewStaticAssetsHandler("fixture", "fixture/invalid-config")
func TestNewStaticAssetsHandlerErrors(t *testing.T) {
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{UIConfigPath: "fixture/invalid-config"})
assert.Error(t, err)

handler, err := NewStaticAssetsHandler("fixture", "fixture/ui-config.json")
require.NoError(t, err)
require.NotNil(t, handler)
html := string(handler.indexHTML)
assert.True(t, strings.Contains(html, `JAEGER_CONFIG = {"x":"y"};`), "actual: %v", html)
for _, base := range []string{"x", "x/", "/x/"} {
_, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{UIConfigPath: "fixture/ui-config.json", BasePath: base})
require.Error(t, err, "basePath=%s", base)
assert.Contains(t, err.Error(), "Invalid base path")
}
}

func TestLoadUIConfig(t *testing.T) {
Expand Down
4 changes: 3 additions & 1 deletion cmd/query/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ func main() {
}

apiHandlerOptions := []app.HandlerOption{
app.HandlerOptions.Prefix(queryOpts.Prefix),
app.HandlerOptions.Logger(logger),
app.HandlerOptions.Tracer(tracer),
}
Expand All @@ -119,6 +118,9 @@ func main() {
dependencyReader,
apiHandlerOptions...)
r := mux.NewRouter()
if queryOpts.BasePath != "/" {
r = r.PathPrefix(queryOpts.BasePath).Subrouter()
}
apiHandler.RegisterRoutes(r)
app.RegisterStaticHandler(r, logger, queryOpts)

Expand Down
Loading