pkg/controller: Fix CR with same name in different namespaces

This patch uses <namespace>/<cluster-name> as key to store the cluster in Controller struct to avoid over-writing same named CR's in different namespaces.

Fixes #1954

Please read https://github.com/coreos/etcd-operator/blob/master/CONTRIBUTING.md#contribution-flow

CHANGELOG.md

@@ -8,6 +8,8 @@
 
 ### Fixed
 
+- Fixed a bug where `same CR names` in different namespaces with cluster-wide operator were not working as expected [#2026](https://github.com/coreos/etcd-operator/pull/2026)
+
 ### Deprecated
 
 ### Security

pkg/cluster/cluster.go

@@ -86,7 +86,7 @@ type Cluster struct {
 }
 
 func New(config Config, cl *api.EtcdCluster) *Cluster {
-	lg := logrus.WithField("pkg", "cluster").WithField("cluster-name", cl.Name)
+	lg := logrus.WithField("pkg", "cluster").WithField("cluster-name", cl.Name).WithField("cluster-namespace", cl.Namespace)
 
 	c := &Cluster{
 		logger:    lg,

pkg/controller/controller.go

@@ -73,7 +73,7 @@ func (c *Controller) handleClusterEvent(event *Event) (bool, error) {
 	if clus.Status.IsFailed() {
 		clustersFailed.Inc()
 		if event.Type == kwatch.Deleted {
-			delete(c.clusters, clus.Name)
+			delete(c.clusters, getNamespacedName(clus))
 			return false, nil
 		}
 		return false, fmt.Errorf("ignore failed cluster (%s). Please delete its CR", clus.Name)
@@ -87,30 +87,30 @@ func (c *Controller) handleClusterEvent(event *Event) (bool, error) {
 
 	switch event.Type {
 	case kwatch.Added:
-		if _, ok := c.clusters[clus.Name]; ok {
+		if _, ok := c.clusters[getNamespacedName(clus)]; ok {
 			return false, fmt.Errorf("unsafe state. cluster (%s) was created before but we received event (%s)", clus.Name, event.Type)
 		}
 
 		nc := cluster.New(c.makeClusterConfig(), clus)
 
-		c.clusters[clus.Name] = nc
+		c.clusters[getNamespacedName(clus)] = nc
 
 		clustersCreated.Inc()
 		clustersTotal.Inc()
 
 	case kwatch.Modified:
-		if _, ok := c.clusters[clus.Name]; !ok {
+		if _, ok := c.clusters[getNamespacedName(clus)]; !ok {
 			return false, fmt.Errorf("unsafe state. cluster (%s) was never created but we received event (%s)", clus.Name, event.Type)
 		}
-		c.clusters[clus.Name].Update(clus)
+		c.clusters[getNamespacedName(clus)].Update(clus)
 		clustersModified.Inc()
 
 	case kwatch.Deleted:
-		if _, ok := c.clusters[clus.Name]; !ok {
+		if _, ok := c.clusters[getNamespacedName(clus)]; !ok {
 			return false, fmt.Errorf("unsafe state. cluster (%s) was never created but we received event (%s)", clus.Name, event.Type)
 		}
-		c.clusters[clus.Name].Delete()
-		delete(c.clusters, clus.Name)
+		c.clusters[getNamespacedName(clus)].Delete()
+		delete(c.clusters, getNamespacedName(clus))
 		clustersDeleted.Inc()
 		clustersTotal.Dec()
 	}
@@ -132,3 +132,7 @@ func (c *Controller) initCRD() error {
 	}
 	return k8sutil.WaitCRDReady(c.KubeExtCli, api.EtcdClusterCRDName)
 }
+
+func getNamespacedName(c *api.EtcdCluster) string {
+	return fmt.Sprintf("%s%c%s", c.Namespace, '/', c.Name)
+}

pkg/controller/controller_test.go

@@ -62,14 +62,14 @@ func TestHandleClusterEventDeleteFailedCluster(t *testing.T) {
 		Object: clus,
 	}
 
-	c.clusters[name] = &cluster.Cluster{}
+	c.clusters[getNamespacedName(clus)] = &cluster.Cluster{}
 
 	if _, err := c.handleClusterEvent(e); err != nil {
 		t.Fatal(err)
 	}
 
-	if c.clusters[name] != nil {
-		t.Errorf("failed cluster not cleaned up after delete event, cluster struct: %v", c.clusters[name])
+	if c.clusters[getNamespacedName(clus)] != nil {
+		t.Errorf("failed cluster not cleaned up after delete event, cluster struct: %v", c.clusters[getNamespacedName(clus)])
 	}
 }
 
@@ -78,7 +78,8 @@ func TestHandleClusterEventClusterwide(t *testing.T) {
 
 	clus := &api.EtcdCluster{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test",
+			Name:      "test",
+			Namespace: "a",
 			Annotations: map[string]string{
 				"etcd.database.coreos.com/scope": "clusterwide",
 			},
@@ -110,6 +111,35 @@ func TestHandleClusterEventClusterwideIgnored(t *testing.T) {
 	}
 }
 
+func TestHandleClusterEventClusterwideAddTwoCR(t *testing.T) {
+	c := New(Config{ClusterWide: true})
+	clusA := &api.EtcdCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: "a",
+		},
+	}
+	clusB := &api.EtcdCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: "b",
+		},
+	}
+	e := &Event{
+		Type:   watch.Added,
+		Object: clusA,
+	}
+	ignored, errA := c.handleClusterEvent(e)
+	if !ignored && errA != nil {
+		t.Errorf("cluster should be Added")
+	}
+	e.Object = clusB
+	ignored, errB := c.handleClusterEvent(e)
+	if !ignored && errB != nil {
+		t.Errorf("cluster should be Added")
+	}
+}
+
 func TestHandleClusterEventNamespacedIgnored(t *testing.T) {
 	c := New(Config{})
 

pkg/controller/informer.go

@@ -129,7 +129,7 @@ func (c *Controller) syncEtcdClus(clus *api.EtcdCluster) {
 	// re-watch or restart could give ADD event.
 	// If for an ADD event the cluster spec is invalid then it is not added to the local cache
 	// so modifying that cluster will result in another ADD event
-	if _, ok := c.clusters[clus.Name]; ok {
+	if _, ok := c.clusters[getNamespacedName(clus)]; ok {
 		ev.Type = kwatch.Modified
 	}