Update GSC with Gramine's new SGX driver requirements #228
Conversation
|
@kailun-qin Please review |
|
These changes now result in |
There was a problem hiding this comment.
Can you go through this PR - Remove support for OOT (out-of-tree) SGX driver by woju · Pull Request #2061 · gramineproject/gramine
It has explanation of why this could be failing, probably older kernel?
Reviewable status: 0 of 4 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners
Thanks @adarshan-intel, I am using the 6.8.0-40-generic kernel -- thus, |
|
@anirbanbasu Have made a recent change in the commit, try running again. |
The correct solution is to install (Downloading the header from the Internet and copying it into |
|
@adarshan-intel, downloading to @woju, this is happening on Ubuntu and Debian. Should I install |
|
While the build process succeeds, I get a separate error when starting the built container as |
Yes, you need to install it wherever you compile Gramine. |
@woju, should this (https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.compile.template) not be the one to install |
Just to make sure that this comment of mine does not confuse anyone, this turned out to be a misconfiguration in my application, which has now been dealt with. |
It should be installed in templates that derive from this one, because package installation is different in different distributions (i.e. in this |
|
The latest commit I have tested basic runs on Ubuntu, Debian, CentOS, CentOS Stream 9, Red Hat, and SLES. Everything is running fine. |
@adarshan-intel @woju in my case, this brought back the |
|
@anirbanbasu The solution I mentioned seems like a bit of a hack and might not be the best approach. According to @woju I've added the necessary package to the @woju Is there something I am missing here? |
|
@anirbanbasu By the way, which Ubuntu version are you using? Is it 20.04, 22.04, 23.10 or 24.04? |
@adarshan-intel I am using Ubuntu 22.04.5 LTS. |
|
@anirbanbasu The changes I made should be working on Ubuntu 22.04.5. Everything looks good from my end, but let's wait for @woju's opinion to confirm. |
|
@anirbanbasu Could you share the logs with me? Also, template files to troubleshoot the issue? |
@adarshan-intel unfortunately, due to confidentiality reasons, I cannot. The template file Regarding the logs, if I remove the manual download of |
There was a problem hiding this comment.
Sorry to be blunt, but without seeing both logs and your actual changes (rather than relying on your descriptions of changes, because you might have made a typo), it's hard to tell anything about the reasons of the error you see. I've tested this PR's ./gsc build-gramine with debian:12, works fine with regard to <asm/sgx.h>, so unless proven otherwise, I'm going to assume the problem is on your side.
woju@[...] ~/src/gsc [git||remotes/origin/pull/228/head] [17:36 4295]% ./gsc build-gramine asdf
/usr/local/lib/python3.8/dist-packages/paramiko/transport.py:219: CryptographyDeprecationWarning: Blowfish has been deprecated
"class": algorithms.Blowfish,
Building base-Gramine Docker image `asdf`...
Step 1/8 : FROM debian:12 AS gramine
[...]
Step 8/8 : RUN cd /gramine && meson setup build/ --prefix="/gramine/meson_build_output" --buildtype=release -Ddirect=enabled -Dsgx=enabled && ninja -C build && ninja -C build install
---> Running in 46f06c28de1e
The Meson build system
[...]
Has header "asm/sgx.h" : YES
[...]
---> 4c336f858e3c
Successfully built 4c336f858e3c
Successfully tagged asdf:latest
Successfully built a base-Gramine Docker image `asdf`.
Or am I missing something?
Reviewed 2 of 3 files at r2, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel and @anirbanbasu)
templates/centos/Dockerfile.compile.template line 22 at r4 (raw file):
git \ httpd \ kernel-devel \
This package needs to be kernel-headers. If you write kernel-devel, it will also install kernel-headers because of dependencies, (kernel-devel → gcc → glibc-devel → kernel-headers), but this is not what you want to write here.
Suggestion:
kernel-headers
templates/centos/Dockerfile.compile.template line 40 at r4 (raw file):
python3-protobuf \ rpm-build \ wget \
I think wget is not used anywhere else, apart from the section that downloaded sgx.h, which you just removed, so you can also remove wget installation (here and everywhere else).
templates/suse/Dockerfile.compile.template line 29 at r4 (raw file):
gcc11-c++ \ git \ kernel-devel \
On SUSE it's called linux-glibc-devel (https://software.opensuse.org/package/linux-glibc-devel)
Suggestion:
linux-glibc-devel
templates/debian/entrypoint.manifest.template line 4 at r4 (raw file):
{% block loader %} loader.entrypoint = "file:/gramine/meson_build_output/lib/x86_64-linux-gnu/gramine/libsysdb.so"
ditto (put it in that second commit, together with the other entrypoint line)
templates/redhat/ubi-minimal/Dockerfile.compile.template line 28 at r4 (raw file):
git \ httpd \ kernel-devel \
Suggestion:
kernel-headers
templates/centos/entrypoint.manifest.template line 4 at r4 (raw file):
{% block loader %} loader.entrypoint = "file:/gramine/meson_build_output/lib64/gramine/libsysdb.so"
This is unrelated change, needs to be in a separate commit.
templates/centos/stream/Dockerfile.compile.template line 21 at r4 (raw file):
git \ httpd \ kernel-devel \
Suggestion:
kernel-headers
templates/redhat/ubi/Dockerfile.compile.template line 28 at r4 (raw file):
git \ httpd \ kernel-devel \
Suggestion:
kernel-headers
@woju, yes, understandable. I will check things at my end.
Okay! |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anirbanbasu and @woju)
templates/centos/Dockerfile.compile.template line 22 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This package needs to be
kernel-headers. If you writekernel-devel, it will also installkernel-headersbecause of dependencies, (kernel-devel→gcc→glibc-devel→kernel-headers), but this is not what you want to write here.
Done.
templates/centos/Dockerfile.compile.template line 40 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I think
wgetis not used anywhere else, apart from the section that downloadedsgx.h, which you just removed, so you can also removewgetinstallation (here and everywhere else).
Done.
templates/suse/Dockerfile.compile.template line 29 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
On SUSE it's called
linux-glibc-devel(https://software.opensuse.org/package/linux-glibc-devel)
Done.
templates/centos/stream/Dockerfile.compile.template line 21 at r4 (raw file):
git \ httpd \ kernel-devel \
Done.
templates/redhat/ubi-minimal/Dockerfile.compile.template line 28 at r4 (raw file):
git \ httpd \ kernel-devel \
Done.
templates/redhat/ubi/Dockerfile.compile.template line 28 at r4 (raw file):
git \ httpd \ kernel-devel \
Done.
There was a problem hiding this comment.
Reviewable status: 4 of 11 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anirbanbasu and @woju)
templates/centos/entrypoint.manifest.template line 4 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This is unrelated change, needs to be in a separate commit.
Done.
Removing this as of now
templates/debian/entrypoint.manifest.template line 4 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
ditto (put it in that second commit, together with the other entrypoint line)
Done.
Removing this as of now
There was a problem hiding this comment.
Reviewable status: 4 of 11 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel and @woju)
templates/Dockerfile.common.compile.template line 28 at r5 (raw file):
&& sha256sum sgx.h | grep -q a34a997ade42b61376b1c5d3d50f839fd28f2253fa047cb9c0e68a1b00477956 {% endif %}
I did a clean check out of this PR and I am met with the same error as before. See the meson build excerpt below.
---> Running in 92a25c93b6ec
The Meson build system
Version: 1.6.0
Source dir: /gramine
Build dir: /gramine/build
Build type: native build
Project name: gramine
Project version: 1.8post~UNRELEASED
C compiler for the host machine: cc (gcc 10.2.1 "cc (Debian 10.2.1-6) 10.2.1 20210110")
C linker for the host machine: cc ld.bfd 2.35.2
C++ compiler for the host machine: c++ (gcc 10.2.1 "c++ (Debian 10.2.1-6) 10.2.1 20210110")
C++ linker for the host machine: c++ ld.bfd 2.35.2
Host machine cpu family: x86_64
Host machine cpu: x86_64
Compiler for C supports arguments -std=c23: NO
Compiler for C supports arguments -std=c2x: YES
meson.build:28: WARNING: Consider using the built-in option for language standard version instead of using "-std=c2x".
Program check-no-reloc.sh found: YES (/gramine/scripts/check-no-reloc.sh)
Program gen-pal-map.py found: YES (/gramine/scripts/gen-pal-map.py)
Program get-python-platlib.py found: YES (/gramine/scripts/get-python-platlib.py)
Program meson-clang-format.sh found: YES (/gramine/scripts/meson-clang-format.sh)
Program meson-render-script.py found: YES (/gramine/scripts/meson-render-script.py)
Fetching value of define "__GLIBC__" : 2
Program objcopy found: YES (/usr/bin/objcopy)
Program python3 found: YES (/usr/bin/python3)
WARNING: You should add the boolean check kwarg to the run_command call.
It currently defaults to false,
but it will default to true in meson 2.0.
See also: https://github.com/mesonbuild/meson/issues/9300
Program nasm found: YES (/usr/bin/nasm)
Compiler for C supports arguments -Wtrampolines: YES
Compiler for C supports arguments -Wnull-dereference: YES
meson.build:85: WARNING: Consider using the built-in warning_level option instead of using "-Wall".
meson.build:85: WARNING: Consider using the built-in warning_level option instead of using "-Wextra".
meson.build:108:7: ERROR: C header 'asm/sgx.h' not found
@anirbanbasu Can you also share logs where apt packages are getting installed |
@anjalirai-intel please see the excerpts below. The step or steps hidden due to confidentiality reasons are for configuring access to a proxy server so that |
|
@anirbanbasu In your logs, it is mentioned that you are using debian:11 Issue is reproducible on debian:11 workload image and also debian:11 distro will not be supported from gramine master. sgx.h is not available as part of linux-libc-dev for debian 11 whereas it is present as part of debian 12 you can refer attached images |
@anjalirai-intel thanks, well spotted. My oversight there! Apologies. However, where is this debian 11 being picked up from? Isn't it supposed to be the same base image that my non-Graminized image built from? In my case, that is |
Okay, it was specified in the default (I do have other errors now but I will check if they are at all related to this PR.) |
There was a problem hiding this comment.
Reviewed 2 of 7 files at r5, all commit messages.
Reviewable status: 6 of 11 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel and @anjalirai-intel)
a discussion (no related file):
@anirbanbasu @anjalirai-intel: Debian 11 has linux-libc-dev 6.1, but in bullseye-backports repository: https://packages.debian.org/bullseye-backports/linux-libc-dev
So either gsc needs to support Distro: debian:bullseye-backports (that would involve doing away with distro[1] | int in some way), or needs to detect version equal to 11 and add backports repo manually, like in this Dockerfile: https://github.com/debuerreotype/docker-debian-artifacts/blob/f5527c9b022448b28981cf274721d9749d8fc5c4/bullseye/backports/Dockerfile
Blocking on this.
templates/centos/Dockerfile.compile.template line 40 at r4 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Done.
Not done, you didn't remove elsewhere.
templates/centos/entrypoint.manifest.template line 4 at r4 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Done.
Removing this as of now
Not done, I don't see a separate commit in commit list that has different commit message that would propertly describe this change, there are only fixups.
templates/debian/entrypoint.manifest.template line 4 at r4 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
Done.
Removing this as of now
ditto (not done)
|
@adarshan-intel @woju now I am facing this error, which is related to: #225 |
There was a problem hiding this comment.
Reviewable status: 6 of 11 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @woju)
templates/centos/entrypoint.manifest.template line 4 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Not done, I don't see a separate commit in commit list that has different commit message that would propertly describe this change, there are only fixups.
I want to clarify that PR #225 already implements this change, so I am not including it here. Once PR #225 is merged, this PR can proceed. Therefore, I am not adding any separate commit.
templates/debian/entrypoint.manifest.template line 4 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
ditto (not done)
ditto
|
Previously, woju (Wojtek Porczyk) wrote…
Done. |
There was a problem hiding this comment.
Reviewable status: 5 of 13 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anirbanbasu, @anjalirai-intel, and @woju)
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
@anirbanbasu @anjalirai-intel: Debian 11 has linux-libc-dev 6.1, but in
bullseye-backportsrepository: https://packages.debian.org/bullseye-backports/linux-libc-devSo either gsc needs to support
Distro: debian:bullseye-backports(that would involve doing away withdistro[1] | intin some way), or needs to detect version equal to11and add backports repo manually, like in this Dockerfile: https://github.com/debuerreotype/docker-debian-artifacts/blob/f5527c9b022448b28981cf274721d9749d8fc5c4/bullseye/backports/DockerfileBlocking on this.
I will discuss this with Anees and get back to you.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: all 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: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel, @anirbanbasu, and @anjalirai-intel)
templates/centos/entrypoint.manifest.template line 4 at r4 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
I want to clarify that PR #225 already implements this change, so I am not including it here. Once PR #225 is merged, this PR can proceed. Therefore, I am not adding any separate commit.
OK, I understand. Yes, that makes sense, but in this case you should rebase the branch on top of that other branch (instead of adding another diff to a commit in this branch) and change the base of the PR in GitHub UI. Without doing that, it's not clear which branch should be merged on top of which one, and you run a risk that you'll have circular dependencies between several PRs. If you expliticly base the PRs on top of one another, then you won't make such a mistake, and we won't make a mistake when reviewing and merging.
Right now, if I'm counting correctly, we have 4 PRs against gsc repo with at least slightly overlapping scope (#223, #225, #226, #228) and it's not clear to me, in which order they should be merged, or if this matter at all. It's up to you to decide, I won't mind any particular order, as long as it won't break anything. If you're sure the order of those other PRs doesn't matter, you can leave them based on master, but at least this stuff needs to be adjusted.
There was a problem hiding this comment.
Reviewable status: 12 of 13 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: ), "fixup! " found in commit messages' one-liners (waiting on @anjalirai-intel and @woju)
a discussion (no related file):
Previously, adarshan-intel (Adarsh Anand) wrote…
I will discuss this with Anees and get back to you.
In the latest commit 47dcb58 have done the 2nd method of adding backports repo manually by detecting the debian 11 version
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all 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: ), "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel)
a discussion (no related file):
Previously, adarshan-intel (Adarsh Anand) wrote…
In the latest commit 47dcb58 have done the 2nd method of adding backports repo manually by detecting the debian 11 version
Thanks!
templates/debian/Dockerfile.compile.template line 37 at r7 (raw file):
{%- endif %} {%- if (distro[0] == "debian" and distro[1] | int == 11) %}
- parens are unnecessary (I suppose you copy-pasted a bit too much from above).
- The repo should be added before
apt-get installabove I think (just below{% set distro %}). If you do that, then you can also install meson, tomli, and tomli-w from the backports repo.
(the logic itself is OK)
Suggestion:
distro[0] == "debian" and distro[1] | int == 11
There was a problem hiding this comment.
Reviewable status: 12 of 13 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: ), "fixup! " found in commit messages' one-liners (waiting on @woju)
templates/debian/Dockerfile.compile.template line 37 at r7 (raw file):
- parens are unnecessary (I suppose you copy-pasted a bit too much from above).
Removed brackets
- The repo should be added before
apt-get installabove I think (just below{% set distro %}). If you do that, then you can also install meson, tomli, and tomli-w from the backports repo.
Have moved{%- if (distro[0] == "debian" and distro[1] | int == 11) %}logic just below{% set distro %}
Yes I moved the logic part below {% set distro %}, if distro is debian:11 then pull the linux headers from the backports repos, for all other distros debian:12 and ubuntu packages it pulls from normal repos.
There was a problem hiding this comment.
Reviewable status: 12 of 13 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: ), "fixup! " found in commit messages' one-liners (waiting on @woju)
templates/debian/Dockerfile.compile.template line 37 at r7 (raw file):
Previously, adarshan-intel (Adarsh Anand) wrote…
- parens are unnecessary (I suppose you copy-pasted a bit too much from above).
Removed brackets
- The repo should be added before
apt-get installabove I think (just below{% set distro %}). If you do that, then you can also install meson, tomli, and tomli-w from the backports repo.
Have moved{%- if (distro[0] == "debian" and distro[1] | int == 11) %}logic just below{% set distro %}Yes I moved the logic part below
{% set distro %}, if distro isdebian:11then pull the linux headers from the backports repos, for all other distrosdebian:12and ubuntu packages it pulls from normal repos.
Also updating linux-headers-generic to linux-libc-dev package for installing kernel headers as both packages cannot be used together due to mismatch
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, 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: ITL), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 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: ITL), "fixup! " found in commit messages' one-liners
|
@mkow @kailun-qin Can you review? |
There was a problem hiding this comment.
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: ITL), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 2 of 3 files at r2, 1 of 9 files at r4, 3 of 7 files at r5, 6 of 7 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 2 of 3 files at r2, 1 of 9 files at r4, 3 of 7 files at r5, 6 of 7 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @adarshan-intel)
-- commits line 3 at r8:
Please Don't Capitalize Every Word In A Sentence :)
Suggestion:
Update GSC with Gramine's new SGX driver requirements
Signed-off-by: Adarsh Anand <[email protected]>
adarshan-intel
force-pushed
the
adarsh/new-sgx-driver
branch
from
ea7435b to
95cc0a5
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkow)
Previously, mkow (Michał Kowalczyk) wrote…
Please Don't Capitalize Every Word In A Sentence :)
Done.
Have updated the commit message and rebased PR
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkow)
|
Can we merge this? |
adarshan-intel
changed the title
|
Any more pending action items on this PR ? |
There was a problem hiding this comment.
Any more pending action items on this PR ?
No action item. Let's just wait a bit for mkow to unblock his last comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkow)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the Changes
This pull request updates the Gramine Shielded Containers (GSC) to comply with the new SGX driver requirements.
As part of the gramineproject/gramine#2061 in the Gramine master branch, the options -Dsgx_driver, -Dsgx_driver_include_path, and -Dsgx_driver_device have been removed. Similar changes should be backported into GSC as well.
It includes modifications to the Dockerfile and entry point templates for CentOS and Debian. Additionally, it removes the
-Dsgx_driverand-Dsgx_driver_include_pathoptions when building Gramine inside the container. For further details, please refer to gramine PR #2061Previously, we explicitly used to locate the
sgx.hpackage in/gramine/driverusingDsgx_driver_include_path.However, we are now installing additional package
linux-libc-devthat provides sgx.h out of the box.Fixes #227
How to Test This PR
Continuous Integration (CI)
This change is