Skip to content

fix: add default tag name if was missed from configuration #158

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.

Sign up for GitHub

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

Merged
nnicora merged 5 commits into from
Mar 17, 2026
Merged

fix: add default tag name if was missed from configuration #158

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7908186
fix: add default tag name if was missed from configuration
nnicora Mar 17, 2026
a2df9b1
fix: add default tag name if was missed from configuration
nnicora Mar 17, 2026
ad8b94f
fix: add default tag name if was missed from configuration
nnicora Mar 17, 2026
48cd576
fix: add default tag name if was missed from configuration
nnicora Mar 17, 2026
0a04261
Merge branch 'main' into fix/some-defaults
nnicora Mar 17, 2026
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
18 changes: 15 additions & 3 deletions internal/healthcheck/check.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,35 @@ func (ch *CachedResponses) process(
) {
wg := sync.WaitGroup{}

service := &cfg.Cluster
if service.Enabled && len(service.Resources) > 0 {
cluster := &cfg.Cluster
if cluster.Enabled && len(cluster.Resources) > 0 {
wg.Go(func() {
ch.processResources(ctx, service, verifyServiceResource, resultCollector)
if cluster.Tag == "" {
cluster.Tag = "cluster"
}
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "1) Find process method and callsites"
rg -nP --type=go '\(\s*ch\s+\*CachedResponses\s*\)\s*process\s*\(|\.process\s*\(' -C2

echo
echo "2) Check whether process is invoked from goroutines"
rg -nP --type=go 'go\s+.*process\s*\(' -C3

echo
echo "3) Check shared Healthcheck pointer reuse patterns"
rg -nP --type=go '\*config\.Healthcheck|config\.Healthcheck' -C2

Repository: openkcm/checker

Length of output: 641

🏁 Script executed:

# Check the context around the process call in cached.go
sed -n '75,100p' internal/healthcheck/cached.go

# Also check if there are goroutines spawning at the cached.go level
rg -nP --type=go 'go\s+' internal/healthcheck/cached.go -C3

Repository: openkcm/checker

Length of output: 725

🏁 Script executed:

# Check the full process method to understand goroutine lifecycle
sed -n '15,60p' internal/healthcheck/check.go

Repository: openkcm/checker

Length of output: 960

🏁 Script executed:

# Find all callers of refresh method
rg -nP --type=go 'refresh\s*\(' internal/healthcheck/

# Check if there are other public methods that might trigger concurrent access
rg -nP --type=go 'func\s*\(\s*ch\s+\*CachedResponses\s*\)\s*[A-Z]' internal/healthcheck/check.go

Repository: openkcm/checker

Length of output: 291

🏁 Script executed:

# Check if Tag fields are accessed elsewhere in the codebase
rg -nP --type=go '\.Tag\s*=' internal/healthcheck/ 

# Check for concurrent accesses or locks around config mutation
rg -nP --type=go 'mu\s*(Sync|RW|Lock)' internal/healthcheck/

Repository: openkcm/checker

Length of output: 413

🏁 Script executed:

# View the context around refresh calls to understand execution flow
sed -n '20,50p' internal/healthcheck/cached.go

Repository: openkcm/checker

Length of output: 660

🏁 Script executed:

# Check all public methods of CachedResponses to see if there's concurrent access
rg -nP --type=go 'func\s*\(\s*ch\s+\*CachedResponses\s*\)\s*[A-Z]' internal/healthcheck/

Repository: openkcm/checker

Length of output: 406

🏁 Script executed:

# Check if there are any other goroutines spawning that might call process or access config
rg -nP --type=go 'go\s+func' internal/healthcheck/ -C2

Repository: openkcm/checker

Length of output: 670

🏁 Script executed:

# Verify there are no other concurrent callers by checking HTTP handlers or public APIs
rg -nP --type=go 'func.*\(.*\*http' internal/healthcheck/ 

# Check the package exports and public API surface
rg -nP --type=go 'func\s+[A-Z]' internal/healthcheck/cached.go

Repository: openkcm/checker

Length of output: 150

Avoid mutating input parameter fields inside goroutines for clarity and defensive safety

While the current code is not subject to concurrent access (single-threaded ticker loop), mutating cluster.Tag, k8s.Tag, and linkerd.Tag inside goroutines is poor practice and creates fragility. If refresh() is ever called concurrently or the pattern is reused elsewhere, this becomes a data race. Compute effective tag values before spawning each goroutine.

Proposed refactor 
 	cluster := &cfg.Cluster
 	if cluster.Enabled && len(cluster.Resources) > 0 {
+		clusterCfg := *cluster
+		if clusterCfg.Tag == "" {
+			clusterCfg.Tag = "cluster"
+		}
 		wg.Go(func() {
-			if cluster.Tag == "" {
-				cluster.Tag = "cluster"
-			}
-
-			ch.processResources(ctx, cluster, verifyServiceResource, resultCollector)
+			ch.processResources(ctx, &clusterCfg, verifyServiceResource, resultCollector)
 		})
 	}
@@
 	k8s := &cfg.Kubernetes
 	if k8s.Enabled && len(k8s.Resources) > 0 {
+		k8sCfg := *k8s
+		if k8sCfg.Tag == "" {
+			k8sCfg.Tag = "kubernetes"
+		}
 		wg.Go(func() {
-			if k8s.Tag == "" {
-				k8s.Tag = "kubernetes"
-			}
-
-			ch.processResources(ctx, k8s, verifyK8SResource, resultCollector)
+			ch.processResources(ctx, &k8sCfg, verifyK8SResource, resultCollector)
 		})
 	}
@@
 	linkerd := &cfg.Linkerd
 	if linkerd.Enabled {
+		linkerdCfg := *linkerd
+		if linkerdCfg.Tag == "" {
+			linkerdCfg.Tag = "linkerd"
+		}
 		wg.Go(func() {
-			if linkerd.Tag == "" {
-				linkerd.Tag = "linkerd"
-			}
-
-			ch.processLinkerdResources(ctx, linkerd, resultCollector)
+			ch.processLinkerdResources(ctx, &linkerdCfg, resultCollector)
 		})
 	}

Also applies to: 36-38, 47-49

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@internal/healthcheck/check.go` around lines 25 - 27, The code is mutating
input structs' Tag fields (cluster.Tag, k8s.Tag, linkerd.Tag) inside goroutines
which can cause data races; compute local effective tag values before spawning
each goroutine instead of assigning into the input structs. For each place where
you currently do "if cluster.Tag == "" { cluster.Tag = "cluster" }" (and the
similar k8s/linkerd blocks) in refresh(), create a local variable (e.g.,
clusterTag, k8sTag, linkerdTag), set it to the existing Tag or the default
("cluster", "k8s", "linkerd") and use that local variable inside the goroutine
closures; do not write back to cluster.Tag, k8s.Tag, or linkerd.Tag. Ensure all
goroutines reference these locals so no input structs are mutated.


ch.processResources(ctx, cluster, verifyServiceResource, resultCollector)
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

k8s := &cfg.Kubernetes
if k8s.Enabled && len(k8s.Resources) > 0 {
wg.Go(func() {
if k8s.Tag == "" {
k8s.Tag = "kubernetes"
}

ch.processResources(ctx, k8s, verifyK8SResource, resultCollector)
})
}

linkerd := &cfg.Linkerd
if linkerd.Enabled {
wg.Go(func() {
if linkerd.Tag == "" {
linkerd.Tag = "linkerd"
}

ch.processLinkerdResources(ctx, linkerd, resultCollector)
})
}
Expand Down
Loading