Skip to content

Commit

Permalink
Schema changes detection with HostClient
Browse files Browse the repository at this point in the history
Related with the issue #244
  • Loading branch information
gabrielperezs authored and kirillDanshin committed Oct 14, 2018
1 parent afcef43 commit 996610f
Show file tree
Hide file tree
Showing 3 changed files with 180 additions and 0 deletions.
25 changes: 25 additions & 0 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -770,9 +770,24 @@ func doRequestFollowRedirects(req *Request, dst []byte, url string, c clientDoer
resp.keepBodyBuffer = true
oldBody := bodyBuf.B
bodyBuf.B = dst
scheme := req.uri.Scheme()
req.schemaUpdate = false

redirectsCount := 0
for {
// In case redirect to different scheme
if redirectsCount > 0 && !bytes.Equal(scheme, req.uri.Scheme()) {
if strings.HasPrefix(url, string(strHTTPS)) {
req.isTLS = true
req.uri.SetSchemeBytes(strHTTPS)
} else {
req.isTLS = false
req.uri.SetSchemeBytes(strHTTP)
}
scheme = req.uri.Scheme()
req.schemaUpdate = true
}

req.parsedURI = false
req.Header.host = req.Header.host[:0]
req.SetRequestURI(url)
Expand Down Expand Up @@ -1068,6 +1083,16 @@ func (c *HostClient) doNonNilReqResp(req *Request, resp *Response) (bool, error)
// so the GC may reclaim these resources (e.g. response body).
resp.Reset()

// If we detected a redirect to another schema
if req.schemaUpdate {
c.IsTLS = bytes.Equal(req.URI().Scheme(), strHTTPS)

This comment has been minimized.

Copy link
@valyala

valyala Apr 28, 2020

Owner

This is awful change:

  • It breaks HostClient maps inside Client. There are two maps there:
    m is for http HostClients
    ms is for https HostClients
    This may update http HostClient to https, while it still remains inside m map, so all the future requests to the given http host will go to https.
  • It changes HostClient.IsTLS, which may be used by concurrent goroutines, i.e. this is racy change.
c.Addr = addMissingPort(string(req.Host()), c.IsTLS)

This comment has been minimized.

Copy link
@valyala

valyala Apr 28, 2020

Owner

This change also suffers from the issues outlined above:

  • It is racy, since HostClient can be used by concurrent goroutines while this change is applied
  • It breaks Client.m and Client.ms mapping, since after this change all the requests to the previous Addr will go to new Addr instead.

I'd suggest reverting this PR.

This comment has been minimized.

Copy link
@erikdubbelboer

erikdubbelboer Apr 29, 2020

Collaborator

I don't think reverting is a good option cause that will completely break redirects to different schemas again. I will fix the race conditions and I'll change the code not to update HostClient.IsTLS.

This comment has been minimized.

Copy link
@valyala

valyala Apr 29, 2020

Owner

Agreed. Note that the change must be performed on Client level instead of HostClient level. It is impossible to change scheme or host for HostClient, since the corresponding settings (IsTLS and Addr) must remain readonly after HostClient is created.

This comment has been minimized.

Copy link
@erikdubbelboer

erikdubbelboer Apr 29, 2020

Collaborator

I haven't looked at the exact code yet but this is exactly why it gets a clientDoer so it can use the client to follow the request. Not sure why HostClient needs to be changed at all. I'll have some time to fix it tomorrow hopefully.

This comment has been minimized.

Copy link
@erikdubbelboer

erikdubbelboer May 2, 2020

Collaborator

@valyala how about this: #800

c.addrIdx = 0

This comment has been minimized.

Copy link
@valyala

valyala Apr 28, 2020

Owner

This is racy update as c.addr = nil on the next line. These fields must be updated under HostClient.addrsLock

c.addrs = nil
req.schemaUpdate = false
req.SetConnectionClose()
}

cc, err := c.acquireConn()
if err != nil {
return false, err
Expand Down
152 changes: 152 additions & 0 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"io"
"net"
"net/url"
"os"
"runtime"
"strings"
Expand Down Expand Up @@ -49,6 +50,157 @@ func TestClientPostArgs(t *testing.T) {
}
}

func TestClientRedirectSameSchema(t *testing.T) {

listenHTTPS1 := testClientRedirectListener(t, true)
defer listenHTTPS1.Close()

listenHTTPS2 := testClientRedirectListener(t, true)
defer listenHTTPS2.Close()

sHTTPS1 := testClientRedirectChangingSchemaServer(t, listenHTTPS1, listenHTTPS1, true)
defer sHTTPS1.Stop()

sHTTPS2 := testClientRedirectChangingSchemaServer(t, listenHTTPS2, listenHTTPS2, false)
defer sHTTPS2.Stop()

destURL := fmt.Sprintf("https://%s/baz", listenHTTPS1.Addr().String())

urlParsed, err := url.Parse(destURL)
if err != nil {
fmt.Println(err)
return
}

var reqClient *HostClient

reqClient = &HostClient{
IsTLS: true,
Addr: urlParsed.Host,
TLSConfig: &tls.Config{
InsecureSkipVerify: true,
},
}

statusCode, _, err := reqClient.GetTimeout(nil, destURL, 4000*time.Millisecond)
if err != nil {
t.Fatalf("HostClient error: %s", err)
return
}

if statusCode != 200 {
t.Fatalf("HostClient error code response %d", statusCode)
return
}

}

func TestClientRedirectChangingSchemaHttp2Https(t *testing.T) {

listenHTTPS := testClientRedirectListener(t, true)
defer listenHTTPS.Close()

listenHTTP := testClientRedirectListener(t, false)
defer listenHTTP.Close()

sHTTPS := testClientRedirectChangingSchemaServer(t, listenHTTPS, listenHTTP, true)
defer sHTTPS.Stop()

sHTTP := testClientRedirectChangingSchemaServer(t, listenHTTPS, listenHTTP, false)
defer sHTTP.Stop()

destURL := fmt.Sprintf("http://%s/baz", listenHTTP.Addr().String())

urlParsed, err := url.Parse(destURL)
if err != nil {
fmt.Println(err)
return
}

var reqClient *HostClient

reqClient = &HostClient{
Addr: urlParsed.Host,
TLSConfig: &tls.Config{
InsecureSkipVerify: true,
},
}

statusCode, _, err := reqClient.GetTimeout(nil, destURL, 4000*time.Millisecond)
if err != nil {
t.Fatalf("HostClient error: %s", err)
return
}

if statusCode != 200 {
t.Fatalf("HostClient error code response %d", statusCode)
return
}

}

func testClientRedirectListener(t *testing.T, isTLS bool) net.Listener {

var ln net.Listener
var err error
var tlsConfig *tls.Config

if isTLS {
certFile := "./ssl-cert-snakeoil.pem"
keyFile := "./ssl-cert-snakeoil.key"
cert, err1 := tls.LoadX509KeyPair(certFile, keyFile)
if err1 != nil {
t.Fatalf("Cannot load TLS certificate: %s", err1)
}
tlsConfig = &tls.Config{
Certificates: []tls.Certificate{cert},
}
ln, err = tls.Listen("tcp", "localhost:0", tlsConfig)
} else {
ln, err = net.Listen("tcp", "localhost:0")
}

if err != nil {
t.Fatalf("cannot listen isTLS %v: %s", isTLS, err)
}

return ln
}

func testClientRedirectChangingSchemaServer(t *testing.T, https, http net.Listener, isTLS bool) *testEchoServer {
s := &Server{
Handler: func(ctx *RequestCtx) {
if ctx.IsTLS() {
ctx.SetStatusCode(200)
} else {
ctx.Redirect(fmt.Sprintf("https://%s/baz", https.Addr().String()), 301)
}
},
}

var ln net.Listener
if isTLS {
ln = https
} else {
ln = http
}

ch := make(chan struct{})
go func() {
err := s.Serve(ln)
if err != nil {
t.Fatalf("unexpected error returned from Serve(): %s", err)
}
close(ch)
}()
return &testEchoServer{
s: s,
ln: ln,
ch: ch,
t: t,
}
}

func TestClientHeaderCase(t *testing.T) {
ln := fasthttputil.NewInmemoryListener()
defer ln.Close()
Expand Down
3 changes: 3 additions & 0 deletions http.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ type Request struct {
keepBodyBuffer bool

isTLS bool

// To detect scheme changes in redirects
schemaUpdate bool
}

// Response represents HTTP response.
Expand Down

0 comments on commit 996610f

Please sign in to comment.