Review comments #1.

Author: easwars

credentials/tls/certprovider/distributor.go

@@ -31,10 +31,10 @@ import (
 //
 // Distributor implements the KeyMaterialReader interface. Provider
 // implementations may choose to do the following:
-//  - return a distributor as part of their KeyMaterialReader() method.
-//  - invoke the distributor's Set() method whenever they have new key material.
-//  - watch the channel returned by the distributor's Done() method to get
-//    notified when the distributor is closed.
+//  - return a Distributor as part of their KeyMaterialReader() method.
+//  - invoke the Distributor's Set() method whenever they have new key material.
+//  - watch the channel returned by the Distributor.Done() method to get
+//    notified when the Distributor is closed.
 type Distributor struct {
 	// mu protects the underlying key material.
 	mu   sync.Mutex
@@ -45,7 +45,7 @@ type Distributor struct {
 	// availability of key material.
 	ready *grpcsync.Event
 	// done channel to notify provider implementations and unblock any
-	// KeyMaterial() calls, once the distributor is closed.
+	// KeyMaterial() calls, once the Distributor is closed.
 	closed *grpcsync.Event
 }
 
@@ -57,15 +57,15 @@ func NewDistributor() *Distributor {
 	}
 }
 
-// Set updates the key material in the distributor with km.
+// Set updates the key material in the Distributor with km.
 //
-// Provider implementations which use the distributor must not modify the
+// Provider implementations which use the Distributor must not modify the
 // contents of the KeyMaterial struct pointed to by km.
 //
 // A non-nil err value indicates the error that the provider implementation ran
 // into when trying to fetch key material, and makes it possible to surface the
-// error to the user. A non-nil error value passed here causes distributor's
-// KeyMaterial() method to return nil key material.
+// error to the user. A non-nil error value passed here causes the
+// Distributor.KeyMaterial() method to return nil key material.
 func (d *Distributor) Set(km *KeyMaterial, err error) {
 	d.mu.Lock()
 	d.km = km
@@ -78,7 +78,7 @@ func (d *Distributor) Set(km *KeyMaterial, err error) {
 	d.mu.Unlock()
 }
 
-// KeyMaterial returns the most recent key material provided to the distributor.
+// KeyMaterial returns the most recent key material provided to the Distributor.
 // If no key material was provided at the time of this call, it will block until
 // the deadline on the context expires or fresh key material arrives.
 func (d *Distributor) KeyMaterial(ctx context.Context) (*KeyMaterial, error) {
@@ -106,13 +106,13 @@ func (d *Distributor) keyMaterial() (*KeyMaterial, error) {
 	return d.km, d.pErr
 }
 
-// Close turns down the distributor, releases allocated resources and fails any
+// Close turns down the Distributor, releases allocated resources and fails any
 // active KeyMaterial() call waiting for new key material.
 func (d *Distributor) Close() {
 	d.closed.Fire()
 }
 
-// Done returns a channel which will be closed when the distributor is closed.
+// Done returns a channel which will be closed when the Distributor is closed.
 func (d *Distributor) Done() <-chan struct{} {
 	return d.closed.Done()
 }

credentials/tls/certprovider/provider.go

@@ -90,7 +90,7 @@ type Provider interface {
 	// KeyMaterialReader returns a reader to read key material sourced by the
 	// provider. Users should call the Close() method on the returned reader
 	// when they are no longer interested in the underlying key material.
-	KeyMaterialReader(opts KeyMaterialOptions) (KeyMaterialReader, error)
+	KeyMaterialReader(KeyMaterialOptions) (KeyMaterialReader, error)
 
 	// Close cleans up resources allocated by the provider.
 	Close()
@@ -100,8 +100,7 @@ type Provider interface {
 // Implementations must be thread-safe.
 type KeyMaterialReader interface {
 	// Returns the key material when they are ready.
-	// Implementations must honor any deadline set in the context.
-	KeyMaterial(ctx context.Context) (*KeyMaterial, error)
+	KeyMaterial(context.Context) (*KeyMaterial, error)
 
 	// Close provides a way for callers to indicate that they are no longer
 	// interested in the underlying key material.

credentials/tls/certprovider/store.go

@@ -41,7 +41,7 @@ type providerKey struct {
 	config string
 }
 
-// distributorKey acts as the key to map of distributors maintained by the
+// distributorKey acts as the key to a map of distributors maintained by the
 // store. These distributor instances are returned by the provider
 // implementation's GetKeyMaterial() method.
 type distributorKey struct {
@@ -83,12 +83,26 @@ func GetKeyMaterialReader(name string, config interface{}, opts KeyMaterialOptio
 	if err != nil {
 		return nil, err
 	}
+
+	// Look for an existing distributor for the (name+config+opts) combo.
+	dk := distributorKey{
+		name:    name,
+		config:  string(stableConfig.Canonical()),
+		options: opts,
+	}
+	if wd, ok := provStore.distributors[dk]; ok {
+		wd.refCount++
+		return wd, nil
+	}
+
+	// We didn't find an existing distributor. So we need to look up the
+	// provider for the (name+config) combo, and if don't find one, we need to
+	// create a new one. Once we have the provider, we need to create a
+	// distributor on it.
 	pk := providerKey{
 		name:   name,
 		config: string(stableConfig.Canonical()),
 	}
-
-	// Create a new provider for the (name+config) combo or use an existing one.
 	var wp *wrappedProvider
 	wp, ok := provStore.providers[pk]
 	if !ok {
@@ -103,31 +117,20 @@ func GetKeyMaterialReader(name string, config interface{}, opts KeyMaterialOptio
 		}
 		provStore.providers[pk] = wp
 	}
-
-	// Create a new distributor for the (name+config+opts) combo or use an
-	// existing one. The reference count for the provider is incremented only
-	// when creating a new distributor.
-	dk := distributorKey{
-		name:    name,
-		config:  string(stableConfig.Canonical()),
-		options: opts,
-	}
-	if wd, ok := provStore.distributors[dk]; ok {
-		wd.refCount++
-		return wd, nil
-	}
-	distributor, err := wp.prov.KeyMaterialReader(opts)
+	dist, err := wp.prov.KeyMaterialReader(opts)
 	if err != nil {
 		return nil, err
 	}
 	wd := &wrappedDistributor{
-		KeyMaterialReader: distributor,
+		KeyMaterialReader: dist,
 		refCount:          1,
 		wp:                wp,
 		storeKey:          dk,
 		store:             provStore,
 	}
 	provStore.distributors[dk] = wd
+	// The reference count for the wrappedProvider is incremented only when
+	// creating a new distributor.
 	wp.refCount++
 	return wd, nil
 }