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

use nginx as HTTP proxy #929

Merged
merged 10 commits into from Jan 10, 2019

Conversation

Projects
None yet
4 participants
@sqs
Copy link
Member

sqs commented Nov 10, 2018

Proposal: Remove anything from site config that could be done fairly easily by nginx. I think this includes things like:

  • httpToHttpsRedirect
  • httpStrictTransportSecurity
  • tlsCert
  • tlsKey
  • tls.letsencrypt
  • canonicalURLRedirect

Reason: Many of our big deployments use nginx anyway, so we should consider it as a first-class thing. This lets us remove code (that is nuanced/has large bug surface area). Also it's easier to Google "nginx how to xyz" than "sourcegraph how to xyz" where xyz is some http/https/etc.-related thing.

Contingent on: Finding a way to replace all those configs above with a simple nginx config snippet; and making this work cleanly in both sourcegraph/server and cluster.

This PR updates the CHANGELOG.md file to describe any user-facing changes.

TODOs:

  • Update Server procfile to run nginx and document how customers can change it
  • Add sentence to docs that says for cluster deployments use nginx ingress
  • Mention nginx in deploy-sourcegraph docs (but can defer full documentation until after 3.0) #1609
  • Ensure this works when deployed to dogfood - blocked on #1610
  • Ensure this works when deployed to Sourcegraph.com - blocked on #1610
  • Write test plan

—-

Issues that would be much easier if we used nginx (incomplete list):

@sqs sqs requested review from beyang and keegancsmith as code owners Nov 10, 2018

@sqs sqs force-pushed the use-http-reverse-proxy branch 3 times, most recently from 773be37 to 288d297 Nov 10, 2018

@keegancsmith keegancsmith removed their request for review Nov 12, 2018

@keegancsmith

This comment has been minimized.

Copy link
Member

keegancsmith commented Nov 12, 2018

Add me back as a reviewer when its ready.

@sqs sqs changed the title WIP: use nginx for HTTP serving WIP: use nginx as HTTP proxy Dec 4, 2018

@sqs sqs added the roadmap label Dec 4, 2018

@sqs sqs added this to the 3.0 milestone Dec 4, 2018

@beyang beyang removed their request for review Dec 4, 2018

@nicksnyder nicksnyder modified the milestones: 3.0-preview.2, 3.0 Dec 14, 2018

@sqs sqs force-pushed the use-http-reverse-proxy branch from 288d297 to 9200ad4 Jan 1, 2019

@sqs sqs requested a review from nicksnyder as a code owner Jan 1, 2019

@sqs sqs changed the title WIP: use nginx as HTTP proxy use nginx as HTTP proxy Jan 1, 2019

@sqs sqs requested a review from keegancsmith Jan 1, 2019

@sqs

This comment has been minimized.

Copy link
Member Author

sqs commented Jan 1, 2019

@keegancsmith This is ready for review.

# 3081. That is why Sourcegraph listens on 3081 despite the externalURL having port 3080.
export SRC_HTTP_ADDR=":3081"
# until they are (re)built and (2) otherwise proxies to nginx running on port 3081 (which proxies to
# Sourcegraph running on port 3082). That is why Sourcegraph listens on 3081 despite the externalURL

This comment has been minimized.

@nicksnyder

nicksnyder Jan 7, 2019

Member
Suggested change Beta
# Sourcegraph running on port 3082). That is why Sourcegraph listens on 3081 despite the externalURL
# Sourcegraph running on port 3082). That is why Sourcegraph listens on 3082 despite the externalURL
@keegancsmith

This comment has been minimized.

Copy link
Member

keegancsmith commented Jan 7, 2019

FYI I have this on my TODO list to review tomorrow.

@nicksnyder

This comment has been minimized.

Copy link
Member

nicksnyder commented Jan 7, 2019

@keegancsmith is going to own testing this and getting it merged by EOW

@nicksnyder

This comment has been minimized.

Copy link
Member

nicksnyder commented Jan 10, 2019

Any update here?

@keegancsmith

This comment has been minimized.

Copy link
Member

keegancsmith commented Jan 10, 2019

Yes, will merge today. Still missing server image integration, otherwise shipping this may break dogfood.

@keegancsmith

This comment has been minimized.

Copy link
Member

keegancsmith commented Jan 10, 2019

Documentation isn't ready, will do another PR. But for now this is ready for dogfood so wanna test that.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 10, 2019

Codecov Report

Merging #929 into master will decrease coverage by 0.09%.
The diff coverage is 10%.

Impacted Files Coverage Δ
pkg/conf/client.go 13.11% <ø> (-2.35%) ⬇️
pkg/conf/parse.go 44% <ø> (+6.06%) ⬆️
cmd/server/shared/nginx.go 0% <0%> (ø)
pkg/conf/computed.go 35.71% <0%> (-1.79%) ⬇️
cmd/server/shared/shared.go 0% <0%> (ø) ⬆️
...terprise/cmd/frontend/auth/gitlaboauth/provider.go 88.37% <100%> (+1.7%) ⬆️
enterprise/cmd/frontend/auth/githuboauth/config.go 52.63% <100%> (+0.13%) ⬆️
cmd/frontend/db/db_helpers.go 0% <0%> (-100%) ⬇️
... and 2 more

@keegancsmith keegancsmith merged commit 2ff1618 into master Jan 10, 2019

1 check passed

buildkite/sourcegraph Build #26390 passed (6 minutes, 29 seconds)
Details

@keegancsmith keegancsmith deleted the use-http-reverse-proxy branch Jan 10, 2019

keegancsmith added a commit that referenced this pull request Jan 10, 2019

keegancsmith added a commit that referenced this pull request Jan 11, 2019

@nicksnyder nicksnyder referenced this pull request Jan 11, 2019

Closed

Test plan for nginx #1781

keegancsmith added a commit that referenced this pull request Jan 11, 2019

keegancsmith added a commit that referenced this pull request Jan 13, 2019

Revert^4 "use nginx as HTTP proxy (#929)"
This reverts commit 5fd4012.

Trying to set a new record.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment