refactor(diagnostics): encapsulate Client channels with send methods

internal/dataplane/kong_client.go

@@ -899,13 +899,13 @@ func prepareSendDiagnosticFn(
 	targetContent *file.Content,
 	deckGenParams deckgen.GenerateDeckContentParams,
 ) sendDiagnosticFn {
-	if diagnosticConfig == (diagnostics.Client{}) {
+	if diagnosticConfig.IsEmpty() {
 		// noop, diagnostics won't be sent
 		return func(diagnostics.DumpMeta, []byte) {}
 	}
 
 	var config *file.Content
-	if diagnosticConfig.DumpsIncludeSensitive {
+	if diagnosticConfig.DumpsIncludeSensitive() {
 		config = targetContent
 	} else {
 		redactedConfig := deckgen.ToDeckContent(ctx,
@@ -923,14 +923,13 @@ func prepareSendDiagnosticFn(
 		// might not see exactly what they intend to see i.e. come failures
 		// or successfully send configs might be covered by those send
 		// later on but we're OK with this limitation of said API.
-		select {
-		case diagnosticConfig.Configs <- diagnostics.ConfigDump{
+		if ok := diagnosticConfig.SendConfig(diagnostics.ConfigDump{
 			Meta:            meta,
 			Config:          *config,
 			RawResponseBody: rawResponseBody,
-		}:
+		}); ok {
 			logger.V(logging.DebugLevel).Info("Shipping config to diagnostic server")
-		default:
+		} else {
 			logger.Error(nil, "Config diagnostic buffer full, dropping diagnostic config")
 		}
 	}
@@ -1081,13 +1080,16 @@ func (c *KongClient) logFallbackCacheMetadata(metadata fallback.GeneratedCacheMe
 }
 
 func (c *KongClient) maybeSendFallbackConfigDiagnostics(ctx context.Context, generatedCacheMetadata fallback.GeneratedCacheMetadata) error {
-	if ch := c.diagnostic.FallbackCacheMetadata; ch != nil {
-		select {
-		case ch <- generatedCacheMetadata:
+	if c.diagnostic.IsEmpty() {
+		return nil
+	}
+	select {
+	case <-ctx.Done():
+		return ctx.Err()
+	default:
+		if ok := c.diagnostic.SendFallbackCacheMetadata(generatedCacheMetadata); ok {
 			c.logger.V(logging.DebugLevel).Info("Shipping fallback cache metadata to diagnostics server")
-		case <-ctx.Done():
-			return ctx.Err()
-		default:
+		} else {
 			c.logger.Error(nil, "Fallback cache metadata buffer full, dropping diagnostics")
 		}
 	}

internal/dataplane/kong_client_test.go

@@ -981,7 +981,7 @@ func TestKongClient_FallbackConfiguration_SuccessfulRecovery(t *testing.T) {
 	ctx := t.Context()
 	configChangeDetector := mocks.ConfigurationChangeDetector{ConfigurationChanged: true}
 	lastValidConfigFetcher := &mockKongLastValidConfigFetcher{}
-	diagnosticsCh := make(chan diagnostics.ConfigDump, 10) // make it buffered to avoid blocking
+	diagnosticClient := diagnostics.NewClient(false, 10, 10, 10)
 
 	// We'll use KongConsumer as an example of a broken object, but it could be any supported type
 	// for the purpose of this test as the fallback config generator is mocked anyway.
@@ -1033,9 +1033,7 @@ func TestKongClient_FallbackConfiguration_SuccessfulRecovery(t *testing.T) {
 				&originalCache,
 				fallbackConfigGenerator,
 				mocks.MetricsRecorder{},
-				WithDiagnosticsClient(diagnostics.Client{
-					Configs: diagnosticsCh,
-				}),
+				WithDiagnosticsClient(diagnosticClient),
 			)
 			require.NoError(t, err)
 
@@ -1126,7 +1124,7 @@ func TestKongClient_FallbackConfiguration_SuccessfulRecovery(t *testing.T) {
 	// silly hack to churn through those until we get to the successful fallback.
 	var dump diagnostics.ConfigDump
 	require.Eventually(t, func() bool {
-		dump = <-diagnosticsCh
+		dump = <-diagnosticClient.Configs()
 		return dump.Meta.Fallback
 	}, time.Second, time.Nanosecond)
 
@@ -1146,7 +1144,7 @@ func TestKongClient_FallbackConfiguration_SkipsUpdateWhenInSync(t *testing.T) {
 	configBuilder := newMockKongConfigBuilder()
 	lastValidConfigFetcher := &mockKongLastValidConfigFetcher{}
 	fallbackConfigGenerator := newMockFallbackConfigGenerator()
-	diagnosticsCh := make(chan diagnostics.ConfigDump, 10) // make it buffered to avoid blocking
+	diagnosticClient := diagnostics.NewClient(false, 10, 10, 10)
 
 	// We'll use KongConsumer as an example of an object, but it could be any supported type
 	// for the purpose of this test as the fallback config generator is mocked anyway.
@@ -1167,9 +1165,7 @@ func TestKongClient_FallbackConfiguration_SkipsUpdateWhenInSync(t *testing.T) {
 		&originalCache,
 		fallbackConfigGenerator,
 		mocks.MetricsRecorder{},
-		WithDiagnosticsClient(diagnostics.Client{
-			Configs: diagnosticsCh,
-		}),
+		WithDiagnosticsClient(diagnosticClient),
 	)
 	require.NoError(t, err)
 
@@ -1290,7 +1286,7 @@ func TestKongClient_FallbackConfiguration_FailedRecovery(t *testing.T) {
 	configBuilder := newMockKongConfigBuilder()
 	lastValidConfigFetcher := &mockKongLastValidConfigFetcher{}
 	fallbackConfigGenerator := newMockFallbackConfigGenerator()
-	diagnosticsCh := make(chan diagnostics.ConfigDump, 10) // make it buffered to avoid blocking
+	diagnosticClient := diagnostics.NewClient(false, 10, 10, 10)
 
 	// We'll use KongConsumer as an example of a broken object, but it could be any supported type
 	// for the purpose of this test as the fallback config generator is mocked anyway.
@@ -1312,9 +1308,7 @@ func TestKongClient_FallbackConfiguration_FailedRecovery(t *testing.T) {
 		&originalCache,
 		fallbackConfigGenerator,
 		mocks.MetricsRecorder{},
-		WithDiagnosticsClient(diagnostics.Client{
-			Configs: diagnosticsCh,
-		}),
+		WithDiagnosticsClient(diagnosticClient),
 	)
 	require.NoError(t, err)
 
@@ -1348,7 +1342,7 @@ func TestKongClient_FallbackConfiguration_FailedRecovery(t *testing.T) {
 	// silly hack to churn through those until we get to the failed fallback.
 	var dump diagnostics.ConfigDump
 	require.Eventually(t, func() bool {
-		dump = <-diagnosticsCh
+		dump = <-diagnosticClient.Configs()
 		return dump.Meta.Fallback
 	}, time.Second, time.Nanosecond)
 
@@ -1494,16 +1488,12 @@ func TestKongClient_ConfigDumpSanitization(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			diagnosticsCh := make(chan diagnostics.ConfigDump, 1) // make it buffered to avoid blocking
-			kongClient.diagnostic = diagnostics.Client{
-				Configs:               diagnosticsCh,
-				DumpsIncludeSensitive: tc.dumpsIncludeSensitive,
-			}
+			kongClient.diagnostic = diagnostics.NewClient(tc.dumpsIncludeSensitive, 1, 1, 1)
 			ctx := t.Context()
 			err := kongClient.Update(ctx)
 			require.NoError(t, err)
 
-			dump := <-diagnosticsCh
+			dump := <-kongClient.diagnostic.Configs()
 			require.NotNil(t, dump.Config)
 			require.Len(t, dump.Config.Certificates, 1)
 			dumpedCert := dump.Config.Certificates[0]

internal/dataplane/sendconfig/dbmode.go

@@ -183,9 +183,9 @@ func (s *UpdateStrategyDBMode) HandleEvents(
 		case <-ctx.Done():
 			// Release resource error lock before sending diffs to diagnostic server to prevent blocking of main procedure of updating.
 			s.resourceErrorLock.Unlock()
-			if diagnostic != nil && diagnostic.Diffs != nil {
+			if diagnostic != nil && !diagnostic.IsEmpty() {
 				diff.Timestamp = time.Now().Format(time.RFC3339)
-				diagnostic.Diffs <- diff
+				diagnostic.SendDiff(diff)
 				s.logger.V(logging.DebugLevel).Info("recorded database update events and diff", "hash", hash)
 			}
 			return

internal/diagnostics/collector.go

@@ -53,12 +53,12 @@ func NewCollector(logger logr.Logger, cfg managercfg.Config) *Collector {
 	return &Collector{
 		logger: logger,
 		diffs:  newDiffMap(diffHistorySize),
-		clientDiagnostic: Client{
-			DumpsIncludeSensitive: cfg.DumpSensitiveConfig,
-			Configs:               make(chan ConfigDump, diagnosticConfigBufferDepth),
-			FallbackCacheMetadata: make(chan fallback.GeneratedCacheMetadata, diagnosticConfigBufferDepth),
-			Diffs:                 make(chan ConfigDiff, diagnosticConfigBufferDepth),
-		},
+		clientDiagnostic: NewClient(
+			cfg.DumpSensitiveConfig,
+			diagnosticConfigBufferDepth,
+			diagnosticConfigBufferDepth,
+			diagnosticConfigBufferDepth,
+		),
 	}
 }
 
@@ -141,11 +141,11 @@ func (s *Collector) AvailableConfigDiffsHashes() []DiffIndex {
 func (s *Collector) receiveDiagnostics(ctx context.Context) error {
 	for {
 		select {
-		case dump := <-s.clientDiagnostic.Configs:
+		case dump := <-s.clientDiagnostic.Configs():
 			s.onConfigDump(dump)
-		case meta := <-s.clientDiagnostic.FallbackCacheMetadata:
+		case meta := <-s.clientDiagnostic.FallbackCacheMetadataCh():
 			s.onFallbackCacheMetadata(meta)
-		case diff := <-s.clientDiagnostic.Diffs:
+		case diff := <-s.clientDiagnostic.DiffsCh():
 			s.onDiff(diff)
 		case <-ctx.Done():
 			if err := ctx.Err(); err != nil && !errors.Is(err, context.Canceled) {

internal/diagnostics/server_test.go

@@ -27,7 +27,7 @@ func TestDiagnosticsServer_ConfigDumps(t *testing.T) {
 	ctx := t.Context()
 
 	client, port := setupTestServer(ctx, t)
-	configsCh := client.Configs
+	configsCh := client.configs
 
 	// Use a WaitGroup to ensure that both the write and read operations are run simultaneously.
 	readWriteWg := sync.WaitGroup{}
@@ -78,7 +78,7 @@ func TestDiagnosticsServer_Diffs(t *testing.T) {
 	ctx, cancel := context.WithCancel(t.Context())
 	defer cancel()
 	client, port := setupTestServer(ctx, t)
-	diffCh := client.Diffs
+	diffCh := client.diffs
 
 	// initially write the max number of cached diffs
 	configDumpsToWrite := diffHistorySize

internal/diagnostics/types.go

@@ -28,20 +28,91 @@ type ConfigDump struct {
 }
 
 // Client contains settings and channels for receiving diagnostic data from the controller's Kong client.
-// TODO(czeslavo): we could consider refactoring this to use private channels and expose methods for sending data.
+// It encapsulates the channels and exposes methods for sending data to ensure controlled access.
 type Client struct {
-	// DumpsIncludeSensitive is true if the configuration dump includes sensitive values, such as certificate private
+	// dumpsIncludeSensitive is true if the configuration dump includes sensitive values, such as certificate private
 	// keys and credential secrets.
-	DumpsIncludeSensitive bool
+	dumpsIncludeSensitive bool
 
-	// Configs is the channel that receives configuration blobs from the configuration update strategy implementation.
-	Configs chan ConfigDump
+	// configs is the channel that receives configuration blobs from the configuration update strategy implementation.
+	configs chan ConfigDump
 
-	// FallbackCacheMetadata is the channel that receives fallback metadata from the fallback cache generator.
-	FallbackCacheMetadata chan fallback.GeneratedCacheMetadata
+	// fallbackCacheMetadata is the channel that receives fallback metadata from the fallback cache generator.
+	fallbackCacheMetadata chan fallback.GeneratedCacheMetadata
 
-	// Diffs is the channel that receives diff info in DB mode.
-	Diffs chan ConfigDiff
+	// diffs is the channel that receives diff info in DB mode.
+	diffs chan ConfigDiff
+}
+
+// NewClient creates a new Client with the given configuration.
+func NewClient(dumpsIncludeSensitive bool, configBufferSize, fallbackBufferSize, diffsBufferSize int) Client {
+	return Client{
+		dumpsIncludeSensitive: dumpsIncludeSensitive,
+		configs:               make(chan ConfigDump, configBufferSize),
+		fallbackCacheMetadata: make(chan fallback.GeneratedCacheMetadata, fallbackBufferSize),
+		diffs:                 make(chan ConfigDiff, diffsBufferSize),
+	}
+}
+
+// DumpsIncludeSensitive returns whether the configuration dump includes sensitive values.
+func (c Client) DumpsIncludeSensitive() bool {
+	return c.dumpsIncludeSensitive
+}
+
+// SendConfig sends a configuration dump to the diagnostics channel.
+// It returns false if the channel buffer is full and the send would block.
+func (c Client) SendConfig(dump ConfigDump) bool {
+	select {
+	case c.configs <- dump:
+		return true
+	default:
+		return false
+	}
+}
+
+// SendFallbackCacheMetadata sends fallback cache metadata to the diagnostics channel.
+// It returns false if the channel buffer is full and the send would block.
+func (c Client) SendFallbackCacheMetadata(meta fallback.GeneratedCacheMetadata) bool {
+	select {
+	case c.fallbackCacheMetadata <- meta:
+		return true
+	default:
+		return false
+	}
+}
+
+// SendDiff sends a configuration diff to the diagnostics channel.
+// It returns false if the channel buffer is full and the send would block.
+func (c Client) SendDiff(diff ConfigDiff) bool {
+	select {
+	case c.diffs <- diff:
+		return true
+	default:
+		return false
+	}
+}
+
+// IsEmpty returns true if the Client has not been initialized (zero value).
+func (c Client) IsEmpty() bool {
+	return c.configs == nil && c.fallbackCacheMetadata == nil && c.diffs == nil
+}
+
+// Configs returns a receive-only channel for reading configuration dumps.
+// This is primarily useful for testing and the diagnostics collector.
+func (c Client) Configs() <-chan ConfigDump {
+	return c.configs
+}
+
+// FallbackCacheMetadataCh returns a receive-only channel for reading fallback cache metadata.
+// This is primarily useful for testing and the diagnostics collector.
+func (c Client) FallbackCacheMetadataCh() <-chan fallback.GeneratedCacheMetadata {
+	return c.fallbackCacheMetadata
+}
+
+// DiffsCh returns a receive-only channel for reading configuration diffs.
+// This is primarily useful for testing and the diagnostics collector.
+func (c Client) DiffsCh() <-chan ConfigDiff {
+	return c.diffs
 }
 
 // AffectedObject is a Kubernetes object associated with diagnostic information.