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.

Sign up for GitHub

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

Jump to bottom

fabric-sdk: do not add resolvers for Fabric peers #375

Merged
merged 2 commits into from
Aug 31, 2022
Merged

fabric-sdk: do not add resolvers for Fabric peers #375

merged 2 commits into from
Aug 31, 2022

Conversation

adecaro
Copy link
Contributor

@adecaro adecaro commented Aug 25, 2022 •
edited
Loading

This PR updates NWO to remove from the resolvers references to Fabric peers.
The FPC integration was relaying in these resolvers. To fix this, the PR updates the FPC integration to leverage Fabric Discovery.

In addition, a flaky unit-test has been disabled.

Signed-off-by: Angelo De Caro adc@zurich.ibm.com

@adecaro adecaro self-assigned this Aug 25, 2022
@adecaro adecaro added enhancement New feature or request cleanup Something needs some cleanup labels Aug 25, 2022
mbrandenburger
mbrandenburger reviewed Aug 26, 2022
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

DO we need another cleanup in the fabric config?

A quick search for resolvers show a few places. Here a few pointers I question if we still need them.

integration/nwo/fsc/tracing/aggregator_test.go
@@ -85,6 +85,7 @@ func measure() {
}

func TestNewAggregatorFakeInput(t *testing.T) {
t.Skip()
Copy link
Member

Choose a reason for hiding this comment

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

🙅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but it was failing continuously for no reason 😢

Copy link
Member

Choose a reason for hiding this comment

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

Captured this temporary disablement in #359

@adecaro
Copy link
Contributor Author

adecaro commented Aug 26, 2022

I didn't remove all resolvers. They are still needed to have some of the integration tests working.

Let's take the IOU test. Alice asks the Approver to approve. The Approver signs using a Fabric identity. Alice must check that this signature is valid, therefore Alice needs to know which key the approver will use. This key must be in Alice's resolvers.

@mbrandenburger
Copy link
Member

Let's take the IOU test. Alice asks the Approver to approve. The Approver signs using a Fabric identity. Alice must check that this signature is valid, therefore Alice needs to know which key the approver will use. This key must be in Alice's resolvers.

Ok - makes sense to me.
However, in that case it's wired to look for these identities in the fabric.endpoint.resolvers section. I would expect to have this in a fabric.identity.resolvers section.

@adecaro
Copy link
Contributor Author

adecaro commented Aug 26, 2022

I understand your point but it is out of scope for this PR to change that configuration path.
I would prefer to dedicate a PR to the configuration restructuring accompanied by proper documentation.
Wdyt?

@mbrandenburger
Copy link
Member

BTW Can't we use the org ca cert to check these identities?

@adecaro
Copy link
Contributor Author

adecaro commented Aug 26, 2022

what do you mean?

@mbrandenburger
Copy link
Member

what do you mean?

If Alice forms a transaction and requires Bob signature, in particular, any signature from Bob's org, for the endorsement. So instead of looking up the identity of Bob, could Alice simply check the Bob's cert (attached with the endorsement) belongs to Bob org? Anyway, this seems orthogonal to this PR.

@adecaro
Copy link
Contributor Author

adecaro commented Aug 26, 2022

do you need any additional change then? :)

@adecaro
Copy link
Contributor Author

adecaro commented Aug 27, 2022

@alexandrosfilios , please, have a look ASAP. Thanks :)

@adecaro adecaro assigned mbrandenburger and unassigned adecaro Aug 29, 2022
@mbrandenburger mbrandenburger self-requested a review August 29, 2022 09:49
mbrandenburger
mbrandenburger approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

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

LGTM! Please angelo, rebase and squash the fixups. We would like to keep the commit for disabling the flaky test separately. :)

@adecaro @mbrandenburger
fabric-sdk: do not add resolvers for Fabric peers
3c7d121 
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@adecaro @mbrandenburger
skip flaky test nwo/fsc/tracing
7eec89e 
Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
@mbrandenburger mbrandenburger merged commit 3abb52f into main Aug 31, 2022
@mbrandenburger mbrandenburger deleted the f-fabric-no-resolvers branch August 31, 2022 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrandenburger mbrandenburger mbrandenburger approved these changes

@alexandrosfilios alexandrosfilios Awaiting requested review from alexandrosfilios

Assignees

@mbrandenburger mbrandenburger

Labels
cleanup Something needs some cleanup enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adecaro @mbrandenburger