Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Release-2.2 BP FAB-2643] #3534

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

kopaygorodsky
Copy link
Contributor

@kopaygorodsky kopaygorodsky commented Jul 19, 2022

This is a backport of pull request #2643

Because we can't upgrade viper to v1.1.1 in which the bug of shadowing a key is fixed and hooks are introduced, I had to find a workaround to simplify the backport as more as possible.

Also, I added a test and a short explanation for overriding the list of system chaincodes. The default approach with env var CORE_CHAINCODE_SYSTEM_XXX: enabled won't work due to a bug in viper in earlier versions. I agree that bumping it to v1.1.1 requires too many changes and might break the LTS release.

updated a few related tests to use strings as viper config values instead of variables.

@kopaygorodsky kopaygorodsky requested a review from a team as a code owner July 19, 2022 14:03
…builders via env var. Added example on how to override the list of system chaincodes using env var due to bugs in viper.

Signed-off-by: Vladyslav Kopaihorodskyi <vlad.kopaygorodsky@gmail.com>
@yacovm
Copy link
Contributor

yacovm commented Jul 19, 2022

code looks fine, but seems like the test failed:

--- FAIL: TestPropagateEnvironment (0.00s)
    config_test.go:455: 
        	Error Trace:	config_test.go:455
        	Error:      	Not equal: 
        	            	expected: &peer.Config{LocalMSPID:"", ListenAddress:"", PeerID:"", PeerAddress:"localhost:8080", NetworkID:"", ChaincodeListenAddress:"", ChaincodeAddress:"", ValidatorPoolSize:2, DeliverClientKeepaliveOptions:comm.KeepaliveOptions{ClientInterval:60000000000, ClientTimeout:20000000000, ServerInterval:7200000000000, ServerTimeout:20000000000, ServerMinInterval:60000000000}, ProfileEnabled:false, ProfileListenAddress:"", DiscoveryEnabled:false, DiscoveryOrgMembersAllowed:false, DiscoveryAuthCacheEnabled:false, DiscoveryAuthCacheMaxSize:0, DiscoveryAuthCachePurgeRetentionRatio:0, LimitsConcurrencyEndorserService:0, LimitsConcurrencyDeliverService:0, PeerTLSEnabled:false, AuthenticationTimeWindow:900000000000, VMEndpoint:"", VMDockerTLSEnabled:false, VMDockerAttachStdout:false, VMNetworkMode:"host", ChaincodePull:false, ExternalBuilders:[]peer.ExternalBuilder{peer.ExternalBuilder{Environment:[]string{"KEY=VALUE"}, PropagateEnvironment:[]string{"KEY=VALUE"}, Name:"testName", Path:"/testPath"}, peer.ExternalBuilder{Environment:[]string(nil), PropagateEnvironment:[]string{"KEY=VALUE"}, Name:"testName", Path:"/testPath"}, peer.ExternalBuilder{Environment:[]string{"KEY=VALUE"}, PropagateEnvironment:[]string{"KEY=VALUE2"}, Name:"testName", Path:"/testPath"}}, OperationsListenAddress:"", OperationsTLSEnabled:false, OperationsTLSCertFile:"", OperationsTLSKeyFile:"", OperationsTLSClientAuthRequired:false, OperationsTLSClientRootCAs:[]string(nil), MetricsProvider:"", StatsdNetwork:"", StatsdAaddress:"", StatsdWriteInterval:0, StatsdPrefix:"", DockerCert:"", DockerKey:"", DockerCA:""}
        	            	actual  : &peer.Config{LocalMSPID:"", ListenAddress:"", PeerID:"", PeerAddress:"localhost:8080", NetworkID:"", ChaincodeListenAddress:"", ChaincodeAddress:"", ValidatorPoolSize:2, DeliverClientKeepaliveOptions:comm.KeepaliveOptions{ClientInterval:60000000000, ClientTimeout:20000000000, ServerInterval:7200000000000, ServerTimeout:20000000000, ServerMinInterval:60000000000}, ProfileEnabled:false, ProfileListenAddress:"", DiscoveryEnabled:false, DiscoveryOrgMembersAllowed:false, DiscoveryAuthCacheEnabled:false, DiscoveryAuthCacheMaxSize:0, DiscoveryAuthCachePurgeRetentionRatio:0, LimitsConcurrencyEndorserService:0, LimitsConcurrencyDeliverService:0, PeerTLSEnabled:false, AuthenticationTimeWindow:900000000000, VMEndpoint:"", VMDockerTLSEnabled:false, VMDockerAttachStdout:false, VMNetworkMode:"host", ChaincodePull:false, ExternalBuilders:[]peer.ExternalBuilder(nil), OperationsListenAddress:"", OperationsTLSEnabled:false, OperationsTLSCertFile:"", OperationsTLSKeyFile:"", OperationsTLSClientAuthRequired:false, OperationsTLSClientRootCAs:[]string(nil), MetricsProvider:"", StatsdNetwork:"", StatsdAaddress:"", StatsdWriteInterval:0, StatsdPrefix:"", DockerCert:"", DockerKey:"", DockerCA:""}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -32,32 +32,3 @@
```

…ers", VARIABLE) instead of stringified data

Signed-off-by: Vladyslav Kopaihorodskyi <vlad.kopaygorodsky@gmail.com>
@kopaygorodsky kopaygorodsky force-pushed the backport-pr-2643 branch 3 times, most recently from f715daf to b80b615 Compare July 19, 2022 23:17
@kopaygorodsky
Copy link
Contributor Author

kopaygorodsky commented Jul 19, 2022

I just realized that the master branch has also a potential issue with PropagateEnvorinment field.
A different implementation/version of the unmarshaller might set builder.PropagateEnvironment to nil or empty slice. In the last case PropagateEnvironment won't have a correct value. The fix is to use a bulletproof check builder.PropagateEnvironment == nil => len(builder.PropagateEnvironment) == 0 as I did in this PR.

if builder.Environment != nil && len(builder.PropagateEnvironment) == 0 {

… cases when value is a string(from env var) or a map(generic structure after unmarshal)

Signed-off-by: Vladyslav Kopaihorodskyi <vlad.kopaygorodsky@gmail.com>
@denyeart denyeart merged commit 5f30d58 into hyperledger:release-2.2 Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants