Skip to content

Allow BCCSP config to be set using environment variables - #1900

Merged
jyellick merged 1 commit into
hyperledger:masterfrom
wlahti:fab-17969
Oct 2, 2020
Merged

Allow BCCSP config to be set using environment variables#1900
jyellick merged 1 commit into
hyperledger:masterfrom
wlahti:fab-17969

Conversation

@wlahti

@wlahti wlahti commented Sep 18, 2020

Copy link
Copy Markdown
Contributor

Type of change

  • Bug fix

Related issues

FAB-17969

@wlahti
wlahti requested a review from a team as a code owner September 18, 2020 00:26
@wlahti wlahti changed the title Allow peer BCCSP config to be set using environment variables Allow BCCSP config to be set using environment variables Sep 22, 2020
@wlahti
wlahti force-pushed the fab-17969 branch 6 times, most recently from 8b80483 to 37d0330 Compare September 23, 2020 16:37
jyellick
jyellick reviewed Sep 29, 2020
View reviewed changes

@jyellick jyellick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks pretty good, not a lot of actual code changes and mostly test which is of course great. Still, a few questions.

Comment thread bccsp/factory/opts.go Outdated

@jyellick jyellick Sep 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you're just moving this code about, but it's really bothersome that the yaml and json keys are differently cased. I'm not sure who would be depending on this behavior, so perhaps it's safest not to change, but just have to point it out.

Comment thread bccsp/factory/opts.go Outdated

@jyellick jyellick Sep 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's fine to consolidate these into a single struct regardless of build tag because we never try to interact with the pkcs11 lib unless we follow the path set out in the pkcs11.go file?

@wlahti wlahti Sep 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was the thinking here.

Comment thread common/viperutil/config_util.go Outdated
@wlahti
wlahti force-pushed the fab-17969 branch 2 times, most recently from 48786aa to bfb217b Compare September 29, 2020 20:52
@wlahti
wlahti marked this pull request as draft September 29, 2020 21:57
@wlahti
wlahti force-pushed the fab-17969 branch 3 times, most recently from 574d6aa to 0a4546c Compare September 30, 2020 15:39
wlahti
wlahti commented Sep 30, 2020
View reviewed changes
Comment thread internal/peer/common/common.go Outdated
Comment on lines 160 to 171

@wlahti wlahti Sep 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove these SW override lookups since BCCSP hardcodes these anyways.

@stephyee stephyee Sep 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@stephyee stephyee Oct 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹

@wlahti
wlahti marked this pull request as ready for review September 30, 2020 16:07
jyellick
jyellick previously approved these changes Oct 2, 2020
View reviewed changes

@jyellick jyellick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love implementing this environment parsing manually, but given the downsides of the implementation through viperutil, it seems like the lesser of the two evils. Did you want to address Will's comment now, or should we merge as is?

@wlahti @stephyee
Allow BCCSP config to be set using env var
bb7839e 
FAB-17969

Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>
Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
@stephyee

stephyee commented Oct 2, 2020

Copy link
Copy Markdown
Contributor

@jyellick removed the overrides for hardcoded SW values

@jyellick
jyellick merged commit 0a9f766 into hyperledger:master Oct 2, 2020
wlahti added a commit to wlahti/fabric that referenced this pull request Oct 2, 2020
@wlahti
Allow BCCSP config to be set using env var (hyperledger#1900)
da6a5c1 
FAB-17969

Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>
Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
ale-linux pushed a commit that referenced this pull request Oct 5, 2020
@wlahti @ale-linux
Allow BCCSP config to be set using env var (#1900)
f2e9e6e 
FAB-17969

Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>
Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
wlahti added a commit to wlahti/fabric that referenced this pull request Oct 13, 2020
@wlahti
Allow BCCSP config to be set using env var (hyperledger#1900)
eceaf93 
FAB-17969

Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>
Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
@lindluni

lindluni commented Oct 20, 2020

Copy link
Copy Markdown
Contributor

This change had some a pretty well hidden side affect. It pushed PKCS11 code outside of the pkcs11 build tag gating, which now pulls the pkcs11 library in, even when you aren't using pkcs11 factories. This prevents cross-compilation of fabric from one platform to another now unless you have a C cross-platform compiler installed and pass the proper CGO options when building since the miekg package uses CGO

@stephyee

stephyee commented Oct 21, 2020

Copy link
Copy Markdown
Contributor

@btl5037 what do you suggest? reverting?

caod123 pushed a commit that referenced this pull request Nov 10, 2020
Brett Logan
Revert "Allow BCCSP config to be set using env var (#1900)" (#2093)
4e7201b 
This reverts commit 0a9f766.
which pushed the pkcs11 code outside of the pkcs11 build tag.
This prevents cross-compilation of Fabric due to the fact the
miekg/pkcs11 package contains CGO which requires a cross-compiler
for each platform you are cross compiling for.

We need to find a better approach to solving this problem
that doesn't pull the code out of the build tag.

Signed-off-by: Brett Logan <brett.t.logan@ibm.com>
caod123 added a commit to caod123/fabric that referenced this pull request Nov 10, 2020
@caod123
Revert "Allow BCCSP config to be set using env var (hyperledger#1900)"
ef10053 
This reverts commit f2e9e6e.
which pushed the pkcs11 code outside of the pkcs11 build tag.
This prevents cross-compilation of Fabric due to the fact the
miekg/pkcs11 package contains CGO which requires a cross-compiler
for each platform you are cross compiling for.

We need to find a better approach to solving this problem
that doesn't pull the code out of the build tag.

Signed-off-by: Danny Cao <dcao@us.ibm.com>
lindluni pushed a commit that referenced this pull request Nov 10, 2020
@caod123 Brett Logan
Revert "Allow BCCSP config to be set using env var (#1900)"
b247ed4 
This reverts commit f2e9e6e.
which pushed the pkcs11 code outside of the pkcs11 build tag.
This prevents cross-compilation of Fabric due to the fact the
miekg/pkcs11 package contains CGO which requires a cross-compiler
for each platform you are cross compiling for.

We need to find a better approach to solving this problem
that doesn't pull the code out of the build tag.

Signed-off-by: Danny Cao <dcao@us.ibm.com>
@jpatchell jpatchell mentioned this pull request Oct 11, 2022
Closed
@denyeart denyeart mentioned this pull request Oct 20, 2022
Merged
@denyeart

denyeart commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

Resolved in #3697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@stephyee stephyee stephyee left review comments
@jyellick jyellick jyellick approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wlahti @stephyee @lindluni @denyeart @jyellick