[CI-Examples] Add Nginx test data files measurements within the manifest #2018
Conversation
vasanth-intel
force-pushed
the
vasanth-intel/nginx_makefile_fix
branch
from
6a41464 to
ffccc46
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
-- commits line 4 at r1:
This whole commit message would need to be changed a bit and rewrapped, we can do it during final merge.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @vasanth-intel)
a discussion (no related file):
How's it possible that our CI didn't detect this? It is tested under SGX...
CI-Examples/nginx/Makefile line 53 at r1 (raw file):
$(foreach mirror,$(NGINX_MIRRORS),--url $(mirror)/$(NGINX_SRC).tar.gz) nginx.manifest: nginx.manifest.template $(INSTALL_DIR)/sbin/nginx testdata \
We shouldn't depend on a .PHONY target, instead it should be $TEST_DATA (and you need to move that variable up).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)
CI-Examples/nginx/Makefile line 53 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
We shouldn't depend on a
.PHONYtarget, instead it should be$TEST_DATA(and you need to move that variable up).
This fix was recommended by Dmitrii as per #2016 (comment). Any issues with the current fix?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow and @vasanth-intel)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
How's it possible that our CI didn't detect this? It is tested under SGX...
The reason is that Nginx falls back to a default index.html (which is created during Nginx build & install and which is put in trusted files).
So even though by default benchmark-http.sh script tries to fetch 10K.html auto-generated page (and not in trusted files), the script gets back index.html and is still happy. You can see how Nginx on current Gramine master falls back to index.html:
$ gramine-sgx ./nginx
...
$ wget -q --no-proxy -O - http://127.0.0.1:8002 | head
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
html { color-scheme: light dark; }
body { width: 35em; margin: 0 auto;
font-family: Tahoma, Verdana, Arial, sans-serif; }
</style>
(--no-proxy is due to my company's proxy, ignore it.)
I guess we could set up Nginx so that it doesn't fall back to index.html but instead returns a proper error?
CI-Examples/nginx/Makefile line 53 at r1 (raw file):
Previously, vasanth-intel wrote…
This fix was recommended by Dmitrii as per #2016 (comment). Any issues with the current fix?
Oops, indeed.
The funny thing is that this target already has $(TEST_DATA) dependency, but since TEST_DATA itself is declared later in this file, it was expanded to an empty string, and thus the dependency was not really "taken".
Here's the git diff that seems to work fine:
diff --git a/CI-Examples/nginx/Makefile b/CI-Examples/nginx/Makefile
index 16853668..169a68a4 100644
--- a/CI-Examples/nginx/Makefile
+++ b/CI-Examples/nginx/Makefile
@@ -17,6 +17,18 @@ LISTEN_HOST ?= 127.0.0.1
LISTEN_PORT ?= 8002
LISTEN_SSL_PORT ?= 8444
+# HTTP docs: Generating random HTML files in $(INSTALL_DIR)/html/random
+RANDOM_DIR = $(INSTALL_DIR)/html/random
+RANDOM_FILES = \
+ $(foreach n,1 2 3 4 5 6 7 8 9 10,2K.$n.html) \
+ $(foreach n,1 2 3 4 5,10K.$n.html) \
+ $(foreach n,1 2 3 4 5,100K.$n.html) \
+ $(foreach n,1 2 3,1M.$n.html) \
+ $(foreach n,1 2 3,10M.$n.html) \
+ $(foreach n,1 2 3,100.$n.html)
+
+TEST_DATA = $(addprefix $(RANDOM_DIR)/,$(RANDOM_FILES))
+
ifeq ($(DEBUG),1)
GRAMINE_LOG_LEVEL = debug
else
@@ -82,18 +94,6 @@ $(INSTALL_DIR)/conf/nginx-gramine.conf: nginx-gramine.conf.template $(INSTALL_DI
-e 's|$$(LISTEN_HOST)|'"$(LISTEN_HOST)"'|g' \
$< > $@
-# HTTP docs: Generating random HTML files in $(INSTALL_DIR)/html/random
-RANDOM_DIR = $(INSTALL_DIR)/html/random
-RANDOM_FILES = \
- $(foreach n,1 2 3 4 5 6 7 8 9 10,2K.$n.html) \
- $(foreach n,1 2 3 4 5,10K.$n.html) \
- $(foreach n,1 2 3 4 5,100K.$n.html) \
- $(foreach n,1 2 3,1M.$n.html) \
- $(foreach n,1 2 3,10M.$n.html) \
- $(foreach n,1 2 3,100.$n.html)
-
-TEST_DATA = $(addprefix $(RANDOM_DIR)/,$(RANDOM_FILES))
-
# We need to first build and install nginx, otherwise nginx' makefiles think that they already
# filled $(INSTALL_DIR)/html and skip copying installation files.
$(RANDOM_DIR)/%.html: $(INSTALL_DIR)/sbin/nginxSo, just moving several lines above this particular target. @vasanth-intel Please re-do according to this git diff.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow and @vasanth-intel)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @vasanth-intel)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The reason is that Nginx falls back to a default
index.html(which is created during Nginx build & install and which is put in trusted files).So even though by default
benchmark-http.shscript tries to fetch10K.htmlauto-generated page (and not in trusted files), the script gets backindex.htmland is still happy. You can see how Nginx on current Gramine master falls back toindex.html:$ gramine-sgx ./nginx ... $ wget -q --no-proxy -O - http://127.0.0.1:8002 | head <!DOCTYPE html> <html> <head> <title>Welcome to nginx!</title> <style> html { color-scheme: light dark; } body { width: 35em; margin: 0 auto; font-family: Tahoma, Verdana, Arial, sans-serif; } </style>(
--no-proxyis due to my company's proxy, ignore it.)I guess we could set up Nginx so that it doesn't fall back to
index.htmlbut instead returns a proper error?
Ouch, that's pretty bad. Yeah, we should either change the config or check the file contents in the benchmark script? (the former is easier, probably)
CI-Examples/nginx/Makefile line 53 at r1 (raw file):
This fix was recommended by Dmitrii as per #2016 (comment). Any issues with the current fix?
@vasanth-intel: I don't understand your comment, I just listed the issue above in my comment... Also, it doesn't matter who suggested something, if it's wrong then we need to fix that.
Here's the git diff that seems to work fine:
Yup, looks good, assuming you'll also check the line with this very comment.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @dimakuv, @kailun-qin, and @mkow)
CI-Examples/nginx/Makefile line 53 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This fix was recommended by Dmitrii as per #2016 (comment). Any issues with the current fix?
@vasanth-intel: I don't understand your comment, I just listed the issue above in my comment... Also, it doesn't matter who suggested something, if it's wrong then we need to fix that.
Here's the git diff that seems to work fine:
Yup, looks good, assuming you'll also check the line with this very comment.
@dimakuv I thought your suggestion was right and hence pointed to your comment. Agree that if some suggestion is wrong we need to fix it.. Have updated and pushed the change as per @mkow git diff suggestion. Please re-review.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @mkow and @vasanth-intel)
a discussion (no related file):
Yeah, we should either change the config or check the file contents in the benchmark script? (the former is easier, probably)
I would say it's a separate PR. (I assume that we want to merge this PR asap, and fix the benchmark script/Nginx conf later.)
CI-Examples/nginx/Makefile line 53 at r1 (raw file):
@dimakuv I thought your suggestion was right and hence pointed to your comment.
Yes, I understand your confusion. Indeed, I also thought that my initial suggestion was good enough. But @mkow is right -- we already have the TEST_DATA dependency listed, so we just need to make this dependency work. (The TEST_DATA variable expands to a list of actual HTML files, and such real-file dependencies are always better than non-real PHONY dependencies.)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @mkow and @vasanth-intel)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @vasanth-intel)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yeah, we should either change the config or check the file contents in the benchmark script? (the former is easier, probably)
I would say it's a separate PR. (I assume that we want to merge this PR asap, and fix the benchmark script/Nginx conf later.)
Could you or @vasanth-intel create an issue?
-- commits line 9 at r2:
@vasanth-intel: Please read our contributing.rst.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @vasanth-intel)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you or @vasanth-intel create an issue?
I created an issue: #2035
Please note that I was wrong initially. The problem is not in Nginx, but in wrk.
In my example script above with wget, I forgot to add the /remote/10K.1.html actual URI, so wget returned index.html (the default URI). So my bad, drawing wrong conclusions from a wrong invocation of wget :(
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @vasanth-intel)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, period found at the end of a one-liner, one-liner is not in imperative mood (waiting on @vasanth-intel)
Commit aef087f "[LibOS] Move trusted and allowed files logic to LibOS" incorrectly modified Makefile of the Nginx example. The test data files (automatically generated by Makefile and put under Nginx install dir `install/html/random`) got generated after the Gramine manifest expansion. This led to those test files not being added as `sgx.trusted_files` to the manifest, and Nginx server could not retrieve them and thus returned error code 403 ("Forbidden"). The fix is simple: move the `$TEST_DATA` variable before the `nginx.manifest` target (which already has `$TEST_DATA` as its dependency). This bug was not detected by our CI because the Nginx test uses `wrk` which ignores error code 403 and happily counts this error response towards its total stats. This needs to be fixed in a separate commit. Signed-off-by: Vasanth Nagaraja <[email protected]>
dimakuv
force-pushed
the
vasanth-intel/nginx_makefile_fix
branch
from
d30b3d0 to
42251d8
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vasanth-intel)
Previously, mkow (Michał Kowalczyk) wrote…
@vasanth-intel: Please read our contributing.rst.
@mkow You're blocking here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mkow and @vasanth-intel)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This whole commit message would need to be changed a bit and rewrapped, we can do it during final merge.
Done
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mkow and @vasanth-intel)
|
Jenkins, retest this please |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
Description of the problem
Commit aef087f "[[LibOS] Move trusted and allowed files logic to LibOS]" modified the Nginx
Makefileafter which it was observed that all the test data files (install/html/random) are generated after the manifest file generation and hence were not added assgx.trusted_filesin manifest.sgx file.Fixes #2016
How to test this PR?
git clone https://github.com/gramineproject/gramine.git.CI-Examples/nginxfolder.make SGX=1command.sgx.trusted_fileswithin the manifest.This change is