Skip to content

Commit

Permalink
feat: store and push last valid config (#4205)
Browse files Browse the repository at this point in the history 
* feat: store and push last valid config

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>

* test(unit): last valid config unit test added

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>

---------

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
  • Loading branch information
@mlavacca
mlavacca committed Jun 28, 2023
1 parent 024284e commit d73df77
Show file tree
Hide file tree
Showing 6 changed files with 184 additions and 15 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ Adding a new version? You'll need three changes:
- Prometheus metrics now include counts of resources that the controller cannot
send to the proxy instances and the last successful configuration push time.
[#4181](https://github.com/Kong/kubernetes-ingress-controller/pull/4181)
- Store the last known good configuration. If Kong rejects the latest
configuration, send the last good configuration to Kong instances with no
configuration. This allows newly-started Kong instances to serve traffic even
if a configuration error prevents the controller from sending the latest
configuration.
[#4205](https://github.com/Kong/kubernetes-ingress-controller/pull/4205)

### Changed

Expand Down
16 changes: 16 additions & 0 deletions internal/dataplane/kong_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ type KongClient struct {
// SHAs is a slice is configuration hashes send in last batch send.
SHAs []string

// lastValidKongState contains the last valid configuration pushed
// to the gateways. It is used as a fallback in case a newer config version is
// somehow broken.
lastValidKongState *kongstate.KongState

// clientsProvider allows retrieving the most recent set of clients.
clientsProvider clients.AdminAPIClientsProvider

Expand Down Expand Up @@ -148,6 +153,7 @@ func NewKongClient(
configChangeDetector sendconfig.ConfigurationChangeDetector,
parser KongConfigBuilder,
cacheStores store.CacheStores,
lastValidKongState *kongstate.KongState,
) (*KongClient, error) {
c := &KongClient{
logger: logger,
Expand All @@ -164,6 +170,7 @@ func NewKongClient(
updateStrategyResolver: updateStrategyResolver,
configChangeDetector: configChangeDetector,
kongConfigBuilder: parser,
lastValidKongState: lastValidKongState,
}

return c, nil
Expand Down Expand Up @@ -388,6 +395,13 @@ func (c *KongClient) Update(ctx context.Context) error {

// In case of a failure in syncing configuration with Gateways, propagate the error.
if gatewaysSyncErr != nil {
if c.lastValidKongState != nil {
_, fallbackSyncErr := c.sendOutToGatewayClients(ctx, c.lastValidKongState, c.kongConfig)
if fallbackSyncErr != nil {
return errors.Join(gatewaysSyncErr, fallbackSyncErr)
}
c.logger.Debug("due to errors in the current config, the last valid config has been pushed to Gateways")
}
return gatewaysSyncErr
}

Expand Down Expand Up @@ -423,6 +437,8 @@ func (c *KongClient) sendOutToGatewayClients(
sort.Strings(shas)
c.SHAs = shas

c.lastValidKongState = s

return previousSHAs, nil
}

Expand Down
142 changes: 130 additions & 12 deletions internal/dataplane/kong_client_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package dataplane_test
package dataplane

import (
"context"
"errors"
"fmt"
"net/http"
"strings"
"sync"
"testing"
"time"
Expand All @@ -14,6 +15,7 @@ import (
gokong "github.com/kong/go-kong/kong"
"github.com/samber/lo"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
netv1 "k8s.io/api/networking/v1"
Expand All @@ -24,7 +26,6 @@ import (

"github.com/kong/kubernetes-ingress-controller/v2/internal/adminapi"
"github.com/kong/kubernetes-ingress-controller/v2/internal/clients"
"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane"
"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/failures"
"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/kongstate"
"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/parser"
Expand Down Expand Up @@ -111,7 +112,7 @@ func TestUniqueObjects(t *testing.T) {
require.NoError(t, err)
translationFailures = append(translationFailures, translationFailure)
}
uniqueObjs := dataplane.UniqueObjects(tc.reportedObjs, translationFailures)
uniqueObjs := UniqueObjects(tc.reportedObjs, translationFailures)
require.Len(t, uniqueObjs, len(tc.uniqueObjs))
require.ElementsMatch(t, tc.uniqueObjs, uniqueObjs)
})
Expand Down Expand Up @@ -147,6 +148,7 @@ type mockUpdateStrategyResolver struct {
shouldReturnErrorOnUpdate map[string]struct{}
t *testing.T
lock sync.RWMutex
singleError bool
}

func newMockUpdateStrategyResolver(t *testing.T) *mockUpdateStrategyResolver {
Expand All @@ -161,7 +163,7 @@ func (f *mockUpdateStrategyResolver) ResolveUpdateStrategy(c sendconfig.UpdateCl
defer f.lock.Unlock()

url := c.AdminAPIClient().BaseRootURL()
return &mockUpdateStrategy{onUpdate: f.updateCalledForURLCallback(url)}
return &mockUpdateStrategy{onUpdate: f.updateCalledForURLCallback(url, f.singleError)}
}

// returnErrorOnUpdate will cause the mockUpdateStrategy with a given Admin API URL to return an error on Update().
Expand All @@ -178,13 +180,16 @@ func (f *mockUpdateStrategyResolver) returnErrorOnUpdate(url string, shouldRetur

// updateCalledForURLCallback returns a function that will be called when the mockUpdateStrategy is called.
// That enables us to track which URLs were called.
func (f *mockUpdateStrategyResolver) updateCalledForURLCallback(url string) func() error {
func (f *mockUpdateStrategyResolver) updateCalledForURLCallback(url string, singleError bool) func() error {
return func() error {
f.lock.Lock()
defer f.lock.Unlock()

f.updateCalledForURLs = append(f.updateCalledForURLs, url)
if _, ok := f.shouldReturnErrorOnUpdate[url]; ok {
if singleError {
delete(f.shouldReturnErrorOnUpdate, url)
}
return errors.New("error on update")
}
return nil
Expand Down Expand Up @@ -316,7 +321,7 @@ func TestKongClientUpdate_AllExpectedClientsAreCalledAndErrorIsPropagated(t *tes
configChangeDetector := mockConfigurationChangeDetector{hasConfigurationChanged: true}
configBuilder := newMockKongConfigBuilder()

kongClient := setupTestKongClient(t, updateStrategyResolver, clientsProvider, configChangeDetector, configBuilder)
kongClient := setupTestKongClient(t, updateStrategyResolver, clientsProvider, configChangeDetector, configBuilder, nil)

err := kongClient.Update(ctx)
if tc.expectError {
Expand Down Expand Up @@ -345,7 +350,7 @@ func TestKongClientUpdate_WhenNoChangeInConfigNoClientGetsCalled(t *testing.T) {
configChangeDetector := mockConfigurationChangeDetector{hasConfigurationChanged: false}
configBuilder := newMockKongConfigBuilder()

kongClient := setupTestKongClient(t, updateStrategyResolver, clientsProvider, configChangeDetector, configBuilder)
kongClient := setupTestKongClient(t, updateStrategyResolver, clientsProvider, configChangeDetector, configBuilder, nil)

ctx := context.Background()
err := kongClient.Update(ctx)
Expand All @@ -372,15 +377,18 @@ func (m *mockConfigStatusQueue) WasNotified() bool {

type mockKongConfigBuilder struct {
translationFailuresToReturn []failures.ResourceFailure
kongState *kongstate.KongState
}

func newMockKongConfigBuilder() *mockKongConfigBuilder {
return &mockKongConfigBuilder{}
return &mockKongConfigBuilder{
kongState: &kongstate.KongState{},
}
}

func (p *mockKongConfigBuilder) BuildKongConfig() parser.KongConfigBuildingResult {
return parser.KongConfigBuildingResult{
KongState: &kongstate.KongState{},
KongState: p.kongState,
TranslationFailures: p.translationFailuresToReturn,
}
}
Expand Down Expand Up @@ -420,7 +428,7 @@ func TestKongClientUpdate_ConfigStatusIsAlwaysNotified(t *testing.T) {
updateStrategyResolver = newMockUpdateStrategyResolver(t)
configChangeDetector = mockConfigurationChangeDetector{hasConfigurationChanged: true}
configBuilder = newMockKongConfigBuilder()
kongClient = setupTestKongClient(t, updateStrategyResolver, clientsProvider, configChangeDetector, configBuilder)
kongClient = setupTestKongClient(t, updateStrategyResolver, clientsProvider, configChangeDetector, configBuilder, nil)
)

testCases := []struct {
Expand Down Expand Up @@ -483,22 +491,131 @@ func TestKongClientUpdate_ConfigStatusIsAlwaysNotified(t *testing.T) {
}
}

func TestKongClientUpdate_PushLastValidConfig(t *testing.T) {
var (
ctx = context.Background()
testGatewayClient = mustSampleGatewayClient(t)

clientsProvider = mockGatewayClientsProvider{
gatewayClients: []*adminapi.Client{testGatewayClient},
}

updateStrategyResolver = newMockUpdateStrategyResolver(t)
configChangeDetector = mockConfigurationChangeDetector{hasConfigurationChanged: true}
lastKongState = &kongstate.KongState{
Services: []kongstate.Service{
{
Service: gokong.Service{
Name: gokong.String("last_service"),
},
Namespace: "last_namespace",
Routes: []kongstate.Route{
{
Route: gokong.Route{
Name: gokong.String("last_route"),
},
},
},
},
},
}
newKongState = &kongstate.KongState{
Services: []kongstate.Service{
{
Service: gokong.Service{
Name: gokong.String("new_service"),
},
Namespace: "new_namespace",
Routes: []kongstate.Route{
{
Route: gokong.Route{
Name: gokong.String("new_route"),
},
},
},
},
},
}
configBuilder = newMockKongConfigBuilder()
)
configBuilder.kongState = newKongState

testCases := []struct {
name string
gatewayFailure bool
translationFailures bool
singleError bool
lastValidKongState *kongstate.KongState
expectedLastKongState *kongstate.KongState
errorsSize int
}{
{
name: "success, new fallback set",
lastValidKongState: lastKongState,
expectedLastKongState: newKongState,
},
{
name: "failure, no fallback",
gatewayFailure: true,
singleError: true,
lastValidKongState: nil,
expectedLastKongState: nil,
errorsSize: 1,
},
{
name: "failure, fallback pushed with success",
gatewayFailure: true,
singleError: true,
lastValidKongState: lastKongState,
expectedLastKongState: lastKongState,
errorsSize: 1,
},
{
name: "failure, fallback pushed with failure",
gatewayFailure: true,
singleError: false,
lastValidKongState: lastKongState,
expectedLastKongState: lastKongState,
errorsSize: 2,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
updateStrategyResolver.returnErrorOnUpdate(testGatewayClient.BaseRootURL(), tc.gatewayFailure)
updateStrategyResolver.singleError = tc.singleError
kongClient := setupTestKongClient(t, updateStrategyResolver, clientsProvider, configChangeDetector, configBuilder, tc.lastValidKongState)

err := kongClient.Update(ctx)
if tc.errorsSize > 0 {
// check if the error is joined with other errors. When there are multiple errors,
// they are separated by \n, hence we count the number of \n.
assert.Equal(t, strings.Count(err.Error(), "\n"), tc.errorsSize-1)
} else {
assert.NoError(t, err)
}
assert.Equal(t, tc.expectedLastKongState, kongClient.lastValidKongState)
})
}
}

// setupTestKongClient creates a KongClient with mocked dependencies.
func setupTestKongClient(
t *testing.T,
updateStrategyResolver *mockUpdateStrategyResolver,
clientsProvider mockGatewayClientsProvider,
configChangeDetector sendconfig.ConfigurationChangeDetector,
configBuilder *mockKongConfigBuilder,
) *dataplane.KongClient {
lastKongState *kongstate.KongState,
) *KongClient {
logger := logrus.New()
timeout := time.Second
ingressClass := "kong"
diagnostic := util.ConfigDumpDiagnostic{}
config := sendconfig.Config{}
dbMode := "off"

kongClient, err := dataplane.NewKongClient(
kongClient, err := NewKongClient(
logger,
timeout,
ingressClass,
Expand All @@ -511,6 +628,7 @@ func setupTestKongClient(
configChangeDetector,
configBuilder,
store.NewCacheStores(),
lastKongState,
)
require.NoError(t, err)
return kongClient
Expand Down
1 change: 1 addition & 0 deletions internal/manager/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ func Run(ctx context.Context, c *Config, diagnostic util.ConfigDumpDiagnostic, d
configurationChangeDetector,
configParser,
cache,
nil,
)
if err != nil {
return fmt.Errorf("failed to initialize kong data-plane client: %w", err)
Expand Down
16 changes: 15 additions & 1 deletion test/e2e/all_in_one_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/stretchr/testify/require"
autoscalingv1 "k8s.io/api/autoscaling/v1"
corev1 "k8s.io/api/core/v1"
netv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
k8stypes "k8s.io/apimachinery/pkg/types"

Expand Down Expand Up @@ -305,7 +306,7 @@ func TestDeployAllInOneDBLESS(t *testing.T) {
deployments := getManifestDeployments(manifestFilePath)

t.Log("running ingress tests to verify all-in-one deployed ingress controller and proxy are functional")
deployIngressWithEchoBackends(ctx, t, env, numberOfEchoBackends)
ingress := deployIngressWithEchoBackends(ctx, t, env, numberOfEchoBackends)
verifyIngressWithEchoBackends(ctx, t, env, numberOfEchoBackends)
ensureAllProxyReplicasAreConfigured(ctx, t, env, deployments.ProxyNN)

Expand All @@ -325,6 +326,19 @@ func TestDeployAllInOneDBLESS(t *testing.T) {
t.Log("scale proxy to 3 replicas and wait for all instances to be ready")
scaleDeployment(ctx, t, env, deployments.ProxyNN, 3)
ensureAllProxyReplicasAreConfigured(ctx, t, env, deployments.ProxyNN)

t.Log("scale proxy to 1 replica")
scaleDeployment(ctx, t, env, deployments.ProxyNN, 1)

t.Log("misconfigure the ingress")
reconfigureExistingIngress(ctx, t, env, ingress, func(i *netv1.Ingress) {
ingress.Spec.Rules[0].HTTP.Paths[0].Path = badEchoPath
})

t.Log("scale proxy to 3 replicas and verify that the new replicas get the old good configuration")
scaleDeployment(ctx, t, env, deployments.ProxyNN, 3)
// verify all the proxy replicas have the last good configuration
ensureAllProxyReplicasAreConfigured(ctx, t, env, deployments.ProxyNN)
}

func ensureAllProxyReplicasAreConfigured(ctx context.Context, t *testing.T, env environments.Environment, proxyDeploymentNN k8stypes.NamespacedName) {
Expand Down
Loading

0 comments on commit d73df77

Please sign in to comment.