better API and logging for identity registry (istio#2145)

* better API and logging for identity registry

* fix linter error

* use pkg/log instead of glog

security/pkg/registry/kube/BUILD

@@ -11,6 +11,7 @@ go_library(
         "//pilot/platform/kube:go_default_library",
         "//security/pkg/pki/ca:go_default_library",
         "//security/pkg/registry:go_default_library",
+        "@com_github_golang_glog//:go_default_library",
         "@io_k8s_api//core/v1:go_default_library",
         "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library",
         "@io_k8s_apimachinery//pkg/runtime:go_default_library",

security/pkg/registry/kube/service.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	"istio.io/istio/pilot/platform/kube"
+	"istio.io/istio/pkg/log"
 	"istio.io/istio/security/pkg/registry"
 )
 
@@ -79,15 +80,21 @@ func (c *ServiceController) serviceAdded(obj interface{}) {
 	svc := obj.(*v1.Service)
 	svcAcct, ok := svc.ObjectMeta.Annotations[kube.KubeServiceAccountsOnVMAnnotation]
 	if ok {
-		c.reg.AddMapping(svcAcct, svcAcct)
+		err := c.reg.AddMapping(svcAcct, svcAcct)
+		if err != nil {
+			log.Errorf("cannot add mapping %q -> %q to registry: %s", svcAcct, svcAcct, err.Error())
+		}
 	}
 }
 
 func (c *ServiceController) serviceDeleted(obj interface{}) {
 	svc := obj.(*v1.Service)
 	svcAcct, ok := svc.ObjectMeta.Annotations[kube.KubeServiceAccountsOnVMAnnotation]
 	if ok {
-		c.reg.DeleteMapping(svcAcct, svcAcct)
+		err := c.reg.DeleteMapping(svcAcct, svcAcct)
+		if err != nil {
+			log.Errorf("cannot delete mapping %q to %q from registry: %s", svcAcct, svcAcct, err.Error())
+		}
 	}
 }
 

security/pkg/registry/kube/serviceaccount.go

@@ -26,6 +26,7 @@ import (
 	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/cache"
 
+	"istio.io/istio/pkg/log"
 	"istio.io/istio/security/pkg/pki/ca"
 	"istio.io/istio/security/pkg/registry"
 )
@@ -81,13 +82,20 @@ func getSpiffeID(sa *v1.ServiceAccount) string {
 func (c *ServiceAccountController) serviceAccountAdded(obj interface{}) {
 	sa := obj.(*v1.ServiceAccount)
 	id := getSpiffeID(sa)
-	c.reg.AddMapping(id, id)
+	err := c.reg.AddMapping(id, id)
+	if err != nil {
+		log.Errorf("cannot add mapping %q -> %q to registry: %s", id, id, err.Error())
+	}
 }
 
 func (c *ServiceAccountController) serviceAccountDeleted(obj interface{}) {
 	sa := obj.(*v1.ServiceAccount)
 	id := getSpiffeID(sa)
-	c.reg.DeleteMapping(id, id)
+	err := c.reg.DeleteMapping(id, id)
+	if err != nil {
+		log.Errorf("cannot delete mapping %q to %q from registry: %s", id, id, err.Error())
+	}
+
 }
 
 func (c *ServiceAccountController) serviceAccountUpdated(oldObj, newObj interface{}) {

security/pkg/registry/registry.go

@@ -15,6 +15,7 @@
 package registry
 
 import (
+	"fmt"
 	"sync"
 
 	// TODO(nmittler): Remove this
@@ -26,8 +27,8 @@ import (
 // Registry is the standard interface for identity registry implementation
 type Registry interface {
 	Check(string, string) bool
-	AddMapping(string, string)
-	DeleteMapping(string, string)
+	AddMapping(string, string) error
+	DeleteMapping(string, string) error
 }
 
 // IdentityRegistry is a naive registry that maintains a mapping between
@@ -55,32 +56,34 @@ func (reg *IdentityRegistry) Check(id1, id2 string) bool {
 	return true
 }
 
-// AddMapping adds a mapping id1 -> id2
-func (reg *IdentityRegistry) AddMapping(id1, id2 string) {
-	reg.RLock()
+// AddMapping adds a mapping id1 -> id2. If id1 is already mapped to
+// something else, add fails.
+func (reg *IdentityRegistry) AddMapping(id1, id2 string) error {
+	reg.Lock()
+	defer reg.Unlock()
 	oldID, ok := reg.Map[id1]
-	reg.RUnlock()
-	if ok {
-		log.Warnf("Overwriting existing mapping: %q -> %q", id1, oldID)
+	if ok && oldID != id2 {
+		return fmt.Errorf("identity %q is already mapped to %q", id1, oldID)
 	}
-	reg.Lock()
+
+	log.Infof("adding registry entry %q -> %q", id1, id2)
 	reg.Map[id1] = id2
-	reg.Unlock()
+	return nil
 }
 
 // DeleteMapping attempts to delete mapping id1 -> id2. If id1 is already
 // mapped to a different identity, deletion fails
-func (reg *IdentityRegistry) DeleteMapping(id1, id2 string) {
-	reg.RLock()
+func (reg *IdentityRegistry) DeleteMapping(id1, id2 string) error {
+	reg.Lock()
+	defer reg.Unlock()
 	oldID, ok := reg.Map[id1]
-	reg.RUnlock()
 	if !ok || oldID != id2 {
-		log.Warnf("Could not delete nonexistent mapping: %q -> %q", id1, id2)
-		return
+		return fmt.Errorf("could not delete nonexistent mapping: %q -> %q", id1, id2)
 	}
-	reg.Lock()
+
+	log.Infof("deleting registry entry %q -> %q", id1, id2)
 	delete(reg.Map, id1)
-	reg.Unlock()
+	return nil
 }
 
 var (

security/pkg/registry/registry_test.go

@@ -23,12 +23,12 @@ func TestIdentityRegistry(t *testing.T) {
 		Map: make(map[string]string),
 	}
 
-	reg.AddMapping("id1", "id2")
+	_ = reg.AddMapping("id1", "id2")
 	if !reg.Check("id1", "id2") {
 		t.Errorf("add mapping: id1 -> id2 should be in registry")
 	}
 
-	reg.DeleteMapping("id1", "id2")
+	_ = reg.DeleteMapping("id1", "id2")
 	if reg.Check("id1", "id2") {
 		t.Errorf("delete mapping: id1 -> id2 should not be in registry")
 	}

security/pkg/server/grpc/authorizer.go

@@ -17,6 +17,7 @@ package grpc
 import (
 	"fmt"
 
+	"istio.io/istio/pkg/log"
 	"istio.io/istio/security/pkg/registry"
 )
 
@@ -70,9 +71,13 @@ func (authZ *registryAuthorizor) authorize(requestor *caller, requestedIDs []str
 		if !valid {
 			return fmt.Errorf("the requestor (%v) is not registered", requestor)
 		}
+
 		// add the requestedIDs to the registry
 		for _, requestedID := range requestedIDs {
-			authZ.reg.AddMapping(requestedID, requestedID)
+			err := authZ.reg.AddMapping(requestedID, requestedID)
+			if err != nil {
+				log.Warnf("cannot add mapping %q -> %q to registry: %s", requestedID, requestedID, err.Error())
+			}
 		}
 		return nil
 	}