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.

Sign up for GitHub

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

Wait and retry connection to test CA server instead of failing (immediately) #2131

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Wait and retry connection to test CA server instead of failing (immediately) #2131

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2a6b5b
Wait till a connection to the CA host/port can be made
hslatman Jan 7, 2025
b3fb927
Add comment to `requireCAServerToBeAvailable`
hslatman Jan 7, 2025
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
38 changes: 35 additions & 3 deletions test/integration/requestid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ func reservePort(t *testing.T) (host, port string) {
}

func Test_reflectRequestID(t *testing.T) {
ctx := context.Background()

dir := t.TempDir()
m, err := minica.New(minica.WithName("Step E2E"))
require.NoError(t, err)
Expand Down Expand Up @@ -133,8 +135,11 @@ func Test_reflectRequestID(t *testing.T) {
require.ErrorIs(t, err, http.ErrServerClosed)
}()

// require the CA server to be available within 10 seconds,
// failing the test if it doesn't.
requireCAServerToBeAvailable(t, net.JoinHostPort("localhost", port), 10*time.Second)

// require OK health response as the baseline
ctx := context.Background()
healthResponse, err := caClient.HealthWithContext(ctx)
require.NoError(t, err)
if assert.NotNil(t, healthResponse) {
Expand Down Expand Up @@ -262,8 +267,8 @@ func newAuthorizingServer(t *testing.T, mca *minica.CA) *httptest.Server {

srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if assert.Equal(t, "signRequestID", r.Header.Get("X-Request-Id")) {
json.NewEncoder(w).Encode(struct{ Allow bool }{Allow: true})
w.WriteHeader(http.StatusOK)
err := json.NewEncoder(w).Encode(struct{ Allow bool }{Allow: true})
require.NoError(t, err)
return
}

Expand All @@ -287,3 +292,30 @@ func newAuthorizingServer(t *testing.T, mca *minica.CA) *httptest.Server {

return srv
}

func requireCAServerToBeAvailable(t *testing.T, address string, timeout time.Duration) {
t.Helper()

ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()

for !canConnect(ctx, address) {
select {
case <-ctx.Done():
require.FailNow(t, fmt.Sprintf("CA server failed to start at https://%s within %s", address, timeout.String()))
case <-time.After(100 * time.Millisecond):
}
}
}

func canConnect(ctx context.Context, address string) bool {
d := net.Dialer{Timeout: 5 * time.Second}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why hardcode a timeout, if you're dialing with a context anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. Added it out of habit. b3fb927.

conn, err := d.DialContext(ctx, "tcp", address)
if err != nil {
return false
}

conn.Close()

return true
Copy link
Contributor

Choose a reason for hiding this comment

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

It's faster if you just re-dial every 100ms, since you avoid running into TCP's exponential backoff.

Also, if the server isn't bound to the port yet, the kernel might send you a RST in response to the SYN immediately, which would make DialContext fail immediately.

Copy link
Member Author

@hslatman hslatman Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Returning an error immediately is OK, and kind of what I want. The CA server is started (or not, yet) in a different goroutine. The goal is to wait for it to become available before continuing the test, and a failure to connect here indicates it's not yet available.

}
Loading