-
Notifications
You must be signed in to change notification settings - Fork 260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @pwmarcz)
a discussion (no related file):
There is no README for SQLite. Please add some minimal README.md
file. Please mention that this uses the system-installed SQLite3 version.
a discussion (no related file):
I tested it manually and looked at the outputs of our Jenkins. Looks good!
Examples/sqlite/Makefile, line 23 at r1 (raw file):
all: sqlite3.manifest ifeq ($(SGX),1) all: sqlite3.manifest.sgx sqlite3.sig sqlite3.token
Technically only the .token
file can be listed here, since all other files are dependencies for it. Not blocking though, it's good for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There is no README for SQLite. Please add some minimal
README.md
file. Please mention that this uses the system-installed SQLite3 version.
I added a README similar to other projects, and wrote a section about limitations of our locks.
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I tested it manually and looked at the outputs of our Jenkins. Looks good!
Thanks for testing!
Examples/sqlite/Makefile, line 23 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Technically only the
.token
file can be listed here, since all other files are dependencies for it. Not blocking though, it's good for readability.
Looks like we're pretty consistent with this:
graphene/Examples $ git grep 'all.*token'
apache/Makefile:all: httpd.manifest.sgx httpd.sig httpd.token
bash/Makefile:all: bash.manifest.sgx bash.sig bash.token
blender/Makefile:all: blender.manifest.sgx blender.sig blender.token
busybox/Makefile:all: busybox.manifest.sgx busybox.sig busybox.token
capnproto/Makefile:all: addressbook.manifest.sgx addressbook.sig addressbook.token
curl/Makefile:all: curl.manifest.sgx curl.sig curl.token
gcc/Makefile:all: gcc.manifest.sgx gcc.sig gcc.token
lighttpd/Makefile:all: lighttpd.manifest.sgx lighttpd.sig lighttpd.token
memcached/Makefile:all: memcached.manifest.sgx memcached.sig memcached.token
nginx/Makefile:all: nginx.manifest.sgx nginx.sig nginx.token
nodejs-express-server/Makefile:all: nodejs.manifest.sgx nodejs.sig nodejs.token
nodejs/Makefile:all: nodejs.manifest.sgx nodejs.sig nodejs.token
python/Makefile:all: python.manifest.sgx python.sig python.token
pytorch/Makefile:all: pytorch.manifest.sgx pytorch.sig pytorch.token
r/Makefile:all: R.manifest.sgx R.sig R.token
redis/Makefile:all: redis-server.manifest.sgx redis-server.sig redis-server.token
tensorflow-lite/Makefile:all: label_image.manifest.sgx label_image.sig label_image.token
So I'd rather leave it, at least until we have a better build-system for the examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)
Examples/sqlite/README.md, line 1 at r2 (raw file):
# Bash example
Bash
?
Examples/sqlite/README.md, line 25 at r2 (raw file):
# Running SQLite with Graphene Here's an example of running Bash scripts under Graphene:
Bash scripts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Examples/sqlite/README.md, line 1 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Bash
?
Oops, I copied it and didn't replace everything, sorry. Fixed.
Examples/sqlite/README.md, line 25 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Bash scripts
?
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 18 files at r3.
Reviewable status: 14 of 31 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)
a discussion (no related file):
Files from your PR crept into this one. Please rebase to the latest master to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 14 of 31 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Files from your PR crept into this one. Please rebase to the latest master to fix it.
Interesting, so that happens when you do PR to a PR... Sorry for the mess, I see the rest of the files landed in "Reverted" now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 17 of 17 files at r4.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 13 files at r1, 1 of 18 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pwmarcz)
a discussion (no related file):
I think we've reached the limit of maintainable examples, we shouldn't add more. It sometimes takes a few hours to do a simple change because you need to fix and test 20 different examples afterwards.
So, if we think SQLite is worth of inclusion, then I'd remove curl and merge nodejs examples to make room for it.
@dimakuv, @boryspoplawski, @pwmarcz: Any thoughts?
.ci/lib/stage-test-sgx.jenkinsfile, line 133 at r4 (raw file):
timeout(time: 5, unit: 'MINUTES') { sh ''' # test SGX remote attestation only on Ubuntu 18.04 to keep internet requests to minimum
btw., @dimakuv: Unrelated to the PR, but I think this should be bumped to 20.04. Or maybe this problem is no longer present and we can test this everywhere?
Examples/sqlite/.gitignore, line 1 at r4 (raw file):
OUTPUT
Should be /OUTPUT
Examples/sqlite/Makefile, line 36 at r4 (raw file):
# Generating the SGX-specific manifest (*.manifest.sgx), the enclave signature, # and the token for enclave initialization.
I'd drop this comment, other places are not commented like this + I don't like this copy-pasting from other examples, all of them diverge too quickly :)
Examples/sqlite/README.md, line 50 at r4 (raw file):
but only within a **single Graphene instance**. In other words, a multi-process Graphene application is OK, but multiple Graphene instances should not access the same database file concurrently.
I'm not sure if this is worth mentioning, this problem is only in test setups. In production setups you have to mount this as protected (or in tmpfs), and you don't have such issues at all then. And I think this is what should be mentioned (that specifying the database as "allowed" is for testing only).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 13 files at r1, 1 of 18 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dimakuv, @mkow, and @pwmarcz)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
I think we've reached the limit of maintainable examples, we shouldn't add more. It sometimes takes a few hours to do a simple change because you need to fix and test 20 different examples afterwards.
So, if we think SQLite is worth of inclusion, then I'd remove curl and merge nodejs examples to make room for it.@dimakuv, @boryspoplawski, @pwmarcz: Any thoughts?
I've never had any problems with examples, when changing in Graphene stuff. What changes other than manifest syntax/options affect examples?
Examples/sqlite/README.md, line 50 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'm not sure if this is worth mentioning, this problem is only in test setups. In production setups you have to mount this as protected (or in tmpfs), and you don't have such issues at all then. And I think this is what should be mentioned (that specifying the database as "allowed" is for testing only).
I think this comment should stay, because people will be testing stuff with allowed files. But I also agree that we should mention that allowed files are for testing purposes only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mkow and @pwmarcz)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
I've never had any problems with examples, when changing in Graphene stuff. What changes other than manifest syntax/options affect examples?
It's time to discuss this with everyone and agree on some sane policy. I would prefer the following:
- We have a minimal set of examples in this repo -- only those that are easy to download & build & check in our Jenkins CI.
- We have a set of all other (big-ish) examples in another repo -- it will be linked as a sub-module.
- All the rest of examples (which we don't consider important) are dumped in graphene-contrib repo.
Here is my take on our minimal set of examples:
- Bash
- Blender
- Busybox
- lighttpd
- memcached
- nginx
- python (but with venv or something!)
- ra-tls-mbedtls
- ra-tls-secret-prov
- redis
- SQLite
(I don't mind dropping Bash from here)
The "extended" examples that go in a separate repo:
- Apache (??? or drop it?)
- Curl
- GCC
- nodejs
- OpenVINO
- PyTorch
- R
- TensorFlow-Lite
(The planned Java, TensorFlow, Google Go examples should also go into this "extended" repo.)
The examples to remove completely or to move to graphene-contrib:
- Apache (???)
- Capnproto
- nodejs-express-server
.ci/lib/stage-test-sgx.jenkinsfile, line 133 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
btw., @dimakuv: Unrelated to the PR, but I think this should be bumped to 20.04. Or maybe this problem is no longer present and we can test this everywhere?
There was no real problem. ra-tls-secret-prov
and ra-tls-mbedtls
(above, you didn't mark it) examples simply use the EPID remote attestation, so they sent TLS requests to the Intel Attestation Service (IAS).
When implementing these CI tests, I simply saw no reason to test these minimalistic examples in both 16.04 and 18.04, and chose to add them only to 18.04. This has the additional benefit of sending only 2 requests to IAS instead of 4 on each CI trigger. (IAS, at least at that time, was known to throttle requests.)
So yes, if you want to, we can:
- Move these two examples from 18.04 to 20.04
- Add these two examples to 20.04 additionally
Whatever you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mkow and @pwmarcz)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
It's time to discuss this with everyone and agree on some sane policy. I would prefer the following:
- We have a minimal set of examples in this repo -- only those that are easy to download & build & check in our Jenkins CI.
- We have a set of all other (big-ish) examples in another repo -- it will be linked as a sub-module.
- All the rest of examples (which we don't consider important) are dumped in graphene-contrib repo.
Here is my take on our minimal set of examples:
- Bash
- Blender
- Busybox
- lighttpd
- memcached
- nginx
- python (but with venv or something!)
- ra-tls-mbedtls
- ra-tls-secret-prov
- redis
- SQLite
(I don't mind dropping Bash from here)
The "extended" examples that go in a separate repo:
- Apache (??? or drop it?)
- Curl
- GCC
- nodejs
- OpenVINO
- PyTorch
- R
- TensorFlow-Lite
(The planned Java, TensorFlow, Google Go examples should also go into this "extended" repo.)
The examples to remove completely or to move to graphene-contrib:
- Apache (???)
- Capnproto
- nodejs-express-server
I would keep only 1 of {Bash, Busybox} in the minimal set.
On the Apache topic: I would keep it at least until the actual release happens. If we get some feedback about need for sysv semaphores, then we can reimplement them and restore Apache example, otherwise I would say we can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 17 of 17 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pwmarcz)
a discussion (no related file):
I've never had any problems with examples, when changing in Graphene stuff.
I think it just was usually me doing changes in these areas, that's why you've never had problems with this.
What changes other than manifest syntax/options affect examples?
In theory only these, but in practice we e.g. often had copy-pasted bugs in their Makefiles.
We have a minimal set of examples in this repo -- only those that are easy to download & build & check in our Jenkins CI.
+1
We have a set of all other (big-ish) examples in another repo -- it will be linked as a sub-module.
No submodules please, they are unmaintainable. I'd just move such examples to graphene-contrib and update them every release.
I would keep only 1 of {Bash, Busybox} in the minimal set.
+1, I'd leave Bash because it contains more black magic inside and happened to trigger bugs in Graphene in the past.
Also, I'd add Go to the examples tested in CI, as it exercises a lot of weird things related to signal handling.
.ci/lib/stage-test-sgx.jenkinsfile, line 133 at r4 (raw file):
IAS, at least at that time, was known to throttle requests
Is this still the case? If so, then we can just bump this to 20.04, if not then test on both Ubuntus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pwmarcz)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
I've never had any problems with examples, when changing in Graphene stuff.
I think it just was usually me doing changes in these areas, that's why you've never had problems with this.
What changes other than manifest syntax/options affect examples?
In theory only these, but in practice we e.g. often had copy-pasted bugs in their Makefiles.
We have a minimal set of examples in this repo -- only those that are easy to download & build & check in our Jenkins CI.
+1
We have a set of all other (big-ish) examples in another repo -- it will be linked as a sub-module.
No submodules please, they are unmaintainable. I'd just move such examples to graphene-contrib and update them every release.
I would keep only 1 of {Bash, Busybox} in the minimal set.
+1, I'd leave Bash because it contains more black magic inside and happened to trigger bugs in Graphene in the past.
Also, I'd add Go to the examples tested in CI, as it exercises a lot of weird things related to signal handling.
The problems with Bash and Go is that they are not "download & build" kinds of examples. On the other hand, it is true that Bash and Go are quite helpful in debugging corner cases. And it also looks like both Bash and Go don't require any modifications to manifest files on different OS distros.
So yes, I'm also happy to keep Bash and Go in our curated Examples list.
But I would also keep Busybox. It's very helpful to e.g. peek inside Graphene FS. And much more lightweight to work with than Bash.
No submodules it is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pwmarcz)
.ci/lib/stage-test-sgx.jenkinsfile, line 133 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
IAS, at least at that time, was known to throttle requests
Is this still the case? If so, then we can just bump this to 20.04, if not then test on both Ubuntus.
Here you go: #2534
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pwmarcz)
.ci/lib/stage-test-sgx.jenkinsfile, line 133 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Here you go: #2534
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pwmarcz)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pwmarcz)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The problems with Bash and Go is that they are not "download & build" kinds of examples. On the other hand, it is true that Bash and Go are quite helpful in debugging corner cases. And it also looks like both Bash and Go don't require any modifications to manifest files on different OS distros.
So yes, I'm also happy to keep Bash and Go in our curated Examples list.
But I would also keep Busybox. It's very helpful to e.g. peek inside Graphene FS. And much more lightweight to work with than Bash.
No submodules it is :)
Used this discussion in this meta-issue on repo migration: #2558
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski and @mkow)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Used this discussion in this meta-issue on repo migration: #2558
I think a large part of the problem is too much boilerplate. For instance, some Makefiles and manifests contain very detailed comments that look like they belong to Graphene documentation. Ideally, the boilerplate should be minimal, and most code/comments should be there only because they're needed for this one example.
I agree with keeping the examples in the same repo. They act as another layer as sanity checks for Graphene, and also we should improve the process of building them first: it will be even harder to keep a separate repo in sync.
No strong opinions about the set of examples, but as a developer, I use Busybox and Python often.
Examples/sqlite/.gitignore, line 1 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Should be
/OUTPUT
Done.
Examples/sqlite/Makefile, line 36 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd drop this comment, other places are not commented like this + I don't like this copy-pasting from other examples, all of them diverge too quickly :)
OK, removed. But I copied this file from bash
example :)
Examples/sqlite/README.md, line 50 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I think this comment should stay, because people will be testing stuff with allowed files. But I also agree that we should mention that allowed files are for testing purposes only.
I added a paragraph about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)
a discussion (no related file):
I think a large part of the problem is too much boilerplate. For instance, some Makefiles and manifests contain very detailed comments that look like they belong to Graphene documentation. Ideally, the boilerplate should be minimal, and most code/comments should be there only because they're needed for this one example.
Yup, definitely this ^
But there's also some overhead we can't easily skip, which is per-example setup commands, configuration, building options, etc. Also, our examples don't even have a common interface currently (not all of them work with just cd some-example && make check
).
And lastly, there's no single command to test them all, like we have with PAL/LibOS regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @pwmarcz)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners
Signed-off-by: Paweł Marczewski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
As requested by @dimakuv. This is on top of #2481 (fcntl locks), because it fails without these.
Description of the changes
A very simple example for
sqlite
. Wraps the Ubuntu executable.How to test this PR?
There is a simple sanity check (
make regression
).This change is