chore: plumb diagnostic to DB mode strategies

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.

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()
 

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,13 +41,15 @@ func NewUpdateStrategyDBMode(
 	dumpConfig dump.Config,
 	version semver.Version,
 	concurrency int,
+	diagnostic *diagnostics.ClientDiagnostic,
 	logger logr.Logger,
 ) UpdateStrategyDBMode {
 	return UpdateStrategyDBMode{
 		client:            client,
 		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
 		}

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

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,
 		)
 	}

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)

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 {

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()
 

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) {