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

Proxy: Better errors + remote custom TLS #1197

Merged
merged 6 commits into from
Oct 13, 2018

Conversation

blaubaer
Copy link
Contributor

  1. Added better logging on connection issues to remote hosts.
  2. Make transport customization to enable (for example) custom remote TLS certificates.

@codecov
Copy link

codecov bot commented Sep 23, 2018

Codecov Report

Merging #1197 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   81.52%   81.61%   +0.09%     
==========================================
  Files          25       27       +2     
  Lines        1921     1931      +10     
==========================================
+ Hits         1566     1576      +10     
  Misses        248      248              
  Partials      107      107
Impacted Files Coverage Δ
echo.go 87.72% <ø> (ø) ⬆️
middleware/proxy_1_11_n.go 100% <100%> (ø)
middleware/proxy_1_11.go 100% <100%> (ø)
middleware/proxy.go 64% <100%> (-0.71%) ⬇️

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 fcdf096...c809fdd. Read the comment docs.

middleware/proxy_1_11_n.go Show resolved Hide resolved
if len(t.Name) > 0 {
descr = fmt.Sprintf("%s(%s)", t.Name, t.URL.String())
}
c.Logger().Warnf("remote %s unreachable, could not forward: %v", descr, err)
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 this is an error, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends what we understand as an error, right? The issue is that the remote is not available is automatically an configuration problem, or a wrong written code of the application. I would say no, but I also can understand if you go for error rather than warn. Your call. 😃

Copy link
Member

Choose a reason for hiding this comment

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

Well configuration is one of the case, it could also be possible where upstream server is not available. I would stick to error as we cannot recover from this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got you point. I will change it accordingly.

middleware/proxy_1_11.go Outdated Show resolved Hide resolved
proxy := httputil.NewSingleHostReverseProxy(t.URL)
proxy.ErrorHandler = func(resp http.ResponseWriter, req *http.Request, err error) {
descr := t.URL.String()
if len(t.Name) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Use t.Name != "", it is more natural.

middleware/proxy_1_11.go Outdated Show resolved Hide resolved
@vishr vishr merged commit bc37a3a into labstack:master Oct 13, 2018
@vishr
Copy link
Member

vishr commented Oct 13, 2018

@blaubaer thanks for your contribution 👍

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.

None yet

2 participants