Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 007ed67. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #15178 +/- ##
==========================================
+ Coverage 49.04% 49.08% +0.03%
==========================================
Files 2635 2640 +5
Lines 195420 195603 +183
==========================================
+ Hits 95852 96008 +156
- Misses 92067 92089 +22
- Partials 7501 7506 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
| # PyYAML > 6.0 requires Python > 3.6. | ||
| PyYAML==6.0 | ||
| # pytest==7.0.1 is the latest available for the quay.io/stackrox-io/apollo-ci:stackrox-test-0.4.8 job container's Python. | ||
| # pytest==7.0.1 is the latest available for the quay.io/stackrox-io/apollo-ci:stackrox-test-0.4.8-3-g679cfb72eb job container's Python. |
There was a problem hiding this comment.
I think this change can be removed, but later can be resolved because UBI9 system python is 3.9 (I checked in quay.io/stackrox-io/apollo-ci:stackrox-test-0.4.8-3-g679cfb72eb).
|
Rebased, added messages to the fixups commits (you can read them by clicking on the kabob / more-options icon), and hopefully fixed the permission issues we were seeing. |
|
Cherry-picking some commits from #15337. One of them should resolve the scanner build errors. Hoping it shares the history cleanly. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
1 similar comment
|
/retest |
822716b to
80f80ef
Compare
|
rebasing and testing a simpler change |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
Missed the actually runtime container images in the last commit (only changed the builder images).
Should resolve the permission issues we're seeing in CI. Regarding why we do all the certificate business in the first place, I think it's because we want to update the trusted certificates in the container with any stackrox-generate certs+any relevant OCP certs. However, there may be a better way to go about it. Needs further investigation and possibly roping in other teams (e.g., Install team).
Update `pg_rhel_major` to 9 for the rest of the download scripts.
Don't globber files when restoring. Should resolve the sensor errors. I suspect this approach won't be the one we ship - just trying to get everything to work for now.
Turns out we weren't running the `update-ca-trust` command from the last fixup since we were restoring `/etc/pki/ca-trust/extracted` that was saved during the container build process. These changes should implement the original fixup correctly and allow the operator-related CA tests to pass.
a39a7d5 to
0cdb846
Compare
|
/retest |
|
Caution There are some errors in your PipelineRun template.
|
|
/retest |
| chmod u+w /etc/pki/ca-trust/extracted/pem/directory-hash | ||
| fi | ||
|
|
||
| # Though /etc/pki/ca-trust/extracted is the default output, update-ca-trust |
There was a problem hiding this comment.
@BradLugo I am a bit confused.
I understood https://bugzilla.redhat.com/show_bug.cgi?id=2241240#c4 so that updating system trust stores as unprivileged user is not supported anymore.
And in this PR you are basically saying(?)
"Let's specify the output directory even though that is the default anyway" and it will magically work?
Isn't exactly that supposed to not work anymore?
What am I missing? 🙂
Thank you.
There was a problem hiding this comment.
It's been a minute since I looked at this, but IIRC, this is due to update-ca-trust extract not attempting to create the default directory and only doing so when the directory is specified via the --output flag: https://src.fedoraproject.org/rpms/ca-certificates/blob/rawhide/f/update-ca-trust#_111-120.
The code block above this comment resolves the permission issue, though it was more of a proof of concept/short-term fix rather than something I think we should use long-term.
There was a problem hiding this comment.
This bit might be problematic, not sure (from the update-ca-trust usage):
Note: This option will not populate the ../pki/tls/certs with the directory-hash symbolic links.
| # contents are then restored by the script `restore-all-dir-contents` | ||
| # during the container start. | ||
| chown -R 4000:4000 /etc/pki/ca-trust /etc/ssl && save-dir-contents /etc/pki/ca-trust /etc/ssl && \ | ||
| chown -R 4000:4000 /etc/pki/ca-trust /etc/ssl && save-dir-contents /etc/pki/ca-trust/source /etc/ssl && \ |
There was a problem hiding this comment.
Hi @BradLugo , could you explain why this is needed?
There was a problem hiding this comment.
This change might not be necessary, but targeting the specific directory we need might be a good idea for clarity, since we don't need everything else /etc/pki/ca-trust. Anyway, I think I was trying to resolve the permission errors we were running into, which I think were ultimately resolved by chmoding in a later commit. I'd say you could probably take or leave this change.
| # contents are then restored by the script `restore-all-dir-contents` | ||
| # during the container start. | ||
| chown -R 65534:65534 /etc/pki/ca-trust /etc/ssl && save-dir-contents /etc/pki/ca-trust /etc/ssl | ||
| chown -R 65534:65534 /etc/pki/ca-trust /etc/ssl && save-dir-contents /etc/pki/ca-trust/source /etc/ssl |
| cp -v -L "$src"/* /etc/pki/ca-trust/source/anchors | ||
| # TODO(DO NOT MERGE): Is `--update` the correct operation here? | ||
| cp --verbose --dereference --update \ | ||
| "$src"/* /etc/pki/ca-trust/source/anchors |
There was a problem hiding this comment.
What problem is this trying to address?
@BradLugo
There was a problem hiding this comment.
IIRC, this was due to ensuring /etc/pki/ca-trust/source/anchors updated with files that were older, which is to say that they will definitely be overwritten by the files that were saved during build time, but not overwrite files that are newer, e.g., if this script is run again (as in the case of sensor, I think) it won't clobber the newer files that were configured (which I think is important because we update the certs after this is run the first time).
| target_dir="/.init-dirs/${dir_no_leading_slash}" | ||
| mkdir -p "${target_dir}" | ||
| cp -rpP "${dir}"/* "${target_dir}" | ||
| cp --recursive --preserve=mode,ownership --no-dereference \ |
There was a problem hiding this comment.
so we are skipping preserving of timestamps, why?
There was a problem hiding this comment.
IIRC, this was to ensure that all files to be restored had a more recent timestamp (the build timestamp). This is important since these changes use cp --update later (relevant to your question above).
|
Never mind, I think I remember how this all fits together now. I highly recommend double-checking my answers/assumptions. I'm definitely not an expert in these scripts. A lot of these changes were "just get it working" and to make a proof of concept. Let me know if there's anything else I can help with 🙂 |
Description
Upgrade to UBI9. This is currently a proof-of-concept to see what will break or otherwise block us from moving forward with this upgrade.
Related PRs:
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!