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

Follow HTTP redirects after failed WS dials #251

Merged
merged 7 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions client/wsclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
requestHeader http.Header

// Websocket dialer and connection.
dialer websocket.Dialer
dialer *websocket.Dialer
echlebek marked this conversation as resolved.
Show resolved Hide resolved
conn *websocket.Conn
connMutex sync.RWMutex

Expand Down Expand Up @@ -57,7 +57,7 @@
}

// Prepare connection settings.
c.dialer = *websocket.DefaultDialer
c.dialer = websocket.DefaultDialer
echlebek marked this conversation as resolved.
Show resolved Hide resolved

var err error
c.url, err = url.Parse(settings.OpAMPServerURL)
Expand Down Expand Up @@ -131,8 +131,26 @@
c.common.Callbacks.OnConnectFailed(ctx, err)
}
if resp != nil {
c.common.Logger.Errorf(ctx, "Server responded with status=%v", resp.Status)
duration := sharedinternal.ExtractRetryAfterHeader(resp)
if resp.StatusCode >= 300 && resp.StatusCode < 400 {
// very liberal handling of 3xx that largely ignores HTTP semantics
redirect, err := resp.Location()
if err != nil {
c.common.Logger.Errorf(ctx, "3xx redirect, but no valid location: %s", err)
echlebek marked this conversation as resolved.
Show resolved Hide resolved
return err, duration

Check warning on line 140 in client/wsclient.go

View check run for this annotation

Codecov / codecov/patch

client/wsclient.go#L139-L140

Added lines #L139 - L140 were not covered by tests
}
if redirect.Scheme == "http" || redirect.Scheme == "" {
redirect.Scheme = "ws"

Check warning on line 143 in client/wsclient.go

View check run for this annotation

Codecov / codecov/patch

client/wsclient.go#L143

Added line #L143 was not covered by tests
} else if redirect.Scheme == "https" {
redirect.Scheme = "wss"

Check warning on line 145 in client/wsclient.go

View check run for this annotation

Codecov / codecov/patch

client/wsclient.go#L145

Added line #L145 was not covered by tests
}
c.common.Logger.Debugf(ctx, "%d redirect to %s", resp.StatusCode, redirect)
// Set the URL to the redirect, so that it connects to it on the
// next cycle.
c.url = redirect
echlebek marked this conversation as resolved.
Show resolved Hide resolved
} else {
c.common.Logger.Errorf(ctx, "Server responded with status=%v", resp.Status)
}
return err, duration
}
return err, sharedinternal.OptionalDuration{Defined: false}
Expand Down
50 changes: 50 additions & 0 deletions client/wsclient_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ package client
import (
"context"
"fmt"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"sync/atomic"
"testing"
Expand Down Expand Up @@ -177,3 +180,50 @@ func TestVerifyWSCompress(t *testing.T) {
})
}
}

func redirectServer(to string) *httptest.Server {
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
http.Redirect(w, req, to, http.StatusSeeOther)
}))
}

func TestRedirectWS(t *testing.T) {
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
redirectee := internal.StartMockServer(t)
redirector := redirectServer("ws://" + redirectee.Endpoint)
defer redirector.Close()

var conn atomic.Value
redirectee.OnWSConnect = func(c *websocket.Conn) {
conn.Store(c)
}

// Start an OpAMP/WebSocket client.
var connected int64
var connectErr atomic.Value
settings := types.StartSettings{
Callbacks: types.CallbacksStruct{
OnConnectFunc: func(ctx context.Context) {
atomic.StoreInt64(&connected, 1)
},
OnConnectFailedFunc: func(ctx context.Context, err error) {
if err != websocket.ErrBadHandshake {
connectErr.Store(err)
}
},
},
}
reURL, err := url.Parse(redirector.URL)
assert.NoError(t, err)
reURL.Scheme = "ws"
settings.OpAMPServerURL = reURL.String()
client := NewWebSocket(nil)
startClient(t, settings, client)

// Wait for connection to be established.
eventually(t, func() bool { return conn.Load() != nil })
assert.True(t, connectErr.Load() == nil)

// Stop the client.
err = client.Stop(context.Background())
assert.NoError(t, err)
}
Loading