Correctly set port and scheme when behind proxy. #116
Conversation
…instead of using Settings
…ig so logic is simpler
kkellyy
changed the title
| @@ -1195,7 +1194,7 @@ def verify_branch_successful(cls, updated_request): | |||
| <p> | |||
| <strong>%(user)s - %(title)s</strong><br /> | |||
| <em>%(repo)s/%(branch)s</em><br /> | |||
| <a href="https://%(pushmanager_servername)s%(pushmanager_port)s/request?id=%(id)s">https://%(pushmanager_servername)s%(pushmanager_port)s/request?id=%(id)s</a> | |||
| <a href="%(pushmanager_url)s/request?id=%(id)s">%(pushmanager_url)srequest?id=%(id)s</a> | |||
There was a problem hiding this comment.
does this one need the slash?
|
Seems fine, how are we going to set the X-blahblahblah headers? Are there plans to produce useful error messages / fallbacks if those are absent? |
|
I left some comments about how to set apache2 or nginx proxies to forward those headers. The fallback for port is the value in the config.yaml and the protocol used in the request. Those should always be valid, but the user will be bypassing the proxy that way. I'm not sure if a warning is useful there or not, though. |
malformed URL generated in git.py.
| 'conflicts_with': ( | ||
| "master" | ||
| if 'conflict-master' in updated_request['tags'] | ||
| else "another pickme" | ||
| ), | ||
| 'pickme_name': updated_request['branch'], | ||
| 'pickme_id': updated_request['id'], |
There was a problem hiding this comment.
pickme_id is still used in msg. Is there a test for this?
There was a problem hiding this comment.
There aren't any tests for these things, I believe because they require a valid git repo. We're about to test these cases manually for things like this. Thanks for the catch.
… git.py of pickme_id being removed in URL generation
| @@ -1105,14 +1106,13 @@ def pickme_conflict_detected(cls, updated_request, send_notifications): | |||
| else "another pickme" | |||
| ), | |||
| 'conflicts': updated_request['conflicts'].replace('\n', '<br/>'), | |||
| 'pushmanager_servername': Settings['main_app']['servername'], | |||
| 'reviewboard_servername': Settings['reviewboard']['servername'], | |||
| 'pushmanager_url' : pushmanager_url, | |||
| 'pushmanager_port': ( | |||
There was a problem hiding this comment.
we don't need pushmanager_port anymore
There was a problem hiding this comment.
It's being used by the reviewboard link here. Is that incorrect?
There was a problem hiding this comment.
Indeed it is. It does look wrong since reviewboard doesn't need pushmanager's port.
There was a problem hiding this comment.
See other comment on same issue.
Correctly set port and scheme when behind proxy. Given a correctly configured web server, the pushmanager url builder is now proxy aware. Needs: X-Forwarded-Proto and X-Forwarded-Port headers
IMPROVEMENTS * tags can contain the string 'conflict' again (asottile, Yelp#119) * 'git-ok' tag now links to gitweb (kkellyy, Yelp#117) * pushmanager is now proxy aware (kkellyy, Yelp#116) BUG FIXES * reviewboard links no longer contains the pushmanager port (kkellyy, Yelp#121) DEPRECATED * 'hoods' tag no longer has post-stage checklist steps (kkellyy, Yelp#120)
IMPROVEMENTS * tags can contain the string 'conflict' again (asottile, Yelp#119) * 'git-ok' tag now links to gitweb (kkellyy, Yelp#117) * pushmanager is now proxy aware (kkellyy, Yelp#116) BUG FIXES * reviewboard links no longer contains the pushmanager port (kkellyy, Yelp#121) DEPRECATED * 'hoods' tag no longer has post-stage checklist steps (kkellyy, Yelp#120)
IMPROVEMENTS * tags can contain the string 'conflict' again (asottile, #119) * 'git-ok' tag now links to gitweb (kkellyy, #117) * pushmanager is now proxy aware (kkellyy, #116) BUG FIXES * reviewboard links no longer contains the pushmanager port (kkellyy, #121) DEPRECATED * 'hoods' tag no longer has post-stage checklist steps (kkellyy, #120)
IMPROVEMENTS * tags can contain the string 'conflict' again (asottile, #119) * 'git-ok' tag now links to gitweb (kkellyy, #117) * pushmanager is now proxy aware (kkellyy, #116) * 'After certifying' checklist now says 'Before certifying' (kkellyy, #122) BUG FIXES * reviewboard links no longer contains the pushmanager port (kkellyy, #121) DEPRECATED * 'hoods' tag no longer has post-stage checklist steps (kkellyy, #120)
Pushmanager now forms its URLs with information from any proxy it might be behind.
For pushmanager to do this, the proxy must forward its port and scheme via HTTP headers. Pushmanager uses the
X-Forwarded-PortandX-Forwarded-Protoheaders for this.X-Forwarded-Portis set, then it will override the default ofSettings['main-app']['port'].X-Forwarded-Protois set, then it will override the default ofhttps.For nginx, add these to
http,server, orlocationblocks:For apache2, can add these (or write logic to determine port & protocol):