OCPBUGS-84516: Add terminationMessagePolicy to build pod containers#5993
OCPBUGS-84516: Add terminationMessagePolicy to build pod containers#5993isabella-janssen wants to merge 1 commit intoopenshift:mainfrom
terminationMessagePolicy to build pod containers#5993Conversation
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
|
@isabella-janssen: This pull request references Jira Issue OCPBUGS-84516, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-bug
|
@isabella-janssen: This pull request references Jira Issue OCPBUGS-84516, which is valid. 3 validation(s) were run on this bug
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
WalkthroughUpdated build pod initialization by adding termination message policy configuration to both the ChangesBuild Pod Initialization Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-csi openshift/origin#31120 |
|
@isabella-janssen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0a5ce180-47e8-11f1-8c03-0252c16e8dd9-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/build/buildrequest/buildrequest.go`:
- Around line 677-685: The create-digest-configmap container is currently given
the full volumeMounts slice (VolumeMounts: volumeMounts) which exposes pull/push
credential secrets unnecessarily; restrict mounts to least privilege by
replacing VolumeMounts: volumeMounts with a dedicated slice containing only the
mount(s) needed to write/read the digest artifact (e.g., create a local variable
like digestVolumeMounts that includes just the digest artifact volume mount and
any minimal tmp/log mounts) and use that in the container spec for the
"create-digest-configmap" container (referencing the container name
"create-digest-configmap", the field VolumeMounts, and the existing volume
definitions used for the digest artifact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f907e52-9f63-42ef-a054-aecb0307d0d2
📒 Files selected for processing (1)
pkg/controller/build/buildrequest/buildrequest.go
| Name: "create-digest-configmap", | ||
| Command: append(command, digestCMScript), | ||
| Image: br.opts.MachineConfig.Spec.OSImageURL, | ||
| Env: env, | ||
| ImagePullPolicy: corev1.PullAlways, | ||
| SecurityContext: securityContext, | ||
| TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, | ||
| VolumeMounts: volumeMounts, | ||
| }, |
There was a problem hiding this comment.
Restrict create-digest-configmap mounts to least privilege.
VolumeMounts: volumeMounts mounts pull/push credential secrets into this container as well. If this container only needs the digest artifact, this unnecessarily broadens secret exposure.
🔧 Suggested narrowing of mounts
- VolumeMounts: volumeMounts,
+ VolumeMounts: []corev1.VolumeMount{
+ {
+ Name: "done",
+ MountPath: "/tmp/done",
+ },
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Name: "create-digest-configmap", | |
| Command: append(command, digestCMScript), | |
| Image: br.opts.MachineConfig.Spec.OSImageURL, | |
| Env: env, | |
| ImagePullPolicy: corev1.PullAlways, | |
| SecurityContext: securityContext, | |
| TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, | |
| VolumeMounts: volumeMounts, | |
| }, | |
| Name: "create-digest-configmap", | |
| Command: append(command, digestCMScript), | |
| Image: br.opts.MachineConfig.Spec.OSImageURL, | |
| Env: env, | |
| ImagePullPolicy: corev1.PullAlways, | |
| SecurityContext: securityContext, | |
| TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, | |
| VolumeMounts: []corev1.VolumeMount{ | |
| { | |
| Name: "done", | |
| MountPath: "/tmp/done", | |
| }, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/build/buildrequest/buildrequest.go` around lines 677 - 685,
The create-digest-configmap container is currently given the full volumeMounts
slice (VolumeMounts: volumeMounts) which exposes pull/push credential secrets
unnecessarily; restrict mounts to least privilege by replacing VolumeMounts:
volumeMounts with a dedicated slice containing only the mount(s) needed to
write/read the digest artifact (e.g., create a local variable like
digestVolumeMounts that includes just the digest artifact volume mount and any
minimal tmp/log mounts) and use that in the container spec for the
"create-digest-configmap" container (referencing the container name
"create-digest-configmap", the field VolumeMounts, and the existing volume
definitions used for the digest artifact).
There was a problem hiding this comment.
Editing the mounts is not in the scope of this bug fix. The line is only showing a diff because the formatting changed.
There was a problem hiding this comment.
@isabella-janssen, understood — the VolumeMounts line is pre-existing and only appears in the diff due to whitespace realignment from adding TerminationMessagePolicy. This concern is not introduced by your change. I'll drop the suggestion.
Would you like me to open a follow-up GitHub issue to track narrowing the create-digest-configmap volume mounts as a separate security hardening task, or would you prefer to skip that entirely?
There was a problem hiding this comment.
I don't think this suggestion is necessary.
|
/verified by @isabella-janssen See the successful run of |
isabella-janssen
changed the title
terminationMessagePolicy to build pod containersterminationMessagePolicy to build pod containers
openshift-ci-robot
added
the
verified
|
@isabella-janssen: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/lgtm Thanks for the quick fix! |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, isabella-janssen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test unit |
|
/cherrypick release-4.22 release-4.21 release-4.20 release-4.19 release-4.18 |
|
@isabella-janssen: once the present PR merges, I will cherry-pick it on top of 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. |
|
/jira refresh |
|
@isabella-janssen: This pull request references Jira Issue OCPBUGS-84516, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
5 participants
Closes: OCPBUGS-84516
- What I did
This adds a
TerminationMessagePolicyof typeTerminationMessageFallbackToLogsOnErrorto the build containers created dynamically.- How to verify it
The
[Monitor:termination-message-policy][sig-arch] all containers in ns/openshift-machine-config-operator must have terminationMessagePolicy=FallbackToLogsOnErrortest should pass when the MCO namespace exception is removed, so when tested with openshift/origin#31120.- Description for the changelog
OCPBUGS-84516: Add
terminationMessagePolicyto build pod containersSummary by CodeRabbit