-
Notifications
You must be signed in to change notification settings - Fork 12
CFE-1124: Install ansible using pip instead of rpm #20
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
build_root_image: | ||
name: release | ||
namespace: openshift | ||
tag: rhel-8-release-golang-1.22-openshift-4.17 | ||
tag: rhel-9-release-golang-1.22-openshift-4.17 |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
FROM registry.ci.openshift.org/ocp/4.17:base-rhel9 | ||
|
||
RUN yum install -y rust cargo libffi-devel python3-devel gcc python3-pip | ||
|
||
COPY ./Pipfile* ./ | ||
|
||
# The build dependencies are required by cachito. Following script | ||
# does exactly the same. More info at: https://github.com/containerbuildsystem/cachito/blob/master/docs/pip.md#build-dependencies | ||
RUN curl -LO https://raw.githubusercontent.com/containerbuildsystem/cachito/master/bin/pip_find_builddeps.py \ | ||
&& chmod +x pip_find_builddeps.py | ||
|
||
# Create requirements.in file from the pipenv created using the | ||
# same Pipfile and Pipfile.lock used for upstream image. Then | ||
# use pip-compile to generate the requirements.txt file. Copy | ||
# setuptools into requirements-build.txt as pip-compile will | ||
# omit it from the requierements.txt file. | ||
RUN python3 -m pip install pipenv==2023.11.15 \ | ||
&& python3 -m pip install pip-tools \ | ||
&& pipenv install --deploy \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this already installs the packages successfully, although in a virtual env. Does it suffice to copy the installed packages from the install path to other layers of the docker? Something similar to upstream : https://github.com/operator-framework/ansible-operator-plugins/blob/42b5d80c75f1ddda8f2dbe1629b9454d366a8d6a/images/ansible-operator/Dockerfile#L62 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Dockerfile is used to generate the requirements files for the OSBS environment. Once the requirements files are generated we can use them to install the packages in main Dockerfile: https://github.com/openshift/ansible-operator-plugins/pull/20/files#diff-06f45889c808dfe42bf9d2705b2d0f9b21417ac6a7b53f95c82d3e2327c18e7a |
||
# NOTE: This ignored vulnerability (70612) was detected in jinja2, \ | ||
# but the vulnerability is disputed and may never be fixed. See: \ | ||
# - https://github.com/advisories/GHSA-f6pv-j8mr-w6rr \ | ||
# - https://github.com/dbt-labs/dbt-core/issues/10250 \ | ||
# - https://data.safetycli.com/v/70612/97c/ \ | ||
# NOTE: This ignored vulnerability (71064) was detected in requests, \ | ||
# but the upgraded version doesn't support the use case (protocol we are using).\ | ||
# Ref: https://github.com/operator-framework/ansible-operator-plugins/pull/67#issuecomment-2189164688 | ||
&& pipenv check --ignore 70612 --ignore 71064 \ | ||
&& pipenv run pip freeze --all > ./requirements.in \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pipenv requirements or pipenv lock -r should help to generate better dependency package list compared to freeze. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried the following commands: # pipenv run pip freeze --all > requirements1.txt
# pipenv requirements --exclude-markers > requirements2.txt
# diff -y requirements1.txt requirements2.txt
> -i https://pypi.org/simple
ansible-core==2.15.11 ansible-core==2.15.11
ansible-runner==2.3.6 ansible-runner==2.3.6
ansible-runner-http==1.0.0 ansible-runner-http==1.0.0
cachetools==5.3.3 cachetools==5.3.3
certifi==2024.2.2 certifi==2024.2.2
cffi==1.16.0 cffi==1.16.0
charset-normalizer==3.3.2 charset-normalizer==3.3.2
cryptography==42.0.7 cryptography==42.0.7
docutils==0.21.2 docutils==0.21.2
google-auth==2.29.0 google-auth==2.29.0
idna==3.7 idna==3.7
importlib-metadata==6.2.1 importlib-metadata==6.2.1
importlib-resources==5.0.7 importlib-resources==5.0.7
Jinja2==3.1.4 | jinja2==3.1.4
kubernetes==29.0.0 kubernetes==29.0.0
lockfile==0.12.2 lockfile==0.12.2
MarkupSafe==2.1.5 | markupsafe==2.1.5
oauthlib==3.2.2 oauthlib==3.2.2
packaging==24.0 packaging==24.0
pexpect==4.9.0 pexpect==4.9.0
pip==24.2 <
ptyprocess==0.7.0 ptyprocess==0.7.0
pyasn1==0.6.0 pyasn1==0.6.0
pyasn1_modules==0.4.0 | pyasn1-modules==0.4.0
pycparser==2.22 pycparser==2.22
python-daemon==3.0.1 python-daemon==3.0.1
python-dateutil==2.9.0.post0 python-dateutil==2.9.0.post0
PyYAML==6.0.1 | pyyaml==6.0.1
requests==2.31.0 requests==2.31.0
requests-oauthlib==2.0.0 requests-oauthlib==2.0.0
requests-unixsocket==0.3.0 requests-unixsocket==0.3.0
resolvelib==1.0.1 resolvelib==1.0.1
rsa==4.9 rsa==4.9
setuptools==69.5.1 setuptools==69.5.1
six==1.16.0 six==1.16.0
urllib3==1.26.18 urllib3==1.26.18
websocket-client==1.8.0 websocket-client==1.8.0
wheel==0.44.0 <
zipp==3.18.1 zipp==3.18.1 As you can see Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding wheel to pipfile resolve the problem? It is best to avoid multiple requirements files for the complexity it adds for debugging in the future package version updates. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot use a single requirements file in OSBS environment for all the packages required by the ansible-operator. The build dependencies are needed to be installed first. Without following that order the build will fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! Got it about the package installation for cachito which is pulled by osbs. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is better to have check for cve using pipenv check or safety to fail-fast before the building the requirements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we proceed if there are CVEs reported by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Depends on the timeline commitments. If the downstream release can wait till there is a release from upstream, then upstream fixes can rebased. Otherwise, the upstream carry commits can be created. Once the fix is available from upstream, the carry PRs can be dropped. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found the following vulnerabilities after using 20.87 Checking Pipfile.lock packages for vulnerabilities...
20.87 Notice: Ignoring Vulnerabilities 70612, 71064
22.84 +==============================================================================+
22.84
22.84 /$$$$$$ /$$
22.84 /$$__ $$ | $$
22.84 /$$$$$$$ /$$$$$$ | $$ \__//$$$$$$ /$$$$$$ /$$ /$$
22.84 /$$_____/ |____ $$| $$$$ /$$__ $$|_ $$_/ | $$ | $$
22.84 | $$$$$$ /$$$$$$$| $$_/ | $$$$$$$$ | $$ | $$ | $$
22.84 \____ $$ /$$__ $$| $$ | $$_____/ | $$ /$$| $$ | $$
22.84 /$$$$$$$/| $$$$$$$| $$ | $$$$$$$ | $$$$/| $$$$$$$
22.84 |_______/ \_______/|__/ \_______/ \___/ \____ $$
22.84 /$$ | $$
22.84 | $$$$$$/
22.84 by pyup.io \______/
22.84
22.84 +==============================================================================+
22.84
22.84 REPORT
22.84
22.84 Safety v2.3.2 is scanning for Vulnerabilities...
22.84 Scanning dependencies in your files:
22.84
22.84 -> /tmp/-x-v5uFv0wqfu2bdb_requirements.txt
22.84
22.84 Found and scanned 37 packages
22.84 Timestamp 2024-09-19 16:09:39
22.84 5 vulnerabilities found
22.84 2 vulnerabilities ignored
22.84
22.84 +==============================================================================+
22.84 VULNERABILITIES FOUND
22.84 +==============================================================================+
22.84
22.84 -> Vulnerability found in certifi version 2024.2.2
22.84 Vulnerability ID: 72083
22.84 Affected spec: >=2021.05.30,<2024.07.04
22.84 ADVISORY: Certifi affected versions recognized root certificates from
22.84 GLOBALTRUST. Certifi patch removes these root certificates from the root...
22.84 CVE-2024-39689
22.84 For more information, please visit
22.84 https://data.safetycli.com/v/72083/742
22.84
22.84
22.84 -> Vulnerability found in cryptography version 42.0.7
22.84 Vulnerability ID: 71681
22.84 Affected spec: <42.0.8
22.84 ADVISORY: The `cryptography` library has updated its BoringSSL and
22.84 OpenSSL dependencies in CI due to a security concern. Specifically, the...
22.84 CVE-2024-4603
22.84 For more information, please visit
22.84 https://data.safetycli.com/v/71681/742
22.84
22.84
22.84 -> Vulnerability found in jinja2 version 3.1.4
22.84 Vulnerability ID: 70612
22.84 This vulnerability is being ignored.
22.84 For more information, please visit
22.84 https://data.safetycli.com/v/70612/742
22.84
22.84
22.84 -> Vulnerability found in requests version 2.31.0
22.84 Vulnerability ID: 71064
22.84 This vulnerability is being ignored.
22.84 For more information, please visit
22.84 https://data.safetycli.com/v/71064/742
22.84
22.84
22.84 -> Vulnerability found in setuptools version 69.5.1
22.84 Vulnerability ID: 72236
22.84 Affected spec: <70.0.0
22.84 ADVISORY: Affected versions of Setuptools allow for remote code
22.84 execution via its download functions. These functions, which are used to...
22.84 CVE-2024-6345
22.84 For more information, please visit
22.84 https://data.safetycli.com/v/72236/742
22.84
22.84
22.84 -> Vulnerability found in urllib3 version 1.26.18
22.84 Vulnerability ID: 71608
22.84 Affected spec: <=1.26.18
22.84 ADVISORY: Urllib3's ProxyManager ensures that the Proxy-Authorization
22.84 header is correctly directed only to configured proxies. However, when...
22.84 CVE-2024-37891
22.84 For more information, please visit
22.84 https://data.safetycli.com/v/71608/742
22.84
22.84
22.84 -> Vulnerability found in zipp version 3.18.1
22.84 Vulnerability ID: 72132
22.84 Affected spec: <3.19.1
22.84 ADVISORY: A Denial of Service (DoS) vulnerability exists in the
22.84 jaraco/zipp library. The vulnerability is triggered when processing a...
22.84 CVE-2024-5569
22.84 For more information, please visit
22.84 https://data.safetycli.com/v/72132/742
22.84
22.84 Scan was completed. 5 vulnerabilities were found. 2 vulnerabilities from 2
22.84 packages were ignored.
22.84
22.84 +==============================================================================+
22.84 REMEDIATIONS
22.84
22.84 5 vulnerabilities were found in 5 packages. For detailed remediation & fix
22.84 recommendations, upgrade to a commercial license.
22.84
22.84 +==============================================================================+ How should we proceed now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I addition to this we have to install cryptography using rpm as it cannot be built using cachito. Following are the versions available for cryptography: bash-5.1$ yum --showduplicates list python3-cryptography
Last metadata expiration check: 0:03:40 ago on Thu Sep 19 17:10:30 2024.
Installed Packages
python3-cryptography.x86_64 36.0.1-2.el9 @ubi-9-appstream-rpms
Available Packages
python3-cryptography.x86_64 36.0.1-2.el9 ubi-9-appstream-rpms
bash-5.1$ yum --showduplicates list python3.11-cryptography
Last metadata expiration check: 0:03:58 ago on Thu Sep 19 17:10:30 2024.
Available Packages
python3.11-cryptography.x86_64 37.0.2-6.el9 ubi-9-appstream-rpms
bash-5.1$ yum --showduplicates list python3.12-cryptography
Last metadata expiration check: 0:04:01 ago on Thu Sep 19 17:10:30 2024.
Available Packages
python3.12-cryptography.x86_64 41.0.7-1.el9 ubi-9-appstream-rpms
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can drop c42e6b6 whenever we pull in the changes from upstream after operator-framework/ansible-operator-plugins#92 is merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the Pipfile to pin urllib3 version to 2.31.0 because of this issue: The requirements.txt file is also updated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Did you mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes. My bad. It's requests package which is pinned to 2.31.0. |
||
# Add wheel and setuptools to requirements-build.in file as these packages | ||
# are part of the build dependencies of some packages in requirements.in file. | ||
&& grep "wheel==" ./requirements.in >> ./requirements-build.in || true \ | ||
&& grep "setuptools==" ./requirements.in >> ./requirements-build.in || true \ | ||
&& pip-compile --output-file=./requirements.txt ./requirements.in --strip-extras \ | ||
# NOTE: Comment out ansible-runner, ansible-runner-http and python-daemon as | ||
# both ansible-runner and ansible-runner-http has python-daemon as dependency. | ||
# pip_find_builddeps.py encounters an error when trying to get the build | ||
# dependencies of python-daemon==3.0.1. | ||
# TODO: Whenever a newer version of python-daemon is released check | ||
# whether pip_find_builddeps.py is able to fetch its build dependencies | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this automated to generate the dependencies on every new build? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Also, the requirements files only needs to be regenerated when there's a change in the upstream Pipfile/Pipfile.lock files. Otherwise, we should be good with the python package dependencies. |
||
# without any error. If so, then remove the following lines which comments | ||
# out ansible-runner, ansible-runner-http and python-daemon. | ||
&& sed -i '/ansible-runner==/s/^/#/g' ./requirements.txt \ | ||
&& sed -i '/ansible-runner-http==/s/^/#/g' ./requirements.txt \ | ||
&& sed -i '/python-daemon==/s/^/#/g' ./requirements.txt \ | ||
&& ./pip_find_builddeps.py requirements.txt -o requirements-build.in --append \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This flow needs to be relooked for leveraging pipenv simplifying the actions in this list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added the details in this comment regarding the flow: https://github.com/openshift/ansible-operator-plugins/pull/20/files#diff-7ad0e6b82f775968960444bb0baafc660c603428d601e19fdcf45a81623775eeR7-R8. |
||
# Uncomment ansible-runner, ansible-runner-http and python-daemon, so that | ||
# they are re-enabled in the requirements.txt file. | ||
&& sed -i '/ansible-runner==/s/^#//g' ./requirements.txt \ | ||
&& sed -i '/ansible-runner-http==/s/^#//g' ./requirements.txt \ | ||
&& sed -i '/python-daemon==/s/^#//g' ./requirements.txt \ | ||
&& pip-compile --output-file=./requirements-build.txt ./requirements-build.in --strip-extras --allow-unsafe \ | ||
# NOTE: Comment out cryptography and its dependencies from the requirements.txt | ||
# and requirements-build.txt files as these packages can't be installed in the | ||
# isolated environment of OSBS image build. These packages will be installed | ||
# through rpms. | ||
&& sed -i '/cryptography==/s/^/#/g' ./requirements.txt \ | ||
&& sed -i '/cffi==/s/^/#/g' ./requirements.txt \ | ||
&& sed -i '/pycparser==/s/^/#/g' ./requirements.txt \ | ||
&& sed -i '/cffi==/s/^/#/g' ./requirements-build.txt \ | ||
&& sed -i '/pycparser==/s/^/#/g' ./requirements-build.txt \ | ||
&& sed -i '/maturin==/s/^/#/g' ./requirements-build.txt \ | ||
# Add flit-core to requirements-pre-build.in file as this package is part of the | ||
# build dependencies of some packages in requirements-build.txt file. | ||
&& grep "flit-core==" ./requirements-build.txt >> ./requirements-pre-build.in || true \ | ||
&& pip-compile --output-file=./requirements-pre-build.txt ./requirements-pre-build.in --strip-extras | ||
|
||
VOLUME /tmp/requirements | ||
ENTRYPOINT ["cp", "./requirements.txt", "./requirements-build.txt", "./requirements-pre-build.txt", "/tmp/requirements/"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
#!/bin/bash | ||
|
||
set -euo pipefail | ||
|
||
PIP_OPTS="--no-cache-dir" | ||
# Check if we are building the image in the OSBS environment. If so, source the | ||
# env vars specific for enabling cachito. | ||
if [ -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps ]; then | ||
source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env | ||
cd ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/openshift | ||
else | ||
cd ${REMOTE_SOURCES_DIR} | ||
fi | ||
|
||
# Install the packages in order of build dependency to avoid issues during installation. | ||
python3 -m pip install ${PIP_OPTS} -r requirements-pre-build.txt | ||
python3 -m pip install ${PIP_OPTS} -r requirements-build.txt | ||
python3 -m pip install ${PIP_OPTS} -r requirements.txt | ||
|
||
rm -rf ${REMOTE_SOURCES_DIR} |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
# | ||
# This file is autogenerated by pip-compile with Python 3.9 | ||
# by the following command: | ||
# | ||
# pip-compile --allow-unsafe --output-file=./requirements-build.txt --strip-extras ./requirements-build.in | ||
# | ||
#cffi==1.17.1 | ||
# via -r ./requirements-build.in | ||
cython==3.0.11 | ||
# via -r ./requirements-build.in | ||
flit-core==3.9.0 | ||
# via -r ./requirements-build.in | ||
#maturin==1.7.1 | ||
# via -r ./requirements-build.in | ||
packaging==24.1 | ||
# via | ||
# -r ./requirements-build.in | ||
# setuptools-scm | ||
pbr==6.1.0 | ||
# via -r ./requirements-build.in | ||
poetry-core==1.9.0 | ||
# via -r ./requirements-build.in | ||
#pycparser==2.22 | ||
# via | ||
# -r ./requirements-build.in | ||
# cffi | ||
semantic-version==2.10.0 | ||
# via | ||
# -r ./requirements-build.in | ||
# setuptools-rust | ||
setuptools-rust==1.10.1 | ||
# via -r ./requirements-build.in | ||
setuptools-scm==7.1.0 | ||
# via -r ./requirements-build.in | ||
tomli==2.0.1 | ||
# via | ||
# -r ./requirements-build.in | ||
# maturin | ||
# setuptools-scm | ||
typing-extensions==4.12.2 | ||
# via | ||
# -r ./requirements-build.in | ||
# setuptools-scm | ||
wheel==0.44.0 | ||
# via -r ./requirements-build.in | ||
|
||
# The following packages are considered to be unsafe in a requirements file: | ||
setuptools==70.0.0 | ||
# via | ||
# -r ./requirements-build.in | ||
# setuptools-rust | ||
# setuptools-scm |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# | ||
# This file is autogenerated by pip-compile with Python 3.9 | ||
# by the following command: | ||
# | ||
# pip-compile --output-file=./requirements-pre-build.txt --strip-extras ./requirements-pre-build.in | ||
# | ||
flit-core==3.9.0 | ||
# via -r ./requirements-pre-build.in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What could be the consequences of not using tini?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two comments summarize it well regarding when to use tini and when not to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the claim here that there are no possibilities of zombie processes or mis-handling of signals? If it is completely assessed and tested, tini may not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the upstream image still uses tini. I think we should try to keep the
ENTRYPOINT
the same unless specifically required.ansible-operator-plugins/images/ansible-operator/Dockerfile
Line 88 in c07954c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upstream image uses ubi8. tini is not available through rpm in ubi9/rhel9. We can use tini but we need to decide how we add it to the Dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the playbook commands are run using exec.command(), it is best to have safeguard in place for production environments to avoid any bugs in dependent libraries causing the issues.
/hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
catatonit
instead oftini
. More context here: https://redhat-internal.slack.com/archives/CBBJY9GSX/p1726751465415449