From 4aacb7614961712ad13bd270b06a8255fb83031c Mon Sep 17 00:00:00 2001 From: markturansky Date: Tue, 1 Dec 2015 10:18:02 -0500 Subject: [PATCH] added cast checks to controllers to prevent nil panics --- ...ersistentvolume_claim_binder_controller.go | 53 +++++++++++++------ ...tentvolume_claim_binder_controller_test.go | 22 ++++++++ .../persistentvolume_recycler_controller.go | 11 ++-- 3 files changed, 66 insertions(+), 20 deletions(-) diff --git a/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller.go b/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller.go index 8ca5ea51f21..9d438203342 100644 --- a/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller.go +++ b/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller.go @@ -94,24 +94,32 @@ func NewPersistentVolumeClaimBinder(kubeClient client.Interface, syncPeriod time return binder } - func (binder *PersistentVolumeClaimBinder) addVolume(obj interface{}) { binder.lock.Lock() defer binder.lock.Unlock() - volume := obj.(*api.PersistentVolume) - err := syncVolume(binder.volumeIndex, binder.client, volume) - if err != nil { - glog.Errorf("PVClaimBinder could not add volume %s: %+v", volume.Name, err) + pv, ok := obj.(*api.PersistentVolume) + if !ok { + glog.Errorf("Expected PersistentVolume but handler received %+v", obj) + return + } + if err := syncVolume(binder.volumeIndex, binder.client, pv); err != nil { + glog.Errorf("PVClaimBinder could not add volume %s: %+v", pv.Name, err) } } func (binder *PersistentVolumeClaimBinder) updateVolume(oldObj, newObj interface{}) { binder.lock.Lock() defer binder.lock.Unlock() - newVolume := newObj.(*api.PersistentVolume) - binder.volumeIndex.Update(newVolume) - err := syncVolume(binder.volumeIndex, binder.client, newVolume) - if err != nil { + newVolume, ok := newObj.(*api.PersistentVolume) + if !ok { + glog.Errorf("Expected PersistentVolume but handler received %+v", newObj) + return + } + if err := binder.volumeIndex.Update(newVolume); err != nil { + glog.Errorf("Error updating volume %s in index: %v", newVolume.Name, err) + return + } + if err := syncVolume(binder.volumeIndex, binder.client, newVolume); err != nil { glog.Errorf("PVClaimBinder could not update volume %s: %+v", newVolume.Name, err) } } @@ -119,16 +127,25 @@ func (binder *PersistentVolumeClaimBinder) updateVolume(oldObj, newObj interface func (binder *PersistentVolumeClaimBinder) deleteVolume(obj interface{}) { binder.lock.Lock() defer binder.lock.Unlock() - volume := obj.(*api.PersistentVolume) - binder.volumeIndex.Delete(volume) + volume, ok := obj.(*api.PersistentVolume) + if !ok { + glog.Errorf("Expected PersistentVolume but handler received %+v", obj) + return + } + if err := binder.volumeIndex.Delete(volume); err != nil { + glog.Errorf("Error deleting volume %s from index: %v", volume.Name, err) + } } func (binder *PersistentVolumeClaimBinder) addClaim(obj interface{}) { binder.lock.Lock() defer binder.lock.Unlock() - claim := obj.(*api.PersistentVolumeClaim) - err := syncClaim(binder.volumeIndex, binder.client, claim) - if err != nil { + claim, ok := obj.(*api.PersistentVolumeClaim) + if !ok { + glog.Errorf("Expected PersistentVolumeClaim but handler received %+v", obj) + return + } + if err := syncClaim(binder.volumeIndex, binder.client, claim); err != nil { glog.Errorf("PVClaimBinder could not add claim %s: %+v", claim.Name, err) } } @@ -136,9 +153,11 @@ func (binder *PersistentVolumeClaimBinder) addClaim(obj interface{}) { func (binder *PersistentVolumeClaimBinder) updateClaim(oldObj, newObj interface{}) { binder.lock.Lock() defer binder.lock.Unlock() - newClaim := newObj.(*api.PersistentVolumeClaim) - err := syncClaim(binder.volumeIndex, binder.client, newClaim) - if err != nil { + newClaim, ok := newObj.(*api.PersistentVolumeClaim) + if !ok { + glog.Errorf("Expected PersistentVolumeClaim but handler received %+v", newObj) + } + if err := syncClaim(binder.volumeIndex, binder.client, newClaim); err != nil { glog.Errorf("PVClaimBinder could not update claim %s: %+v", newClaim.Name, err) } } diff --git a/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller_test.go b/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller_test.go index 2f7688cffa9..96762d6bdab 100644 --- a/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller_test.go +++ b/pkg/controller/persistentvolume/persistentvolume_claim_binder_controller_test.go @@ -25,6 +25,7 @@ import ( "k8s.io/kubernetes/pkg/api/errors" "k8s.io/kubernetes/pkg/api/resource" "k8s.io/kubernetes/pkg/api/testapi" + "k8s.io/kubernetes/pkg/client/cache" "k8s.io/kubernetes/pkg/client/unversioned/testclient" "k8s.io/kubernetes/pkg/volume" "k8s.io/kubernetes/pkg/volume/host_path" @@ -369,6 +370,27 @@ func TestBindingWithExamples(t *testing.T) { } } +func TestCasting(t *testing.T) { + client := &testclient.Fake{} + binder := NewPersistentVolumeClaimBinder(client, 1*time.Second) + + pv := &api.PersistentVolume{} + unk := cache.DeletedFinalStateUnknown{} + pvc := &api.PersistentVolumeClaim{ + ObjectMeta: api.ObjectMeta{Name: "foo"}, + Status: api.PersistentVolumeClaimStatus{Phase: api.ClaimBound}, + } + + // none of these should fail casting. + // the real test is not failing when passed DeletedFinalStateUnknown in the deleteHandler + binder.addVolume(pv) + binder.updateVolume(pv, pv) + binder.deleteVolume(pv) + binder.deleteVolume(unk) + binder.addClaim(pvc) + binder.updateClaim(pvc, pvc) +} + type mockBinderClient struct { volume *api.PersistentVolume claim *api.PersistentVolumeClaim diff --git a/pkg/controller/persistentvolume/persistentvolume_recycler_controller.go b/pkg/controller/persistentvolume/persistentvolume_recycler_controller.go index 3d85dac9e6a..30077a41c22 100644 --- a/pkg/controller/persistentvolume/persistentvolume_recycler_controller.go +++ b/pkg/controller/persistentvolume/persistentvolume_recycler_controller.go @@ -71,15 +71,20 @@ func NewPersistentVolumeRecycler(kubeClient client.Interface, syncPeriod time.Du }, }, &api.PersistentVolume{}, - // TODO: Can we have much longer period here? syncPeriod, framework.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - pv := obj.(*api.PersistentVolume) + pv, ok := obj.(*api.PersistentVolume) + if !ok { + glog.Errorf("Error casting object to PersistentVolume: %v", obj) + } recycler.reclaimVolume(pv) }, UpdateFunc: func(oldObj, newObj interface{}) { - pv := newObj.(*api.PersistentVolume) + pv, ok := newObj.(*api.PersistentVolume) + if !ok { + glog.Errorf("Error casting object to PersistentVolume: %v", newObj) + } recycler.reclaimVolume(pv) }, },