fix: add default tag name if was missed from configuration#158
Merged
Conversation
WalkthroughSet default Tag values for cluster, Kubernetes, and Linkerd resources when empty, and rename the local Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/healthcheck/check.go`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f5e8173-51ab-4b50-b826-fbe518e20361
📒 Files selected for processing (1)
internal/healthcheck/check.go
Comment on lines
+25
to
+27
| if cluster.Tag == "" { | ||
| cluster.Tag = "cluster" | ||
| } |
There was a problem hiding this comment.
🧩 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' -C2Repository: 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 -C3Repository: openkcm/checker
Length of output: 725
🏁 Script executed:
# Check the full process method to understand goroutine lifecycle
sed -n '15,60p' internal/healthcheck/check.goRepository: 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.goRepository: 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.goRepository: 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/ -C2Repository: 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.goRepository: 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/healthcheck/check.go`:
- Around line 24-29: Add a blank line before each wg.Go(...) call to satisfy the
linter: locate the wg.Go invocations that call ch.processResources(ctx, cluster,
verifyServiceResource, resultCollector) (and the other wg.Go call around lines
34-39) and insert a single empty line between the preceding closing brace and
the wg.Go(...) expression so there is whitespace above the expression statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08d6142c-c49d-401f-bb48-c354fd17cd33
📒 Files selected for processing (1)
internal/healthcheck/check.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/healthcheck/check.go (1)
22-30: 🛠️ Refactor suggestion | 🟠 MajorAvoid mutating shared input config when applying default tags.
Line 24, Line 35, and Line 46 write defaults back into
cfg-backed structs. This is still fragile for concurrent callers ofprocess/refresh; prefer local copies (or local effective tag values) and pass those into goroutines.Proposed refactor
- cluster := &cfg.Cluster - if cluster.Enabled && len(cluster.Resources) > 0 { - if cluster.Tag == "" { - cluster.Tag = "cluster" - } + clusterCfg := cfg.Cluster + if clusterCfg.Enabled && len(clusterCfg.Resources) > 0 { + if clusterCfg.Tag == "" { + clusterCfg.Tag = "cluster" + } wg.Go(func() { - ch.processResources(ctx, cluster, verifyServiceResource, resultCollector) + ch.processResources(ctx, &clusterCfg, verifyServiceResource, resultCollector) }) } @@ - k8s := &cfg.Kubernetes - if k8s.Enabled && len(k8s.Resources) > 0 { - if k8s.Tag == "" { - k8s.Tag = "kubernetes" - } + k8sCfg := cfg.Kubernetes + if k8sCfg.Enabled && len(k8sCfg.Resources) > 0 { + if k8sCfg.Tag == "" { + k8sCfg.Tag = "kubernetes" + } wg.Go(func() { - ch.processResources(ctx, k8s, verifyK8SResource, resultCollector) + ch.processResources(ctx, &k8sCfg, verifyK8SResource, resultCollector) }) } @@ - linkerd := &cfg.Linkerd - if linkerd.Enabled { - if linkerd.Tag == "" { - linkerd.Tag = "linkerd" - } + linkerdCfg := cfg.Linkerd + if linkerdCfg.Enabled { + if linkerdCfg.Tag == "" { + linkerdCfg.Tag = "linkerd" + } wg.Go(func() { - ch.processLinkerdResources(ctx, linkerd, resultCollector) + ch.processLinkerdResources(ctx, &linkerdCfg, resultCollector) }) }Verification goal: confirm whether
processcan run concurrently and whether mutatingcfg.*.Tagcan race in current call paths.#!/bin/bash set -euo pipefail echo "1) Locate process() definition and call sites" rg -nP --type=go '\(\s*ch\s+\*CachedResponses\s*\)\s*process\s*\(|\.process\s*\(' -C3 echo echo "2) Find refresh() and its scheduling/concurrency context" rg -nP --type=go '\(\s*ch\s+\*CachedResponses\s*\)\s*refresh\s*\(|\.refresh\s*\(' -C3 echo echo "3) Check for goroutine-based invocations around healthcheck flows" rg -nP --type=go 'go\s+func|go\s+.*\.(process|refresh)\s*\(' internal/healthcheck -C3 echo echo "4) Enumerate Tag assignments in healthcheck package" rg -nP --type=go '\.(Tag)\s*=' internal/healthcheck -C2As per coding guidelines "
**/*.go: Prioritize identifying security vulnerabilities, unhandled errors, and concurrency issues (e.g., goroutine leaks, race conditions)."Also applies to: 35-40, 46-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/healthcheck/check.go` around lines 22 - 30, The code mutates cfg-backed structs (e.g., cfg.Cluster.Tag) before spawning goroutines which can race; instead compute a local effectiveTag (or copy the struct) and pass that into the goroutine rather than writing back into cfg. For example, when handling cfg.Cluster use a local variable like effectiveTag := cluster.Tag; if empty set effectiveTag = "cluster" (or deep-copy cluster into localCluster with localCluster.Tag set) and call ch.processResources(ctx, localCluster, verifyServiceResource, resultCollector) or pass effectiveTag into processResources; apply the same change for other instances that set cfg.*.Tag (the other Tag assignments referenced) and avoid mutating shared cfg from within the goroutine setup (wg.Go / processResources).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/healthcheck/check.go`:
- Around line 22-30: The code mutates cfg-backed structs (e.g., cfg.Cluster.Tag)
before spawning goroutines which can race; instead compute a local effectiveTag
(or copy the struct) and pass that into the goroutine rather than writing back
into cfg. For example, when handling cfg.Cluster use a local variable like
effectiveTag := cluster.Tag; if empty set effectiveTag = "cluster" (or deep-copy
cluster into localCluster with localCluster.Tag set) and call
ch.processResources(ctx, localCluster, verifyServiceResource, resultCollector)
or pass effectiveTag into processResources; apply the same change for other
instances that set cfg.*.Tag (the other Tag assignments referenced) and avoid
mutating shared cfg from within the goroutine setup (wg.Go / processResources).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6260511-6d74-4464-8e1e-606dcf534d15
📒 Files selected for processing (1)
internal/healthcheck/check.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Chores