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

Adding gramine-manifest-check before signing container #223

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DukeDavis12
Copy link

@DukeDavis12 DukeDavis12 commented Nov 5, 2024

Adding gramine-manifest-check before signing container.

Description of the changes

Adding gramine-manifest-check before signing container. Fixes issue #219.

How to test this PR?


This change is Reviewable

  Adding gramine-manifest-check before signing container.

Signed-off-by: Davis Benny <[email protected]>
@DukeDavis12 DukeDavis12 marked this pull request as draft November 5, 2024 16:14
Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Did you test this, or you just edited, committed, and sent a PR without even running it a single time?

Reviewable status: 0 of 1 files reviewed, all discussions resolved, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), one-liner is not in imperative mood

@DukeDavis12
Copy link
Author

DukeDavis12 commented Nov 6, 2024

@woju Don't you see the draft status on PR ?

Testing is in progress; Plus, I am ramping on Gramine / TDX.

Will mark the PR "Ready for review"; once one complete round of testing is done.

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

No, sorry, I didn't see it, it wasn't in email notification and also missed it in Reviewable, because it's only on the very top.

Reviewable status: 0 of 1 files reviewed, all discussions resolved, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), one-liner is not in imperative mood

@DukeDavis12 DukeDavis12 marked this pull request as ready for review November 11, 2024 21:02
@mkow
Copy link
Member

mkow commented Nov 11, 2024

@DukeDavis12: Please don't paste screenshots of text...

@woju: Could you review this now?

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r2.
Reviewable status: all 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: ), one-liner is not in imperative mood (waiting on @DukeDavis12)


templates/entrypoint.common.manifest.template line 15 at r2 (raw file):

fs.start_dir = "{{working_dir}}"
fs.mounts = [
]

Why this is needed?


templates/debian/Dockerfile.build.template line 54 at r2 (raw file):

{% endblock %}

{% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib -type d -path '*/site-packages')" &&{% endblock %}

I feel like there's too much duplication of this PATH, between different distros and also between different templates (build vs sign), which you've surely saw (I suppose that's why you moved this to build and not sign). Isn't there any way to specify this only twice in the whole project, once for debian-based distros and once for rhel-based?


templates/centos/Dockerfile.build.template line 42 at r2 (raw file):

{% endblock %}

{% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib64 -type d -path '*/site-packages')" &&{% endblock %}

no newline at EOF


templates/redhat/ubi-minimal/Dockerfile.build.template line 50 at r2 (raw file):

{% endblock %}

{% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib64 -type d -path '*/site-packages')" &&{% endblock %}

no newline at EOF

@DukeDavis12
Copy link
Author

DukeDavis12 commented Nov 27, 2024

Reviewed 10 of 10 files at r2.
Reviewable status: all 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: ), one-liner is not in imperative mood (waiting on @DukeDavis12)

templates/entrypoint.common.manifest.template line 15 at r2 (raw file):

fs.start_dir = "{{working_dir}}"
fs.mounts = [
]

Why this is needed?

templates/debian/Dockerfile.build.template line 54 at r2 (raw file):

{% endblock %}

{% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib -type d -path '*/site-packages')" &&{% endblock %}

I feel like there's too much duplication of this PATH, between different distros and also between different templates (build vs sign), which you've surely saw (I suppose that's why you moved this to build and not sign). Isn't there any way to specify this only twice in the whole project, once for debian-based distros and once for rhel-based?

templates/centos/Dockerfile.build.template line 42 at r2 (raw file):

{% endblock %}

{% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib64 -type d -path '*/site-packages')" &&{% endblock %}

no newline at EOF

  • Updated.

templates/redhat/ubi-minimal/Dockerfile.build.template line 50 at r2 (raw file):

{% endblock %}

{% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib64 -type d -path '*/site-packages')" &&{% endblock %}

no newline at EOF

  • Updated.

Signed-off-by: Davis Benny <[email protected]>
Copy link

@chiache chiache left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 10 files at r2, 3 of 4 files at r5, 1 of 2 files at r6, 2 of 2 files at r7.
Reviewable status: 10 of 11 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), one-liner is not in imperative mood (waiting on @DukeDavis12 and @woju)


finalize_manifest.py line 161 at r7 (raw file):

    if 'uri' not in rendered_manifest_dict['loader']['entrypoint']:
        rendered_manifest_dict['loader']['entrypoint']['uri'] = "file:/gramine/meson_build_output/lib/x86_64-linux-gnu/gramine/libsysdb.so"

Why not add /gramine/meson_build_output/lib64/gramine/runtime/glibc to loader.env.LD_LIBRARY_PATH in the script?


templates/debian/entrypoint.manifest.template line 7 at r7 (raw file):

# Add "/usr/lib/x86_64-linux-gnu" explicitly because ldconfig in Ubuntu 21.04 doesn't
# produce it; note that this Debian template is used by Ubuntu templates as well
loader.env.LD_LIBRARY_PATH = "/gramine/meson_build_output/lib/x86_64-linux-gnu/gramine/runtime/glibc:/usr/lib/x86_64-linux-gnu:{{"{{library_paths}}"}}"

ditto


templates/centos/entrypoint.manifest.template line 5 at r7 (raw file):

{% block loader %}
loader.env.LD_LIBRARY_PATH = "/gramine/meson_build_output/lib64/gramine/runtime/glibc:/usr/lib64:{{"{{library_paths}}"}}"
{% endblock %}

ditto

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.

4 participants