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.

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

chore: split config change detector into gw and konnect flavor #6942

Merged
merged 1 commit into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion internal/dataplane/kong_client_golden_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ func runKongClientGoldenTest(t *testing.T, tc kongClientGoldenTestCase) {
dpconf.DBModeOff, // Test will run in DB-less mode only for now. In the future, we may want to test DB mode as well.
clientsProvider,
updateStrategyResolver,
sendconfig.NewDefaultConfigurationChangeDetector(logger),
sendconfig.NewKongGatewayConfigurationChangeDetector(logger),
lastValidConfigFetcher,
p,
&cacheStores,
Expand Down
57 changes: 30 additions & 27 deletions internal/dataplane/sendconfig/config_change_detector.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,55 +18,42 @@ const (
)

type ConfigurationChangeDetector interface {
// HasConfigurationChanged verifies whether configuration has changed by comparing
// old and new config's SHAs.
// In case the SHAs are equal, it still can return true if a client is considered
// crashed or just booted up based on its status.
// In case the status indicates an empty config and the desired config is also empty
// this will return false to prevent continuously sending empty configuration to Gateway.
HasConfigurationChanged(
ctx context.Context,
oldSHA, newSHA []byte,
targetConfig *file.Content,
client KonnectAwareClient,
statusClient StatusClient,
) (bool, error)
}

type KonnectAwareClient interface {
IsKonnect() bool
}

type StatusClient interface {
Status(context.Context) (*kong.Status, error)
}

type DefaultConfigurationChangeDetector struct {
// KongGatewayConfigurationChangeDetector detects changes in Kong Gateway configuration. Besides comparing SHA hashes,
// it also checks if the Kong Gateway has no configuration yet or if the configuration to be pushed is empty.
type KongGatewayConfigurationChangeDetector struct {
logger logr.Logger
}

func NewDefaultConfigurationChangeDetector(logger logr.Logger) *DefaultConfigurationChangeDetector {
return &DefaultConfigurationChangeDetector{logger: logger}
func NewKongGatewayConfigurationChangeDetector(logger logr.Logger) *KongGatewayConfigurationChangeDetector {
return &KongGatewayConfigurationChangeDetector{logger: logger}
}

func (d *DefaultConfigurationChangeDetector) HasConfigurationChanged(
// HasConfigurationChanged verifies whether configuration has changed by comparing
// old and new config's SHAs. In case the SHAs are equal, it still can return true if a client is considered crashed or
// just booted up based on its status. In case the status indicates an empty config and the desired config is also empty
// this will return false to prevent continuously sending empty configuration to Gateway.
func (d *KongGatewayConfigurationChangeDetector) HasConfigurationChanged(
ctx context.Context,
oldSHA, newSHA []byte,
targetConfig *file.Content,
client KonnectAwareClient,
statusClient StatusClient,
) (bool, error) {
if !bytes.Equal(oldSHA, newSHA) {
return true, nil
}

// In case of Konnect, we skip further steps that are meant to detect Kong instances crash/reset
// that are not relevant for Konnect.
// We're sure that if oldSHA and newSHA are equal, we are safe to skip the update.
if client.IsKonnect() {
return false, nil
}

// Check if a Kong instance has no configuration yet (could mean it crashed, was rebooted, etc.).
hasNoConfiguration, err := kongHasNoConfiguration(ctx, statusClient)
if err != nil {
Expand All @@ -86,10 +73,9 @@ func (d *DefaultConfigurationChangeDetector) HasConfigurationChanged(
return false, nil
}

// kongHasNoConfiguration checks Kong's status endpoint and read its config hash.
// If the config hash reported by Kong is the known empty hash, it's considered crashed.
// This allows providing configuration to Kong instances that have unexpectedly crashed and
// lost their configuration.
// kongHasNoConfiguration checks Kong's status endpoint and read its config hash. If the config hash reported by Kong is
// the known empty hash, it's considered crashed. This allows providing configuration to Kong instances that have
// unexpectedly crashed and lost their configuration.
func kongHasNoConfiguration(ctx context.Context, client StatusClient) (bool, error) {
status, err := client.Status(ctx)
if err != nil {
Expand All @@ -102,3 +88,20 @@ func kongHasNoConfiguration(ctx context.Context, client StatusClient) (bool, err

return false, nil
}

// KonnectConfigurationChangeDetector detects changes in Konnect configuration by comparing SHA hashes only.
type KonnectConfigurationChangeDetector struct{}

func NewKonnectConfigurationChangeDetector() *KonnectConfigurationChangeDetector {
return &KonnectConfigurationChangeDetector{}
}

// HasConfigurationChanged verifies whether configuration has changed by comparing old and new config's SHAs.
func (d *KonnectConfigurationChangeDetector) HasConfigurationChanged(
_ context.Context,
oldSHA, newSHA []byte,
_ *file.Content,
_ StatusClient,
) (bool, error) {
return !bytes.Equal(oldSHA, newSHA), nil
}
61 changes: 41 additions & 20 deletions internal/dataplane/sendconfig/config_change_detector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,6 @@ import (
"github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/sendconfig"
)

type konnectAwareClientMock struct {
expected bool
}

func (c konnectAwareClientMock) IsKonnect() bool {
return c.expected
}

type statusClientMock struct {
expectedValue *kong.Status
expectedError error
Expand Down Expand Up @@ -104,15 +96,6 @@ func TestDefaultConfigurationChangeDetector_HasConfigurationChanged(t *testing.T
statusSHA: string(testSHAs[1]),
expectedResult: false,
},
{
name: "oldSHA == newSHA, status would signal crash, but it's konnect",
oldSHA: testSHAs[0],
newSHA: testSHAs[0],
targetConfig: createConfigContent(),
statusSHA: sendconfig.WellKnownInitialHash,
isKonnect: true,
expectedResult: false,
},
{
name: "oldSHA == newSHA, status returns error",
oldSHA: testSHAs[0],
Expand All @@ -125,16 +108,15 @@ func TestDefaultConfigurationChangeDetector_HasConfigurationChanged(t *testing.T

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
konnectAwareClient := konnectAwareClientMock{expected: tc.isKonnect}
statusClient := statusClientMock{
expectedValue: &kong.Status{
ConfigurationHash: tc.statusSHA,
},
expectedError: tc.statusError,
}
detector := sendconfig.NewDefaultConfigurationChangeDetector(zapr.NewLogger(zap.NewNop()))
detector := sendconfig.NewKongGatewayConfigurationChangeDetector(zapr.NewLogger(zap.NewNop()))

result, err := detector.HasConfigurationChanged(ctx, tc.oldSHA, tc.newSHA, tc.targetConfig, konnectAwareClient, statusClient)
result, err := detector.HasConfigurationChanged(ctx, tc.oldSHA, tc.newSHA, tc.targetConfig, statusClient)
if tc.expectError {
require.Error(t, err)
return
Expand All @@ -145,3 +127,42 @@ func TestDefaultConfigurationChangeDetector_HasConfigurationChanged(t *testing.T
})
}
}

func TestKonnectConfigurationChangeDetector(t *testing.T) {
ctx := context.Background()
testSHAs := [][]byte{
[]byte("2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824"),
[]byte("82e35a63ceba37e9646434c5dd412ea577147f1e4a41ccde1614253187e3dbf9"),
}

testCases := []struct {
name string
oldSHA, newSHA []byte
expectedResult bool
expectError bool
}{
{
name: "oldSHA != newSHA",
oldSHA: testSHAs[0],
newSHA: testSHAs[1],
expectedResult: true,
},
{
name: "oldSHA == newSHA",
oldSHA: testSHAs[0],
newSHA: testSHAs[0],
expectedResult: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
detector := sendconfig.NewKonnectConfigurationChangeDetector()

// Passing nil for content and status client explicitly, as they are not used in this detector.
result, err := detector.HasConfigurationChanged(ctx, tc.oldSHA, tc.newSHA, nil, nil)
require.NoError(t, err)
require.Equal(t, tc.expectedResult, result)
})
}
}
2 changes: 1 addition & 1 deletion internal/dataplane/sendconfig/sendconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func PerformUpdate(

// disable optimization if reverse sync is enabled
if !config.EnableReverseSync {
configurationChanged, err := configChangeDetector.HasConfigurationChanged(ctx, oldSHA, newSHA, targetContent, client, client.AdminAPIClient())
configurationChanged, err := configChangeDetector.HasConfigurationChanged(ctx, oldSHA, newSHA, targetContent, client.AdminAPIClient())
if err != nil {
return nil, fmt.Errorf("failed to detect configuration change: %w", err)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/konnect/config_synchronizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestConfigSynchronizer_UpdatesKongConfigAccordingly(t *testing.T) {
ConfigUploadTicker: clock.NewTickerWithDuration(testSendConfigPeriod),
KonnectClientFactory: &mocks.KonnectClientFactory{Client: testKonnectClient},
UpdateStrategyResolver: resolver,
ConfigChangeDetector: sendconfig.NewDefaultConfigurationChangeDetector(log),
ConfigChangeDetector: sendconfig.NewKongGatewayConfigurationChangeDetector(log),
ConfigStatusNotifier: configStatusNotifier,
MetricsRecorder: &mocks.MetricsRecorder{},
},
Expand Down Expand Up @@ -141,7 +141,7 @@ func TestConfigSynchronizer_ConfigIsSanitizedWhenConfiguredSo(t *testing.T) {
ConfigUploadTicker: clock.NewTickerWithDuration(testSendConfigPeriod),
KonnectClientFactory: &mocks.KonnectClientFactory{Client: testKonnectClient},
UpdateStrategyResolver: resolver,
ConfigChangeDetector: sendconfig.NewDefaultConfigurationChangeDetector(log),
ConfigChangeDetector: sendconfig.NewKongGatewayConfigurationChangeDetector(log),
ConfigStatusNotifier: configStatusNotifier,
MetricsRecorder: &mocks.MetricsRecorder{},
},
Expand Down
2 changes: 1 addition & 1 deletion internal/manager/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func Run(
}

updateStrategyResolver := sendconfig.NewDefaultUpdateStrategyResolver(kongConfig, logger)
configurationChangeDetector := sendconfig.NewDefaultConfigurationChangeDetector(logger)
configurationChangeDetector := sendconfig.NewKongGatewayConfigurationChangeDetector(logger)
kongConfigFetcher := configfetcher.NewDefaultKongLastGoodConfigFetcher(translatorFeatureFlags.FillIDs, c.KongWorkspace)
fallbackConfigGenerator := fallback.NewGenerator(fallback.NewDefaultCacheGraphProvider(), logger)
metricsRecorder := metrics.NewGlobalCtrlRuntimeMetricsRecorder()
Expand Down
10 changes: 4 additions & 6 deletions internal/manager/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ func setupLicenseGetter(
return nil, nil
}

// setupKonnectConfigSynchronizerWithMgr sets up Konnect config sychronizer and adds it to the manager runnables.
// setupKonnectConfigSynchronizerWithMgr sets up Konnect config synchronizer and adds it to the manager runnables.
func setupKonnectConfigSynchronizerWithMgr(
ctx context.Context,
mgr manager.Manager,
Expand All @@ -520,11 +520,9 @@ func setupKonnectConfigSynchronizerWithMgr(
ConfigUploadTicker: clock.NewTickerWithDuration(cfg.Konnect.UploadConfigPeriod),
KonnectClientFactory: adminapi.NewKonnectClientFactory(cfg.Konnect, ctrl.LoggerFrom(ctx).WithName("konnect-client-factory")),
UpdateStrategyResolver: updateStrategyResolver,
ConfigChangeDetector: sendconfig.NewDefaultConfigurationChangeDetector(
ctrl.LoggerFrom(ctx).WithName("konnect-config-change-detector"),
),
ConfigStatusNotifier: configStatusNotifier,
MetricsRecorder: metricsRecorder,
ConfigChangeDetector: sendconfig.NewKonnectConfigurationChangeDetector(),
ConfigStatusNotifier: configStatusNotifier,
MetricsRecorder: metricsRecorder,
},
)
err := mgr.Add(s)
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/configuration_change_detector.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ type ConfigurationChangeDetector struct {
}

func (m ConfigurationChangeDetector) HasConfigurationChanged(
context.Context, []byte, []byte, *file.Content, sendconfig.KonnectAwareClient, sendconfig.StatusClient,
context.Context, []byte, []byte, *file.Content, sendconfig.StatusClient,
) (bool, error) {
return m.ConfigurationChanged, nil
}
Loading