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

support/http: remove behind proxy option for muxes #2051

Merged
merged 2 commits into from
Dec 13, 2019
Merged

support/http: remove behind proxy option for muxes #2051

merged 2 commits into from
Dec 13, 2019

Conversation

leighmcculloch
Copy link
Member

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Remove the ability to configure support/http muxes/routers with the behind proxy flag.

Why

We use these mux functions in five places across four applications, but always with behind proxy as false. We never offer a way to configure it, it's always hardcoded. The middleware that behind proxy adds is the chi RealIP middleware, and even in Horizon we don't use it. It seems like something we added for a specific use case but never used. Removing it to make this code simpler.

Known limitations

This is a non-breaking change for the applications in the repo because I'm updating all the callers to not pass the value, but this is a breaking change for anyone outside this repository who might be importing github.com/stellar/go/support/http. As I understand it the code that lives under the support package is primarily intended for use by services within this repository and so the likelihood of this being a breaking change for anyone is small. According to GoDoc.org there are no importers of support/http who aren't a fork of this repository.

We'd benefit from a formal backwards compatibility statement that classified each package and what its compatibility promise is. If we ever decide one I think we should keep the support package outside any compatibility promise.

@leighmcculloch leighmcculloch merged commit 8904b44 into stellar:master Dec 13, 2019
@leighmcculloch leighmcculloch deleted the support-http-no-more-behind-proxy branch December 13, 2019 14:56
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