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

Prefix URLs in templates with AtlantisURL #314

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

jml
Copy link
Contributor

@jml jml commented Oct 12, 2018

Resolves #213

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

make test passes for me, but make test-all does not, both on this branch and an unchanged master.

@codecov
Copy link

codecov bot commented Oct 12, 2018

Codecov Report

Merging #314 into master will increase coverage by 0.03%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #314      +/-   ##
=========================================
+ Coverage   70.67%   70.7%   +0.03%     
=========================================
  Files          61      62       +1     
  Lines        3655    3673      +18     
=========================================
+ Hits         2583    2597      +14     
- Misses        893     895       +2     
- Partials      179     181       +2
Impacted Files Coverage Δ
cmd/server.go 79.02% <ø> (ø) ⬆️
server/url.go 100% <100%> (ø)
server/locks_controller.go 92.42% <100%> (+0.11%) ⬆️
server/router.go 100% <100%> (ø) ⬆️
server/server.go 62.2% <63.63%> (-0.25%) ⬇️

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 d52b3fd...670131f. Read the comment docs.

Copy link
Member

@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.

Thanks for the PR!

A couple thoughts:

  1. Ideally we'd use a function like linkify but the template is compiled as a var so it would require a refactor to get the AtlantisURL into a FuncMap before the template is compiled. Thus for now I think this is an okay way to do it.
  2. We need to add some validation into cmd/server.go to ensure that users don't pass in an atlantis-url with a trailing /
  3. The docs for --atlantis-url should be updated to point out that a URL prefix is supported:
    URL that Atlantis can be reached at. Defaults to
    http://$(hostname):$port where $port is from --port.
    Supports a base path, ex. https://example.com/basepath.
    

<link rel="stylesheet" href="{{ .AtlantisURL }}/static/css/skeleton.css">
<link rel="stylesheet" href="{{ .AtlantisURL }}/static/css/custom.css">
<link rel="icon" type="image/png" href="{{ .AtlantisURL }}/static/images/atlantis-icon.png">
<scnript src="{{ .AtlantisURL }}/static/js/jquery-3.2.1.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like things got messed up here (snscript, </hea)

</head>
<body>
<div class="container">
<section class="header">
<a title="atlantis" href="/"><img src="/static/images/atlantis-icon.png"/></a>
<a title="atlantis" href="{{ .AtlantisURL }}/"><img src="{{ .AtlantisURL }}/static/images/atlantis-icon.png"/></a>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need the / at the end here for <a title="atlantis" href="{{ .AtlantisURL }}/">

@@ -83,7 +84,7 @@ var indexTemplate = template.Must(template.New("index.html.tmpl").Parse(`
<p class="title-heading small"><strong>Locks</strong></p>
{{ if .Locks }}
{{ range .Locks }}
<a href="{{.LockURL}}">
<a href="{{ .AtlantisURL }}/{{.LockURL}}">
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure .LockURL doesn't start with /?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, it always starts with a / (unless it starts with "https://..."!). Have adjusted accordingly.

<div class="container">
<section class="header">
<a title="atlantis" href="/"><img src="/static/images/atlantis-icon.png"/></a>
<a title="atlantis" href="{{ .AtlantisURL }}/"><img src="{{ .AtlantisURL }}/static/images/atlantis-icon.png"/></a>
Copy link
Member

Choose a reason for hiding this comment

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

again, don't need the / here.

@jml
Copy link
Contributor Author

jml commented Oct 17, 2018

Thanks! All good comments, I'll get to them as soon as I can.

We need to add some validation into cmd/server.go to ensure that users don't pass in an atlantis-url with a trailing /

Good point. I think what I'd like to do is:

  • parse the --atlantis-url value into a url.URL as soon as we can
  • ensure it doesn't have a trailing slash
  • pass that around everywhere

I think if a user passes one with a trailing slash, we should silently remove it.

@lkysow
Copy link
Member

lkysow commented Oct 17, 2018

Okay that sounds good. Just note that url.Parse is super lenient and will not error out on a lot of URLs so you have to inspect the result and ensure that things aren't blank.
ex. url.Parse("hi") will succeed

@jml
Copy link
Contributor Author

jml commented Oct 31, 2018

I've addressed all the comments. I want to do some more local testing to ensure this doesn't mess up the routing.

@jml
Copy link
Contributor Author

jml commented Oct 31, 2018

Also, congratulations on your recent acquisition!

@lkysow
Copy link
Member

lkysow commented Nov 2, 2018

@jml the site is pretty broken at the moment. Can you squash your changes into one commit and then I can do the remaining fixes.

Resolves runatlantis#213

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

jml commented Nov 6, 2018

Done. Thanks.

@lkysow
Copy link
Member

lkysow commented Nov 21, 2018

For future reference here is how I've been testing this.

docker-compose.yaml

version: '2'

services:
  reverseproxy:
    image: mynginx
    ports:
    - 8080:8080
    restart: always

  atlantis:
    ports:
    - 4141:4141
    depends_on:
    - reverseproxy
    image: reverse-proxy
    restart: "no"
    command: [server, --gh-user, atlantisbot, --gh-token, ****, --log-level, debug, --repo-whitelist, "github.com/*", --atlantis-url, "https://f80439fb.ngrok.io/basepath"]

Building my nginx proxy image

FROM nginx:alpine

COPY nginx.conf /etc/nginx/nginx.conf
worker_processes 1;

events { worker_connections 1024; }

http {

    sendfile on;

    upstream docker-atlantis {
        server atlantis:4141;
    }

    server {
        listen 8080;

        location = /basepath {
            return 302 /basepath/;
        }

        location /basepath/ {
            proxy_pass         http://docker-atlantis/;
            proxy_redirect     off;
            proxy_set_header   Host $host;
            proxy_set_header   X-Real-IP $remote_addr;
            proxy_set_header   X-Forwarded-For $proxy_add_x_forwarded_for;
            proxy_set_header   X-Forwarded-Host $server_name;
        }
    }
}

lkysow added a commit that referenced this pull request Nov 21, 2018
- 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 670131f into runatlantis:master Nov 21, 2018
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