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 76f1a3c
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 13 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ Adding a new version? You'll need three changes:
first observed value, as that value is not necessarily the desired value.
[#4171](https://github.com/Kong/kubernetes-ingress-controller/pull/4171)

### Fixed

- Fix KIC crash which occurred when invalid config was applied in DB mode.
[#4213](https://github.com/Kong/kubernetes-ingress-controller/pull/4213)

## [2.10.0]

> Release date: 2023-06-02
Expand Down
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
37 changes: 36 additions & 1 deletion internal/dataplane/sendconfig/backoff_strategy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/deckerrors"
"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/sendconfig"
"github.com/kong/kubernetes-ingress-controller/v2/internal/metrics"
)
Expand Down Expand Up @@ -107,7 +108,7 @@ func TestUpdateStrategyWithBackoff(t *testing.T) {
updateShouldBeAllowed: false,

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

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

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

t.Run("errors.Is()", func(t *testing.T) {
assert.False(t,
errors.Is(skippedErr, deckerrors.ConfigConflictError{
Err: sendconfig.NewUpdateSkippedDueToBackoffStrategyError("different reason"),
}),
"shouldn't panic when using errors.Is() with NewUpdateSkippedDueToBackoffStrategyError",
)

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 76f1a3c

Please sign in to comment.