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 running Atlantis behind a path-based proxy #357

Merged
merged 2 commits into from
Nov 21, 2018
Merged

Allow running Atlantis behind a path-based proxy #357

merged 2 commits into from
Nov 21, 2018

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Nov 21, 2018

Allows users to run Atlantis behind a path-based proxy (ex. https://proxy.com/basepath). This change will cause the Atlantis UI to add /basepath to all its URLs.

To use, run atlantis server --atlantis-url https://proxy.com/basepath.

Notes

Resolves #213

Allows users to run Atlantis behind a shared reverse proxy, which is a fairly
common use case.
Copy link
Member Author

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

@jml I've added a bunch of comments to the changes I made to your code. Thought this would be faster than commenting on each one but still lets you know why I wanted certain changes made.

@@ -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 + ".",
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.",
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a . at end to match rest of help output.

@@ -16,6 +16,7 @@ import (
// LocksController handles all requests relating to Atlantis locks.
type LocksController struct {
AtlantisVersion string
AtlantisURL *url.URL
Copy link
Member Author

Choose a reason for hiding this comment

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

Passing around the reference because that's what's returned from url.Parse and would rather trust that they return a reference for a reason.

@@ -57,8 +58,12 @@ func (l *LocksController) GetLock(w http.ResponseWriter, r *http.Request) {
LockedBy: lock.Pull.Author,
Workspace: lock.Workspace,
AtlantisVersion: l.AtlantisVersion,
CleanedBasePath: l.AtlantisURL.Path,
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed this from AtlantisURL and am only passing the path around because think it's a best practice to only use paths in html so that it doesn't necessarily have to be served from the original domain.

queryParam := "queryparam"
routeName := "routename"
atlantisURL := "https://example.com"
cases := []struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved all your tests into a single case. No real reason other than it was more concise. The way you had it was fine too.

@@ -229,9 +231,14 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
projectLocker := &events.DefaultProjectLocker{
Locker: lockingClient,
}
parsedURL, err := ParseAtlantisURL(userConfig.AtlantisURL)
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the call to url.Parse inside this function so that all the validation happened in the same place

// 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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I kept this in server.go because it's small and I didn't feel it warranted its own file. Just personal preference here.

if err != nil {
return nil, err
}
if !(parsed.Scheme == "http" || parsed.Scheme == "https") {
Copy link
Member Author

Choose a reason for hiding this comment

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

The call to .IsAbs() returns true if the scheme isn't empty so we didn't need that since we're already checking the scheme here.

@@ -116,6 +122,80 @@ func TestHealthz(t *testing.T) {
}`, string(body))
}

func TestParseAtlantisURL(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, here is where I consolidated your tests I mean

@@ -82,8 +83,9 @@ var indexTemplate = template.Must(template.New("index.html.tmpl").Parse(`
<section>
<p class="title-heading small"><strong>Locks</strong></p>
{{ if .Locks }}
{{ $basePath := .CleanedBasePath }}
Copy link
Member Author

Choose a reason for hiding this comment

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

I think you were missing this from your PR which caused the template to not render if there were any locks. You can't reference .CleanedBasePath from inside the range because . is redefined to be the element in the range. Thus you need to define a variable reference to it before entering the range.

@codecov
Copy link

codecov bot commented Nov 21, 2018

Codecov Report

Merging #357 into master will increase coverage by 0.05%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   70.67%   70.72%   +0.05%     
==========================================
  Files          61       61              
  Lines        3655     3676      +21     
==========================================
+ Hits         2583     2600      +17     
- Misses        893      895       +2     
- Partials      179      181       +2
Impacted Files Coverage Δ
server/router.go 100% <100%> (ø) ⬆️
cmd/server.go 79.16% <100%> (+0.14%) ⬆️
server/locks_controller.go 89.7% <50%> (-2.61%) ⬇️
server/server.go 64.12% <90%> (+1.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6dcd65...fa4aad4. Read the comment docs.

@jml
Copy link
Contributor

jml commented Nov 21, 2018

Thanks!

- 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.
@lkysow lkysow merged commit f1cced0 into master Nov 21, 2018
@lkysow lkysow deleted the basepath branch November 21, 2018 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants