Skip to content

Commit

Permalink
fix: fix incorrect .Is() method on update skipped due to backoff error
Browse files Browse the repository at this point in the history
  • Loading branch information
pmalek committed Jun 23, 2023
1 parent 7f4ab19 commit 53b2a4a
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 13 deletions.
2 changes: 1 addition & 1 deletion internal/dataplane/kong_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ func (c *KongClient) maybeSendOutToKonnectClient(ctx context.Context, s *kongsta
// In case of an error, we only log it since we don't want the Konnect to affect the basic functionality
// of the controller.

if errors.Is(err, sendconfig.ErrUpdateSkippedDueToBackoffStrategy{}) {
if errors.As(err, &sendconfig.UpdateSkippedDueToBackoffStrategyError{}) {
c.logger.WithError(err).Warn("Skipped pushing configuration to Konnect")
} else {
c.logger.WithError(err).Warn("Failed pushing configuration to Konnect")
Expand Down
15 changes: 5 additions & 10 deletions internal/dataplane/sendconfig/backoff_strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package sendconfig

import (
"context"
"errors"
"fmt"

"github.com/sirupsen/logrus"
Expand All @@ -11,22 +10,18 @@ import (
"github.com/kong/kubernetes-ingress-controller/v2/internal/metrics"
)

type ErrUpdateSkippedDueToBackoffStrategy struct {
type UpdateSkippedDueToBackoffStrategyError struct {
explanation string
}

func NewErrUpdateSkippedDueToBackoffStrategy(explanation string) ErrUpdateSkippedDueToBackoffStrategy {
return ErrUpdateSkippedDueToBackoffStrategy{explanation: explanation}
func NewUpdateSkippedDueToBackoffStrategyError(explanation string) UpdateSkippedDueToBackoffStrategyError {
return UpdateSkippedDueToBackoffStrategyError{explanation: explanation}
}

func (e ErrUpdateSkippedDueToBackoffStrategy) Error() string {
func (e UpdateSkippedDueToBackoffStrategyError) Error() string {
return fmt.Sprintf("update skipped due to a backoff strategy not being satisfied: %s", e.explanation)
}

func (e ErrUpdateSkippedDueToBackoffStrategy) Is(err error) bool {
return errors.Is(err, ErrUpdateSkippedDueToBackoffStrategy{})
}

// UpdateStrategyWithBackoff decorates any UpdateStrategy to respect a passed adminapi.UpdateBackoffStrategy.
type UpdateStrategyWithBackoff struct {
decorated UpdateStrategy
Expand Down Expand Up @@ -57,7 +52,7 @@ func (s UpdateStrategyWithBackoff) Update(ctx context.Context, targetContent Con
resourceErrorsParseErr error,
) {
if canUpdate, whyNot := s.backoffStrategy.CanUpdate(targetContent.Hash); !canUpdate {
return NewErrUpdateSkippedDueToBackoffStrategy(whyNot), nil, nil
return NewUpdateSkippedDueToBackoffStrategyError(whyNot), nil, nil
}

err, resourceErrors, resourceErrorsParseErr = s.decorated.Update(ctx, targetContent)
Expand Down
29 changes: 28 additions & 1 deletion internal/dataplane/sendconfig/backoff_strategy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func TestUpdateStrategyWithBackoff(t *testing.T) {
updateShouldBeAllowed: false,

expectUpdateCalled: false,
expectError: sendconfig.NewErrUpdateSkippedDueToBackoffStrategy("some reason"),
expectError: sendconfig.NewUpdateSkippedDueToBackoffStrategyError("some reason"),
},
}

Expand All @@ -130,3 +130,30 @@ func TestUpdateStrategyWithBackoff(t *testing.T) {
})
}
}

func TestConflictError(t *testing.T) {
skippedErr := sendconfig.NewUpdateSkippedDueToBackoffStrategyError("reason")

t.Run("errors.Is()", func(t *testing.T) {
assert.False(t, errors.Is(skippedErr, errors.New("")),
"empty error doesn't match",
)
assert.False(t, errors.Is(skippedErr, sendconfig.NewUpdateSkippedDueToBackoffStrategyError("different reason")),
"error with different reason shouldn't match",
)
assert.True(t, errors.Is(skippedErr, skippedErr),
"error with the same reason should match",
)
})

t.Run("errors.As()", func(t *testing.T) {
err := sendconfig.NewUpdateSkippedDueToBackoffStrategyError("reason")
assert.True(t, errors.As(skippedErr, &err),
"error with the same reason but wrapped should match",
)
err2 := sendconfig.NewUpdateSkippedDueToBackoffStrategyError("reason 2")
assert.True(t, errors.As(skippedErr, &err2),
"error with different reason should match",
)
})
}
2 changes: 1 addition & 1 deletion internal/dataplane/sendconfig/sendconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func PerformUpdate(
metricsProtocol := updateStrategy.MetricsProtocol()
if err != nil {
// Not pushing metrics in case it's an update skip due to a backoff.
if errors.Is(err, ErrUpdateSkippedDueToBackoffStrategy{}) {
if errors.As(err, &UpdateSkippedDueToBackoffStrategyError{}) {
return nil, []failures.ResourceFailure{}, err
}

Expand Down

0 comments on commit 53b2a4a

Please sign in to comment.