OCPBUGS-88309: Updating ose-cluster-baremetal-operator-container image to be consistent with ART for 5.0#613
Conversation
|
Created by ART pipeline job run https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Fsync-ci-images/269 |
WalkthroughA warning comment is added to the top of the Dockerfile signaling that ART metadata configuration declares a different number of ChangesART Metadata Mismatch Warning
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile (2)
7-9: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCopy specific files instead of entire context.
Line 8 copies the entire build context with
COPY . ., which violates container security guidelines. This can inadvertently include sensitive files, large artifacts, or unnecessary content in the build context.🔒 Proposed fix to copy specific files
WORKDIR /go/src/github.com/openshift/cluster-baremetal-operator -COPY . . +COPY go.mod go.sum ./ +COPY cmd/ ./cmd/ +COPY pkg/ ./pkg/ +COPY api/ ./api/ +COPY controllers/ ./controllers/ +COPY provisioning/ ./provisioning/ +COPY manifests/ ./manifests/ +COPY vendor/ ./vendor/ RUN make buildAdjust the list based on what
make buildactually requires.As per coding guidelines: "COPY specific files, not entire context" from the container security (prodsec-skills) section for Dockerfile patterns.
🤖 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 `@Dockerfile` around lines 7 - 9, The Dockerfile currently uses COPY . . which pulls the entire build context into the image; replace that with explicit COPY statements for only the files/dirs needed for make build (e.g., COPY Go module files and source dirs and the Makefile) so sensitive/large files are not included. Update the Dockerfile around the WORKDIR, COPY . ., RUN make build sequence: remove COPY . ., add explicit COPY entries for go.mod, go.sum, Makefile and the source directories used by the build (e.g., cmd/, pkg/, internal/ or whatever this repository requires), then run RUN make build as before.Source: Coding guidelines
21-29:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Missing USER directive violates container security guidelines.
The Dockerfile runs as root by default because no USER directive is specified. As per coding guidelines, containers must run as non-root. This is a critical security issue that should be addressed when reconciling the ART metadata mismatch.
🔒 Proposed fix to add non-root user
Add a USER directive before the ENTRYPOINT in the final stage:
COPY --from=test-extension-builder /go/src/github.com/openshift/cluster-baremetal-operator/tests-extension/bin/cluster-baremetal-operator-test-extension.tar.gz /usr/bin/ LABEL io.openshift.release.operator=true +USER 1001 ENTRYPOINT ["/usr/bin/cluster-baremetal-operator"]As per coding guidelines: "USER non-root; never run as root" from the container security (prodsec-skills) section for Dockerfile patterns.
Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (1)
Dockerfile (1)
28-29: ⚡ Quick winConsider adding a HEALTHCHECK directive.
The Dockerfile lacks a HEALTHCHECK directive, which is required by container security guidelines. Health checks enable Kubernetes to detect and restart unhealthy pods.
💊 Proposed HEALTHCHECK addition
LABEL io.openshift.release.operator=true +HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ + CMD ["/usr/bin/cluster-baremetal-operator", "health-check"] || exit 1 ENTRYPOINT ["/usr/bin/cluster-baremetal-operator"]Note: Adjust the health-check command based on what the operator binary supports. If the operator doesn't have a health endpoint, you may need to add one or use a simple process check.
As per coding guidelines: "HEALTHCHECK defined" from the container security (prodsec-skills) section for Dockerfile patterns.
🤖 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 `@Dockerfile` around lines 28 - 29, Add a Docker HEALTHCHECK so the container runtime can detect unhealthy operator processes; modify the Dockerfile (near the ENTRYPOINT line for /usr/bin/cluster-baremetal-operator) to include a HEALTHCHECK that runs a suitable probe (e.g., the operator's built‑in health endpoint or a lightweight check such as invoking the binary with a health/status flag or verifying the process is alive) and set appropriate --interval/--timeout/--retries values; ensure the command returns proper exit codes so Kubernetes can restart failing pods.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@Dockerfile`:
- Around line 7-9: The Dockerfile currently uses COPY . . which pulls the entire
build context into the image; replace that with explicit COPY statements for
only the files/dirs needed for make build (e.g., COPY Go module files and source
dirs and the Makefile) so sensitive/large files are not included. Update the
Dockerfile around the WORKDIR, COPY . ., RUN make build sequence: remove COPY .
., add explicit COPY entries for go.mod, go.sum, Makefile and the source
directories used by the build (e.g., cmd/, pkg/, internal/ or whatever this
repository requires), then run RUN make build as before.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 28-29: Add a Docker HEALTHCHECK so the container runtime can
detect unhealthy operator processes; modify the Dockerfile (near the ENTRYPOINT
line for /usr/bin/cluster-baremetal-operator) to include a HEALTHCHECK that runs
a suitable probe (e.g., the operator's built‑in health endpoint or a lightweight
check such as invoking the binary with a health/status flag or verifying the
process is alive) and set appropriate --interval/--timeout/--retries values;
ensure the command returns proper exit codes so Kubernetes can restart failing
pods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e6ff62e-eba3-4959-919c-fb53bce1b305
📒 Files selected for processing (1)
Dockerfile
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-88309, 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-bot: This pull request references Jira Issue OCPBUGS-88309, 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. |
a69e56b to
5877ce3
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: openshift-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Dockerfile`:
- Around line 20-28: Add a HEALTHCHECK directive to the Dockerfile to meet
container security guidelines. Insert the HEALTHCHECK instruction before the
ENTRYPOINT directive (which runs cluster-baremetal-operator). The health check
command should be based on what health-check mechanism the
cluster-baremetal-operator binary actually supports (such as a health
subcommand, an HTTP endpoint, or a readiness probe alternative) - verify what
endpoint or mechanism the operator exposes for health checking and use that in
the HEALTHCHECK command.
- Around line 27-28: The Dockerfile currently runs the container as root because
there is no USER directive before the ENTRYPOINT command. Add a USER directive
before the ENTRYPOINT to specify a non-root user (e.g., USER 65534:65534 for the
nobody user, or a dedicated user created in the base image). Insert this
directive between the LABEL statement and the ENTRYPOINT line to ensure the
container runs with proper non-root privileges and satisfies the security
requirement.
- Around line 1-3: Add three missing security configurations and resolve the ART
metadata mismatch. First, add a USER directive with UID 1000:1000 before the
ENTRYPOINT instruction to prevent the container from running as root. Second,
add a HEALTHCHECK directive per container security guidelines. Third, resolve
the ART metadata FROM count mismatch by either updating the ART metadata
configuration in
openshift-eng/ocp-build-data/images/ose-cluster-baremetal-operator.yml to expect
3 FROM statements (builder, test-extension-builder, and final stages) or
restructuring the Dockerfile to match the expected FROM count in the metadata.
Once the mismatch is resolved, remove the warning comment at lines 1-3.
🪄 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: 96f57050-0797-46f1-a3ff-a83d500db7d7
📒 Files selected for processing (1)
Dockerfile
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@Dockerfile`:
- Around line 20-28: Add a HEALTHCHECK directive to the Dockerfile to meet
container security guidelines. Insert the HEALTHCHECK instruction before the
ENTRYPOINT directive (which runs cluster-baremetal-operator). The health check
command should be based on what health-check mechanism the
cluster-baremetal-operator binary actually supports (such as a health
subcommand, an HTTP endpoint, or a readiness probe alternative) - verify what
endpoint or mechanism the operator exposes for health checking and use that in
the HEALTHCHECK command.
- Around line 27-28: The Dockerfile currently runs the container as root because
there is no USER directive before the ENTRYPOINT command. Add a USER directive
before the ENTRYPOINT to specify a non-root user (e.g., USER 65534:65534 for the
nobody user, or a dedicated user created in the base image). Insert this
directive between the LABEL statement and the ENTRYPOINT line to ensure the
container runs with proper non-root privileges and satisfies the security
requirement.
- Around line 1-3: Add three missing security configurations and resolve the ART
metadata mismatch. First, add a USER directive with UID 1000:1000 before the
ENTRYPOINT instruction to prevent the container from running as root. Second,
add a HEALTHCHECK directive per container security guidelines. Third, resolve
the ART metadata FROM count mismatch by either updating the ART metadata
configuration in
openshift-eng/ocp-build-data/images/ose-cluster-baremetal-operator.yml to expect
3 FROM statements (builder, test-extension-builder, and final stages) or
restructuring the Dockerfile to match the expected FROM count in the metadata.
Once the mismatch is resolved, remove the warning comment at lines 1-3.
🪄 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: 96f57050-0797-46f1-a3ff-a83d500db7d7
📒 Files selected for processing (1)
Dockerfile
🛑 Comments failed to post (3)
Dockerfile (3)
1-3:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
# Find and examine the Dockerfile find . -name "Dockerfile" -o -name "Containerfile" | head -20Repository: openshift/cluster-baremetal-operator
Length of output: 399
🏁 Script executed:
# Search for ART metadata configuration files find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "ose-cluster-baremetal-operator" 2>/dev/null | head -10Repository: openshift/cluster-baremetal-operator
Length of output: 62
🏁 Script executed:
# Look for any images configuration in standard locations find . -path "*/images/*" -name "*.yml" -o -path "*/images/*" -name "*.yaml" | head -20Repository: openshift/cluster-baremetal-operator
Length of output: 62
🏁 Script executed:
# Examine the main Dockerfile cat -n ./DockerfileRepository: openshift/cluster-baremetal-operator
Length of output: 1751
🏁 Script executed:
# Search for ART-related configuration files find . -type f -name "*.yml" -o -name "*.yaml" | grep -v vendor | head -30Repository: openshift/cluster-baremetal-operator
Length of output: 1417
🏁 Script executed:
# Search for ART or metadata references in the codebase rg "ART|ose-cluster-baremetal-operator" --type yaml --type yml -i | head -20Repository: openshift/cluster-baremetal-operator
Length of output: 108
🏁 Script executed:
# Check if there's a FROM count in any configuration rg "FROM" -i | grep -v vendor | head -30Repository: openshift/cluster-baremetal-operator
Length of output: 3142
🏁 Script executed:
# Examine CI operator configuration for ART metadata cat -n ./.ci-operator.yaml | head -100Repository: openshift/cluster-baremetal-operator
Length of output: 213
🏁 Script executed:
# Search for any references to the warning comment or ART issues rg "ART metadata|different number of FROMs" --type yamlRepository: openshift/cluster-baremetal-operator
Length of output: 62
🏁 Script executed:
# Look for build configuration related to FROM statements or image metadata rg "FROM|metadata|image" .ci-operator.yaml -A 2 -B 2 | head -50Repository: openshift/cluster-baremetal-operator
Length of output: 137
🏁 Script executed:
# Check if there's an openshift/release repo reference or similar rg "openshift/release|ose-cluster-baremetal" -iRepository: openshift/cluster-baremetal-operator
Length of output: 932
🏁 Script executed:
# Check git history to understand when this warning comment was added git log --oneline -p -- Dockerfile | grep -A 5 -B 5 "ART metadata" | head -40Repository: openshift/cluster-baremetal-operator
Length of output: 580
🏁 Script executed:
# Search for any issue or PR references related to this warning rg "test-extension|ote-migration" --type yamlRepository: openshift/cluster-baremetal-operator
Length of output: 62
🏁 Script executed:
# Check if there are any container security scanning tools or validation in the repo find . -name "*trivy*" -o -name "*security*" | grep -v vendorRepository: openshift/cluster-baremetal-operator
Length of output: 62
🏁 Script executed:
# Look for any documentation or scripts about the test-extension migration rg "ote-migration|test-extension-builder" -lRepository: openshift/cluster-baremetal-operator
Length of output: 91
🏁 Script executed:
# Verify the exact status - see if there's any indication this has been resolved git log --all --oneline -- Dockerfile | head -10Repository: openshift/cluster-baremetal-operator
Length of output: 1792
🏁 Script executed:
# Check the most recent commit that touched Dockerfile git show 5877ce34 -- Dockerfile | head -80Repository: openshift/cluster-baremetal-operator
Length of output: 2111
🏁 Script executed:
# Verify current container security issues: USER directive grep -n "^USER" DockerfileRepository: openshift/cluster-baremetal-operator
Length of output: 62
🏁 Script executed:
# Verify HEALTHCHECK presence grep -n "HEALTHCHECK" DockerfileRepository: openshift/cluster-baremetal-operator
Length of output: 62
Add USER directive and HEALTHCHECK; address the ART metadata FROM count mismatch flagged in the warning comment.
The Dockerfile has three container security guideline violations:
Missing USER directive: Container runs as root by default. Add
USER 1000:1000(or appropriate non-root UID) beforeENTRYPOINT.Missing HEALTHCHECK: Required per guidelines. Add a
HEALTHCHECKdirective.ART metadata FROM count mismatch: The warning comment (lines 1-3) indicates the automated ART reconciliation process detected a mismatch between the Dockerfile's FROM statement count and the ART metadata configuration. This warning was generated by the AOS Automation Release Team during the most recent update but remains unresolved. The FROM count (3 statements: builder, test-extension-builder, final) must align with the ART metadata in
openshift-eng/ocp-build-data/images/ose-cluster-baremetal-operator.yml. Either update the ART metadata to expect 3 FROMs, or restructure the Dockerfile to match the expected count. The warning comment itself should be removed once the mismatch is resolved.🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 `@Dockerfile` around lines 1 - 3, Add three missing security configurations and resolve the ART metadata mismatch. First, add a USER directive with UID 1000:1000 before the ENTRYPOINT instruction to prevent the container from running as root. Second, add a HEALTHCHECK directive per container security guidelines. Third, resolve the ART metadata FROM count mismatch by either updating the ART metadata configuration in openshift-eng/ocp-build-data/images/ose-cluster-baremetal-operator.yml to expect 3 FROM statements (builder, test-extension-builder, and final stages) or restructuring the Dockerfile to match the expected FROM count in the metadata. Once the mismatch is resolved, remove the warning comment at lines 1-3.
20-28:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
HEALTHCHECKdirective.The container security guidelines require a
HEALTHCHECKto be defined. Add a health check to allow orchestrators (e.g., OpenShift) to monitor container health.💚 Proposed fix to add HEALTHCHECK
FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 COPY --from=builder /go/src/github.com/openshift/cluster-baremetal-operator/bin/cluster-baremetal-operator /usr/bin/cluster-baremetal-operator COPY --from=builder /go/src/github.com/openshift/cluster-baremetal-operator/manifests /manifests # Copy test extension binary (added by ote-migration) COPY --from=test-extension-builder /go/src/github.com/openshift/cluster-baremetal-operator/tests-extension/bin/cluster-baremetal-operator-tests-ext.gz /usr/bin/ LABEL io.openshift.release.operator=true +HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ + CMD /usr/bin/cluster-baremetal-operator health USER 65534:65534 ENTRYPOINT ["/usr/bin/cluster-baremetal-operator"]Note: This assumes the operator binary supports a
healthsubcommand or similar health-check mechanism. Adjust the command based on your operator's actual health-check interface.🤖 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 `@Dockerfile` around lines 20 - 28, Add a HEALTHCHECK directive to the Dockerfile to meet container security guidelines. Insert the HEALTHCHECK instruction before the ENTRYPOINT directive (which runs cluster-baremetal-operator). The health check command should be based on what health-check mechanism the cluster-baremetal-operator binary actually supports (such as a health subcommand, an HTTP endpoint, or a readiness probe alternative) - verify what endpoint or mechanism the operator exposes for health checking and use that in the HEALTHCHECK command.Source: Coding guidelines
27-28:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
USERnon-root directive: container will run as root.The final stage has no
USERdirective before theENTRYPOINT, so the container defaults to running as root. This violates the container security guideline "USER non-root; never run as root" and the Trivy static check (DS-0002). Add aUSERline specifying a non-root user (e.g.,USER 65534:65534for thenobodyuser or create a dedicated user in the base image).🔒 Proposed fix to add non-root USER
COPY --from=test-extension-builder /go/src/github.com/openshift/cluster-baremetal-operator/tests-extension/bin/cluster-baremetal-operator-tests-ext.gz /usr/bin/ LABEL io.openshift.release.operator=true +USER 65534:65534 ENTRYPOINT ["/usr/bin/cluster-baremetal-operator"]🤖 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 `@Dockerfile` around lines 27 - 28, The Dockerfile currently runs the container as root because there is no USER directive before the ENTRYPOINT command. Add a USER directive before the ENTRYPOINT to specify a non-root user (e.g., USER 65534:65534 for the nobody user, or a dedicated user created in the base image). Insert this directive between the LABEL statement and the ENTRYPOINT line to ensure the container runs with proper non-root privileges and satisfies the security requirement.Source: Coding guidelines
|
@openshift-bot: 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. |
Updating ose-cluster-baremetal-operator-container image to be consistent with ART for 5.0
TLDR:
Product builds by ART can be configured for different base and builder images than corresponding CI
builds. This automated PR requests a change to CI configuration to align with ART's configuration;
please take steps to merge it quickly or contact ART to coordinate changes.
The configuration in the following ART component metadata is driving this alignment request:
ose-cluster-baremetal-operator.yml.
Detail:
This repository is out of sync with the downstream product builds for this component. The CI
configuration for at least one image differs from ART's expected product configuration. This should
be addressed to ensure that the component's CI testing accurate reflects what customers will
experience.
Most of these PRs are opened as an ART-driven proposal to migrate base image or builder(s) to a
different version, usually prior to GA. The intent is to effect changes in both configurations
simultaneously without breaking either CI or ART builds, so usually ART builds are configured to
consider CI as canonical and attempt to match CI config until the PR merges to align both. ART may
also configure changes in GA releases with CI remaining canonical for a brief grace period to enable
CI to succeed and the alignment PR to merge. In either case, ART configuration will be made
canonical at some point (typically at branch-cut before GA or release dev-cut after GA), so it is
important to align CI configuration as soon as possible.
PRs are also triggered when CI configuration changes without ART coordination, for instance to
change the number of builder images or to use a different golang version. These changes should be
coordinated with ART; whether ART configuration is canonical or not, preferably it would be updated
first to enable the changes to occur simultaneously in both CI and ART at the same time. This also
gives ART a chance to validate the intended changes first. For instance, ART compiles most
components with the Golang version being used by the control plane for a given OpenShift release.
Exceptions to this convention (i.e. you believe your component must be compiled with a Golang
version independent from the control plane) must be granted by the OpenShift staff engineers and
communicated to the ART team.
Roles & Responsibilities:
tests OR that necessary metadata changes are reported to the ART team
in
#forum-ocp-arton Slack. If necessary, the changes required by this pull request can beintroduced with a separate PR opened by the component team. Once the repository is aligned,
this PR will be closed automatically.
verify-depsis complaining. In that case, please opena new PR with the dependency issues addressed (and base images bumped). ART-9595 for reference.
any required labels to ensure the PR merges once tests are passing. In cases where ART config is
canonical, downstream builds are already being built with these changes, and merging this PR
only improves the fidelity of our CI. In cases where ART config is not canonical, this provides
a grace period for the component team to align their CI with ART's configuration before it becomes
canonical in product builds.
ART has been configured to reconcile your CI build root image (see https://docs.ci.openshift.org/docs/architecture/ci-operator/#build-root-image).
In order for your upstream .ci-operator.yaml configuration to be honored, you must set the following in your openshift/release ci-operator configuration file:
Change behavior of future PRs:
set up automatically. This means that such a PR would merge without human intervention (and awareness!) in the future.
To do so, open a PR to set the
auto_labelattribute in the image configuration. ExampleUPSTREAM: <carry>:. An example.If you have any questions about this pull request, please reach out in the
#forum-ocp-artSlack channel.