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

Integration Test #79

Merged
merged 3 commits into from
Jun 17, 2019

Conversation

g2flyer
Copy link
Contributor

@g2flyer g2flyer commented Jun 1, 2019

subject says it all (or mostly: besides address issue #67, i also did some related cleanup / re-org of fabric/sgxconfig/* and addressed issue #53 )

PS: eventually, some of the .sh libraries could also be put into common but for now i left it self-contained ./integration ...

Copy link
Contributor

@bvavala bvavala left a comment

Choose a reason for hiding this comment

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

the integration test is nice, it simplifies our life a lot!
I am not sure about the refactoring (sybmolic links) and the keys, but I am fine creating issues and take care of these later.

.gitignore Show resolved Hide resolved
README.md Show resolved Hide resolved
fabric/sgxconfig/ias Show resolved Hide resolved
integration/common_ledger.sh Show resolved Hide resolved
integration/common_ledger.sh Show resolved Hide resolved
integration/config/ias Show resolved Hide resolved
Copy link
Contributor

@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.

I get an error. see here

auction_test.sh: FIRST RUN: this should fail due to docker-switcheroo ...
auction_test.sh: - setup ledger
/home/bur/fabric-secure-chaincode/integration/common_ledger.sh: line 56: kill: (4565) - No such process
 auction_test.sh: Orderer quit too quickly: (for log see /tmp/hyperledger/test//orderer.out & /tmp/hyperledger/test//orderer.err)
/home/bur/fabric-secure-chaincode/integration/common_ledger.sh: line 96: kill: (4565) - No such process
Makefile:14: recipe for target 'auction_test' failed

Makefile Show resolved Hide resolved
fabric/sgxconfig/demo/common.sh Show resolved Hide resolved
fabric/sgxconfig/demo/create_channel.sh Show resolved Hide resolved
fabric/sgxconfig/demo/start_orderer.sh Show resolved Hide resolved
fabric/sgxconfig/demo/start_peer.sh Show resolved Hide resolved
integration/auction_test.sh Show resolved Hide resolved
integration/common_ledger.sh Show resolved Hide resolved
README.md Show resolved Hide resolved
@g2flyer g2flyer force-pushed the msteiner.integration-test branch from de1685d to b439b41 Compare June 7, 2019 17:27
@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 7, 2019

I get an error. see here

auction_test.sh: FIRST RUN: this should fail due to docker-switcheroo ...
auction_test.sh: - setup ledger
/home/bur/fabric-secure-chaincode/integration/common_ledger.sh: line 56: kill: (4565) - No such process
 auction_test.sh: Orderer quit too quickly: (for log see /tmp/hyperledger/test//orderer.out & /tmp/hyperledger/test//orderer.err)
/home/bur/fabric-secure-chaincode/integration/common_ledger.sh: line 96: kill: (4565) - No such process
Makefile:14: recipe for target 'auction_test' failed

I saw the same problem yesterday also. I'm not sure what exactly made it fail but making fabric config and cc-name etc test-specific seemed to remove the problem. Test again with the new version.

BTW: this should hopefully be obsolete anyway soon: i'm mid-way through a patch which removes the switcheroo (or more proactively, creates the correct container before the peer generates the wrong one ...)

@bvavala
Copy link
Contributor

bvavala commented Jun 13, 2019

I am also running into that problem. Any updates?

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

I am also running into that problem. Any updates?

You were running with the latest version? I couldn't reproduce the issue with that version. Actually, thinking of it i'm actually not sure i ever saw exactly that problem. What do the log-files show (see the error message for location of log-files)?

@bvavala
Copy link
Contributor

bvavala commented Jun 13, 2019

alright, I was missing credentials in integration first, the log helped
then I missed the credentials in usual place... I don't think I have found anything hinting at that

filed an issue #82 for this: if you want to address it now fine, if you want to address it later good!

@bvavala bvavala self-requested a review June 13, 2019 17:59
@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

alright, I was missing credentials in integration first, the log helped
then I missed the credentials in usual place... I don't think I have found anything hinting at that

filed an issue #82 for this: if you want to address it now fine, if you want to address it later good!

Updated the PR with corresponding tests and a bit of generalizations (see #82 for comments on how the common_* scripts can (and should) be moved to a global scope as part of the SDK refactoring ..)

@g2flyer g2flyer requested a review from bvavala June 13, 2019 20:10
Copy link
Contributor

@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.

I get few errors when I run make.

First - linter complains.

 fabric-private-chaincode git:(g2flyer-msteiner.integration-test) ✗ pwd
/home/bur/go/src/github.com/hyperledger-labs/fabric-private-chaincode
➜  fabric-private-chaincode git:(g2flyer-msteiner.integration-test) ✗ make checks
License: Running licence checks..
head: error reading 'fabric/sgxconfig/ias': Is a directory
head: error reading 'integration/config/ias': Is a directory
grep: fabric/sgxconfig/ias: Is a directory
grep: integration/config/ias: Is a directory
The following files are missing SPDX-License-Identifier headers:
config/ias/.gitignore
fabric/sgxconfig/ias
.gitignore
integration/config/ias

Please replace the Apache license header comment text with:
SPDX-License-Identifier: Apache-2.0

Checking committed files for traditional Apache License headers ...
grep: fabric/sgxconfig/ias: Is a directory
grep: integration/config/ias: Is a directory
The following files are missing traditional Apache 2.0 headers:
config/ias/.gitignore
fabric/sgxconfig/ias
.gitignore
integration/config/ias
Fatal Error - All files must have a license header
Makefile:16: recipe for target 'license' failed
make: *** [license] Error 1

And second, somehow the auction script does not run with my setup. Note that in my $GOPATH/src/github.com/hyperledger-labs/ I have a symlink to /home/bur/fabric-private-chaincode, seems this causes the problem?!?!

➜  integration git:(g2flyer-msteiner.integration-test) ✗ pwd
/home/bur/go/src/github.com/hyperledger-labs/fabric-private-chaincode/integration
➜  integration git:(g2flyer-msteiner.integration-test) ✗ make
./auction_test.sh
 auction_test.sh: FABRIC_BIN_DIR not properly defined as '/home/bur/fabric-secure-chaincode/integration/../../../hyperledger/fabric/.build/bin'
Makefile:14: recipe for target 'auction_test' failed
make: *** [auction_test] Error 111

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

Regarding symlinks, in general you should haveonly symlinks into GOPATH, but not out of GOPATH as you seem to do: In this particular case you could define FABRIC_BIN_DIR env variable but i think other things won't work reliabilty if you do not have a proper (bidirectional) GOPATH tree. Just move your directory into GOPATH and then symlink from ~/ and you still have quick access?

Regarding license checks failing, clearly these files cannot have a license header. Also, doesn't fail for me. (Anyway, this is a fabric script so you might be anyway the better person to debug ;-)

. ${SCRIPTDIR}/common_utils.sh
. ${SCRIPTDIR}/common_ledger.sh

CC_ID=ecc_auction_test
Copy link
Contributor

Choose a reason for hiding this comment

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

It is failing for me and this should be the problem:
you have updated the id but not vscc;
there is also an mrenclave not found -- I am still debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you have updated the id but not vscc;
i only changed id to make docker images distinct if you run multiple setups, e.g., integration test and example, on same machine. In any case, not sure what id you mean when you refer to vscc? This is a reference to code (and not an instance of the chaincode)?

there is also an mrenclave not found -- I am still debugging
not sure what you are referring to here

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

Regarding symlinks, in general you should haveonly symlinks into GOPATH, but not out of GOPATH as you seem to do: In this particular case you could define FABRIC_BIN_DIR env variable but i think other things won't work reliabilty if you do not have a proper (bidirectional) GOPATH tree. Just move your directory into GOPATH and then symlink from ~/ and you still have quick access?

Regarding license checks failing, clearly these files cannot have a license header. Also, doesn't fail for me. (Anyway, this is a fabric script so you might be anyway the better person to debug ;-)

Actually, now it is failing also but only after i noticed somebody merged master into this branch and I (force) pulled the new version.

PS: I'd prefer if unless handing over control i would own my own PR branches and the merge of master and alike (e.g., i would rather have done a rebase than a merge and it is also easier to understand what happens if you report problems)

@bvavala
Copy link
Contributor

bvavala commented Jun 13, 2019

got it, and fine with that -- just wanted to speed up a possible approval, since it was good before.
Anyway, it did not work for me even before I updated the branch.

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

got it, and fine with that -- just wanted to speed up a possible approval, since it was good before.
Anyway, it did not work for me even before I updated the branch.

Hmm, didn't you approve before my test for the config (which i took as a confirmation that it works) ? Are you saying the version after todays force-push but before the merge failed but did work before my merge-push with config test? If so, how did it fail?

@bvavala
Copy link
Contributor

bvavala commented Jun 13, 2019

Yes, it worked before.
Then you pushed a big commit -- just for checking the existence of the key files!!!
This solved the key checking part, but I think you modified the CC_ID of the auction test, and vscc does not recognize it anymore. There is also a problem with getting the mrenclave -- it turns out to be empty.

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

Regarding symlinks, in general you should haveonly symlinks into GOPATH, but not out of GOPATH as you seem to do: In this particular case you could define FABRIC_BIN_DIR env variable but i think other things won't work reliabilty if you do not have a proper (bidirectional) GOPATH tree. Just move your directory into GOPATH and then symlink from ~/ and you still have quick access?

Regarding license checks failing, clearly these files cannot have a license header. Also, doesn't fail for me. (Anyway, this is a fabric script so you might be anyway the better person to debug ;-)

Looking now at the license check, i notice that it actually only tests files (and directories) modified in the current commit and the attempt to filter out files which should not have a license is very brittle. I don't think that this is something we realistically can make robust (and i'm pretty sure it will fail for fabric also on quite some commits), so i would suggest to leave it as is and just for such failing license checks manually peruse the files which make it fail and if bening ignore the errors ...

We might, though, remove the dependency on 'check' for target 'test' and move 'check' to last in list for target 'all' in build.mk? Any reason test should depend on checks?

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

Yes, it worked before.
Then you pushed a big commit -- just for checking the existence of the key files!!!
it was an amend commit. the changes were not thaat bug: addition of ledger_precond_check which does all the checks and changed some variable definitions to be conditioned on being not yet defined. If we already check then we might check everything and also do so that it is general and robust (e.g., reading file location from config)?

This solved the key checking part, but I think you modified the CC_ID of the auction test, and vscc does not recognize it anymore. There is also a problem with getting the mrenclave -- it turns out to be empty.

Nope, nothing changed in terms of id in todays diff. (if you click on the force-push in the history, you get the actual delta pushed, e.g., https://github.com/hyperledger-labs/fabric-private-chaincode/compare/b439b413cb06806b1ccb186aed9656976a6880e8..e58d4a3f515b29990878775cf764b81b70c13285 in this case)

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

Yes, it worked before.
Then you pushed a big commit -- just for checking the existence of the key files!!!
it was an amend commit. the changes were not thaat bug: addition of ledger_precond_check which does all the checks and changed some variable definitions to be conditioned on being not yet defined. If we already check then we might check everything and also do so that it is general and robust (e.g., reading file location from config)?

This solved the key checking part, but I think you modified the CC_ID of the auction test, and vscc does not recognize it anymore. There is also a problem with getting the mrenclave -- it turns out to be empty.

Nope, nothing changed in terms of id in todays diff. (if you click on the force-push in the history, you get the actual delta pushed, e.g., https://github.com/hyperledger-labs/fabric-private-chaincode/compare/b439b413cb06806b1ccb186aed9656976a6880e8..e58d4a3f515b29990878775cf764b81b70c13285 in this case)

btw: did you do a 'make clean' before you run the test?

@bvavala
Copy link
Contributor

bvavala commented Jun 13, 2019

Yes, it worked before.
Then you pushed a big commit -- just for checking the existence of the key files!!!
it was an amend commit. the changes were not thaat bug: addition of ledger_precond_check which does all the checks and changed some variable definitions to be conditioned on being not yet defined. If we already check then we might check everything and also do so that it is general and robust (e.g., reading file location from config)?

This solved the key checking part, but I think you modified the CC_ID of the auction test, and vscc does not recognize it anymore. There is also a problem with getting the mrenclave -- it turns out to be empty.

Nope, nothing changed in terms of id in todays diff. (if you click on the force-push in the history, you get the actual delta pushed, e.g., https://github.com/hyperledger-labs/fabric-private-chaincode/compare/b439b413cb06806b1ccb186aed9656976a6880e8..e58d4a3f515b29990878775cf764b81b70c13285 in this case)

btw: did you do a 'make clean' before you run the test?

Of course I did.
You PR https://github.com/hyperledger-labs/fabric-private-chaincode/pull/79/commits
contains 3 commits. The last one (the branch update) is mine.
The diff of the other two is this one
https://github.com/hyperledger-labs/fabric-private-chaincode/compare/c8aa61da0cbe13c1c61124400f59af87f001e0d5..e58d4a3f515b29990878775cf764b81b70c13285
and it's big and contains CC_ID=ecc_auction_test

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

Yes, it worked before.
Then you pushed a big commit -- just for checking the existence of the key files!!!
it was an amend commit. the changes were not thaat bug: addition of ledger_precond_check which does all the checks and changed some variable definitions to be conditioned on being not yet defined. If we already check then we might check everything and also do so that it is general and robust (e.g., reading file location from config)?

This solved the key checking part, but I think you modified the CC_ID of the auction test, and vscc does not recognize it anymore. There is also a problem with getting the mrenclave -- it turns out to be empty.

Nope, nothing changed in terms of id in todays diff. (if you click on the force-push in the history, you get the actual delta pushed, e.g., https://github.com/hyperledger-labs/fabric-private-chaincode/compare/b439b413cb06806b1ccb186aed9656976a6880e8..e58d4a3f515b29990878775cf764b81b70c13285 in this case)

btw: did you do a 'make clean' before you run the test?

Of course I did.
You PR https://github.com/hyperledger-labs/fabric-private-chaincode/pull/79/commits
contains 3 commits. The last one (the branch update) is mine.
The diff of the other two is this one
https://github.com/hyperledger-labs/fabric-private-chaincode/compare/c8aa61da0cbe13c1c61124400f59af87f001e0d5..e58d4a3f515b29990878775cf764b81b70c13285
and it's big and contains CC_ID=ecc_auction_test

That change is from a week ago (in this force-push/rebase/delta-commit: https://github.com/hyperledger-labs/fabric-private-chaincode/compare/de1685d52ca581aede1c1d76f95ca1cf568faa36..b439b413cb06806b1ccb186aed9656976a6880e8). I've done rebase to add the patches to keep a clean commit history, so just looking at the final commits doesn't tell you the full story ....

Anyway, what exactly is the problem/error you get with vscc?

@bvavala
Copy link
Contributor

bvavala commented Jun 13, 2019

2019-06-13 07:46:02.936 PDT [tl-enclave] golog -> INFO 05a ERROR [Enclave] Ledger: Only SGX chaincodes supported currently!
2019-06-13 07:46:06.615 PDT [vscc] Validate -> ERRO 067 VSCC error: checkAttestation failed, err mrenclave is empty
2019-06-13 07:46:06.615 PDT [committer.txvalidator] validateTx -> ERRO 068 VSCCValidateTx for transaction txId = b852984355600635b5e8e83d6adf92c7dc15734b1117f8b0d94d23b82973020d returned error: mrenclave is empty
2019-06-13 07:46:06.822 PDT [tl-enclave] golog -> INFO 06c ERROR [Enclave] Ledger: >>> NO MRENCLAVE found for: ecc.MRENCLAVE

then it crashes.

About the changes and commits. I have no idea of what you are talking about.
Your PR "only" contains: c8aa61d, e58d4a3.

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

About the changes and commits. I have no idea of what you are talking about.
Your PR "only" contains: c8aa61d, e58d4a3.

Did you click on the links i provided? They correspond (as mentioned) to the incremental changes in introduced in the rebase of the commits you mention. They show up as force-push (links) in the history on this page.

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

2019-06-13 07:46:02.936 PDT [tl-enclave] golog -> INFO 05a ERROR [Enclave] Ledger: Only SGX chaincodes supported currently!
2019-06-13 07:46:06.615 PDT [vscc] Validate -> ERRO 067 VSCC error: checkAttestation failed, err mrenclave is empty
2019-06-13 07:46:06.615 PDT [committer.txvalidator] validateTx -> ERRO 068 VSCCValidateTx for transaction txId = b852984355600635b5e8e83d6adf92c7dc15734b1117f8b0d94d23b82973020d returned error: mrenclave is empty
2019-06-13 07:46:06.822 PDT [tl-enclave] golog -> INFO 06c ERROR [Enclave] Ledger: >>> NO MRENCLAVE found for: ecc.MRENCLAVE

then it crashes.

This sounds like an error in the extraction of the MRENCLAVE. As i pointed out in an earlier PR #75, we didn't do proper return code checking and anyway extract the MRENCLAVE much more complex/brittle than necessary. The latter part is issue #77 the former i thought we fixed but maybe not ... (doesn't seem so, at least *_enclave/generate_mrenclave.sh still doesn't check any errors ..)

* integration-test dir structure with externalized sgx-config (also for demo)
* better logging defaults for peer
* a few more common ignores

Signed-off-by: Michael Steiner <michael.steiner@intel.com>
@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 13, 2019

This sounds like an error in the extraction of the MRENCLAVE. As i pointed out in an earlier PR #75, we didn't do proper return code checking and anyway extract the MRENCLAVE much more complex/brittle than necessary. The latter part is issue #77 the former i thought we fixed but maybe not ... (doesn't seem so, at least *_enclave/generate_mrenclave.sh still doesn't check any errors ..)

Did add now fix with missing error check (as well as pushing 'checks' make target to last, see above). Hopefully, that helps root-causing problem Bruno faces ...

* added various missing license headers
* integration test based on auction CC

Signed-off-by: Michael Steiner <michael.steiner@intel.com>
,

  though still remaining issue hyperledger#77 on simplifying logic)
  and also removed duplication of script itself.
* make checks target last and test not depending on it as license check is
  rather brittle and requires often manual error-checking (and ignoring thereof)
* small tweak in make checks target to treat both scripts identical

Signed-off-by: Michael Steiner <michael.steiner@intel.com>
@bvavala
Copy link
Contributor

bvavala commented Jun 14, 2019

It works for me on 7dc6d35

@g2flyer
Copy link
Contributor Author

g2flyer commented Jun 14, 2019

7dc6d35 reverts the name of the cc back to ecc from ecc_auction_test. The latter would have been nicer due to keeping dockers and alike of tests separate from other runs but before we fix issue #86 i guess we have to stick with "ecc" ... :-(

Copy link
Contributor

@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.

This looks good to me. I am also able to run the integration test successfully!

@mbrandenburger mbrandenburger merged commit 70f3e65 into hyperledger:master Jun 17, 2019
@mbrandenburger
Copy link
Contributor

Great job! Thanks for this PR!

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.

None yet

3 participants