fix(kubernetes): informer tombstones panic the deployment/statefulset watchers#1018
Merged
Conversation
… watchers The deployment and statefulset event handlers asserted event objects unchecked (obj.(*appsv1.Deployment)). Informers deliver cache.DeletedFinalStateUnknown tombstones to DeleteFunc when a watch disconnect made them miss the final delete; the assertion then panics inside the informer processor goroutine and kills the whole process. The CNPG watcher already unwrapped tombstones (clusterFromObject); the two older watchers never got the same fix. All handlers now go through a shared generic eventObject[T] helper that unwraps tombstones and refuses unexpected types, and clusterFromObject delegates to it. UpdateFunc additionally guarded: Spec.Replicas is a *int32 and was dereferenced without a nil check; replicasOf defaults it to 1 like the API server does. Reproduced by feeding tombstones through the real handlers (extracted as named methods for testability): interface conversion panic before, clean Removed+Stopped events after.
|
Test Results✅ All tests passed! | 757 tests in 145.822s
|
|
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.
Finding
The deployment and statefulset informer handlers assert event objects unchecked:
client-go delivers
cache.DeletedFinalStateUnknowntombstones toDeleteFuncwhenever a watch disconnect made the informer miss the final delete (it finds out about the deletion from the relist). On a tombstone, the unchecked assertion panics inside the informer's processor goroutine - there is no HTTP recovery middleware there, so the panic kills the entire Sablier process.The trigger is mundane: any API-server hiccup / network blip that drops the watch while a managed workload gets deleted. Exactly the moment you want Sablier to stay up.
Two adjacent latent panics in the same handlers:
UpdateFuncdereferences*oldDeployment.Spec.Replicaswithout a nil check - the field is a*int32.AddFunc/UpdateFunc(tombstones are delete-only in practice, but the assertions are equally unguarded against anything unexpected).The telling part
The CNPG watcher in the same package already solves this -
clusterFromObjectunwraps tombstones correctly (cnpgcluster_events.go). The newest of the three watchers got the fix; the two older copies never did. Classic copy-paste divergence.Discovery
Found during a provider-layer design review (cross-comparing the three informer watchers). Reproduced by feeding a tombstone through the real handlers (extracted as named methods for testability - a behavior-preserving refactor):
Fix
eventObject[T any](obj any) (T, bool)helper: returns the typed object, unwrapping a tombstone when present, and refuses anything unexpected instead of panicking. All three watchers (deployment, statefulset, CNPG) now use one mechanism -clusterFromObjectdelegates to it.replicasOf(*int32)guards theSpec.Replicasdereferences, defaulting to 1 exactly like the API server.watchDeployments/watchStatefulSetsnow build their handlers via named methods (deploymentEventHandler,statefulSetEventHandler) so the handlers are unit-testable; informer wiring is unchanged.Tests
TestDeploymentDeleteHandlesTombstone/TestStatefulSetDeleteHandlesTombstone- panic before, emit the expectedRemoved+Stoppedevents after.TestUpdateHandlesNilReplicas- pins the nil-Spec.Replicasguard.Package tests and
golangci-lintpass.Docs
No user-facing docs impact (internal crash fix; no config, label, or API change).