[FAB-10279] endpointConfig TLS/network refactoring

- Fixed TLSConfig.Bytes() not to read from disk everytime
- Refactored EndpointConfig.TLSClientCerts private key loading to reuse
	existing util functions
- Fixed EndpointConfig.TLSClientCerts threadsafety issues
- Removed EntityMatchers from NetworkConfig since there
	were no apparent reasons to expose them
- Removed EndpointConfig.PeerMSPID() from interface since it
	wasn't being used anywhere
- Moved EndpointConfig.MSPID() to comm.MSPID() as a static utility function



Change-Id: I7d9139ea47aedccf53084f7207494569c557c920
Signed-off-by: Sudesh Shetty <[email protected]>

pkg/common/providers/fab/network.go

@@ -23,7 +23,6 @@ type NetworkConfig struct {
 	Orderers               map[string]OrdererConfig
 	Peers                  map[string]PeerConfig
 	CertificateAuthorities map[string]msp.CAConfig
-	EntityMatchers         map[string][]MatchConfig
 }
 
 // ChannelNetworkConfig provides the definition of channels for the network

pkg/common/providers/fab/provider.go

@@ -80,8 +80,6 @@ type CommManager interface {
 //EndpointConfig contains endpoint network configurations
 type EndpointConfig interface {
 	Timeout(TimeoutType) time.Duration
-	MSPID(org string) (string, bool)
-	PeerMSPID(name string) (string, bool)
 	OrderersConfig() ([]OrdererConfig, bool)
 	OrdererConfig(nameOrURL string) (*OrdererConfig, bool)
 	PeersConfig(org string) ([]PeerConfig, bool)

pkg/core/config/endpoint/endpoint.go

@@ -77,43 +77,40 @@ type TLSConfig struct {
 	// If Path is available, then it will be used to load the cert
 	// if Pem is available, then it has the raw data of the cert it will be used as-is
 	// Certificate root certificate path
+	// If both Path and Pem are available, pem takes the precedence
 	Path string
 	// Certificate actual content
 	Pem string
+	//bytes from Pem/Path
+	bytes []byte
 }
 
-// Bytes returns the tls certificate as a byte array by loading it either from the embedded Pem or Path
-func (cfg TLSConfig) Bytes() ([]byte, error) {
-	var bytes []byte
-	var err error
+// Bytes returns the tls certificate as a byte array
+func (cfg *TLSConfig) Bytes() []byte {
+	return cfg.bytes
+}
 
+//LoadBytes preloads bytes from Pem/Path
+//Pem takes precedence over Path
+//TODO to be removed since separate TLSConfig should only be used in parsing
+func (cfg *TLSConfig) LoadBytes() error {
+	var err error
 	if cfg.Pem != "" {
-		bytes = []byte(cfg.Pem)
+		cfg.bytes = []byte(cfg.Pem)
 	} else if cfg.Path != "" {
-		bytes, err = ioutil.ReadFile(cfg.Path)
-
+		cfg.bytes, err = ioutil.ReadFile(cfg.Path)
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to load pem bytes from path %s", cfg.Path)
+			return errors.Wrapf(err, "failed to load pem bytes from path %s", cfg.Path)
 		}
 	}
-
-	return bytes, nil
+	return nil
 }
 
 // TLSCert returns the tls certificate as a *x509.Certificate by loading it either from the embedded Pem or Path
-func (cfg TLSConfig) TLSCert() (*x509.Certificate, error) {
-	bytes, err := cfg.Bytes()
-
-	if err != nil {
-		return nil, err
-	}
-
-	return loadCert(bytes)
-}
+//TODO to be removed since separate TLSConfig should only be used in parsing
+func (cfg *TLSConfig) TLSCert() (*x509.Certificate, error) {
 
-// loadCAKey
-func loadCert(rawData []byte) (*x509.Certificate, error) {
-	block, _ := pem.Decode(rawData)
+	block, _ := pem.Decode(cfg.bytes)
 
 	if block != nil {
 		pub, err := x509.ParseCertificate(block.Bytes)

pkg/core/config/endpoint/endpoint_test.go

@@ -9,6 +9,8 @@ package endpoint
 import (
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestIsTLSEnabled(t *testing.T) {
@@ -105,10 +107,11 @@ O94CDp7l2k7hMQI0zQ==
 		Pem:  pPem,
 	}
 
-	b, e := tlsConfig.Bytes()
+	e := tlsConfig.LoadBytes()
 	if e != nil {
 		t.Fatalf("error loading bytes for sample cert %s", e)
 	}
+	b := tlsConfig.Bytes()
 	if len(b) == 0 {
 		t.Fatalf("cert's Bytes() call returned empty byte array")
 	}
@@ -118,20 +121,23 @@ O94CDp7l2k7hMQI0zQ==
 
 	// test with empty pem
 	tlsConfig.Pem = ""
-	b, e = tlsConfig.Bytes()
+	tlsConfig.Path = "../testdata/config_test.yaml"
+	e = tlsConfig.LoadBytes()
 	if e != nil {
 		t.Fatalf("error loading bytes for empty pem cert %s", e)
 	}
-	if len(b) > 0 {
-		t.Fatalf("cert's Bytes() call returned non empty byte array for empty pem")
+	b = tlsConfig.Bytes()
+	if len(b) == 0 {
+		t.Fatalf("cert's Bytes() call returned empty byte array")
 	}
 
 	// test with wrong pem
 	tlsConfig.Pem = "wrongpemvalue"
-	b, e = tlsConfig.Bytes()
+	e = tlsConfig.LoadBytes()
 	if e != nil {
 		t.Fatalf("error loading bytes for wrong pem cert %s", e)
 	}
+	b = tlsConfig.Bytes()
 	if len(b) != len([]byte("wrongpemvalue")) {
 		t.Fatalf("cert's Bytes() call returned different byte array for wrong pem")
 	}
@@ -143,6 +149,11 @@ func TestTLSConfig_TLSCertPostive(t *testing.T) {
 		Pem:  "",
 	}
 
+	e := tlsConfig.LoadBytes()
+	if e != nil {
+		t.Fatalf("error loading certificate for sample cert path %s", e)
+	}
+
 	c, e := tlsConfig.TLSCert()
 	if e != nil {
 		t.Fatalf("error loading certificate for sample cert path %s", e)
@@ -215,3 +226,35 @@ func TestTLSConfig_TLSCertNegative(t *testing.T) {
 	}
 
 }
+
+func TestTLSConfigBytes(t *testing.T) {
+
+	// test with wrong path
+	tlsConfig := &TLSConfig{
+		Path: "../testdata/config_test.yaml",
+		Pem:  "",
+	}
+
+	err := tlsConfig.LoadBytes()
+	bytes1 := tlsConfig.Bytes()
+	assert.Nil(t, err, "tlsConfig.Bytes supposed to succeed")
+	assert.NotEmpty(t, bytes1, "supposed to get valid bytes")
+
+	tlsConfig.Path = "../testdata/config_test_pem.yaml"
+	bytes2 := tlsConfig.Bytes()
+	assert.Nil(t, err, "tlsConfig.Bytes supposed to succeed")
+	assert.NotEmpty(t, bytes2, "supposed to get valid bytes")
+
+	//even after changing path, it should return previous bytes
+	assert.Equal(t, bytes1, bytes2, "any update to tlsconfig path after load bytes call should not take effect")
+
+	//call preload now
+	err = tlsConfig.LoadBytes()
+	bytes2 = tlsConfig.Bytes()
+	assert.Nil(t, err, "tlsConfig.Bytes supposed to succeed")
+	assert.NotEmpty(t, bytes2, "supposed to get valid bytes")
+
+	//even after changing path, it should return previous bytes
+	assert.NotEqual(t, bytes1, bytes2, "tlsConfig.LoadBytes() should refresh bytes")
+
+}

pkg/core/config/lookup/lookup_test.go

@@ -31,6 +31,10 @@ const orgChannelID = "orgchannel"
 
 var backend *mocks.MockConfigBackend
 
+type testEntityMatchers struct {
+	matchers map[string][]fab.MatchConfig
+}
+
 func TestMain(m *testing.M) {
 	backend = setupCustomBackend("key")
 	r := m.Run()
@@ -227,10 +231,12 @@ func TestUnmarshalWithMultipleBackend(t *testing.T) {
 
 	//output struct
 	networkConfig := fab.NetworkConfig{}
+	entityMatchers := testEntityMatchers{}
+
 	assert.Nil(t, testLookup.UnmarshalKey("client", &networkConfig.Client), "unmarshalKey supposed to succeed")
 	assert.Nil(t, testLookup.UnmarshalKey("channels", &networkConfig.Channels), "unmarshalKey supposed to succeed")
 	assert.Nil(t, testLookup.UnmarshalKey("certificateAuthorities", &networkConfig.CertificateAuthorities), "unmarshalKey supposed to succeed")
-	assert.Nil(t, testLookup.UnmarshalKey("entityMatchers", &networkConfig.EntityMatchers), "unmarshalKey supposed to succeed")
+	assert.Nil(t, testLookup.UnmarshalKey("entityMatchers", &entityMatchers.matchers), "unmarshalKey supposed to succeed")
 	assert.Nil(t, testLookup.UnmarshalKey("organizations", &networkConfig.Organizations), "unmarshalKey supposed to succeed")
 	assert.Nil(t, testLookup.UnmarshalKey("orderers", &networkConfig.Orderers), "unmarshalKey supposed to succeed")
 	assert.Nil(t, testLookup.UnmarshalKey("peers", &networkConfig.Peers), "unmarshalKey supposed to succeed")
@@ -253,15 +259,15 @@ func TestUnmarshalWithMultipleBackend(t *testing.T) {
 	assert.Equal(t, networkConfig.CertificateAuthorities["local.ca.org2.example.com"].URL, "https://ca.org2.example.com:8054")
 
 	//EntityMatchers
-	assert.Equal(t, len(networkConfig.EntityMatchers), 4)
-	assert.Equal(t, len(networkConfig.EntityMatchers["peer"]), 8)
-	assert.Equal(t, networkConfig.EntityMatchers["peer"][0].MappedHost, "local.peer0.org1.example.com")
-	assert.Equal(t, len(networkConfig.EntityMatchers["orderer"]), 4)
-	assert.Equal(t, networkConfig.EntityMatchers["orderer"][0].MappedHost, "local.orderer.example.com")
-	assert.Equal(t, len(networkConfig.EntityMatchers["certificateauthority"]), 2)
-	assert.Equal(t, networkConfig.EntityMatchers["certificateauthority"][0].MappedHost, "local.ca.org1.example.com")
-	assert.Equal(t, len(networkConfig.EntityMatchers["channel"]), 1)
-	assert.Equal(t, networkConfig.EntityMatchers["channel"][0].MappedName, "ch1")
+	assert.Equal(t, len(entityMatchers.matchers), 4)
+	assert.Equal(t, len(entityMatchers.matchers["peer"]), 8)
+	assert.Equal(t, entityMatchers.matchers["peer"][0].MappedHost, "local.peer0.org1.example.com")
+	assert.Equal(t, len(entityMatchers.matchers["orderer"]), 4)
+	assert.Equal(t, entityMatchers.matchers["orderer"][0].MappedHost, "local.orderer.example.com")
+	assert.Equal(t, len(entityMatchers.matchers["certificateauthority"]), 2)
+	assert.Equal(t, entityMatchers.matchers["certificateauthority"][0].MappedHost, "local.ca.org1.example.com")
+	assert.Equal(t, len(entityMatchers.matchers["channel"]), 1)
+	assert.Equal(t, entityMatchers.matchers["channel"][0].MappedName, "ch1")
 
 	//Organizations
 	assert.Equal(t, len(networkConfig.Organizations), 3)
@@ -331,15 +337,17 @@ func TestLookupUnmarshalAgainstViperUnmarshal(t *testing.T) {
 	/*
 		TEST NETWORK CONFIG ENTITY MATCHERS
 	*/
+	entityMatchers := testEntityMatchers{}
 	//get entityMatchers backend lookup
-	err = testLookup.UnmarshalKey("entityMatchers", &networkConfig.EntityMatchers)
+	err = testLookup.UnmarshalKey("entityMatchers", &entityMatchers.matchers)
 	if err != nil {
 		t.Fatal(err)
 	}
 	//get entityMatchers from viper
-	sampleViper.UnmarshalKey("entityMatchers", &networkConfigViper.EntityMatchers)
+	viperEntityMatchers := testEntityMatchers{}
+	sampleViper.UnmarshalKey("entityMatchers", &viperEntityMatchers.matchers)
 	//now compare
-	assert.True(t, reflect.DeepEqual(&networkConfig.EntityMatchers, &networkConfigViper.EntityMatchers), "unmarshalled value from config lookup supposed to match unmarshalled value from viper")
+	assert.True(t, reflect.DeepEqual(&entityMatchers, &viperEntityMatchers), "unmarshalled value from config lookup supposed to match unmarshalled value from viper")
 
 	/*
 		TEST NETWORK CONFIG ORGANIZATIONS
@@ -382,10 +390,10 @@ func TestLookupUnmarshalAgainstViperUnmarshal(t *testing.T) {
 
 	//Just to make sure that empty values are not being compared
 	assert.True(t, len(networkConfigViper.Channels) > 0, "expected to get valid unmarshalled value")
-	assert.True(t, len(networkConfigViper.Organizations) > 0, "expected to get valid unmarshalled value")
+	assert.True(t, len(viperEntityMatchers.matchers) > 0, "expected to get valid unmarshalled value")
 	assert.True(t, len(networkConfigViper.Orderers) > 0, "expected to get valid unmarshalled value")
 	assert.True(t, len(networkConfigViper.Peers) > 0, "expected to get valid unmarshalled value")
-	assert.True(t, len(networkConfigViper.EntityMatchers) > 0, "expected to get valid unmarshalled value")
+	assert.True(t, len(entityMatchers.matchers) > 0, "expected to get valid unmarshalled value")
 	assert.True(t, networkConfigViper.Client.Organization != "", "expected to get valid unmarshalled value")
 
 }

pkg/core/config/testdata/config_test.yaml

@@ -330,4 +330,5 @@ certificateAuthorities:
       enrollId: admin
       enrollSecret: adminpw
     # [Optional] The optional name of the CA.
-    caName: ca.org2.example.com
+    caName: ca.org2.example.com
+    #first one

pkg/core/config/testdata/config_test_pem.yaml

@@ -382,3 +382,4 @@ certificateAuthorities:
       enrollSecret: adminpw
     # [Optional] The optional name of the CA.
     caName: ca.org2.example.com
+#second one

pkg/fab/comm/network.go

@@ -7,6 +7,8 @@ SPDX-License-Identifier: Apache-2.0
 package comm
 
 import (
+	"strings"
+
 	"github.com/hyperledger/fabric-sdk-go/pkg/common/providers/fab"
 	"github.com/pkg/errors"
 )
@@ -63,3 +65,18 @@ func SearchPeerConfigFromURL(cfg fab.EndpointConfig, url string) (*fab.PeerConfi
 
 	return nil, errors.Errorf("unable to get peerconfig for given url : %s", url)
 }
+
+// MSPID returns the MSP ID for the requested organization
+func MSPID(cfg fab.EndpointConfig, org string) (string, bool) {
+	networkConfig, ok := cfg.NetworkConfig()
+	if !ok {
+		return "", false
+	}
+	// viper lowercases all key maps, org is lower case
+	mspID := networkConfig.Organizations[strings.ToLower(org)].MSPID
+	if mspID == "" {
+		return "", false
+	}
+
+	return mspID, true
+}

pkg/fab/comm/network_test.go

@@ -88,3 +88,24 @@ func TestSearchPeerConfigFromURL(t *testing.T) {
 	assert.Equal(t, peer0Org1.EventURL, peerConfig.EventURL)
 
 }
+
+func TestMSPID(t *testing.T) {
+	configBackend, err := config.FromFile(configTestFilePath)()
+	if err != nil {
+		t.Fatalf("Unexpected error reading config backend: %v", err)
+	}
+
+	sampleConfig, err := fabImpl.ConfigFromBackend(configBackend...)
+	if err != nil {
+		t.Fatalf("Unexpected error reading config: %v", err)
+	}
+
+	mspID, ok := MSPID(sampleConfig, "invalid")
+	assert.False(t, ok, "supposed to fail for invalid org name")
+	assert.Empty(t, mspID, "supposed to get valid MSP ID")
+
+	mspID, ok = MSPID(sampleConfig, "org1")
+	assert.True(t, ok, "supposed to pass with valid org name")
+	assert.NotEmpty(t, mspID, "supposed to get valid MSP ID")
+	assert.Equal(t, "Org1MSP", mspID, "supposed to get valid MSP ID")
+}