diff --git a/CHANGELOG.md b/CHANGELOG.md index ecb0a363de..a2ff039290 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ - `v1api`: - **New:** Add package which can be used for communication with the STACKIT automation v1 API - **Feature:** Add waiter method for the API +- `core`: [v0.27.1](core/CHANGELOG.md#v0271) + - **Bugfix:** `WaitWithContext` no longer returns `(nil, nil)` after a single retryable `502`/`504` error - `iaas`: - [v1.14.4](services/iaas/CHANGELOG.md#v1144) - **Dependencies:** Bump STACKIT SDK resourcemanager module from `v0.25.1` to `v0.26.0` diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 61dc112528..f872c34cee 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -1,3 +1,6 @@ +## v0.27.1 +- **Bugfix:** `WaitWithContext` no longer returns `(nil, nil)` after a single retryable `502`/`504` error. `WaiterHelper.Wait()` now correctly returns `waitFinished = false` on generic fetch errors + ## v0.27.0 - **Feature:** Added experimental paginate package for AIP compliant pagination diff --git a/core/VERSION b/core/VERSION index 0a8bf80d69..04e94578bd 100644 --- a/core/VERSION +++ b/core/VERSION @@ -1 +1 @@ -v0.27.0 +v0.27.1 diff --git a/core/wait/wait.go b/core/wait/wait.go index 86db3fa3a1..960b19974c 100644 --- a/core/wait/wait.go +++ b/core/wait/wait.go @@ -102,8 +102,7 @@ func (h *AsyncActionHandler[T]) WaitWithContext(ctx context.Context) (res *T, er if err != nil { return res, err } - } - if done { + } else if done { return res, nil } diff --git a/core/wait/wait_test.go b/core/wait/wait_test.go index 5abbf2fe1e..57f5a97b4d 100644 --- a/core/wait/wait_test.go +++ b/core/wait/wait_test.go @@ -386,6 +386,46 @@ func TestWaitWithContext(t *testing.T) { } } +// TestWaitWithContext_RetryableErrorReportedAsDone is a regression test for a bug where a checkFn +// that reports waitFinished=true alongside a retryable error caused WaitWithContext to return (nil, nil) +// instead of retrying, because `done` stayed true even after handleError swallowed the error. +func TestWaitWithContext_RetryableErrorReportedAsDone(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + type respType struct{ Name string } + + numberCheckFnCalls := 0 + checkFn := func() (waitFinished bool, response *respType, err error) { + numberCheckFnCalls++ + if numberCheckFnCalls == 1 { + // here the return true is the offending line => should be false + return true, nil, &oapierror.GenericOpenAPIError{ + StatusCode: RetryHttpErrorStatusCodes[0], + ErrorMessage: "temporary error", + } + } + return true, &respType{Name: "my-resource"}, nil + } + handler := AsyncActionHandler[respType]{ + checkFn: checkFn, + throttle: 10 * time.Millisecond, + timeout: 5 * time.Second, + tempErrRetryLimit: 5, + } + + resp, err := handler.WaitWithContext(context.Background()) + + if err != nil { + t.Errorf("expected no error, got %v", err) + } + if resp == nil || resp.Name != "my-resource" { + t.Errorf("expected a resolved response, got %v", resp) + } + if numberCheckFnCalls != 2 { + t.Errorf("expected checkFn to be called twice (initial + retry), got %d calls", numberCheckFnCalls) + } + }) +} + func TestHandleError(t *testing.T) { for _, tt := range []struct { desc string diff --git a/core/wait/waiterhelper.go b/core/wait/waiterhelper.go index 6cfa041ab3..fbad5003d6 100644 --- a/core/wait/waiterhelper.go +++ b/core/wait/waiterhelper.go @@ -44,12 +44,12 @@ func (w *WaiterHelper[T, S]) Wait() AsyncActionCheck[T] { return true, nil, nil } } - return true, nil, err + return false, nil, err } state, err := w.GetState(instance) if err != nil { - return true, nil, err + return false, nil, err } // 1. Check if the operation succeeded diff --git a/core/wait/waiterhelper_test.go b/core/wait/waiterhelper_test.go index fe727aa708..22b405df67 100644 --- a/core/wait/waiterhelper_test.go +++ b/core/wait/waiterhelper_test.go @@ -1,9 +1,12 @@ package wait import ( + "context" "fmt" "net/http" "testing" + "testing/synctest" + "time" "github.com/google/go-cmp/cmp" @@ -85,7 +88,7 @@ func TestWaiterHelper_Wait(t *testing.T) { // If ActiveState is empty, it assumes we are waiting for a deletion activeStates: nil, deleteHttpErrorStatusCodes: []int{http.StatusNotFound}, - wantFinished: true, + wantFinished: false, wantErr: true, wantResponse: nil, }, @@ -95,7 +98,7 @@ func TestWaiterHelper_Wait(t *testing.T) { // If ActiveState is empty, it assumes we are waiting for a deletion activeStates: nil, deleteHttpErrorStatusCodes: []int{http.StatusNotFound}, - wantFinished: true, + wantFinished: false, wantErr: true, wantResponse: nil, }, @@ -104,14 +107,14 @@ func TestWaiterHelper_Wait(t *testing.T) { fetchErr: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadRequest}, // If ActiveState is empty, it assumes we are waiting for a deletion activeStates: nil, - wantFinished: true, + wantFinished: false, wantErr: true, }, { - name: "Success - Error on fetch instance (400 Bad Request)", + name: "Failure - Error on fetch instance (403 Forbidden)", fetchErr: &oapierror.GenericOpenAPIError{StatusCode: http.StatusForbidden}, activeStates: []string{"READY"}, - wantFinished: true, + wantFinished: false, wantErr: true, wantResponse: nil, }, @@ -119,7 +122,7 @@ func TestWaiterHelper_Wait(t *testing.T) { name: "Failure - GetState failed", fetchResult: &MockResource{Error: fmt.Errorf("can not read state")}, activeStates: []string{"READY"}, - wantFinished: true, + wantFinished: false, wantErr: true, }, } @@ -161,3 +164,178 @@ func TestWaiterHelper_Wait(t *testing.T) { }) } } + +func TestWaiterHelper_WaitWithContext(t *testing.T) { + type fetchResponse struct { + res *MockResource + err error + } + + tests := []struct { + name string + // fetchResponses is the array of responses the mocked endpoint will answer in sequence + fetchResponses []fetchResponse + activeStates []string + errorStates []string + deleteHttpErrorStatusCodes []int + wantCalls int + wantErr bool + wantResponse *MockResource + }{ + { + name: "Success - Retryable 502 Gateway Error followed by Active State", + fetchResponses: []fetchResponse{ + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}}, + {res: &MockResource{Status: "ACTIVE"}, err: nil}, + }, + activeStates: []string{"ACTIVE"}, + errorStates: []string{"ERROR"}, + wantCalls: 2, + wantErr: false, + wantResponse: &MockResource{Status: "ACTIVE"}, + }, + { + name: "Success - Retryable 504 Error followed by Active State", + fetchResponses: []fetchResponse{ + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusGatewayTimeout}}, + {res: &MockResource{Status: "ACTIVE"}, err: nil}, + }, + activeStates: []string{"ACTIVE"}, + errorStates: []string{"ERROR"}, + wantCalls: 2, + wantErr: false, + wantResponse: &MockResource{Status: "ACTIVE"}, + }, + { + name: "Success - Retryable 502 Gateway Error during Deletion followed by 404", + fetchResponses: []fetchResponse{ + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}}, + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound}}, + }, + activeStates: nil, + wantCalls: 2, + wantErr: false, + wantResponse: nil, + }, + { + name: "Success - Immediate Active State", + fetchResponses: []fetchResponse{ + {res: &MockResource{Status: "ACTIVE"}, err: nil}, + }, + activeStates: []string{"ACTIVE"}, + wantCalls: 1, + wantErr: false, + wantResponse: &MockResource{Status: "ACTIVE"}, + }, + { + name: "Success - Pending State transitioned to Active State", + fetchResponses: []fetchResponse{ + {res: &MockResource{Status: "CREATING"}, err: nil}, + {res: &MockResource{Status: "ACTIVE"}, err: nil}, + }, + activeStates: []string{"ACTIVE"}, + wantCalls: 2, + wantErr: false, + wantResponse: &MockResource{Status: "ACTIVE"}, + }, + { + name: "Success - Deletion (404 Not Found)", + fetchResponses: []fetchResponse{ + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusNotFound}}, + }, + activeStates: nil, + wantCalls: 1, + wantErr: false, + wantResponse: nil, + }, + { + name: "Failure - Non-retryable HTTP Error (400 Bad Request)", + fetchResponses: []fetchResponse{ + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadRequest}}, + }, + activeStates: []string{"ACTIVE"}, + wantCalls: 1, + wantErr: true, + wantResponse: nil, + }, + { + name: "Failure - Retry limit reached for temporary error", + fetchResponses: []fetchResponse{ + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}}, + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}}, + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}}, + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}}, + {res: nil, err: &oapierror.GenericOpenAPIError{StatusCode: http.StatusBadGateway}}, + }, + activeStates: []string{"ACTIVE"}, + wantCalls: 5, + wantErr: true, + wantResponse: nil, + }, + { + name: "Failure - Pending State transitioned to Error State", + fetchResponses: []fetchResponse{ + {res: &MockResource{Status: "CREATING"}, err: nil}, + {res: &MockResource{Status: "FAILED"}, err: nil}, + }, + activeStates: []string{"ACTIVE"}, + errorStates: []string{"FAILED"}, + wantCalls: 2, + wantErr: true, + wantResponse: &MockResource{Status: "FAILED"}, + }, + { + name: "Failure - Non-GenericOpenAPIError on fetch", + fetchResponses: []fetchResponse{ + {res: nil, err: fmt.Errorf("network connection failure")}, + }, + activeStates: []string{"ACTIVE"}, + wantCalls: 1, + wantErr: true, + wantResponse: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // synctest for the fake clock + synctest.Test(t, func(t *testing.T) { + calls := 0 + w := &WaiterHelper[MockResource, string]{ + FetchInstance: func() (*MockResource, error) { + calls++ + if calls <= len(tt.fetchResponses) { + resp := tt.fetchResponses[calls-1] + return resp.res, resp.err + } + return nil, fmt.Errorf("unexpected fetch call %d", calls) + }, + GetState: func(m *MockResource) (string, error) { + return m.Status, m.Error + }, + DeleteHttpErrorStatusCodes: tt.deleteHttpErrorStatusCodes, + ActiveState: tt.activeStates, + ErrorState: tt.errorStates, + } + + handler := New(w.Wait()).SetThrottle(10 * time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + res, err := handler.WaitWithContext(ctx) + + if (err != nil) != tt.wantErr { + t.Fatalf("WaitWithContext() error = %v, wantErr %v", err, tt.wantErr) + } + + if tt.wantCalls > 0 && calls != tt.wantCalls { + t.Errorf("FetchInstance calls = %d, want %d", calls, tt.wantCalls) + } + + if diff := cmp.Diff(tt.wantResponse, res); diff != "" { + t.Errorf("WaitWithContext() response mismatch (-want +got):\n%s", diff) + } + }) + }) + } +}