Refine basepath changes.

- Refactoring work from #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.
runatlantis · Nov 21, 2018 · fa4aad4 · fa4aad4
1 parent 670131f
commit fa4aad4
Show file tree

Hide file tree

Showing 10 changed files with 230 additions and 147 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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)
+	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, "{queryparam}").Name(routeName)
+	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,
+			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
@@ -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
+}