From d2a4c5d3325dd1a0a2972a4472032cafc8760e9d Mon Sep 17 00:00:00 2001 From: Travis Raines <571832+rainest@users.noreply.github.com> Date: Thu, 25 Jul 2024 16:45:36 -0700 Subject: [PATCH] chore: plumb diagnostic to DB mode strategies --- internal/dataplane/kong_client.go | 1 + internal/dataplane/kong_client_test.go | 5 ++- internal/dataplane/sendconfig/dbmode.go | 34 +++++++++++++------ internal/dataplane/sendconfig/sendconfig.go | 8 ++--- internal/dataplane/sendconfig/strategy.go | 24 +++++++++++-- .../dataplane/sendconfig/strategy_test.go | 2 +- internal/konnect/config_synchronizer.go | 1 + internal/konnect/config_synchronizer_test.go | 6 +++- .../kong_client_golden_tests_outputs_test.go | 2 +- 9 files changed, 62 insertions(+), 21 deletions(-) diff --git a/internal/dataplane/kong_client.go b/internal/dataplane/kong_client.go index e62d365ce9..4576a54dfb 100644 --- a/internal/dataplane/kong_client.go +++ b/internal/dataplane/kong_client.go @@ -828,6 +828,7 @@ func (c *KongClient) sendToClient( c.prometheusMetrics, c.updateStrategyResolver, c.configChangeDetector, + &c.diagnostic, isFallback, ) // Only record events on applying configuration to Kong gateway here. diff --git a/internal/dataplane/kong_client_test.go b/internal/dataplane/kong_client_test.go index 4cf0077604..29eb00d3c7 100644 --- a/internal/dataplane/kong_client_test.go +++ b/internal/dataplane/kong_client_test.go @@ -189,7 +189,10 @@ func newMockUpdateStrategyResolver(t *testing.T) *mockUpdateStrategyResolver { } } -func (f *mockUpdateStrategyResolver) ResolveUpdateStrategy(c sendconfig.UpdateClient) sendconfig.UpdateStrategy { +func (f *mockUpdateStrategyResolver) ResolveUpdateStrategy( + c sendconfig.UpdateClient, + diagnostics *diagnostics.ClientDiagnostic, +) sendconfig.UpdateStrategy { f.lock.Lock() defer f.lock.Unlock() diff --git a/internal/dataplane/sendconfig/dbmode.go b/internal/dataplane/sendconfig/dbmode.go index ca5cf6015b..2ecc0c7687 100644 --- a/internal/dataplane/sendconfig/dbmode.go +++ b/internal/dataplane/sendconfig/dbmode.go @@ -29,6 +29,7 @@ type UpdateStrategyDBMode struct { dumpConfig dump.Config version semver.Version concurrency int + diagnostic *diagnostics.ClientDiagnostic isKonnect bool logger logr.Logger resourceErrors []ResourceError @@ -40,6 +41,7 @@ func NewUpdateStrategyDBMode( dumpConfig dump.Config, version semver.Version, concurrency int, + diagnostic *diagnostics.ClientDiagnostic, logger logr.Logger, ) UpdateStrategyDBMode { return UpdateStrategyDBMode{ @@ -47,6 +49,7 @@ func NewUpdateStrategyDBMode( dumpConfig: dumpConfig, version: version, concurrency: concurrency, + diagnostic: diagnostic, logger: logger, resourceErrors: []ResourceError{}, resourceErrorLock: &sync.Mutex{}, @@ -58,9 +61,10 @@ func NewUpdateStrategyDBModeKonnect( dumpConfig dump.Config, version semver.Version, concurrency int, + diagnostic *diagnostics.ClientDiagnostic, logger logr.Logger, ) UpdateStrategyDBMode { - s := NewUpdateStrategyDBMode(client, dumpConfig, version, concurrency, logger) + s := NewUpdateStrategyDBMode(client, dumpConfig, version, concurrency, diagnostic, logger) s.isKonnect = true return s } @@ -92,9 +96,7 @@ func (s UpdateStrategyDBMode) Update(ctx context.Context, targetContent ContentW ctx, cancel := context.WithCancel(ctx) // TRR this is where db mode update strat handles events. resultchan is the entityaction channel // TRR targetContent.Hash is the config hash - // TRR TODO need to plumb the actual channel from the diag server over here. standin black hole for now - diffs := make(chan diagnostics.ConfigDiff, 3) - go s.HandleEvents(ctx, syncer.GetResultChan(), diffs, string(targetContent.Hash)) + go s.HandleEvents(ctx, syncer.GetResultChan(), s.diagnostic, string(targetContent.Hash)) _, errs, _ := syncer.Solve(ctx, s.concurrency, false, false) cancel() @@ -150,16 +152,10 @@ func (s UpdateStrategyDBMode) Update(ctx context.Context, targetContent ContentW func (s *UpdateStrategyDBMode) HandleEvents( ctx context.Context, events chan diff.EntityAction, - diffChan chan diagnostics.ConfigDiff, + diagnostic *diagnostics.ClientDiagnostic, hash string, ) { - // TRR this is where we get the diff info from deck s.resourceErrorLock.Lock() - // TRR TODO this accumulator isn't great since we need to append to the array, which is... probably unsafe? maybe - // the for can't actually handle multiple select inbounds at once, but I think it can, and append calls would cause - // havoc. can maybe use something from https://pkg.go.dev/sync/atomic to increment a counter, use that as a map key - // and then convert the values into a slice for the Done handler. otherwise this would need to send individual - // EntityDiffs down a channel to the diag server, which seems less than ideal diff := diagnostics.ConfigDiff{ Hash: hash, Entities: []diagnostics.EntityDiff{}, @@ -179,6 +175,22 @@ func (s *UpdateStrategyDBMode) HandleEvents( } } case <-ctx.Done(): + // The DB mode update strategy is used for both DB mode gateways and Konnect-integrated controllers. In the + // Konnect case, we don't actually want to collect diffs, and don't actually provide a diagnostic when setting + // it up, so we only collect and send diffs if we're talking to a gateway. + // + // TRR TODO maybe this is wrong? I'm not sure if we actually support (or if not, explicitly prohibit) + // configuring a controller to use both DB mode and talk to Konnect, or if we only support DB-less when using + // Konnect. If those are mutually exclusive, maybe we can just collect diffs for Konnect mode? If they're + // not mutually exclusive, trying to do diagnostics diff updates for both the updates would have both attempt + // to store diffs. This is... maybe okay. They should be identical, but that's a load-bearing "should": we know + // Konnect can sometimes differ in what it accepts versus the gateway, and we have some Konnect configuration + // (consumer exclude, sensitive value mask) where they're _definitely_ different. That same configuration could + // make the diff confusing even if it's DB mode only, since it doesn't reflect what we're sending to the gateway + // in some cases. + if diagnostic != nil { + diagnostic.Diffs <- diff + } s.resourceErrorLock.Unlock() return } diff --git a/internal/dataplane/sendconfig/sendconfig.go b/internal/dataplane/sendconfig/sendconfig.go index 73ff568a49..4f7106fe86 100644 --- a/internal/dataplane/sendconfig/sendconfig.go +++ b/internal/dataplane/sendconfig/sendconfig.go @@ -11,6 +11,7 @@ import ( "github.com/kong/go-kong/kong" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/deckgen" + "github.com/kong/kubernetes-ingress-controller/v3/internal/diagnostics" "github.com/kong/kubernetes-ingress-controller/v3/internal/logging" "github.com/kong/kubernetes-ingress-controller/v3/internal/metrics" "github.com/kong/kubernetes-ingress-controller/v3/internal/store" @@ -22,7 +23,7 @@ import ( // ----------------------------------------------------------------------------- type UpdateStrategyResolver interface { - ResolveUpdateStrategy(client UpdateClient) UpdateStrategy + ResolveUpdateStrategy(client UpdateClient, diagnostic *diagnostics.ClientDiagnostic) UpdateStrategy } type AdminAPIClient interface { @@ -48,6 +49,7 @@ func PerformUpdate( promMetrics *metrics.CtrlFuncMetrics, updateStrategyResolver UpdateStrategyResolver, configChangeDetector ConfigurationChangeDetector, + diagnostic *diagnostics.ClientDiagnostic, isFallback bool, ) ([]byte, error) { oldSHA := client.LastConfigSHA() @@ -72,9 +74,7 @@ func PerformUpdate( } } - // TRR probably plumb the diag server channel down to here. this builds either strategy, but can have the dbless - // one just discard it - updateStrategy := updateStrategyResolver.ResolveUpdateStrategy(client) + updateStrategy := updateStrategyResolver.ResolveUpdateStrategy(client, diagnostic) logger = logger.WithValues("update_strategy", updateStrategy.Type()) timeStart := time.Now() // TRR this is the point in sendconfig where db events maybe happen, but it's not differentiated by strategy here diff --git a/internal/dataplane/sendconfig/strategy.go b/internal/dataplane/sendconfig/strategy.go index 2dc02ea36f..543f7a6de9 100644 --- a/internal/dataplane/sendconfig/strategy.go +++ b/internal/dataplane/sendconfig/strategy.go @@ -10,6 +10,7 @@ import ( "github.com/kong/go-kong/kong/custom" "github.com/kong/kubernetes-ingress-controller/v3/internal/adminapi" + "github.com/kong/kubernetes-ingress-controller/v3/internal/diagnostics" "github.com/kong/kubernetes-ingress-controller/v3/internal/metrics" ) @@ -77,8 +78,9 @@ func NewDefaultUpdateStrategyResolver(config Config, logger logr.Logger) Default // with the backoff strategy it provides. func (r DefaultUpdateStrategyResolver) ResolveUpdateStrategy( client UpdateClient, + diagnostic *diagnostics.ClientDiagnostic, ) UpdateStrategy { - updateStrategy := r.resolveUpdateStrategy(client) + updateStrategy := r.resolveUpdateStrategy(client, diagnostic) if clientWithBackoff, ok := client.(UpdateClientWithBackoff); ok { return NewUpdateStrategyWithBackoff(updateStrategy, clientWithBackoff.BackoffStrategy(), r.logger) @@ -87,7 +89,10 @@ func (r DefaultUpdateStrategyResolver) ResolveUpdateStrategy( return updateStrategy } -func (r DefaultUpdateStrategyResolver) resolveUpdateStrategy(client UpdateClient) UpdateStrategy { +func (r DefaultUpdateStrategyResolver) resolveUpdateStrategy( + client UpdateClient, + diagnostic *diagnostics.ClientDiagnostic, +) UpdateStrategy { adminAPIClient := client.AdminAPIClient() // In case the client communicates with Konnect Admin API, we know it has to use DB-mode. There's no need to check @@ -100,6 +105,20 @@ func (r DefaultUpdateStrategyResolver) resolveUpdateStrategy(client UpdateClient }, r.config.Version, r.config.Concurrency, + // The DB mode update strategy is used for both DB mode gateways and Konnect-integrated controllers. In the + // Konnect case, we don't actually want to collect diffs, and don't actually provide a diagnostic when setting + // it up, so we only collect and send diffs if we're talking to a gateway. + // + // TRR TODO maybe this is wrong? I'm not sure if we actually support (or if not, explicitly prohibit) + // configuring a controller to use both DB mode and talk to Konnect, or if we only support DB-less when using + // Konnect. If those are mutually exclusive, maybe we can just collect diffs for Konnect mode? If they're + // not mutually exclusive, trying to do diagnostics diff updates for both the updates would have both attempt + // to store diffs. This is... maybe okay. They should be identical, but that's a load-bearing "should": we know + // Konnect can sometimes differ in what it accepts versus the gateway, and we have some Konnect configuration + // (consumer exclude, sensitive value mask) where they're _definitely_ different. That same configuration could + // make the diff confusing even if it's DB mode only, since it doesn't reflect what we're sending to the gateway + // in some cases. + nil, r.logger, ) } @@ -114,6 +133,7 @@ func (r DefaultUpdateStrategyResolver) resolveUpdateStrategy(client UpdateClient }, r.config.Version, r.config.Concurrency, + diagnostic, r.logger, ) } diff --git a/internal/dataplane/sendconfig/strategy_test.go b/internal/dataplane/sendconfig/strategy_test.go index 666accd27c..094b2c228f 100644 --- a/internal/dataplane/sendconfig/strategy_test.go +++ b/internal/dataplane/sendconfig/strategy_test.go @@ -92,7 +92,7 @@ func TestDefaultUpdateStrategyResolver_ResolveUpdateStrategy(t *testing.T) { InMemory: tc.inMemory, }, zapr.NewLogger(zap.NewNop())) - strategy := resolver.ResolveUpdateStrategy(updateClient) + strategy := resolver.ResolveUpdateStrategy(updateClient, nil) require.Equal(t, tc.expectedStrategyType, strategy.Type()) assert.True(t, client.adminAPIClientWasCalled) assert.Equal(t, tc.expectKonnectControlPlaneCall, client.konnectControlPlaneWasCalled) diff --git a/internal/konnect/config_synchronizer.go b/internal/konnect/config_synchronizer.go index 36f5d05753..664eac2ca8 100644 --- a/internal/konnect/config_synchronizer.go +++ b/internal/konnect/config_synchronizer.go @@ -128,6 +128,7 @@ func (s *ConfigSynchronizer) uploadConfig(ctx context.Context, client *adminapi. s.prometheusMetrics, s.updateStrategyResolver, s.configChangeDetector, + nil, isFallback, ) if err != nil { diff --git a/internal/konnect/config_synchronizer_test.go b/internal/konnect/config_synchronizer_test.go index a0d38d2550..a4340abd95 100644 --- a/internal/konnect/config_synchronizer_test.go +++ b/internal/konnect/config_synchronizer_test.go @@ -19,6 +19,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v3/internal/adminapi" dpconf "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/config" "github.com/kong/kubernetes-ingress-controller/v3/internal/dataplane/sendconfig" + "github.com/kong/kubernetes-ingress-controller/v3/internal/diagnostics" "github.com/kong/kubernetes-ingress-controller/v3/internal/metrics" ) @@ -94,7 +95,10 @@ func newMockUpdateStrategyResolver() *mockUpdateStrategyResolver { } } -func (f *mockUpdateStrategyResolver) ResolveUpdateStrategy(c sendconfig.UpdateClient) sendconfig.UpdateStrategy { +func (f *mockUpdateStrategyResolver) ResolveUpdateStrategy( + c sendconfig.UpdateClient, + diagnostic *diagnostics.ClientDiagnostic, +) sendconfig.UpdateStrategy { f.lock.Lock() defer f.lock.Unlock() diff --git a/test/kongintegration/kong_client_golden_tests_outputs_test.go b/test/kongintegration/kong_client_golden_tests_outputs_test.go index bba8d8e01c..0db13ea584 100644 --- a/test/kongintegration/kong_client_golden_tests_outputs_test.go +++ b/test/kongintegration/kong_client_golden_tests_outputs_test.go @@ -104,7 +104,7 @@ func TestKongClientGoldenTestsOutputs_Konnect(t *testing.T) { updateStrategy := sendconfig.NewUpdateStrategyDBModeKonnect(adminAPIClient.AdminAPIClient(), dump.Config{ SkipCACerts: true, KonnectControlPlane: cpID, - }, semver.MustParse("3.5.0"), 10, logr.Discard()) + }, semver.MustParse("3.5.0"), 10, nil, logr.Discard()) for _, goldenTestOutputPath := range allGoldenTestsOutputsPaths(t) { t.Run(goldenTestOutputPath, func(t *testing.T) {