From fa4aad4a74af41dabe7922137f603addbb03cd79 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Wed, 21 Nov 2018 10:20:48 -0600 Subject: [PATCH] Refine basepath changes. - Refactoring work from https://github.com/runatlantis/atlantis/pull/314 - Use just the path in templates, not the fully qualified URL since that is a best practice. - Add logging to errors when rendering templates. - Silence help output on cli errors since the help output is now so large that you have to scroll up to see the errors. --- cmd/server.go | 5 +- server/locks_controller.go | 9 ++- server/locks_controller_test.go | 6 ++ server/router.go | 16 +++-- server/router_test.go | 63 ++++++++++++++----- server/server.go | 48 ++++++++++----- server/server_test.go | 106 ++++++++++++++++++++++++++++++-- server/url.go | 24 -------- server/url_test.go | 62 ------------------- server/web_templates.go | 44 +++++++------ 10 files changed, 233 insertions(+), 150 deletions(-) delete mode 100644 server/url.go delete mode 100644 server/url_test.go diff --git a/cmd/server.go b/cmd/server.go index 62e9174f7..36bd28e50 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -74,7 +74,7 @@ const redTermEnd = "\033[39m" var stringFlags = []stringFlag{ { name: AtlantisURLFlag, - description: "URL that Atlantis can be reached at. Defaults to http://$(hostname):$port where $port is from --" + PortFlag + ". Supports a base path, e.g. https://example.com/basepath", + description: "URL that Atlantis can be reached at. Defaults to http://$(hostname):$port where $port is from --" + PortFlag + ". Supports a base path ex. https://example.com/basepath.", }, { name: BitbucketUserFlag, @@ -254,7 +254,7 @@ func (s *ServerCmd) Init() *cobra.Command { Short: "Start the atlantis server", Long: `Start the atlantis server and listen for webhook calls.`, SilenceErrors: true, - SilenceUsage: s.SilenceOutput, + SilenceUsage: true, PreRunE: s.withErrPrint(func(cmd *cobra.Command, args []string) error { return s.preRun() }), @@ -344,6 +344,7 @@ func (s *ServerCmd) run() error { server, err := s.ServerCreator.NewServer(userConfig, server.Config{ AllowForkPRsFlag: AllowForkPRsFlag, AllowRepoConfigFlag: AllowRepoConfigFlag, + AtlantisURLFlag: AtlantisURLFlag, AtlantisVersion: s.AtlantisVersion, }) if err != nil { diff --git a/server/locks_controller.go b/server/locks_controller.go index 83a5c21cb..9c69abfa5 100644 --- a/server/locks_controller.go +++ b/server/locks_controller.go @@ -16,7 +16,7 @@ import ( // LocksController handles all requests relating to Atlantis locks. type LocksController struct { AtlantisVersion string - AtlantisURL url.URL + AtlantisURL *url.URL Locker locking.Locker Logger *logging.SimpleLogger VCSClient vcs.ClientProxy @@ -58,9 +58,12 @@ func (l *LocksController) GetLock(w http.ResponseWriter, r *http.Request) { LockedBy: lock.Pull.Author, Workspace: lock.Workspace, AtlantisVersion: l.AtlantisVersion, - AtlantisURL: l.AtlantisURL, + CleanedBasePath: l.AtlantisURL.Path, + } + err = l.LockDetailTemplate.Execute(w, viewData) + if err != nil { + l.Logger.Err(err.Error()) } - l.LockDetailTemplate.Execute(w, viewData) // nolint: errcheck } // DeleteLock handles deleting the lock at id and commenting back on the diff --git a/server/locks_controller_test.go b/server/locks_controller_test.go index ce6929e49..7d76130aa 100644 --- a/server/locks_controller_test.go +++ b/server/locks_controller_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "net/url" "reflect" "testing" @@ -18,6 +19,7 @@ import ( vcsmocks "github.com/runatlantis/atlantis/server/events/vcs/mocks" "github.com/runatlantis/atlantis/server/logging" sMocks "github.com/runatlantis/atlantis/server/mocks" + . "github.com/runatlantis/atlantis/testing" ) func AnyRepo() models.Repo { @@ -90,11 +92,14 @@ func TestGetLock_Success(t *testing.T) { Workspace: "workspace", }, nil) tmpl := sMocks.NewMockTemplateWriter() + atlantisURL, err := url.Parse("https://example.com/basepath") + Ok(t, err) lc := server.LocksController{ Logger: logging.NewNoopLogger(), Locker: l, LockDetailTemplate: tmpl, AtlantisVersion: "1300135", + AtlantisURL: atlantisURL, } req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req = mux.SetURLVars(req, map[string]string{"id": "id"}) @@ -109,6 +114,7 @@ func TestGetLock_Success(t *testing.T) { LockedBy: "lkysow", Workspace: "workspace", AtlantisVersion: "1300135", + CleanedBasePath: "/basepath", }) responseContains(t, w, http.StatusOK, "") } diff --git a/server/router.go b/server/router.go index f9f284d5d..18e105594 100644 --- a/server/router.go +++ b/server/router.go @@ -1,7 +1,6 @@ package server import ( - "fmt" "net/url" "github.com/gorilla/mux" @@ -19,13 +18,18 @@ type Router struct { // LockViewRouteIDQueryParam is the query parameter needed to construct the // lock view: underlying.Get(LockViewRouteName).URL(LockViewRouteIDQueryParam, "my id"). LockViewRouteIDQueryParam string - // AtlantisURL is the fully qualified URL (scheme included) that Atlantis is - // being served at, ex: https://example.com. - AtlantisURL url.URL + // AtlantisURL is the fully qualified URL that Atlantis is + // accessible from externally. + AtlantisURL *url.URL } // GenerateLockURL returns a fully qualified URL to view the lock at lockID. func (r *Router) GenerateLockURL(lockID string) string { - path, _ := r.Underlying.Get(r.LockViewRouteName).URL(r.LockViewRouteIDQueryParam, url.QueryEscape(lockID)) - return fmt.Sprintf("%s%s", r.AtlantisURL.String(), path) + lockURL, _ := r.Underlying.Get(r.LockViewRouteName).URL(r.LockViewRouteIDQueryParam, url.QueryEscape(lockID)) + // At this point, lockURL will just be a path because r.Underlying isn't + // configured with host or scheme information. So to generate the fully + // qualified LockURL we just append the router's url to our base url. + // We're not doing anything fancy here with the actual url object because + // golang likes to double escape the lockURL path when using url.Parse(). + return r.AtlantisURL.String() + lockURL.String() } diff --git a/server/router_test.go b/server/router_test.go index 6efef7e4a..3a79d5640 100644 --- a/server/router_test.go +++ b/server/router_test.go @@ -2,7 +2,6 @@ package server_test import ( "net/http" - "net/url" "testing" "github.com/gorilla/mux" @@ -11,19 +10,53 @@ import ( ) func TestRouter_GenerateLockURL(t *testing.T) { - queryParam := "queryparam" - routeName := "routename" - atlantisURL, err := url.Parse("https://example.com") - Ok(t, err) - - underlyingRouter := mux.NewRouter() - underlyingRouter.HandleFunc("/lock", func(_ http.ResponseWriter, _ *http.Request) {}).Methods("GET").Queries(queryParam, "{queryparam}").Name(routeName) - - router := &server.Router{ - AtlantisURL: *atlantisURL, - LockViewRouteIDQueryParam: queryParam, - LockViewRouteName: routeName, - Underlying: underlyingRouter, + cases := []struct { + AtlantisURL string + ExpURL string + }{ + { + "http://localhost:4141", + "http://localhost:4141/lock?id=lkysow%252Fatlantis-example%252F.%252Fdefault", + }, + { + "https://localhost:4141", + "https://localhost:4141/lock?id=lkysow%252Fatlantis-example%252F.%252Fdefault", + }, + { + "https://localhost:4141/", + "https://localhost:4141/lock?id=lkysow%252Fatlantis-example%252F.%252Fdefault", + }, + { + "https://example.com/basepath", + "https://example.com/basepath/lock?id=lkysow%252Fatlantis-example%252F.%252Fdefault", + }, + { + "https://example.com/basepath/", + "https://example.com/basepath/lock?id=lkysow%252Fatlantis-example%252F.%252Fdefault", + }, + { + "https://example.com/path/1/", + "https://example.com/path/1/lock?id=lkysow%252Fatlantis-example%252F.%252Fdefault", + }, + } + + queryParam := "id" + routeName := "routename" + underlyingRouter := mux.NewRouter() + underlyingRouter.HandleFunc("/lock", func(_ http.ResponseWriter, _ *http.Request) {}).Methods("GET").Queries(queryParam, "{id}").Name(routeName) + + for _, c := range cases { + t.Run(c.AtlantisURL, func(t *testing.T) { + atlantisURL, err := server.ParseAtlantisURL(c.AtlantisURL) + Ok(t, err) + + router := &server.Router{ + AtlantisURL: atlantisURL, + LockViewRouteIDQueryParam: queryParam, + LockViewRouteName: routeName, + Underlying: underlyingRouter, + } + Equals(t, c.ExpURL, router.GenerateLockURL("lkysow/atlantis-example/./default")) + }) } - Equals(t, "https://example.com/lock?queryparam=myid", router.GenerateLockURL("myid")) } diff --git a/server/server.go b/server/server.go index ac74e48b1..c81f649cc 100644 --- a/server/server.go +++ b/server/server.go @@ -64,7 +64,7 @@ const ( // Server runs the Atlantis web server. type Server struct { AtlantisVersion string - AtlantisURL url.URL + AtlantisURL *url.URL Router *mux.Router Port int CommandRunner *events.DefaultCommandRunner @@ -115,6 +115,7 @@ type UserConfig struct { type Config struct { AllowForkPRsFlag string AllowRepoConfigFlag string + AtlantisURLFlag string AtlantisVersion string } @@ -230,17 +231,14 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { projectLocker := &events.DefaultProjectLocker{ Locker: lockingClient, } - atlantisURL, err := url.Parse(userConfig.AtlantisURL) + parsedURL, err := ParseAtlantisURL(userConfig.AtlantisURL) if err != nil { - return nil, errors.Wrap(err, "parsing atlantis URL") - } - atlantisURL, err = NormalizeBaseURL(atlantisURL) - if err != nil { - return nil, errors.Wrap(err, "normalizing atlantis URL") + return nil, errors.Wrapf(err, + "parsing --%s flag %q", config.AtlantisURLFlag, userConfig.AtlantisURL) } underlyingRouter := mux.NewRouter() router := &Router{ - AtlantisURL: *atlantisURL, + AtlantisURL: parsedURL, LockViewRouteIDQueryParam: LockViewRouteIDQueryParam, LockViewRouteName: LockViewRouteName, Underlying: underlyingRouter, @@ -318,7 +316,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { } locksController := &LocksController{ AtlantisVersion: config.AtlantisVersion, - AtlantisURL: *atlantisURL, + AtlantisURL: parsedURL, Locker: lockingClient, Logger: logger, VCSClient: vcsClient, @@ -344,7 +342,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { } return &Server{ AtlantisVersion: config.AtlantisVersion, - AtlantisURL: *atlantisURL, + AtlantisURL: parsedURL, Router: underlyingRouter, Port: userConfig.Port, CommandRunner: commandRunner, @@ -422,18 +420,22 @@ func (s *Server) Index(w http.ResponseWriter, _ *http.Request) { for id, v := range locks { lockURL, _ := s.Router.Get(LockViewRouteName).URL("id", url.QueryEscape(id)) lockResults = append(lockResults, LockIndexData{ - LockURL: *lockURL, + // NOTE: must use .String() instead of .Path because we need the + // query params as part of the lock URL. + LockPath: lockURL.String(), RepoFullName: v.Project.RepoFullName, PullNum: v.Pull.Num, Time: v.Time, }) } - // nolint: errcheck - s.IndexTemplate.Execute(w, IndexData{ + err = s.IndexTemplate.Execute(w, IndexData{ Locks: lockResults, AtlantisVersion: s.AtlantisVersion, - AtlantisURL: s.AtlantisURL, + CleanedBasePath: s.AtlantisURL.Path, }) + if err != nil { + s.Logger.Err(err.Error()) + } } // Healthz returns the health check response. It always returns a 200 currently. @@ -451,3 +453,21 @@ func (s *Server) Healthz(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") w.Write(data) // nolint: errcheck } + +// ParseAtlantisURL parses the user-passed atlantis URL to ensure it is valid +// and we can use it in our templates. +// It removes any trailing slashes from the path so we can concatenate it +// with other paths without checking. +func ParseAtlantisURL(u string) (*url.URL, error) { + parsed, err := url.Parse(u) + if err != nil { + return nil, err + } + if !(parsed.Scheme == "http" || parsed.Scheme == "https") { + return nil, errors.New("http or https must be specified") + } + // We want the path to end without a trailing slash so we know how to + // use it in the rest of the program. + parsed.Path = strings.TrimSuffix(parsed.Path, "/") + return parsed, nil +} diff --git a/server/server_test.go b/server/server_test.go index 7a9147cbb..951d1f6e1 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -44,6 +44,18 @@ func TestNewServer(t *testing.T) { Ok(t, err) } +func TestNewServer_InvalidAtlantisURL(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "") + Ok(t, err) + _, err = server.NewServer(server.UserConfig{ + DataDir: tmpDir, + AtlantisURL: "example.com", + }, server.Config{ + AtlantisURLFlag: "atlantis-url", + }) + ErrEquals(t, "parsing --atlantis-url flag \"example.com\": http or https must be specified", err) +} + func TestIndex_LockErr(t *testing.T) { t.Log("index should return a 503 if unable to list locks") RegisterMockTestingT(t) @@ -65,12 +77,12 @@ func TestIndex_Success(t *testing.T) { // These are the locks that we expect to be rendered. now := time.Now() locks := map[string]models.ProjectLock{ - "id1": { + "lkysow/atlantis-example/./default": { Pull: models.PullRequest{ Num: 9, }, Project: models.Project{ - RepoFullName: "owner/repo", + RepoFullName: "lkysow/atlantis-example", }, Time: now, }, @@ -80,12 +92,16 @@ func TestIndex_Success(t *testing.T) { r := mux.NewRouter() atlantisVersion := "0.3.1" // Need to create a lock route since the server expects this route to exist. - r.NewRoute().Path("").Name(server.LockViewRouteName) + r.NewRoute().Path("/lock"). + Queries("id", "{id}").Name(server.LockViewRouteName) + u, err := url.Parse("https://example.com") + Ok(t, err) s := server.Server{ Locker: l, IndexTemplate: it, Router: r, AtlantisVersion: atlantisVersion, + AtlantisURL: u, } req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) w := httptest.NewRecorder() @@ -93,8 +109,8 @@ func TestIndex_Success(t *testing.T) { it.VerifyWasCalledOnce().Execute(w, server.IndexData{ Locks: []server.LockIndexData{ { - LockURL: url.URL{}, - RepoFullName: "owner/repo", + LockPath: "/lock?id=lkysow%252Fatlantis-example%252F.%252Fdefault", + RepoFullName: "lkysow/atlantis-example", PullNum: 9, Time: now, }, @@ -118,6 +134,86 @@ func TestHealthz(t *testing.T) { }`, string(body)) } +func TestParseAtlantisURL(t *testing.T) { + cases := []struct { + In string + ExpErr string + ExpURL string + }{ + // Valid URLs should work. + { + In: "https://example.com", + ExpURL: "https://example.com", + }, + { + In: "http://example.com", + ExpURL: "http://example.com", + }, + { + In: "http://example.com/", + ExpURL: "http://example.com", + }, + { + In: "http://example.com", + ExpURL: "http://example.com", + }, + { + In: "http://example.com:4141", + ExpURL: "http://example.com:4141", + }, + { + In: "http://example.com:4141/", + ExpURL: "http://example.com:4141", + }, + { + In: "http://example.com/baseurl", + ExpURL: "http://example.com/baseurl", + }, + { + In: "http://example.com/baseurl/", + ExpURL: "http://example.com/baseurl", + }, + { + In: "http://example.com/baseurl/test", + ExpURL: "http://example.com/baseurl/test", + }, + + // Must be valid URL. + { + In: "::", + ExpErr: "parse ::: missing protocol scheme", + }, + + // Must be absolute. + { + In: "/hi", + ExpErr: "http or https must be specified", + }, + + // Must have http or https scheme.. + { + In: "localhost/test", + ExpErr: "http or https must be specified", + }, + { + In: "httpl://localhost/test", + ExpErr: "http or https must be specified", + }, + } + + for _, c := range cases { + t.Run(c.In, func(t *testing.T) { + act, err := server.ParseAtlantisURL(c.In) + if c.ExpErr != "" { + ErrEquals(t, c.ExpErr, err) + } else { + Ok(t, err) + Equals(t, c.ExpURL, act.String()) + } + }) + } +} + func responseContains(t *testing.T, r *httptest.ResponseRecorder, status int, bodySubstr string) { t.Helper() body, err := ioutil.ReadAll(r.Result().Body) diff --git a/server/url.go b/server/url.go deleted file mode 100644 index 8bfb7e57d..000000000 --- a/server/url.go +++ /dev/null @@ -1,24 +0,0 @@ -package server - -import ( - "fmt" - "net/url" - "strings" -) - -// NormalizeBaseURL ensures the given URL is a valid base URL for Atlantis. -// -// URLs that are fundamentally invalid (e.g. "hi") will return an error. -// Otherwise, the returned URL will have no trailing slashes and be guaranteed -// to be suitable for use as a base URL. -func NormalizeBaseURL(u *url.URL) (*url.URL, error) { - if !u.IsAbs() { - return nil, fmt.Errorf("Base URLs must be absolute.") - } - if !(u.Scheme == "http" || u.Scheme == "https") { - return nil, fmt.Errorf("Base URLs must be HTTP or HTTPS.") - } - out := *u - out.Path = strings.TrimRight(out.Path, "/") - return &out, nil -} diff --git a/server/url_test.go b/server/url_test.go deleted file mode 100644 index 3fb001c59..000000000 --- a/server/url_test.go +++ /dev/null @@ -1,62 +0,0 @@ -package server_test - -import ( - "net/url" - "testing" - - "github.com/runatlantis/atlantis/server" - . "github.com/runatlantis/atlantis/testing" -) - -func TestNormalizeBaseURL_Valid(t *testing.T) { - t.Log("When given a valid base URL, NormalizeBaseURL returns such URLs unchanged.") - examples := []string{ - "https://example.com", - "https://example.com/some/path", - "http://example.com:8080", - } - for _, example := range examples { - url, err := url.Parse(example) - Ok(t, err) - normalized, err := server.NormalizeBaseURL(url) - Ok(t, err) - Equals(t, url, normalized) - } -} - -func TestNormalizeBaseURL_Relative(t *testing.T) { - t.Log("We do not allow relative URLs as base URLs.") - _, err := server.NormalizeBaseURL(&url.URL{Path: "hi"}) - Assert(t, err != nil, "should be an error") - Equals(t, "Base URLs must be absolute.", err.Error()) -} - -func TestNormalizeBaseURL_NonHTTP(t *testing.T) { - t.Log("Base URLs must be http or https.") - _, err := server.NormalizeBaseURL(&url.URL{Scheme: "ftp", Host: "example", Path: "hi"}) - Assert(t, err != nil, "should be an error") - Equals(t, "Base URLs must be HTTP or HTTPS.", err.Error()) -} - -func TestNormalizeBaseURL_TrailingSlashes(t *testing.T) { - t.Log("We strip off any trailing slashes from the base URL.") - examples := []struct { - input string - output string - }{ - {"https://example.com/", "https://example.com"}, - {"https://example.com/some/path/", "https://example.com/some/path"}, - {"http://example.com:8080/", "http://example.com:8080"}, - {"https://example.com//", "https://example.com"}, - {"https://example.com/path///", "https://example.com/path"}, - } - for _, example := range examples { - inputURL, err := url.Parse(example.input) - Ok(t, err) - outputURL, err := url.Parse(example.output) - Ok(t, err) - normalized, err := server.NormalizeBaseURL(inputURL) - Ok(t, err) - Equals(t, outputURL, normalized) - } -} diff --git a/server/web_templates.go b/server/web_templates.go index b25e14195..17ac82d1b 100644 --- a/server/web_templates.go +++ b/server/web_templates.go @@ -16,7 +16,6 @@ package server import ( "html/template" "io" - "net/url" "time" ) @@ -32,7 +31,7 @@ type TemplateWriter interface { // LockIndexData holds the fields needed to display the index view for locks. type LockIndexData struct { - LockURL url.URL + LockPath string RepoFullName string PullNum int Time time.Time @@ -42,7 +41,10 @@ type LockIndexData struct { type IndexData struct { Locks []LockIndexData AtlantisVersion string - AtlantisURL url.URL + // CleanedBasePath is the path Atlantis is accessible at externally. If + // not using a path-based proxy, this will be an empty string. Never ends + // in a '/' (hence "cleaned"). + CleanedBasePath string } var indexTemplate = template.Must(template.New("index.html.tmpl").Parse(` @@ -54,7 +56,7 @@ var indexTemplate = template.Must(template.New("index.html.tmpl").Parse(` - + - - - - + + + +
- +

atlantis

Plan discarded and unlocked!

@@ -84,8 +86,9 @@ var indexTemplate = template.Must(template.New("index.html.tmpl").Parse(`

Locks

{{ if .Locks }} + {{ $basePath := .CleanedBasePath }} {{ range .Locks }} - +
{{.RepoFullName}} - #{{.PullNum}}
Locked
@@ -116,7 +119,10 @@ type LockDetailData struct { Workspace string Time time.Time AtlantisVersion string - AtlantisURL url.URL + // CleanedBasePath is the path Atlantis is accessible at externally. If + // not using a path-based proxy, this will be an empty string. Never ends + // in a '/' (hence "cleaned"). + CleanedBasePath string } var lockTemplate = template.Must(template.New("lock.html.tmpl").Parse(` @@ -128,16 +134,16 @@ var lockTemplate = template.Must(template.New("lock.html.tmpl").Parse(` - - - - - + + + + +
- +

atlantis

{{.LockKey}} Locked

@@ -202,10 +208,10 @@ v{{ .AtlantisVersion }} btnDiscard.click(function() { $.ajax({ - url: '{{ .AtlantisURL }}/locks?id='+lockId, + url: '{{ .CleanedBasePath }}/locks?id='+lockId, type: 'DELETE', success: function(result) { - window.location.replace("{{ .AtlantisURL }}/?discard=true"); + window.location.replace("{{ .CleanedBasePath }}/?discard=true"); } }); });