Add ubi8, ubi8-minimal support #168
Conversation
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @sahason)
-- commits line 2 at r1:
-> UBI8
Code quote:
rhel8
gsc.py line 221 at r1 (raw file):
distro, _ = distro.split(':') distro = distro.split('/')[0]
Shouldn't the distro name be ubi instead of redhat?
Code quote:
distro = distro.split('/')[0]templates/redhat/Dockerfile.build.template line 11 at r1 (raw file):
{% set distro = Distro.split(':') %} {% set pkg_manager = 'dnf' %} {% if distro[0] == "redhat/ubi8" %}
Shouldn't it be ubi? What if we support e.g., ubi9 in the future?
Code quote:
"redhat/ubi8"
templates/redhat/Dockerfile.build.template line 19 at r1 (raw file):
&& dnf config-manager --add-repo http://vault.centos.org/centos/8/AppStream/x86_64/os {% else %} {% set pkg_manager = 'microdnf' %}
I guess the current ELSE applies to ubi-minimal only? If so, I suggest to make it explicit since there are also ubi-init and ubi-micro images.
Code quote:
{% else %}
{% set pkg_manager = 'microdnf' %}
templates/redhat/Dockerfile.build.template line 20 at r1 (raw file):
{% else %} {% set pkg_manager = 'microdnf' %} RUN rpm -ivh https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm
superfluous leading space
Code quote:
rpm
templates/redhat/Dockerfile.compile.template line 18 at r1 (raw file):
{% else %} {% set pkg_manager = 'microdnf' %} RUN rpm -ivh https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm
ditto
Code quote:
rpm
sahason
force-pushed
the
sahason/add-rhel8-support
branch
from
8036f45 to
007427b
Compare
There was a problem hiding this comment.
Reviewable status: 6 of 8 files reviewed, 6 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 @kailun-qin)
Previously, kailun-qin (Kailun Qin) wrote…
->
UBI8
Modified the commit message
templates/redhat/Dockerfile.build.template line 11 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Shouldn't it be
ubi? What if we support e.g.,ubi9in the future?
The link like https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm may change for ubi9 as this is specific to a version. As this PR is supports ubi8 the scope is limited to ubi8.
templates/redhat/Dockerfile.build.template line 19 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I guess the current ELSE applies to
ubi-minimalonly? If so, I suggest to make it explicit since there are alsoubi-initandubi-microimages.
Done.
templates/redhat/Dockerfile.build.template line 20 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
superfluous leading space
Done.
templates/redhat/Dockerfile.compile.template line 18 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
ditto
Done.
There was a problem hiding this comment.
Reviewed 6 of 8 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 15 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 @kailun-qin and @sahason)
a discussion (no related file):
Please also update the documentation correspondingly.
a discussion (no related file):
@woju @mkow What do we do with such PRs? Do we want to support these RedHat UBI and RedHat UBI Minimal?
gsc.py line 221 at r2 (raw file):
distro, _ = distro.split(':') distro = distro.split('/')[0]
This is definitely wrong. This extracts redhat instead of ubi.
Why don't you like redhat/ubi8? This is actually very convenient: you'll just create your Dockerfile templates under the two-layered directories templates/redhat/ubi8/ and templates/redhat/ubi8-minimal/ (the latter will just have extends keywords to the former).
gsc.py line 286 at r2 (raw file):
# Available at https://www.centos.org/keys/RPM-GPG-KEY-CentOS-Official shutil.copyfile('keys/RPM-GPG-KEY-CentOS-Official', tmp_build_path / 'RPM-GPG-KEY-CentOS-Official')
I don't like this; we'll have to add keys for every single distro in our generic GSC scripts...
Instead, I recommend to copy the whole directory keys/ instead of copying each key. Plus you can move these comments about keys in a new file keys/README.
gsc.py line 350 at r2 (raw file):
# Available at https://www.centos.org/keys/RPM-GPG-KEY-CentOS-Official shutil.copyfile('keys/RPM-GPG-KEY-CentOS-Official', tmp_build_path / 'RPM-GPG-KEY-CentOS-Official')
ditto
keys/RPM-GPG-KEY-CentOS-Official line 1 at r2 (raw file):
-----BEGIN PGP PUBLIC KEY BLOCK-----
The key looks correct.
templates/redhat/Dockerfile.build.template line 11 at r1 (raw file):
Previously, sahason wrote…
The link like https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm may change for ubi9 as this is specific to a version. As this PR is supports ubi8 the scope is limited to ubi8.
I'm fine with currently hard-coding to ubi8 (i.e. to the exact version). This can be easily fixed in the future if needed.
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
&& echo "enabled=1" >> /etc/yum.repos.d/ubi.repo \ && echo "gpgcheck=1" >> /etc/yum.repos.d/ubi.repo \ && echo "gpgkey=file:///RPM-GPG-KEY-CentOS-Official" >> /etc/yum.repos.d/ubi.repo
Please use multi-line cat instead of this ugliness: https://stackoverflow.com/a/10970616
templates/redhat/Dockerfile.build.template line 68 at r2 (raw file):
strace {% endif %} {% endblock %}
Sorry but this is unreadable to me. Why don't you want to split into multiple files, like this:
templates/redhat/ubi8/Dockerfile.build.templatetemplates/redhat/ubi8-minimal/Dockerfile.build.template
Yes, you'll have some duplication, but it will be much easier to comprehend than this kung-fu with Jinja variables.
templates/redhat/Dockerfile.compile.template line 39 at r2 (raw file):
&& echo "enabled=1" >> /etc/yum.repos.d/ubi.repo \ && echo "gpgcheck=1" >> /etc/yum.repos.d/ubi.repo \ && echo "gpgkey=file:///RPM-GPG-KEY-CentOS-Official" >> /etc/yum.repos.d/ubi.repo
Please use multi-line cat instead of this ugliness: https://stackoverflow.com/a/10970616
templates/redhat/Dockerfile.compile.template line 75 at r2 (raw file):
&& /usr/bin/python3 -B -m pip install 'tomli>=1.1.0' 'tomli-w>=0.4.0' 'meson>=0.56,!=1.2.*' {% endblock %}
ditto
There was a problem hiding this comment.
Reviewable status: all files reviewed, 15 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 @kailun-qin, @mkow, and @woju)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@woju @mkow What do we do with such PRs? Do we want to support these RedHat UBI and RedHat UBI Minimal?
Partially fixes #162 where customer's base image is ubi8-minimal.
There was a problem hiding this comment.
Reviewed 5 of 8 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 17 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 @dimakuv, @kailun-qin, @mkow, and @sahason)
a discussion (no related file):
Previously, sahason wrote…
Partially fixes #162 where customer's base image is ubi8-minimal.
UBI is RHEL packaged for containers (without kernel). If we already support RHEL family (centos), then I don't mind yet another flavour, but with usual reservation that if RH tries yet another trick to screw us and the community (that would impact us and/or our ability to test this), we might pull the support without warning or even just abandon it, stop accepting bugs and let it rot.
a discussion (no related file):
To sum up all the different questions about redhat/ vs rhel vs ubi vs ubi8 etc., I'd like a general guideline somewhere about how we name thing and how we keep them consistent. I don't particularly care where and how (might be in Documentation/ or in README somewhere or in yet another place), and also don't care much how we name it (e.g. we might just defer to docker.io naming convention), but I want a clear guideline and consistency.
gsc.py line 221 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is definitely wrong. This extracts
redhatinstead ofubi.Why don't you like
redhat/ubi8? This is actually very convenient: you'll just create your Dockerfile templates under the two-layered directoriestemplates/redhat/ubi8/andtemplates/redhat/ubi8-minimal/(the latter will just haveextendskeywords to the former).
I guess it's really the same problem that Kailun pointed out above. I agree with both.
gsc.py line 286 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't like this; we'll have to add keys for every single distro in our generic GSC scripts...
Instead, I recommend to copy the whole directory
keys/instead of copying each key. Plus you can move these comments about keys in a new filekeys/README.
Generally agree, but you need to keep in mind that different distros have different keys, formats and tooling around them. I guess if you install them one by one in the template, then it's OK.
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
RUN rpm --import /RPM-GPG-KEY-CentOS-Official \ && dnf config-manager --add-repo http://vault.centos.org/centos/8/BaseOS/x86_64/os \
Mixing CentOS' BaseOS with UBI repos is wrong. AppStream might be correct if you need some packages from there, but definitely not BaseOS, and if it doesn't work without it, then we should find another solution.
Aren't those packages (that you've mentioned in PR description) available from EPEL?
templates/redhat/Dockerfile.build.template line 68 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Sorry but this is unreadable to me. Why don't you want to split into multiple files, like this:
templates/redhat/ubi8/Dockerfile.build.templatetemplates/redhat/ubi8-minimal/Dockerfile.build.templateYes, you'll have some duplication, but it will be much easier to comprehend than this kung-fu with Jinja variables.
Yes, and if you have some common areas, you can write {{ super() }} inside a block, which substitutes the entirety of the block from parrent template. Or you can start writing macros, if you need to parametrise it.
There was a problem hiding this comment.
Reviewable status: 2 of 14 files reviewed, 17 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 @dimakuv, @kailun-qin, @mkow, and @woju)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please also update the documentation correspondingly.
Done.
gsc.py line 221 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I guess it's really the same problem that Kailun pointed out above. I agree with both.
Created two separate Dockerfile templates.
gsc.py line 286 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Generally agree, but you need to keep in mind that different distros have different keys, formats and tooling around them. I guess if you install them one by one in the template, then it's OK.
Keeping it as it is as the key is copied in the template based on the distro.
gsc.py line 350 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Keeping it as it is as the key is copied in the template based on the distro.
templates/redhat/Dockerfile.build.template line 11 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'm fine with currently hard-coding to
ubi8(i.e. to the exact version). This can be easily fixed in the future if needed.
Kept it as it is.
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Mixing CentOS' BaseOS with UBI repos is wrong. AppStream might be correct if you need some packages from there, but definitely not BaseOS, and if it doesn't work without it, then we should find another solution.
Aren't those packages (that you've mentioned in PR description) available from EPEL?
These packages are not available from EPEL repo. Could not found another solution to do this.
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please use multi-line
catinstead of this ugliness: https://stackoverflow.com/a/10970616
Here documents with cat syntax work fine in Dockerfile. But in GSC when I added the same syntax in jinja2 template I ended up with this warning while building the image -bin/sh: warning: here-document at line 0 delimited by end-of-file (wanted \EOF). I have tried different syntax but everything ended up in the same warning and the repo configuration file is not updated. Here is an alternate solution with echo command which works fine.
"RUN echo -e "\n
[CentOSBaseOS] \n
name = CentOSBaseOS \n
baseurl = http://vault.centos.org/centos/8/BaseOS/x86_64/os \n
enable = 1 \n
gpgcheck = 1 \n
gpgkey = file:///RPM-GPG-KEY-CentOS-Official \n
" >> /etc/yum.repos.d/ubi.repo"
Hence proposing this solution.
templates/redhat/Dockerfile.build.template line 68 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Yes, and if you have some common areas, you can write
{{ super() }}inside a block, which substitutes the entirety of the block from parrent template. Or you can start writing macros, if you need to parametrise it.
Created separete files.
templates/redhat/Dockerfile.compile.template line 39 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please use multi-line
catinstead of this ugliness: https://stackoverflow.com/a/10970616
Here documents with cat syntax work fine in Dockerfile. But in GSC when I added the same syntax in jinja2 template I ended up with this warning while building the image -bin/sh: warning: here-document at line 0 delimited by end-of-file (wanted \EOF). I have tried different syntax but everything ended up in the same warning and the repo configuration file is not updated. Here is an alternate solution with echo command which works fine.
"RUN echo -e "\n
[CentOSBaseOS] \n
name = CentOSBaseOS \n
baseurl = http://vault.centos.org/centos/8/BaseOS/x86_64/os \n
enable = 1 \n
gpgcheck = 1 \n
gpgkey = file:///RPM-GPG-KEY-CentOS-Official \n
" >> /etc/yum.repos.d/ubi.repo"
Hence proposing this solution.
templates/redhat/Dockerfile.compile.template line 75 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done
There was a problem hiding this comment.
Reviewed 11 of 12 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 22 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 @kailun-qin, @sahason, and @woju)
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
UBI is RHEL packaged for containers (without kernel). If we already support RHEL family (centos), then I don't mind yet another flavour, but with usual reservation that if RH tries yet another trick to screw us and the community (that would impact us and/or our ability to test this), we might pull the support without warning or even just abandon it, stop accepting bugs and let it rot.
Ok, let's have it.
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
To sum up all the different questions about
redhat/vsrhelvsubivsubi8etc., I'd like a general guideline somewhere about how we name thing and how we keep them consistent. I don't particularly care where and how (might be inDocumentation/or in README somewhere or in yet another place), and also don't care much how we name it (e.g. we might just defer to docker.io naming convention), but I want a clear guideline and consistency.
Well, yes, we should put the guideline somewhere (I'd say mention in README).
We use the Dockerhub naming convention historically, so I guess that's what it is.
templates/redhat/ubi8/Dockerfile.build.template line 27 at r5 (raw file):
python3-cryptography \ python3-pip \ python3-protobuf \
Please sort this list
templates/redhat/ubi8/Dockerfile.build.template line 28 at r5 (raw file):
python3-pip \ python3-protobuf \ && /usr/bin/python3 -B -m pip install click jinja2 protobuf \
Why do you have protobuf here, even though you already have python3-protobuf above? Seems like duplication of the same package.
templates/redhat/ubi8/Dockerfile.build.template line 32 at r5 (raw file):
&& dnf repolist \ # Install pyelftools after the installation of epel-release as it is provided by the EPEL repo && dnf install -y python3-pyelftools \
Looks like you don't need this special treatment of python3-pyelftools, because you install epel in a completely separate step? Please remove the comment and move this package together with the other packages.
templates/redhat/ubi8/Dockerfile.build.template line 41 at r5 (raw file):
libunwind \ python3-pytest \ strace
Did you also verify that your PR works with gsc build -b debug?
templates/redhat/ubi8/Dockerfile.compile.template line 21 at r5 (raw file):
curl \ elfutils-libelf-devel \ epel-release \
? Didn't you already install epel-release by the very first command?
Please explain why you even have the first command, if you can install epel-release in a normal way?
templates/redhat/ubi8-minimal/Dockerfile.sign.template line 5 at r5 (raw file):
{% block uninstall %} RUN \ pip3 uninstall -y click jinja2 protobuf \
ditto (why do you have two protobuf)
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
Previously, sahason wrote…
Here documents with
catsyntax work fine inDockerfile. But in GSC when I added the same syntax in jinja2 template I ended up with this warning while building the image -bin/sh: warning: here-document at line 0 delimited by end-of-file (wanted \EOF). I have tried different syntax but everything ended up in the same warning and the repo configuration file is not updated. Here is an alternate solution withechocommand which works fine.
"RUN echo -e "\n
[CentOSBaseOS] \n
name = CentOSBaseOS \n
baseurl = http://vault.centos.org/centos/8/BaseOS/x86_64/os \n
enable = 1 \n
gpgcheck = 1 \n
gpgkey = file:///RPM-GPG-KEY-CentOS-Official \n
" >> /etc/yum.repos.d/ubi.repo"
Hence proposing this solution.
I like the alternate solution better, please use it then.
templates/redhat/Dockerfile.compile.template line 39 at r2 (raw file):
Previously, sahason wrote…
Here documents with
catsyntax work fine inDockerfile. But in GSC when I added the same syntax in jinja2 template I ended up with this warning while building the image -bin/sh: warning: here-document at line 0 delimited by end-of-file (wanted \EOF). I have tried different syntax but everything ended up in the same warning and the repo configuration file is not updated. Here is an alternate solution withechocommand which works fine.
"RUN echo -e "\n
[CentOSBaseOS] \n
name = CentOSBaseOS \n
baseurl = http://vault.centos.org/centos/8/BaseOS/x86_64/os \n
enable = 1 \n
gpgcheck = 1 \n
gpgkey = file:///RPM-GPG-KEY-CentOS-Official \n
" >> /etc/yum.repos.d/ubi.repo"
Hence proposing this solution.
ditto (use the alternate solution)
templates/redhat/ubi8-minimal/Dockerfile.build.template line 6 at r5 (raw file):
# Combine all installation and removal steps in a single RUN command to reduce the final image size. # This is because each Dockerfile command creates a new layer which necessarily adds size to the # final image. This trick allow
Sentence is not finished.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 38 at r5 (raw file):
python3-protobuf \ findutils \ which \
Please sort
templates/redhat/ubi8-minimal/Dockerfile.build.template line 39 at r5 (raw file):
findutils \ which \ && /usr/bin/python3 -B -m pip install click jinja2 protobuf \
ditto (why protobuf twice)
templates/redhat/ubi8-minimal/Dockerfile.build.template line 43 at r5 (raw file):
&& microdnf repolist \ # Install pyelftools after the installation of epel-release as it is provided by the EPEL repo && microdnf install -y python3-pyelftools \
ditto (you installed epel-release in a separate command)
templates/redhat/ubi8-minimal/Dockerfile.build.template line 52 at r5 (raw file):
libunwind \ python3-pytest \ strace
ditto (does it work in debug mode?)
templates/redhat/ubi8-minimal/Dockerfile.compile.template line 36 at r5 (raw file):
curl \ elfutils-libelf-devel \ epel-release \
ditto (why two installations?)
There was a problem hiding this comment.
Reviewable status: 9 of 14 files reviewed, 22 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 @dimakuv, @kailun-qin, and @woju)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Well, yes, we should put the guideline somewhere (I'd say mention in README).
We use the Dockerhub naming convention historically, so I guess that's what it is.
Will this be part of this PR or separate PR?
gsc.py line 221 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Shouldn't the distro name be
ubiinstead ofredhat?
Distros uesd are redhat/ubi8 and redhat/ubi8-minimal.
templates/redhat/ubi8/Dockerfile.build.template line 27 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please sort this list
Done.
templates/redhat/ubi8/Dockerfile.build.template line 28 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you have
protobufhere, even though you already havepython3-protobufabove? Seems like duplication of the same package.
Yes, after deleting this package build is fine so removed it.
templates/redhat/ubi8/Dockerfile.build.template line 32 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Looks like you don't need this special treatment of
python3-pyelftools, because you install epel in a completely separate step? Please remove the comment and move this package together with the other packages.
Done
templates/redhat/ubi8/Dockerfile.build.template line 41 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Did you also verify that your PR works with
gsc build -b debug?
Yes
templates/redhat/ubi8/Dockerfile.compile.template line 21 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
? Didn't you already install
epel-releaseby the very first command?Please explain why you even have the first command, if you can install epel-release in a normal way?
My mistake. First command is required for installing epel-release. In absence of first commad and presense of second command it will throw Error: Unable to find a match: epel-release. But in presencse of both it gives Package epel-release-8-19.el8.noarch is already installed.
templates/redhat/ubi8-minimal/Dockerfile.sign.template line 5 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (why do you have two
protobuf)
Done.
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I like the alternate solution better, please use it then.
Done.
templates/redhat/Dockerfile.compile.template line 39 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (use the alternate solution)
Done.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 6 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Sentence is not finished.
Done.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 38 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please sort
Done.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 39 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (why
protobuftwice)
Done.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 43 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (you installed epel-release in a separate command)
Done.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 52 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (does it work in debug mode?)
Yes
templates/redhat/ubi8-minimal/Dockerfile.compile.template line 36 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (why two installations?)
My mistake. First command is required for installing epel-release. In absence of first commad and presense of secone command it will throw Error: Unable to find a match: epel-release. But in presencse of both it gives Package epel-release-8-19.el8.noarch is already installed.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r6, 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 @kailun-qin, @sahason, and @woju)
a discussion (no related file):
Previously, sahason wrote…
Will this be part of this PR or separate PR?
Separate PR, not this PR.
templates/redhat/ubi8/Dockerfile.compile.template line 21 at r5 (raw file):
Previously, sahason wrote…
My mistake. First command is required for installing epel-release. In absence of first commad and presense of second command it will throw
Error: Unable to find a match: epel-release. But in presencse of both it givesPackage epel-release-8-19.el8.noarch is already installed.
Ok, thanks for explanation.
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
Previously, sahason wrote…
Done.
Thanks, more readable now.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 6 at r6 (raw file):
# Combine all installation and removal steps in a single RUN command to reduce the final image size. # This is because each Dockerfile command creates a new layer which necessarily adds size to the # final image. This trick allow to decrease the image size by hundreds of MBs.
allow -> allows
There was a problem hiding this comment.
Reviewable status: 12 of 14 files reviewed, 10 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 @dimakuv, @kailun-qin, and @woju)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Separate PR, not this PR.
Ok.
templates/redhat/ubi8-minimal/Dockerfile.sign.template line 11 at r6 (raw file):
expect \ openssl \ pkgconfig \
I updated to keep it consistent with Centos and Debian sign templates as these are already tested with many workloads.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 6 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
allow->allows
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @sahason, and @woju)
templates/redhat/ubi8-minimal/Dockerfile.sign.template line 11 at r6 (raw file):
Previously, sahason wrote…
I updated to keep it consistent with Centos and Debian sign templates as these are already tested with many workloads.
Good. Deleting lines is always good :)
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 10 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 @dimakuv, @kailun-qin, and @woju)
templates/redhat/ubi8-minimal/Dockerfile.sign.template line 12 at r7 (raw file):
openssl \ python3-cryptography \ python3-pip \
Sorry, had to remove this as well as signing with --remove-gramine-depscaused issue.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @woju)
There was a problem hiding this comment.
Reviewed 4 of 12 files at r3, 1 of 5 files at r6, 1 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)
There was a problem hiding this comment.
Reviewed 7 of 12 files at r3, 3 of 5 files at r6, 1 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
a discussion (no related file):
Previously, sahason wrote…
Ok.
At least please describe anywhere (even here in the replying comment) what schema was guiding choices in this PR.
templates/redhat/ubi8/Dockerfile.build.template line 6 at r8 (raw file):
# Combine all installation and removal steps in a single RUN command to reduce the final image size. # This is because each Dockerfile command creates a new layer which necessarily adds size to the # final image. This trick allows to decrease the image size by hundreds of MBs.
This comment sounds misplaced. 1) Was it copied from another template (for another distro) and just left alone? 2) Shouldn't we actually follow the comment, combining all dnf and rpm commands below the COPY?
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
COPY RPM-GPG-KEY-CentOS-Official / RUN dnf install -y https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm \
EPEL docs (https://docs.fedoraproject.org/en-US/epel/#_rhel_8) say you need to also enable codeready repo. Why this instruction is not followed?
templates/redhat/ubi8/Dockerfile.build.template line 14 at r8 (raw file):
RUN rpm --import /RPM-GPG-KEY-CentOS-Official \ && dnf config-manager --add-repo http://vault.centos.org/centos/8/BaseOS/x86_64/os \ && dnf config-manager --add-repo http://vault.centos.org/centos/8/AppStream/x86_64/os
Why do we add PowerTools in Dockerfile.compile.template and not here?
Unless there is a compelling reason to not have some repo available (e.g., a conflict between some packages/repos?), I wouls like to avoid such differences between containers, and I would prefer if there was a common file with a single macro to install dependencies (one per distro I guess), {% import %}ed or {% extend %}ed from.
templates/redhat/ubi8/Dockerfile.compile.template line 6 at r8 (raw file):
COPY RPM-GPG-KEY-CentOS-Official / RUN dnf install -y https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm \
ditto (question about combining RUN steps, if applicable, depending on the discussion in the other thread).
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
Previously, sahason wrote…
These packages are not available from EPEL repo. Could not found another solution to do this.
Do I understand correctly: it works on ubi* and doesn't work on ubi*-minimal? If so, then we don't support ubi*-minimal.
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Thanks, more readable now.
I don't like this. I think if you can't cat <<EOF those, then commit them as separate files and COPY them like the key.
templates/redhat/Dockerfile.compile.template line 39 at r2 (raw file):
Previously, sahason wrote…
Done.
ditto (COPY)
There was a problem hiding this comment.
Reviewable status: 9 of 15 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 @dimakuv, @kailun-qin, and @woju)
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
At least please describe anywhere (even here in the replying comment) what schema was guiding choices in this PR.
docker.io naming convection [registry]/[namespace]/[repository]:[tag] is followed.
In this PR:
registry - Default registry Docker Hub
namespace - redhat
repository - ubi8 and ubi8-minimal
tag - 8.8
templates/redhat/ubi8/Dockerfile.build.template line 6 at r8 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This comment sounds misplaced. 1) Was it copied from another template (for another distro) and just left alone? 2) Shouldn't we actually follow the comment, combining all
dnfandrpmcommands below theCOPY?
Addressed 2nd point.
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
EPEL docs (https://docs.fedoraproject.org/en-US/epel/#_rhel_8) say you need to also enable codeready repo. Why this instruction is not followed?
It throws error when the instruction is added as subscription-manager is disabled when running inside a container. Please refer to your host system for subscription management. while building GSC image.
templates/redhat/ubi8/Dockerfile.build.template line 14 at r8 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Why do we add PowerTools in
Dockerfile.compile.templateand not here?Unless there is a compelling reason to not have some repo available (e.g., a conflict between some packages/repos?), I wouls like to avoid such differences between containers, and I would prefer if there was a common file with a single macro to install dependencies (one per distro I guess),
{% import %}ed or{% extend %}ed from.
It is required for nasm package.
templates/redhat/ubi8/Dockerfile.compile.template line 6 at r8 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
ditto (question about combining
RUNsteps, if applicable, depending on the discussion in the other thread).
Done
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Do I understand correctly: it works on
ubi*and doesn't work onubi*-minimal? If so, then we don't supportubi*-minimal.
It does not work on both.
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I don't like this. I think if you can't
cat <<EOFthose, then commit them as separate files andCOPYthem like the key.
Created separate file.
templates/redhat/Dockerfile.compile.template line 39 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
ditto (
COPY)
Done
There was a problem hiding this comment.
Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)
templates/redhat/ubi8/Dockerfile.build.template line 14 at r8 (raw file):
Not a real comment from me, but more a history of GSC development:
Unless there is a compelling reason to not have some repo available (e.g., a conflict between some packages/repos?)
Actually, the original idea behind two separate phases of Gramine + app build (Dockerfile.compile.template and Dockerfile.build.template) was: the first phase builds/installs Gramine proper and the second phase installs the run-time tools and dependencies required for Gramine SGX enclave execution.
So the first phase was supposed to be interchangeable: one could build Gramine from sources (as is done currently in Gramine) or install Gramine from the packages (like our official Gramine DockerHub image: https://github.com/gramineproject/gramine/blob/master/packaging/docker/Dockerfile).
The "install Gramine from the packages" idea never materialized in GSC (well, not yet, but probably never). Now, if you imagine that this would be implemented, you'll see that Dockerfile.compile.template and Dockerfile.build.template would be drastically different and wouldn't have much shareable code.
That was the original idea and that explains why we have a lot of duplication of code between the two files/phases. I don't know if that ship has sailed, or we'll indeed allow the "install from packages" step in GSC...
I would prefer if there was a common file with a single macro to install dependencies
If we would have Dockerfile.compile.template containing the "install from packages" logic, then there will be a lot of unused but installed dependencies (e.g. meson), which would break the principle of minimality. Again, that was the original idea :)
templates/redhat/Dockerfile.build.template line 34 at r2 (raw file):
Previously, sahason wrote…
Created separate file.
Right, this actually looks much cleaner! Thanks Woju for the idea.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @woju)
templates/redhat/ubi8/Dockerfile.build.template line 14 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not a real comment from me, but more a history of GSC development:
Unless there is a compelling reason to not have some repo available (e.g., a conflict between some packages/repos?)
Actually, the original idea behind two separate phases of Gramine + app build (
Dockerfile.compile.templateandDockerfile.build.template) was: the first phase builds/installs Gramine proper and the second phase installs the run-time tools and dependencies required for Gramine SGX enclave execution.So the first phase was supposed to be interchangeable: one could build Gramine from sources (as is done currently in Gramine) or install Gramine from the packages (like our official Gramine DockerHub image: https://github.com/gramineproject/gramine/blob/master/packaging/docker/Dockerfile).
The "install Gramine from the packages" idea never materialized in GSC (well, not yet, but probably never). Now, if you imagine that this would be implemented, you'll see that
Dockerfile.compile.templateandDockerfile.build.templatewould be drastically different and wouldn't have much shareable code.That was the original idea and that explains why we have a lot of duplication of code between the two files/phases. I don't know if that ship has sailed, or we'll indeed allow the "install from packages" step in GSC...
I would prefer if there was a common file with a single macro to install dependencies
If we would have
Dockerfile.compile.templatecontaining the "install from packages" logic, then there will be a lot of unused but installed dependencies (e.g.meson), which would break the principle of minimality. Again, that was the original idea :)
Just a quick note: as a workaround, we can build nasm manually from https://github.com/netwide-assembler/nasm, it is trivial to build and the build itself doesn't require any dependencies.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r7, 6 of 6 files at r9, all commit messages.
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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
a discussion (no related file):
Previously, sahason wrote…
docker.io naming convection [registry]/[namespace]/[repository]:[tag] is followed.
In this PR:
registry - Default registry Docker Hub
namespace - redhat
repository - ubi8 and ubi8-minimal
tag - 8.8
Thanks, that's OK. Filenames look correct.
templates/redhat/ubi8/Dockerfile.build.template line 6 at r8 (raw file):
Previously, sahason wrote…
Addressed 2nd point.
Yep, thanks. Moving COPY on top is Good™.
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, sahason wrote…
It throws error when the instruction is added as
subscription-manager is disabled when running inside a container. Please refer to your host system for subscription management.while building GSC image.
I guess you need to configure those subscriptions on a host then. Isn't it actually a correct solution, and a missing point in readme/docs that to run UBI images you need to run on properly subscribed (and paid for...) RHEL?
templates/redhat/ubi8/Dockerfile.build.template line 14 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Just a quick note: as a workaround, we can build
nasmmanually from https://github.com/netwide-assembler/nasm, it is trivial to build and the build itself doesn't require any dependencies.
OK, if those images are substantially different then I think it's OK to have them different. Thanks for explanation.
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
Previously, sahason wrote…
It does not work on both.
I strongly protest against including BaseOS repository from CentOS. This is no-go, because if you do that, you're not really running UBI anymore, it's a franken-distro that contains base packages from both CentOS and UBI, and you can't predict what packages will get pulled on e.g. dnf install/update. It's "ubi" in the name only, that is, FROM redhat/ubi8 becomes a lie at this point. I guess CentOS' AppStream and PowerTools depend on BaseOS, so it might be that we can't use them either.
Are those repos really unavailable for UBI? Note that Red Hat messes regularly with names of those repos. About 20 s of ddg-ing found me this:
- https://access.redhat.com/discussions/5417621
- https://access.redhat.com/articles/4238681
(the terms were 'ubi powertoolsandubi appstream`). It seems implausible to me that those would be unavailable.
nasm is available there: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/package_manifest/codereadylinuxbuilder-repository, but I can't find expect nor protobuf-c-devel (^F doesn't find that package on this docs page), but I guess are available from appstream and/or epel?
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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I strongly protest against including BaseOS repository from CentOS. This is no-go, because if you do that, you're not really running UBI anymore, it's a franken-distro that contains base packages from both CentOS and UBI, and you can't predict what packages will get pulled on e.g.
dnf install/update. It's "ubi" in the name only, that is,FROM redhat/ubi8becomes a lie at this point. I guess CentOS' AppStream and PowerTools depend on BaseOS, so it might be that we can't use them either.Are those repos really unavailable for UBI? Note that Red Hat messes regularly with names of those repos. About 20 s of ddg-ing found me this:
- https://access.redhat.com/discussions/5417621
- https://access.redhat.com/articles/4238681
(the terms were 'ubi powertoolsandubi appstream`). It seems implausible to me that those would be unavailable.
nasmis available there: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/package_manifest/codereadylinuxbuilder-repository, but I can't findexpectnorprotobuf-c-devel(^F doesn't find that package on this docs page), but I guess are available from appstream and/or epel?
ubi-8-appstream-rpms, ubi-8-baseos-rpms and ubi-8-codeready-builder-rpms are already present with ubi8 and ubi8-minimal images. But the missing packages are not available in these repos. But there is no powertools in this link https://access.redhat.com/articles/4238681.
To get https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/package_manifest/codereadylinuxbuilder-repository, package we will need redhat subscription.
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), "fixup! " found in commit messages' one-liners (waiting on @sahason and @woju)
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
Previously, sahason wrote…
ubi-8-appstream-rpms,ubi-8-baseos-rpmsandubi-8-codeready-builder-rpmsare already present with ubi8 and ubi8-minimal images. But the missing packages are not available in these repos. But there is nopowertoolsin this link https://access.redhat.com/articles/4238681.To get https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/package_manifest/codereadylinuxbuilder-repository, package we will need redhat subscription.
Two notes from me:
- I can see
nasmandprotobuf-devel(note! slight difference in name fromprotobuf-c-devel, but I think it's the same package that we need) in the CodeReady Linux Builder repository. expectseems to be unavailable in UBI8, see https://stackoverflow.com/questions/63658868/redhat-ubi-8-install-expect-package. However, I also feel like we can get rid of theexpectutility in GSC now.
So if we only have problems with these 3 packages, looks like we can remove expect and rely on CodeReady repo. Which in turn means requiring a RedHat subscription in order for GSC + UBI8 to work...
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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
But there is no
powertoolsin this link https://access.redhat.com/articles/4238681.
I think powertools and codeready-builder is the same repo really, only renamed between major versions. I might be slightly wrong on the details, but the package set is mostly the same.
To get https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/package_manifest/codereadylinuxbuilder-repository, package we will need redhat subscription.
Yeah, so this is kind of expected. If UBI needs subscription, then GSC user needs subscription to use UBI in GSC, right?
So we either
- allow the paid option, merge on a "best effort" basis, warning people that UBI is not supported by Gramine project, because we don't test it as nobody contributed paid access for CI for this distro; or
- we don't merge at all and tell people to use one of the rebuilds (Rocky or AlmaLinux), and if Intel badly wants UBI support delivered, then Intel is free to maintain a fork.
protobuf-devel(note! slight difference in name fromprotobuf-c-devel
@dimakuv You sure this is the same package? In debian there are two of those, protobuf-compiler and protobuf-c-compiler and those are different things.
I have no problem with removing expect, but if we want to do this, it would have to be done either in this PR or another PR merged before this one. And the SO answer you've linked says that it is available in paid repos? So if we merge anyway (and tell people to have subscription if they use UBI), this might not be needed at all.
There was a problem hiding this comment.
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 @sahason)
There was a problem hiding this comment.
Reviewable status: 8 of 15 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: ) (waiting on @kailun-qin, @sahason, and @woju)
a discussion (no related file):
@sahason Is this PR ready for review, or you're still working on it? I see the new commit, but you didn't leave any new messages in Reviewable.
There was a problem hiding this comment.
Reviewable status: 8 of 15 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: ) (waiting on @dimakuv, @kailun-qin, and @woju)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@sahason Is this PR ready for review, or you're still working on it? I see the new commit, but you didn't leave any new messages in Reviewable.
I will update the PR and Reviewable by today.
There was a problem hiding this comment.
Reviewable status: 7 of 15 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 @dimakuv, @kailun-qin, and @woju)
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I guess you need to configure those subscriptions on a host then. Isn't it actually a correct solution, and a missing point in readme/docs that to run UBI images you need to run on properly subscribed (and paid for...) RHEL?
I have updated the PR such that it requires RHEL subscription. But from rhel8 onwards it uses podman-docker and current version of GSC does not support podman-docker and podman python library. When you use podman-docker your redhat.repo in UBI8 base image will automatically be updated with '' rhel-8-for-x86_64-baseos-rpms" and "rhel-8-for-x86_64-appstream-rpms". But this does not happen when you use docker instead podman-docker. As an alternate solution the redhat.repo is copied from host RHEL path /etc/yum.repos.d to the UBI8 image. Then the redhat.repo file is parsed to get path of SSL CA certificate and SSL client key directory and the contents are copied to UBI8 base image.
There was a problem hiding this comment.
Reviewed 6 of 7 files at r10, 2 of 2 files at r11, all commit messages.
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: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, sahason wrote…
I have updated the PR such that it requires RHEL subscription. But from rhel8 onwards it uses
podman-dockerand current version of GSC does not supportpodman-dockerandpodmanpython library. When you usepodman-dockeryourredhat.repoin UBI8 base image will automatically be updated with '' rhel-8-for-x86_64-baseos-rpms" and "rhel-8-for-x86_64-appstream-rpms". But this does not happen when you usedockerinsteadpodman-docker. As an alternate solution theredhat.repois copied from host RHEL path /etc/yum.repos.d to the UBI8 image. Then theredhat.repofile is parsed to get path of SSL CA certificate and SSL client key directory and the contents are copied to UBI8 base image.
Looks OK to me. I guess this is what RH users get when they engage in this RH's DRM. When those regexps break, you can think about rewriting this in python's configparser (https://docs.python.org/3/library/configparser.html), but regexps are OK as long as they work.
Note that IIUC this means that you're shipping private keys to RH subscription with your container. Is this really what you want?
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: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
There was a problem hiding this comment.
Reviewed 5 of 7 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 27 unresolved discussions, 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 (waiting on @sahason and @woju)
gsc.py line 23 at r11 (raw file):
import tomli_w # pylint: disable=import-error import yaml # pylint: disable=import-error import re
Can we move this import to the first set of imports (the one that starts with argparse) and put it there sorted?
gsc.py line 181 at r11 (raw file):
def handle_redhat_repo_configs(distro, tmp_build_path): if distro in {"redhat/ubi8", "redhat/ubi8-minimal"}:
Please do the other way around, for readability of the code:
if distro not in {"redhat/ubi8", "redhat/ubi8-minimal"}:
return
repo_name = "rhel-8-for-x86_64-baseos-rpms"
with open('/etc/yum.repos.d/redhat.repo') as redhat_repo:
...
gsc.py line 186 at r11 (raw file):
redhat_repo_contents = redhat_repo.read() if not re.search(repo_name, redhat_repo_contents):
Why this search is enough? Can you show what is typically located in this file /etc/yum.repos.d/redhat.repo ? And explain the contents of this file?
gsc.py line 188 at r11 (raw file):
if not re.search(repo_name, redhat_repo_contents): print(f'Register and subscribe your RHEL system to the Red Hat Customer ' f'Portal using Red Hat Subscription-Manager.')
Please prepend with a sentence that describes the actual problem:
Cannot find {repo_name} in /etc/yum.repos.d/redhat.repo. Register and subscribe ...
gsc.py line 189 at r11 (raw file):
print(f'Register and subscribe your RHEL system to the Red Hat Customer ' f'Portal using Red Hat Subscription-Manager.') sys.exit(0)
Why 0? It should be 1 -- to indicate an error
gsc.py line 191 at r11 (raw file):
sys.exit(0) # Copy the Red Hat repository configuration file to the temporary build location
Comment just duplicates what the code does. Remove this comment.
gsc.py line 196 at r11 (raw file):
pattern_sslcacert = re.compile(r'(?<!#)sslcacert\s*=\s*(.*)') # Search for and extract the SSL client key path from the repository configuration
Comment just duplicates what the code does. Remove this comment.
gsc.py line 203 at r11 (raw file):
else: print(f'Register and subscribe your RHEL system to the Red Hat Customer ' f'Portal using Red Hat Subscription-Manager.')
Please prepend with a sentence that describes the actual problem:
Cannot find SSL client key path in /etc/yum.repos.d/redhat.repo. Register and subscribe ...
gsc.py line 204 at r11 (raw file):
print(f'Register and subscribe your RHEL system to the Red Hat Customer ' f'Portal using Red Hat Subscription-Manager.') sys.exit(0)
1
gsc.py line 207 at r11 (raw file):
# Search for and extract the SSL CA certificate path from the repository # configuration
Comment just duplicates what the code does. Remove this comment.
gsc.py line 213 at r11 (raw file):
else: print(f'Register and subscribe your RHEL system to the Red Hat Customer ' f'Portal using Red Hat Subscription-Manager.')
Please prepend with a sentence that describes the actual problem:
Cannot find SSL CA certificate path in /etc/yum.repos.d/redhat.repo. Register and subscribe ...
gsc.py line 214 at r11 (raw file):
print(f'Register and subscribe your RHEL system to the Red Hat Customer ' f'Portal using Red Hat Subscription-Manager.') sys.exit(0)
1
gsc.py line 216 at r11 (raw file):
sys.exit(0) # Copy the SSL CA certificate to the temporary build location
Remove the comment
gsc.py line 217 at r11 (raw file):
# Copy the SSL CA certificate to the temporary build location shutil.copyfile(sslcacert_path, tmp_build_path / 'redhat-uep.pem')
What does uep stand for? Please either rename to a more meaningful name, or add a comment
gsc.py line 222 at r11 (raw file):
shutil.rmtree(tmp_build_path / 'pki') # Copy the SSL client key directory to the temporary build location
Remove the comment, or rewrite it such that it explains why we need to do this
gsc.py line 223 at r11 (raw file):
# Copy the SSL client key directory to the temporary build location shutil.copytree(sslclientkey_dir, tmp_build_path / 'pki/entitlement')
What does this tree contain? Why do we need these files inside Docker container?
Documentation/index.rst line 348 at r11 (raw file):
8 and Red Hat Universal Base Image (UBI) 8 minimal. Please register and subscribe your host RHEL system to the Red Hat Customer Portal to use Red Hat Universal Base Image (UBI) 8 and Red Hat Universal Base Image (UBI) 8 minimal distros. Default
You repeat Red Hat Universal Base Image (UBI) twice. Just say ...to use Red Hat Universal Base Image (UBI) 8 and UBI8-minimal distros.
Documentation/index.rst line 349 at r11 (raw file):
your host RHEL system to the Red Hat Customer Portal to use Red Hat Universal Base Image (UBI) 8 and Red Hat Universal Base Image (UBI) 8 minimal distros. Default value is ``ubuntu:20.04``.
Don't put this info inside this paragraph. Make a separate warning-style paragraph, like this:
...
8 and Red Hat Universal Base Image (UBI) 8 minimal. Default value is
``ubuntu:20.04``.
.. warning::
Please register and subscribe your host RHEL system to the Red Hat
Customer Portal to use Red Hat Universal Base Image (UBI) 8 and
Red Hat Universal Base Image (UBI) 8 minimal distros.
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Note that IIUC this means that you're shipping private keys to RH subscription with your container. Is this really what you want?
I want to block on this. Indeed, looks like we leak private subscription keys in the GSC-generated Docker image. This needs to be fixed.
templates/redhat/ubi8/Dockerfile.build.template line 11 at r11 (raw file):
# This is because each Dockerfile command creates a new layer which necessarily adds size to the # final image. This trick allows to decrease the image size by hundreds of MBs. RUN rm -rf /etc/rhsm-host \
What is this magic with deleting /etc/rhsm-host?
templates/redhat/ubi8/Dockerfile.build.template line 13 at r11 (raw file):
RUN rm -rf /etc/rhsm-host \ && subscription-manager repos --enable codeready-builder-for-rhel-8-x86_64-rpms \ && dnf install -y https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm \
So we still need this line? I thought that by enabling CodeReady Builder repo, we don't need to do this weirdness with manual install of EPEL.
templates/redhat/ubi8/Dockerfile.compile.template line 9 at r11 (raw file):
# NOTE: meson v1.2.* has a bug that leads to Gramine build failure because of not found `libcurl.a` RUN rm -rf /etc/rhsm-host \
ditto
templates/redhat/ubi8/Dockerfile.compile.template line 11 at r11 (raw file):
RUN rm -rf /etc/rhsm-host \ && subscription-manager repos --enable codeready-builder-for-rhel-8-x86_64-rpms \ && dnf install -y https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm \
ditto
templates/redhat/Dockerfile.build.template line 16 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
But there is no
powertoolsin this link https://access.redhat.com/articles/4238681.I think
powertoolsandcodeready-builderis the same repo really, only renamed between major versions. I might be slightly wrong on the details, but the package set is mostly the same.To get https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/package_manifest/codereadylinuxbuilder-repository, package we will need redhat subscription.
Yeah, so this is kind of expected. If UBI needs subscription, then GSC user needs subscription to use UBI in GSC, right?
So we either
- allow the paid option, merge on a "best effort" basis, warning people that UBI is not supported by Gramine project, because we don't test it as nobody contributed paid access for CI for this distro; or
- we don't merge at all and tell people to use one of the rebuilds (Rocky or AlmaLinux), and if Intel badly wants UBI support delivered, then Intel is free to maintain a fork.
protobuf-devel(note! slight difference in name fromprotobuf-c-devel@dimakuv You sure this is the same package? In debian there are two of those,
protobuf-compilerandprotobuf-c-compilerand those are different things.I have no problem with removing
expect, but if we want to do this, it would have to be done either in this PR or another PR merged before this one. And the SO answer you've linked says that it is available in paid repos? So if we merge anyway (and tell people to have subscription if they use UBI), this might not be needed at all.
Since we moved to the paid-subscription option, this discussion is not relevant anymore.
templates/redhat/ubi8-minimal/Dockerfile.build.template line 11 at r11 (raw file):
# This is because each Dockerfile command creates a new layer which necessarily adds size to the # final image. This trick allows to decrease the image size by hundreds of MBs. RUN rm -rf /etc/rhsm-host \
ditto
templates/redhat/ubi8-minimal/Dockerfile.build.template line 12 at r11 (raw file):
# final image. This trick allows to decrease the image size by hundreds of MBs. RUN rm -rf /etc/rhsm-host \ && rpm -ivh https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm \
ditto
templates/redhat/ubi8-minimal/Dockerfile.compile.template line 9 at r11 (raw file):
# NOTE: meson v1.2.* has a bug that leads to Gramine build failure because of not found `libcurl.a` RUN rm -rf /etc/rhsm-host \
ditto
templates/redhat/ubi8-minimal/Dockerfile.compile.template line 10 at r11 (raw file):
# NOTE: meson v1.2.* has a bug that leads to Gramine build failure because of not found `libcurl.a` RUN rm -rf /etc/rhsm-host \ && rpm -ivh https://dl.fedoraproject.org/pub/epel/epel-release-latest-8.noarch.rpm \
ditto
There was a problem hiding this comment.
Reviewable status: 11 of 15 files reviewed, 27 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 @dimakuv and @woju)
gsc.py line 23 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can we move this import to the first set of imports (the one that starts with
argparse) and put it there sorted?
Done.
gsc.py line 181 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please do the other way around, for readability of the code:
if distro not in {"redhat/ubi8", "redhat/ubi8-minimal"}: return repo_name = "rhel-8-for-x86_64-baseos-rpms" with open('/etc/yum.repos.d/redhat.repo') as redhat_repo: ...
Done
gsc.py line 186 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why this search is enough? Can you show what is typically located in this file
/etc/yum.repos.d/redhat.repo? And explain the contents of this file?
Whether the rhel8 system has a subscription or not the /etc/yum.repos.d/redhat.repo file exists so we can't have this condition based on the presence of the file. Now if the system does not have any subscription the file will have a few comments only but if the system has a subscription it will have repository configurations. So I have added the condition to check if one of the repo configurations exists in the file (preferably the one from which we are installing packages that are missing in the UBI image). A snippet of some parts of this file is attached.
gsc.py line 188 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please prepend with a sentence that describes the actual problem:
Cannot find {repo_name} in /etc/yum.repos.d/redhat.repo. Register and subscribe ...
Done.
gsc.py line 189 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why
0? It should be1-- to indicate an error
Done.
gsc.py line 191 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Comment just duplicates what the code does. Remove this comment.
Done.
gsc.py line 196 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Comment just duplicates what the code does. Remove this comment.
Done.
gsc.py line 203 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please prepend with a sentence that describes the actual problem:
Cannot find SSL client key path in /etc/yum.repos.d/redhat.repo. Register and subscribe ...
Done.
gsc.py line 204 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
1
Done.
gsc.py line 207 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Comment just duplicates what the code does. Remove this comment.
Done.
gsc.py line 213 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please prepend with a sentence that describes the actual problem:
Cannot find SSL CA certificate path in /etc/yum.repos.d/redhat.repo. Register and subscribe ...
Done.
gsc.py line 214 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
1
Done.
gsc.py line 216 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove the comment
Done.
gsc.py line 217 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What does
uepstand for? Please either rename to a more meaningful name, or add a comment
Added a comment
gsc.py line 222 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove the comment, or rewrite it such that it explains why we need to do this
Done
gsc.py line 223 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What does this tree contain? Why do we need these files inside Docker container?
I have updated the PR such that it requires RHEL subscription. But from rhel8 onwards it uses podman and current version of GSC does not support podman and podman python library. When you use podman your redhat.repo in UBI8 base image will automatically be updated with '' rhel-8-for-x86_64-baseos-rpms" and "rhel-8-for-x86_64-appstream-rpms". But this does not happen when you use docker instead podman.
As an alternate solution the redhat.repo is copied from host RHEL path /etc/yum.repos.d to the UBI8 image. Then the redhat.repo file is parsed to get path of SSL CA certificate and SSL client key directory and the contents are copied to UBI8 base image. So, the contents of /etc/rhsm/ca and /etc/pki/entitlements are copied inside the Docker container for the redhat subscription to work inside Docker container.
Documentation/index.rst line 348 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You repeat
Red Hat Universal Base Image (UBI)twice. Just say...to use Red Hat Universal Base Image (UBI) 8 and UBI8-minimal distros.
Done.
Documentation/index.rst line 349 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Don't put this info inside this paragraph. Make a separate warning-style paragraph, like this:
... 8 and Red Hat Universal Base Image (UBI) 8 minimal. Default value is ``ubuntu:20.04``. .. warning:: Please register and subscribe your host RHEL system to the Red Hat Customer Portal to use Red Hat Universal Base Image (UBI) 8 and Red Hat Universal Base Image (UBI) 8 minimal distros.
Done.
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Note that IIUC this means that you're shipping private keys to RH subscription with your container. Is this really what you want?
I want to block on this. Indeed, looks like we leak private subscription keys in the GSC-generated Docker image. This needs to be fixed.
The keys are deleted now after the packages are installed.
templates/redhat/ubi8/Dockerfile.build.template line 11 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is this magic with deleting
/etc/rhsm-host?
As the entitlements are copied to the image, it is required to use entitlements from the build container.
Few links:
https://docs.openshift.com/container-platform/4.2/builds/running-entitled-builds.html#builds-strategy-docker-entitled-subman_running-entitled-builds
templates/redhat/ubi8/Dockerfile.build.template line 13 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So we still need this line? I thought that by enabling CodeReady Builder repo, we don't need to do this weirdness with manual install of EPEL.
It is required. If I remove this and build I get error: No package matches 'python3-pyelftools. And if I try to install epel-release I get error: No package matches 'epel-release'
templates/redhat/ubi8/Dockerfile.compile.template line 9 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
As the entitlements are copied to the image, it is required to use entitlements from the build container.
Few links:
https://docs.openshift.com/container-platform/4.2/builds/running-entitled-builds.html#builds-strategy-docker-entitled-subman_running-entitled-builds
templates/redhat/ubi8/Dockerfile.compile.template line 11 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
It is required. If I remove this and build I get error: No package matches 'python3-pyelftools. And if I try to install epel-release I get error: No package matches 'epel-release'
templates/redhat/ubi8-minimal/Dockerfile.build.template line 11 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
As the entitlements are copied to the image, it is required to use entitlements from the build container.
Few links:
https://docs.openshift.com/container-platform/4.2/builds/running-entitled-builds.html#builds-strategy-docker-entitled-subman_running-entitled-builds
templates/redhat/ubi8-minimal/Dockerfile.build.template line 12 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
It is required. If I remove this and build I get error: No package matches 'python3-pyelftools. And if I try to install epel-release I get error: No package matches 'epel-release'
templates/redhat/ubi8-minimal/Dockerfile.compile.template line 9 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
As the entitlements are copied to the image, it is required to use entitlements from the build container.
Few links:
https://docs.openshift.com/container-platform/4.2/builds/running-entitled-builds.html#builds-strategy-docker-entitled-subman_running-entitled-builds
templates/redhat/ubi8-minimal/Dockerfile.compile.template line 10 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
It is required. If I remove this and build I get error: No package matches 'python3-pyelftools. And if I try to install epel-release I get error: No package matches 'epel-release'
There was a problem hiding this comment.
Reviewed 1 of 7 files at r10, 4 of 4 files at r12, all commit messages.
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: ), "fixup! " found in commit messages' one-liners (waiting on @mkow, @oshogbo, and @sahason)
gsc.py line 186 at r11 (raw file):
Previously, sahason wrote…
Whether the rhel8 system has a subscription or not the
/etc/yum.repos.d/redhat.repofile exists so we can't have this condition based on the presence of the file. Now if the system does not have any subscription the file will have a few comments only but if the system has a subscription it will have repository configurations. So I have added the condition to check if one of the repo configurations exists in the file (preferably the one from which we are installing packages that are missing in the UBI image). A snippet of some parts of this file is attached.
.
One last question. You check for the existence of rhel-8-for-x86_64-baseos-rpms repo here, but in the Dockerfiles you seem to use the codeready-builder-for-rhel-8-x86_64-rpms. How are these two repos related? Aren't you supposed to check for codeready-builder-... in this Python code? Or maybe for both these repos, but not just one?
Documentation/index.rst line 346 at r12 (raw file):
results may be unpredictable. Currently supported distros are Ubuntu 20.04, Ubuntu 21.04, Debian 10, Debian 11, CentOS 8, Red Hat Universal Base Image (UBI) 8 and Red Hat Universal Base Image (UBI) 8 minimal. Default value is ``ubuntu:20.04``.
Our documentation has 80 chars per line limit (in contrast to our C/Python code, which has 100 chars per limit). Please fix and in the warning below.
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, sahason wrote…
The keys are deleted now after the packages are installed.
Yes, the keys are deleted from the final Docker image, but they are still in intermediate Docker images... Same problem as in #173... How do we proceed with this? Looks like we need a proper solution for all such cases, or just assume that the developer host is surely trusted, and don't bother with intermediate stuff.
By this logic, we also have a problem with our multi-stage build trick and removing the enclave-signing key...
@woju @oshogbo @mkow Any comments?
templates/redhat/ubi8/Dockerfile.build.template line 11 at r11 (raw file):
Previously, sahason wrote…
As the entitlements are copied to the image, it is required to use entitlements from the build container.
Few links:
https://docs.openshift.com/container-platform/4.2/builds/running-entitled-builds.html#builds-strategy-docker-entitled-subman_running-entitled-builds
Thanks!
templates/redhat/ubi8/Dockerfile.compile.template line 41 at r12 (raw file):
rpm-build \ wget \ && /usr/bin/python3 -B -m pip install 'tomli>=1.1.0' 'tomli-w>=0.4.0' 'meson>=0.56,!=1.2.*'
For other reviewers: we don't need to remove stuff (like the subscription key) in this .compile.template, because it is anyway always succeeded by the .build.template step, where this key will be removed from the final image.
And we don't care about these intermediate images for now, I guess... There seems to be a larger problem of using secrets (like private keys) inside our intermediate Docker containers, see also discussions in #173 and #175
There was a problem hiding this comment.
Reviewable status: 14 of 15 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 @dimakuv, @mkow, and @oshogbo)
gsc.py line 186 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
One last question. You check for the existence of
rhel-8-for-x86_64-baseos-rpmsrepo here, but in the Dockerfiles you seem to use thecodeready-builder-for-rhel-8-x86_64-rpms. How are these two repos related? Aren't you supposed to check forcodeready-builder-...in this Python code? Or maybe for both these repos, but not just one?
The modified redhat.repo after subscription has codeready-builder-for-rhel-8-x86_64-rpms disabled by default. So, it is enabled later as nasm package is available in this repo.
The condition in this python code is to check if the system is registered and subscribed for this any of the repo configuration availability check is sufficient.
Documentation/index.rst line 346 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Our documentation has 80 chars per line limit (in contrast to our C/Python code, which has 100 chars per limit). Please fix and in the
warningbelow.
Fixed.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r13, 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), "fixup! " found in commit messages' one-liners
gsc.py line 186 at r11 (raw file):
Previously, sahason wrote…
The modified
redhat.repoafter subscription hascodeready-builder-for-rhel-8-x86_64-rpmsdisabled by default. So, it is enabled later asnasmpackage is available in this repo.The condition in this python code is to check if the system is registered and subscribed for this any of the repo configuration availability check is sufficient.
Ok, thanks for the explanation.
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, the keys are deleted from the final Docker image, but they are still in intermediate Docker images... Same problem as in #173... How do we proceed with this? Looks like we need a proper solution for all such cases, or just assume that the developer host is surely trusted, and don't bother with intermediate stuff.
By this logic, we also have a problem with our multi-stage build trick and removing the enclave-signing key...
The current consensus seems to be to keep things (and the threat model) as is for now, and later @oshogbo will suggest a proper fix with a proper Docker technique to forward secrets into the Docker image build process. I am resolving this issue.
There was a problem hiding this comment.
Reviewed 3 of 4 files at r12, 1 of 1 files at r13, 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.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The current consensus seems to be to keep things (and the threat model) as is for now, and later @oshogbo will suggest a proper fix with a proper Docker technique to forward secrets into the Docker image build process. I am resolving this issue.
For the record, leaking RHSM secrets is not wrong in our model, because leaking those keys does not weaken Gramine or SGX security in any way. It's true that we shouldn't do it, but ultimately that's another discussion than #173/#175, because it's a very different threat model. That's why I don't insist here on any particular fix here (or any at all). The thing I do care about is enclave singing key, but it's not this PR.
There was a problem hiding this comment.
Reviewed 2 of 12 files at r3, 1 of 1 files at r8, 4 of 7 files at r10, 3 of 4 files at r12, 1 of 1 files at r13, 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.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
templates/redhat/ubi8/Dockerfile.build.template line 9 at r8 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
For the record, leaking RHSM secrets is not wrong in our model, because leaking those keys does not weaken Gramine or SGX security in any way. It's true that we shouldn't do it, but ultimately that's another discussion than #173/#175, because it's a very different threat model. That's why I don't insist here on any particular fix here (or any at all). The thing I do care about is enclave singing key, but it's not this PR.
Thanks @woju for correcting me. You're absolutely right.
To use these distros, need to register and subscribe the host RHEL system to the Red Hat Customer Portal. Signed-off-by: Sonali Saha <[email protected]>
dimakuv
dismissed stale reviews from kailun-qin, woju, and themself
via
4472304
dimakuv
force-pushed
the
sahason/add-rhel8-support
branch
from
ab9bc38 to
4472304
Compare
There was a problem hiding this comment.
Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @sahason)
a discussion (no related file):
Please note that I removed the expect tool because we merged this PR in the meantime (that removes the need for expect): #173
a discussion (no related file):
@sahason Please test this rebased-and-ready-for-merge version of the PR on UBI8 and UBI8-minimal. The rebase was a bit non-trivial, and I can't test it myself (I don't have RHEL host and I don't have the Red Hat subscription).
There was a problem hiding this comment.
Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @sahason)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 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…
@sahason Please test this rebased-and-ready-for-merge version of the PR on UBI8 and UBI8-minimal. The rebase was a bit non-trivial, and I can't test it myself (I don't have RHEL host and I don't have the Red Hat subscription).
Testing looks fine.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
a discussion (no related file):
Previously, sahason wrote…
Testing looks fine.
Thanks for double-checking!
Description of the changes
Add
ubi8,ubi8-minimalsupport.Implementation note: Due to unavailability of
expect,protobuf-c-develandnasmpackages in ubi8 CentOS repository is made available under ubi8 to install these 3 packages.How to test this PR?
Take the
hello-worldexample in the test folder and modify it to useredhat/ubi8:8.8andredhat/ubi8-minimal:8.8as base image.Edit the
config.yamlfile to select the distro asredhat/ubi8:8.8andredhat/ubi8-minimal:8.8.Do the
gsc buildandgsc signcommands.This change is