Conversation
📝 WalkthroughWalkthroughThe change adds host connector health checks to ChangesConnector health monitoring
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Controller
participant Bindings
participant ConnectorTarget
participant Router
Controller->>Bindings: Start health-check loop
Bindings->>ConnectorTarget: Dial host and port
ConnectorTarget-->>Bindings: Return reachability result
Bindings->>Router: Refresh bridge configuration
Bindings->>Controller: Report connector health change
Controller->>Router: Apply updated router configuration
Merge Risk: 🟠 High · up to Concurrent health and connector updates can misroute traffic or crash Kubernetes controller processing. These issues should be fixed before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: e99a1ca1-5720-45a4-aba6-b54ded2b32bd
📒 Files selected for processing (4)
internal/kube/site/extended_bindings.gointernal/nonkube/controller/system_adaptor_handler.gointernal/nonkube/controller/system_adaptor_handler_test.gointernal/site/bindings.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| func (a *ExtendedBindings) onConnectorHealthChanged(changedConnectors []site.ConnectorHealthChange) { | ||
| if a.site != nil { | ||
| a.logger.Info("Connector health status changed, updating router config", slog.Any("connectors", changedConnectors)) | ||
| if err := a.site.updateRouterConfig(a); err != nil { | ||
| a.logger.Error("Failed to update router config on health check change", slog.Any("error", err)) | ||
| } | ||
| for _, res := range changedConnectors { | ||
| connector := a.bindings.GetConnector(res.Name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,220p' internal/kube/site/extended_bindings.go
sed -n '600,670p' internal/kube/site/extended_bindings.go
rg -n 'updateRouterConfig|GetConnector|updateConnector|deleteConnector|connectors|onConnectorHealthChanged' internal/kube/site internal/site/bindings.goRepository: skupperproject/skupper
Length of output: 18741
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- internal/site/bindings.go:1-220 ---'
sed -n '1,220p' internal/site/bindings.go
printf '%s\n' '--- internal/site/bindings.go:300-550 ---'
sed -n '300,550p' internal/site/bindings.go
printf '%s\n' '--- extended_bindings.go:360-610 ---'
sed -n '360,610p' internal/kube/site/extended_bindings.go
printf '%s\n' '--- event processor files ---'
fd -t f . internal | rg 'watchers|event|processor'
printf '%s\n' '--- EventProcessor references ---'
rg -n -C 4 'type EventProcessor|NewEventProcessor|EventProcessor|controller\.|Process.*Event|Handle.*Event' internal/kube internal | head -n 500
printf '%s\n' '--- router update and lifecycle references ---'
sed -n '240,300p' internal/kube/site/site.go
sed -n '1015,1075p' internal/kube/site/site.go
sed -n '1,130p' internal/kube/site/bindings.goRepository: skupperproject/skupper
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,220p' internal/site/bindings.go
sed -n '300,550p' internal/site/bindings.go
sed -n '360,610p' internal/kube/site/extended_bindings.go
fd -t f . internal | rg 'watchers|event|processor'
rg -n -C 4 'type EventProcessor|NewEventProcessor|EventProcessor|Process.*Event|Handle.*Event' internal/kube internal
sed -n '240,300p' internal/kube/site/site.go
sed -n '1015,1075p' internal/kube/site/site.go
sed -n '1,130p' internal/kube/site/bindings.goRepository: skupperproject/skupper
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- event processor core ---'
sed -n '170,285p' internal/kube/watchers/watchers.go
printf '%s\n' '--- site binding watcher/handlers ---'
rg -n -C 5 'Watch.*Connector|checkConnector|ConnectorUpdated|ConnectorDeleted|UpdateConnector|SetBindingEventHandler|bindings\.' internal/kube/site --glob '*.go' | head -n 260
printf '%s\n' '--- site router update ---'
sed -n '1018,1068p' internal/kube/site/site.go
printf '%s\n' '--- site initialization around binding setup ---'
rg -n -C 12 'NewSite|SetSite|init\(|Watch.*Connector|Watch.*Listener|WatchAttached' internal/kube/site/site.go internal/kube/site/*.go | head -n 320
printf '%s\n' '--- exact relevant mutation definitions ---'
sed -n '180,275p' internal/site/bindings.go
sed -n '480,610p' internal/kube/site/extended_bindings.goRepository: skupperproject/skupper
Length of output: 49095
🏁 Script executed:
#!/bin/bash
set -e
sed -n '170,285p' internal/kube/watchers/watchers.go
rg -n -C 5 'Watch.*Connector|checkConnector|ConnectorUpdated|ConnectorDeleted|UpdateConnector|SetBindingEventHandler|bindings\.' internal/kube/site --glob '*.go' | head -n 260
sed -n '1018,1068p' internal/kube/site/site.go
rg -n -C 12 'NewSite|SetSite|init\(|Watch.*Connector|Watch.*Listener|WatchAttached' internal/kube/site/site.go internal/kube/site/*.go | head -n 320
sed -n '180,275p' internal/site/bindings.go
sed -n '480,610p' internal/kube/site/extended_bindings.goRepository: skupperproject/skupper
Length of output: 48904
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- router update consumer ---'
rg -n -C 8 'func UpdateRouterConfig|type ConfigUpdate|Apply\(config' internal/kube internal/qdr --glob '*.go' | head -n 220
printf '%s\n' '--- controller paths into Site binding methods ---'
rg -n -C 5 'CheckConnector|CheckListener|CheckAttachedConnector|AttachedConnectorUpdated|UpdateConnector\(|UpdateListener\(|UpdateMultiKeyListener\(' internal --glob '*.go' | head -n 300Repository: skupperproject/skupper
Length of output: 30539
🏁 Script executed:
#!/bin/bash
set -e
rg -n -C 8 'func UpdateRouterConfig|type ConfigUpdate|Apply\(config' internal/kube internal/qdr --glob '*.go' | head -n 220
rg -n -C 5 'CheckConnector|CheckListener|CheckAttachedConnector|AttachedConnectorUpdated|UpdateConnector\(|UpdateListener\(|UpdateMultiKeyListener\(' internal --glob '*.go' | head -n 300Repository: skupperproject/skupper
Length of output: 29543
Serialize health-change processing with controller updates.
Bindings.PerformHealthChecks invokes this callback from the health-check goroutine after releasing healthMu. The controller event loop can concurrently call Bindings.UpdateConnector, which mutates Bindings.connectors without a shared lock. The callback also passes ExtendedBindings to updateRouterConfig; ExtendedBindings.Apply iterates its maps while controller handlers may mutate them. GetConnector reads the same unprotected binding map.
This can cause a concurrent-map panic or build router configuration from mixed state. Dispatch the callback through the controller event loop, or protect all shared binding state with one synchronization mechanism.
| if os.Getuid() == 0 { | ||
| api.DefaultRootDataHome = tempDir |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,240p' internal/nonkube/controller/system_adaptor_handler_test.go
rg -n 'DefaultRootDataHome|t\.Parallel\(' internal/nonkube pkg/nonkubeRepository: skupperproject/skupper
Length of output: 8094
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- environment.go ---'
cat -n pkg/nonkube/api/environment.go
printf '%s\n' '--- controller assignments and relevant test setup ---'
rg -n -C 8 'DefaultRootDataHome|GetInternalOutputPath|GetDataHome' internal/nonkube/controller
printf '%s\n' '--- all non-kube test references ---'
rg -n -C 5 'DefaultRootDataHome|GetInternalOutputPath|GetDataHome' internal/nonkube pkg/nonkube --glob '*_test.go' --glob '*.go'Repository: skupperproject/skupper
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- controller test functions and packages ---'
rg -n '^(func Test|package )' internal/nonkube/controller --glob '*_test.go'
printf '%s\n' '--- controller test path consumers ---'
rg -l 'GetInternalOutputPath|GetDefaultOutput|GetDataHome|GetHostDataHome|DefaultRootDataHome' internal/nonkube/controller --glob '*_test.go' |
while read -r f; do
printf '%s\n' "--- $f"
rg -n 'DefaultRootDataHome|GetInternalOutputPath|GetDefaultOutput|GetDataHome|GetHostDataHome' "$f"
done
printf '%s\n' '--- direct controller test ordering/concurrency controls ---'
rg -n 't\.Parallel|t\.Run|shuffle|TestMain' internal/nonkube/controller --glob '*_test.go' || trueRepository: skupperproject/skupper
Length of output: 8363
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- system adaptor production path ---'
cat -n internal/nonkube/controller/system_adaptor_handler.go | sed -n '1,125p'
printf '%s\n' '--- runtime address lookup ---'
rg -n -C 12 'func GetLocalRouterAddress|GetInternalOutputPath|RuntimeSiteStatePath' internal/nonkube/client/runtime
printf '%s\n' '--- no-setup controller tests and constructors ---'
cat -n internal/nonkube/controller/input_resource_handler_test.go | sed -n '1,140p'
rg -n -C 8 'func NewInputResourceHandler|GetInternalOutputPath|PathProvider' internal/nonkube/controller/input_resource_handler.go internal/nonkube/controller/*.goRepository: skupperproject/skupper
Length of output: 37744
Restore api.DefaultRootDataHome in both reconciliation tests.
When tests run as root, GetDataHome uses api.DefaultRootDataHome. Each test sets it to t.TempDir(), but cleanup deletes that directory while the global retains its path. A subsequent test that uses GetInternalOutputPath can then access a deleted path. Save the original value and restore it with t.Cleanup at both lines 80 and 156.
| if _, exists := b.hostConnectors[res.Name]; !exists { | ||
| continue | ||
| } | ||
| prevHealthy, ok := b.connectorHealth[res.Name] | ||
| if !ok || prevHealthy != res.Healthy { | ||
| b.connectorHealth[res.Name] = res.Healthy |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject health results for superseded endpoints.
A health check can start for one endpoint while reconciliation changes the same connector name to another endpoint. The current existence check accepts the old result and applies it to the replacement endpoint. This can remove a healthy replacement connector or restore an unavailable connector.
Compare the current Host and Port with the result before updating connectorHealth.
Proposed fix
- if _, exists := b.hostConnectors[res.Name]; !exists {
+ current, exists := b.hostConnectors[res.Name]
+ if !exists || current.Host != res.Host || current.Port != res.Port {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, exists := b.hostConnectors[res.Name]; !exists { | |
| continue | |
| } | |
| prevHealthy, ok := b.connectorHealth[res.Name] | |
| if !ok || prevHealthy != res.Healthy { | |
| b.connectorHealth[res.Name] = res.Healthy | |
| current, exists := b.hostConnectors[res.Name] | |
| if !exists || current.Host != res.Host || current.Port != res.Port { | |
| continue | |
| } | |
| prevHealthy, ok := b.connectorHealth[res.Name] | |
| if !ok || prevHealthy != res.Healthy { | |
| b.connectorHealth[res.Name] = res.Healthy |
1 participant
Resolves #2078
Summary by CodeRabbit
New Features
Bug Fixes