Merge pull request #4599 from dkeven/fixmappanic

Fix: deepcopy before mutating shared objects

pkg/models/resources/v1alpha2/persistentvolumeclaim/persistentvolumeclaims.go

@@ -123,6 +123,7 @@ func (s *persistentVolumeClaimSearcher) Search(namespace string, conditions *par
 
 	r := make([]interface{}, 0)
 	for _, i := range result {
+		i = i.DeepCopy()
 		inUse := s.countPods(i.Name, i.Namespace)
 		isSnapshotAllow := s.isSnapshotAllowed(i.GetAnnotations()["volume.beta.kubernetes.io/storage-provisioner"])
 		if i.Annotations == nil {

pkg/models/resources/v1alpha2/storageclass/storageclasses.go

@@ -91,6 +91,7 @@ func (s *storageClassesSearcher) Search(namespace string, conditions *params.Con
 
 	r := make([]interface{}, 0)
 	for _, i := range result {
+		i = i.DeepCopy()
 		count := s.countPersistentVolumeClaims(i.Name)
 		if i.Annotations == nil {
 			i.Annotations = make(map[string]string)

pkg/models/resources/v1alpha2/storageclass/storageclasses_test.go

@@ -22,8 +22,19 @@ var (
 		},
 	}
 
+	sc1Expected = &v1.StorageClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "sc1",
+			Annotations: map[string]string{
+				"kubesphere.io/pvc-count": "1",
+			},
+		},
+	}
+
 	scs = []interface{}{sc1}
 
+	scsExpected = []interface{}{sc1Expected}
+
 	pvc1 = &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "pvc1",
@@ -85,7 +96,7 @@ func TestSearch(t *testing.T) {
 			conditions:  &params.Conditions{},
 			orderBy:     "name",
 			reverse:     true,
-			expected:    scs,
+			expected:    scsExpected,
 			expectedErr: nil,
 		},
 	}

pkg/models/resources/v1alpha3/node/nodes.go

@@ -19,8 +19,6 @@ package node
 import (
 	"fmt"
 	"sort"
-	"sync"
-	"time"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -36,7 +34,6 @@ import (
 
 // Those annotations were added to node only for display purposes
 const (
-	nodeAnnotatedAt                                 = "node.kubesphere.io/last-annotated-at"
 	nodeCPURequests                                 = "node.kubesphere.io/cpu-requests"
 	nodeMemoryRequests                              = "node.kubesphere.io/memory-requests"
 	nodeCPULimits                                   = "node.kubesphere.io/cpu-limits"
@@ -54,13 +51,11 @@ const (
 
 type nodesGetter struct {
 	informers informers.SharedInformerFactory
-	mutex     sync.Mutex
 }
 
 func New(informers informers.SharedInformerFactory) v1alpha3.Interface {
 	return &nodesGetter{
 		informers: informers,
-		mutex:     sync.Mutex{},
 	}
 }
 
@@ -72,6 +67,20 @@ func (c *nodesGetter) Get(_, name string) (runtime.Object, error) {
 
 	// ignore the error, skip annotating process if error happened
 	pods, _ := c.informers.Core().V1().Pods().Lister().Pods("").List(labels.Everything())
+
+	// Never mutate original objects!
+	// Caches are shared across controllers,
+	// this means that if you mutate your "copy" (actually a reference or shallow copy) of an object,
+	// you'll mess up other controllers (not just your own).
+	// Also, if the mutated field is a map,
+	// a "concurrent map (read &) write" panic might occur,
+	// causing the ks-apiserver to crash.
+	// Refer:
+	// https://github.com/kubesphere/kubesphere/issues/4357
+	// https://github.com/kubesphere/kubesphere/issues/3469
+	// https://github.com/kubesphere/kubesphere/pull/4599
+	// https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/controllers.md
+	node = node.DeepCopy()
 	c.annotateNode(node, pods)
 
 	return node, nil
@@ -124,6 +133,7 @@ func (c *nodesGetter) List(_ string, q *query.Query) (*api.ListResult, error) {
 
 	var result = make([]interface{}, 0)
 	for _, node := range selectedNodes {
+		node = node.DeepCopy()
 		c.annotateNode(node, nonTerminatedPodsList)
 		result = append(result, node)
 	}
@@ -162,25 +172,13 @@ func (c *nodesGetter) filter(object runtime.Object, filter query.Filter) bool {
 }
 
 // annotateNode adds cpu/memory requests usage data to node's annotations
-// this operation is resource consuming, so avoid annotating on every query
+// this operation mutates the *v1.Node passed in
+// so DO A DEEPCOPY before calling
 func (c *nodesGetter) annotateNode(node *v1.Node, pods []*v1.Pod) {
-	// only annotate node when two consecutive annotating gap bigger than 30s
-	c.mutex.Lock()
 	if node.Annotations == nil {
 		node.Annotations = make(map[string]string)
 	}
 
-	if lastAnnotatedAt, ok := node.Annotations[nodeAnnotatedAt]; ok {
-		if lastAnnotationTimeStamp, err := time.Parse(time.RFC3339, lastAnnotatedAt); err != nil {
-			if lastAnnotationTimeStamp.Add(30 * time.Second).After(time.Now()) {
-				c.mutex.Unlock()
-				return
-			}
-		}
-	}
-	node.Annotations[nodeAnnotatedAt] = time.Now().Format(time.RFC3339)
-	c.mutex.Unlock()
-
 	if len(pods) == 0 {
 		return
 	}

pkg/models/resources/v1alpha3/node/nodes_test.go

@@ -145,8 +145,6 @@ func TestNodesGetterGet(t *testing.T) {
 	}
 	nodeGot := got.(*corev1.Node)
 
-	// ignore last-annotated-at annotation
-	delete(nodeGot.Annotations, nodeAnnotatedAt)
 	if diff := cmp.Diff(nodeGot.Annotations, expectedAnnotations); len(diff) != 0 {
 		t.Errorf("%T, diff(-got, +expected), %v", expectedAnnotations, nodeGot.Annotations)
 	}
@@ -170,7 +168,7 @@ func TestListNodes(t *testing.T) {
 			},
 			&api.ListResult{
 				Items: []interface{}{
-					node2,
+					node2Expected,
 				},
 				TotalItems: 1,
 			},
@@ -187,7 +185,7 @@ func TestListNodes(t *testing.T) {
 			},
 			&api.ListResult{
 				Items: []interface{}{
-					node1,
+					node1Expected,
 				},
 				TotalItems: 1,
 			},
@@ -204,7 +202,7 @@ func TestListNodes(t *testing.T) {
 			},
 			&api.ListResult{
 				Items: []interface{}{
-					node2,
+					node2Expected,
 				},
 				TotalItems: 1,
 			},
@@ -239,6 +237,17 @@ var (
 			Unschedulable: true,
 		},
 	}
+
+	node1Expected = &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "node1",
+			Annotations: map[string]string{},
+		},
+		Spec: corev1.NodeSpec{
+			Unschedulable: true,
+		},
+	}
+
 	node2 = &corev1.Node{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "node2",
@@ -247,6 +256,17 @@ var (
 			Unschedulable: false,
 		},
 	}
+
+	node2Expected = &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "node2",
+			Annotations: map[string]string{},
+		},
+		Spec: corev1.NodeSpec{
+			Unschedulable: false,
+		},
+	}
+
 	nodes = []*corev1.Node{node1, node2}
 )
 

pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim.go

@@ -53,6 +53,8 @@ func (p *persistentVolumeClaimGetter) Get(namespace, name string) (runtime.Objec
 	if err != nil {
 		return pvc, err
 	}
+	// we should never mutate the shared objects from informers
+	pvc = pvc.DeepCopy()
 	p.annotatePVC(pvc)
 	return pvc, nil
 }
@@ -65,6 +67,7 @@ func (p *persistentVolumeClaimGetter) List(namespace string, query *query.Query)
 
 	var result []runtime.Object
 	for _, pvc := range all {
+		pvc = pvc.DeepCopy()
 		p.annotatePVC(pvc)
 		result = append(result, pvc)
 	}

pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim_test.go

@@ -37,7 +37,7 @@ var (
 	testStorageClassName = "test-csi"
 )
 
-func TestListPods(t *testing.T) {
+func TestListPVCs(t *testing.T) {
 	tests := []struct {
 		description string
 		namespace   string
@@ -58,7 +58,7 @@ func TestListPods(t *testing.T) {
 				Filters:   map[query.Field]query.Value{query.FieldNamespace: query.Value("default")},
 			},
 			&api.ListResult{
-				Items:      []interface{}{pvc3, pvc2, pvc1},
+				Items:      []interface{}{pvc3Expected, pvc2Expected, pvc1Expected},
 				TotalItems: len(persistentVolumeClaims),
 			},
 			nil,
@@ -79,7 +79,7 @@ func TestListPods(t *testing.T) {
 				},
 			},
 			&api.ListResult{
-				Items:      []interface{}{pvc1},
+				Items:      []interface{}{pvc1Expected},
 				TotalItems: 1,
 			},
 			nil,
@@ -100,7 +100,7 @@ func TestListPods(t *testing.T) {
 				},
 			},
 			&api.ListResult{
-				Items:      []interface{}{pvcGet2},
+				Items:      []interface{}{pvc2Expected},
 				TotalItems: 1,
 			},
 			nil,
@@ -121,7 +121,7 @@ func TestListPods(t *testing.T) {
 				},
 			},
 			&api.ListResult{
-				Items:      []interface{}{pvcGet3},
+				Items:      []interface{}{pvc3Expected},
 				TotalItems: 1,
 			},
 			nil,
@@ -154,6 +154,21 @@ var (
 			Phase: corev1.ClaimPending,
 		},
 	}
+
+	pvc1Expected = &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "pvc-1",
+			Namespace: "default",
+			Annotations: map[string]string{
+				annotationInUse:         "false",
+				annotationAllowSnapshot: "false",
+			},
+		},
+		Status: corev1.PersistentVolumeClaimStatus{
+			Phase: corev1.ClaimPending,
+		},
+	}
+
 	pvc2 = &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "pvc-2",
@@ -167,7 +182,7 @@ var (
 		},
 	}
 
-	pvcGet2 = &corev1.PersistentVolumeClaim{
+	pvc2Expected = &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "pvc-2",
 			Namespace: "default",
@@ -189,7 +204,7 @@ var (
 		},
 	}
 
-	pvcGet3 = &corev1.PersistentVolumeClaim{
+	pvc3Expected = &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "pvc-3",
 			Namespace: "default",