Closed
Conversation
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.
|
Skipping CI for Draft Pull Request. |
Contributor
Author
|
/test ? |
|
@davdhacs: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider consolidating the repetitive container image version updates across workflows (e.g. via a variable or reusable workflow) to simplify future base image upgrades.
- Double-check that the updated save-dir-contents paths (pointing to /etc/pki/ca-trust/source) exist and work correctly under UBI9.
- Clean up or fix the su-exec block in central-entrypoint.sh: either remove the obsolete TODO or make the conditional POSIX-compliant and ensure su-exec is installed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider consolidating the repetitive container image version updates across workflows (e.g. via a variable or reusable workflow) to simplify future base image upgrades.
- Double-check that the updated save-dir-contents paths (pointing to /etc/pki/ca-trust/source) exist and work correctly under UBI9.
- Clean up or fix the su-exec block in central-entrypoint.sh: either remove the obsolete TODO or make the conditional POSIX-compliant and ensure su-exec is installed.
## Individual Comments
### Comment 1
<location> `image/rhel/static-bin/central-entrypoint.sh:21-25` </location>
<code_context>
echo >&2 "Warning: failed to change permissions of one or more directories. Startup may fail."
fi
+ # TODO(DO NOT MERGE): exec: su-exec: not found
+ # Is this entire conditional still relevant? If so, perhaps we can make it
+ # better.
+ # Doesn't seem to work in UBI8 either.
+ # Also, the conditional itself isn't POSIX-compliant (should be `[ "$(id -u)" = "0" ]`)
exec su-exec 4000:4000 "$0" "$@"
fi
</code_context>
<issue_to_address>
**issue:** The TODO block should be removed before merging.
TODOs marked 'DO NOT MERGE' in production code can cause confusion and risk incomplete changes being deployed. Please resolve or remove this block before merging.
</issue_to_address>
### Comment 2
<location> `image/rhel/static-bin/central-entrypoint.sh:26` </location>
<code_context>
+ # better.
+ # Doesn't seem to work in UBI8 either.
+ # Also, the conditional itself isn't POSIX-compliant (should be `[ "$(id -u)" = "0" ]`)
exec su-exec 4000:4000 "$0" "$@"
fi
</code_context>
<issue_to_address>
**issue (bug_risk):** su-exec may not be available in UBI9 images.
Consider replacing su-exec with a more widely available alternative like 'gosu' or 'runuser' to prevent runtime failures from missing binaries.
</issue_to_address>
### Comment 3
<location> `scanner/image/scanner/Dockerfile:48` </location>
<code_context>
# by the script `save-dir-contents` during the image build. The directory
# 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
# This is equivalent to nobody:nobody.
</code_context>
<issue_to_address>
**issue (bug_risk):** Saving only /etc/pki/ca-trust/source may omit important trust anchors.
Verify whether the application requires the entire /etc/pki/ca-trust directory or just the 'source' subdirectory. Update the logic if additional files are needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # by the script `save-dir-contents` during the image build. The directory | ||
| # 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 |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Saving only /etc/pki/ca-trust/source may omit important trust anchors.
Verify whether the application requires the entire /etc/pki/ca-trust directory or just the 'source' subdirectory. Update the logic if additional files are needed.
Contributor
|
Images are ready for the commit at 095fe95. 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 #17874 +/- ##
==========================================
- Coverage 49.15% 49.15% -0.01%
==========================================
Files 2739 2739
Lines 201205 201205
==========================================
- Hits 98903 98901 -2
- Misses 94599 94600 +1
- Partials 7703 7704 +1
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:
|
Contributor
Author
|
/test gke-nongroovy-e2e-tests gke-qa-e2e-tests |
Contributor
Author
|
/test gke-upgrade-tests |
Contributor
Author
|
/test aks-qa-e2e-tests gke-qa-e2e-tests |
Contributor
Author
|
/retest |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 20170519 | Triggered | JSON Web Token | dc5e0ed | central/auth/m2m/id_token_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
Author
|
/test ocp-4-20-compliance-e2e-tests ocp-4-20-operator-e2e-tests ocp-4-20-qa-e2e-tests ocp-next-candidate-fips-qa-e2e-tests ocp-next-candidate-qa-e2e-tests ocp-next-candidate-operator-e2e-tests ocp-next-candidate-compliance-e2e-tests |
Contributor
Author
|
/test ocp-4-20-compliance-e2e-tests ocp-4-20-operator-e2e-tests ocp-4-20-qa-e2e-tests ocp-next-candidate-fips-qa-e2e-tests ocp-next-candidate-qa-e2e-tests ocp-next-candidate-operator-e2e-tests ocp-next-candidate-compliance-e2e-tests |
|
@davdhacs: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Contributor
Author
|
re-doing on master after rox-ci-image update to 0.5.1: #18364 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
re-run of UBI8->9 testing in #15178 on latest master (post 4.9).
rox-ci-image update from PR stackrox/rox-ci-image#234