MGMT-24453: upgrade to golang 1.26#2171
Open
shay23bra wants to merge 1 commit into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@shay23bra: This pull request references MGMT-24453 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughThis pull request upgrades the Go toolchain from 1.25 to 1.26 across the repository: ChangesGo 1.26 toolchain upgrade
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
openshift-ci
Bot
added
the
size/S
openshift-ci
Bot
added
the
downstream-change-needed
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shay23bra 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2171 +/- ##
=======================================
Coverage 48.82% 48.82%
=======================================
Files 20 20
Lines 4397 4397
=======================================
Hits 2147 2147
Misses 2026 2026
Partials 224 224 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
Dockerfile.assisted-installer-mce (3)
1-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefine a healthcheck for the final image.
This Dockerfile lacks
HEALTHCHECK.As per coding guidelines: "HEALTHCHECK defined."
🤖 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.assisted-installer-mce` around lines 1 - 51, Add a HEALTHCHECK to the final image: in the final stage (after ENTRYPOINT and COPY of /installer) add a HEALTHCHECK instruction that runs a lightweight command against the running binary (e.g., invoking /installer's health probe or checking a local HTTP endpoint or PID) with sensible interval, timeout and retries; ensure the instruction references the installed binary (/installer) or its health endpoint so container platforms can detect service health.
15-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
COPYto required files only.This copies the full repository context into the image build context.
As per coding guidelines: "COPY specific files, not entire context."
🤖 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.assisted-installer-mce` at line 15, The Dockerfile currently uses a broad COPY --chown=${USER_UID} . /app which pulls the entire repo into the image; change this to copy only the required artifacts (e.g., specific files and directories like package.json, package-lock.json, src/, bin/, build outputs, or other runtime assets) by replacing the single-dot COPY with explicit COPY lines (still using --chown=${USER_UID}) for each needed file/dir and add/verify a .dockerignore to exclude everything else; update the COPY statement in Dockerfile.assisted-installer-mce (the COPY --chown=${USER_UID} . /app line) accordingly and ensure the build context contains the listed files.
20-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a non-root runtime user and add a HEALTHCHECK to the final image.
- The final (ubi-minimal) stage has no
USER, so the container runs as root by default.- There is no
HEALTHCHECKdirective in the Dockerfile.- The only
COPY ... . /apphappens in the builder stage (COPY --chown=${USER_UID} . /app); if the guideline is meant to apply to runtime only, this part is not in the final stage.🤖 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.assisted-installer-mce` around lines 20 - 30, Add a non-root runtime user in the final image and a HEALTHCHECK: create a dedicated user/group (e.g., installer user) in the final stage, ensure ownership of runtime assets copied into the image (chown /installer and /assisted-installer-controller or recreate files with correct ownership), switch to that user with USER before the ENTRYPOINT, and add a HEALTHCHECK directive that probes the running service (return non-zero on failure) to allow container health monitoring; reference the final-stage artifacts (/installer, /assisted-installer-controller) and the existing ENTRYPOINT when making these changes.Dockerfile.assisted-installer-downstream (3)
13-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid copying the entire build context.
COPY --chown=${USER_UID} . /apppulls everything from context; scope this to required files/directories.As per coding guidelines: "COPY specific files, not entire context."
🤖 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.assisted-installer-downstream` at line 13, The Dockerfile currently uses a broad COPY instruction ("COPY --chown=${USER_UID} . /app") that copies the entire build context; change it to copy only the required files and directories (e.g., specific source dirs, package files, build artifacts) using explicit COPY lines with the same --chown=${USER_UID} and target /app (or subpaths) so unintended files aren’t included; update any related build steps that expect those files and remove the blanket "COPY --chown=${USER_UID} . /app" entry.
1-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a container healthcheck.
No
HEALTHCHECKis defined for this image.As per coding guidelines: "HEALTHCHECK defined."
🤖 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.assisted-installer-downstream` around lines 1 - 48, Add a Docker HEALTHCHECK after the existing ENTRYPOINT to satisfy the "HEALTHCHECK defined" guideline: for this image (ENTRYPOINT ["/installer"]) add a HEALTHCHECK stanza (e.g., HEALTHCHECK --interval=30s --timeout=5s --start-period=30s CMD pgrep -f /installer >/dev/null || exit 1) so the container runtime can detect unhealthy /installer processes; update the Dockerfile.assisted-installer-downstream by inserting that HEALTHCHECK instruction after the ENTRYPOINT line.
17-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the final stage as non-root
Dockerfile.assisted-installer-downstream’s runtime stage (theubi-minimalFROM ...) has noUSERinstruction, so it will run as root by default.🤖 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.assisted-installer-downstream` around lines 17 - 28, The final image stage runs as root because there is no USER set; create a non-root user and switch to it in this runtime stage: add creation of a dedicated unprivileged user (e.g., installer or appuser) and group, chown the copied runtime artifacts (/installer and /assisted-installer-controller/deploy) to that user, and then add a USER instruction before the ENTRYPOINT so ENTRYPOINT ["/installer"] runs non-root; ensure file permissions allow execution by that user.Dockerfile.assisted-installer.ocp (2)
10-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a
HEALTHCHECKto the runtime image.The final stage does not define a healthcheck.
As per coding guidelines: "HEALTHCHECK defined."
🤖 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.assisted-installer.ocp` around lines 10 - 18, The final image stage is missing a HEALTHCHECK; add a Docker HEALTHCHECK instruction to the runtime stage (the stage that sets ENTRYPOINT [/usr/bin/installer] and copies /usr/bin/installer and /assisted-installer-controller/deploy) that performs a lightweight readiness probe (for example, an HTTP GET against the service readiness endpoint or invoking a built-in health command on /usr/bin/installer), and configure sensible parameters (interval, timeout, start-period, retries) so container orchestration can detect unhealthy containers.
10-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRuntime stage still runs as root by default.
No
USERinstruction is set in the final stage.As per coding guidelines: "
USERnon-root; never run as root."🤖 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.assisted-installer.ocp` around lines 10 - 18, The final stage of the Dockerfile runs as root because there is no USER instruction; modify the final stage (after the COPY lines and before ENTRYPOINT) to switch to a non-root user: create or use a dedicated non-root user (or UID/GID), ensure the copied binaries (/usr/bin/installer and /assisted-installer-controller/deploy) are owned/accessible by that user (use chown in the COPY or run chown/chmod earlier), and then add USER <non-root-user-or-uid> so ENTRYPOINT ["/usr/bin/installer"] runs without root privileges.
🤖 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.assisted-installer.ocp`:
- Line 1: Update the Dockerfile base image and runtime security directives:
replace the tag-only FROM registry.ci.openshift.org image with a
catalog.redhat.com UBI or approved distroless base (update the FROM instruction
identified by the current builder stage), pin to a digest, avoid using tag-only
image sources; do not rely on root—add a non-root USER instruction and ensure
any build-only user changes happen in the builder stage; add a HEALTHCHECK
instruction to provide runtime liveness information; and stop using COPY . . —
change the COPY that currently copies the entire build context to copy only the
specific source/artifacts needed for the build.
---
Outside diff comments:
In `@Dockerfile.assisted-installer-downstream`:
- Line 13: The Dockerfile currently uses a broad COPY instruction ("COPY
--chown=${USER_UID} . /app") that copies the entire build context; change it to
copy only the required files and directories (e.g., specific source dirs,
package files, build artifacts) using explicit COPY lines with the same
--chown=${USER_UID} and target /app (or subpaths) so unintended files aren’t
included; update any related build steps that expect those files and remove the
blanket "COPY --chown=${USER_UID} . /app" entry.
- Around line 1-48: Add a Docker HEALTHCHECK after the existing ENTRYPOINT to
satisfy the "HEALTHCHECK defined" guideline: for this image (ENTRYPOINT
["/installer"]) add a HEALTHCHECK stanza (e.g., HEALTHCHECK --interval=30s
--timeout=5s --start-period=30s CMD pgrep -f /installer >/dev/null || exit 1) so
the container runtime can detect unhealthy /installer processes; update the
Dockerfile.assisted-installer-downstream by inserting that HEALTHCHECK
instruction after the ENTRYPOINT line.
- Around line 17-28: The final image stage runs as root because there is no USER
set; create a non-root user and switch to it in this runtime stage: add creation
of a dedicated unprivileged user (e.g., installer or appuser) and group, chown
the copied runtime artifacts (/installer and
/assisted-installer-controller/deploy) to that user, and then add a USER
instruction before the ENTRYPOINT so ENTRYPOINT ["/installer"] runs non-root;
ensure file permissions allow execution by that user.
In `@Dockerfile.assisted-installer-mce`:
- Around line 1-51: Add a HEALTHCHECK to the final image: in the final stage
(after ENTRYPOINT and COPY of /installer) add a HEALTHCHECK instruction that
runs a lightweight command against the running binary (e.g., invoking
/installer's health probe or checking a local HTTP endpoint or PID) with
sensible interval, timeout and retries; ensure the instruction references the
installed binary (/installer) or its health endpoint so container platforms can
detect service health.
- Line 15: The Dockerfile currently uses a broad COPY --chown=${USER_UID} . /app
which pulls the entire repo into the image; change this to copy only the
required artifacts (e.g., specific files and directories like package.json,
package-lock.json, src/, bin/, build outputs, or other runtime assets) by
replacing the single-dot COPY with explicit COPY lines (still using
--chown=${USER_UID}) for each needed file/dir and add/verify a .dockerignore to
exclude everything else; update the COPY statement in
Dockerfile.assisted-installer-mce (the COPY --chown=${USER_UID} . /app line)
accordingly and ensure the build context contains the listed files.
- Around line 20-30: Add a non-root runtime user in the final image and a
HEALTHCHECK: create a dedicated user/group (e.g., installer user) in the final
stage, ensure ownership of runtime assets copied into the image (chown
/installer and /assisted-installer-controller or recreate files with correct
ownership), switch to that user with USER before the ENTRYPOINT, and add a
HEALTHCHECK directive that probes the running service (return non-zero on
failure) to allow container health monitoring; reference the final-stage
artifacts (/installer, /assisted-installer-controller) and the existing
ENTRYPOINT when making these changes.
In `@Dockerfile.assisted-installer.ocp`:
- Around line 10-18: The final image stage is missing a HEALTHCHECK; add a
Docker HEALTHCHECK instruction to the runtime stage (the stage that sets
ENTRYPOINT [/usr/bin/installer] and copies /usr/bin/installer and
/assisted-installer-controller/deploy) that performs a lightweight readiness
probe (for example, an HTTP GET against the service readiness endpoint or
invoking a built-in health command on /usr/bin/installer), and configure
sensible parameters (interval, timeout, start-period, retries) so container
orchestration can detect unhealthy containers.
- Around line 10-18: The final stage of the Dockerfile runs as root because
there is no USER instruction; modify the final stage (after the COPY lines and
before ENTRYPOINT) to switch to a non-root user: create or use a dedicated
non-root user (or UID/GID), ensure the copied binaries (/usr/bin/installer and
/assisted-installer-controller/deploy) are owned/accessible by that user (use
chown in the COPY or run chown/chmod earlier), and then add USER
<non-root-user-or-uid> so ENTRYPOINT ["/usr/bin/installer"] runs without root
privileges.
🪄 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: 172fffc3-0a28-4a42-9258-4291981b206d
📒 Files selected for processing (11)
.ci-operator.yamlDockerfile.assisted-installerDockerfile.assisted-installer-buildDockerfile.assisted-installer-controllerDockerfile.assisted-installer-controller-downstreamDockerfile.assisted-installer-controller-mceDockerfile.assisted-installer-controller.ocpDockerfile.assisted-installer-downstreamDockerfile.assisted-installer-mceDockerfile.assisted-installer.ocpgo.mod
| @@ -1,4 +1,4 @@ | |||
| FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.25-openshift-4.21 AS builder | |||
| FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.26-openshift-5.0 AS builder | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect base images and whether they are digest pinned
nl -ba Dockerfile.assisted-installer.ocp | sed -n '1,12p'Repository: openshift/assisted-installer
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Dockerfile.assisted-installer.ocp"
echo "== File =="
ls -la "$FILE" || true
echo "== Line count =="
wc -l "$FILE" || true
echo "== First 80 lines =="
sed -n '1,80p' "$FILE" || true
echo "== Lines containing key directives =="
rg -n "^(FROM|USER|HEALTHCHECK|COPY|RUN|ARG|ENV)" "$FILE" || true
echo "== FROM lines (all stages) =="
rg -n "^FROM " "$FILE" || trueRepository: openshift/assisted-installer
Length of output: 1482
Fix container base image policy and missing runtime security directives in Dockerfile.assisted-installer.ocp.
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.26-openshift-5.0 AS builder
- Base images are from
registry.ci.openshift.organd are tag-only (no digest pinning), notcatalog.redhat.comUBI/distroless. - No
USERis set (runtime defaults to root). - No
HEALTHCHECKis defined. COPY . .copies the entire build context instead of specific files.
🤖 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.assisted-installer.ocp` at line 1, Update the Dockerfile base
image and runtime security directives: replace the tag-only FROM
registry.ci.openshift.org image with a catalog.redhat.com UBI or approved
distroless base (update the FROM instruction identified by the current builder
stage), pin to a digest, avoid using tag-only image sources; do not rely on
root—add a non-root USER instruction and ensure any build-only user changes
happen in the builder stage; add a HEALTHCHECK instruction to provide runtime
liveness information; and stop using COPY . . — change the COPY that currently
copies the entire build context to copy only the specific source/artifacts
needed for the build.
shay23bra
force-pushed
the
MGMT-24453-upgrade-golang-1.26
branch
2 times, most recently
from
7eb73c7 to
1107df4
Compare
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 (3)
Dockerfile.assisted-installer-controller.ocp (1)
9-17: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd runtime security directives to the final stage.
The runtime stage is missing:
USERdirective (defaults to root)HEALTHCHECKinstructionAdditionally, line 6 uses
COPY . .which copies the entire build context instead of specific files.As per coding guidelines, containers should run as non-root, include health checks, and copy only necessary files.
🛡️ Recommended security improvements
FROM registry.ci.openshift.org/ocp/4.21:base-rhel9 LABEL io.openshift.release.operator=true COPY --from=builder /go/src/github.com/openshift/assisted-installer/build/assisted-installer-controller /usr/bin/assisted-installer-controller RUN dnf install -y --nodocs --setopt=install_weak_deps=False openshift-clients \ && dnf clean all && rm -rf /var/cache/* +# Create non-root user +RUN useradd -u 1001 -r -g 0 -s /sbin/nologin controller + +USER 1001 + +HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ + CMD ["/usr/bin/assisted-installer-controller", "--health-check"] || exit 1 + ENTRYPOINT ["/usr/bin/assisted-installer-controller"]For the builder stage at line 6:
-COPY . . +COPY go.mod go.sum ./ +COPY cmd/ cmd/ +COPY pkg/ pkg/ +COPY vendor/ vendor/Note: Adjust the HEALTHCHECK and COPY paths to match your actual application structure.
As per coding guidelines for container security.
🤖 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.assisted-installer-controller.ocp` around lines 9 - 17, The final image should run non-root and include a healthcheck and narrow COPYs: replace any broad COPY . . usage in the builder/final stage with explicit file copies (e.g., copying only the built binary and required assets instead of the whole context), add a USER directive (e.g., create or switch to a non-root account and add USER <username|uid>) before ENTRYPOINT, and add a HEALTHCHECK instruction that exercises the service (for example an HTTP GET or exec that checks /health) referencing the existing ENTRYPOINT (/usr/bin/assisted-installer-controller) so the container reports liveness; ensure file ownership/permissions allow the non-root user to execute the binary.Dockerfile.assisted-installer-build (1)
13-34:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAddress container security violations in the final stage.
The final stage has multiple security issues:
- Line 13: Base image is
quay.io/centos/centos:stream9, not UBI minimal or distroless from catalog.redhat.com- No
USERdirective (defaults to root)- No
HEALTHCHECKdefined- Lines 31-32:
COPY --from=golang $GOPATH $GOPATHandCOPY --from=golang $GOROOT $GOROOTcopy entire directories instead of specific filesAs per coding guidelines, production container images should use UBI minimal or distroless base images, run as non-root, include health checks, and copy only necessary files.
🛡️ Recommended security improvements
Consider these changes to align with container security guidelines:
- Replace the base image with UBI minimal:
-FROM quay.io/centos/centos:stream9 +FROM registry.access.redhat.com/ubi9/ubi-minimal:latest
- Add a non-root user:
+RUN microdnf install -y shadow-utils && \ + useradd -u 1001 -r -g 0 -s /sbin/nologin builder && \ + microdnf remove shadow-utils && \ + microdnf clean all
- Set USER before COPY commands:
+USER 1001
- Add a HEALTHCHECK if applicable to your build environment.
As per coding guidelines for container security.
🤖 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.assisted-installer-build` around lines 13 - 34, The final build stage uses an insecure base and copies entire Go directories as root; update the Dockerfile to use a UBI minimal or distroless base instead of quay.io/centos/centos:stream9, create and switch to a non-root user via a USER directive (add user/group and set ownership) before any COPY operations, replace the broad COPY --from=golang $GOPATH $GOPATH and COPY --from=golang $GOROOT $GOROOT with copying only the required binaries/artifacts (referencing the COPY --from=golang entries), and add an appropriate HEALTHCHECK line; ensure ENV GOROOT/GOPATH remain correct and adjust file ownership/permissions for the non-root user.Dockerfile.assisted-installer-controller (1)
22-37: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd runtime security directives to the final stage.
The runtime stage is missing:
USERdirective (defaults to root)HEALTHCHECKinstructionAs per coding guidelines, containers should run as non-root and include health checks.
🛡️ Recommended security improvements
FROM registry.access.redhat.com/ubi9/ubi-minimal:latest@sha256:6fc28bcb6776e387d7a35a2056d9d2b985dc4e26031e98a2bd35a7137cd6fd71 ARG TARGETPLATFORM RUN microdnf install -y tar gzip rsync which procps-ng findutils && microdnf clean all + +# Create non-root user +RUN microdnf install -y shadow-utils && \ + useradd -u 1001 -r -g 0 -s /sbin/nologin controller && \ + microdnf remove shadow-utils && \ + microdnf clean all # openshift clients RUN case $TARGETPLATFORM in "") platform=amd64;; *) platform=`echo $TARGETPLATFORM | sed 's#linux/##'` ;; esac ; \ curl -k -L -s "https://mirror.openshift.com/pub/openshift-v4/multi/clients/ocp/latest/amd64/openshift-client-linux-${platform}-rhel9.tar.gz" | tar xvz -C /usr/bin; \ chmod +x /usr/bin/oc /usr/bin/kubectl COPY --from=builder /go/src/github.com/openshift/assisted-installer/build/assisted-installer-controller /usr/bin/assisted-installer-controller # Copy the commit reference from the builder COPY --from=builder /commit-reference.txt /commit-reference.txt +USER 1001 + +HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ + CMD ["/usr/bin/assisted-installer-controller", "--health-check"] || exit 1 + ENTRYPOINT ["/usr/bin/assisted-installer-controller"]Note: Adjust the HEALTHCHECK command to match the actual health check endpoint/flag supported by your controller.
As per coding guidelines for container security.
🤖 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.assisted-installer-controller` around lines 22 - 37, The final runtime stage in the Dockerfile for assisted-installer-controller is missing required security directives. Update the last stage after the COPY steps to add a non-root USER and a HEALTHCHECK using a command supported by assisted-installer-controller so the container does not default to root and exposes a liveness check. Keep the changes in the final stage only, and make sure the USER and HEALTHCHECK are placed before ENTRYPOINT.
♻️ Duplicate comments (1)
Dockerfile.assisted-installer.ocp (1)
1-17: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd runtime security directives; note that CI registry images are acceptable.
Similar to previous review comments, this Dockerfile is missing:
USERdirective (defaults to root)HEALTHCHECKinstructionAdditionally, line 6 uses
COPY . .which copies the entire build context.Note: The use of
registry.ci.openshift.orgimages with floating tags is acceptable per coding guidelines ("Red Hat images: use floating tags"). The previous comment's concern about usingcatalog.redhat.comdoes not apply to OpenShift CI internal builds.🛡️ Recommended security improvements
FROM registry.ci.openshift.org/ocp/4.21:base-rhel9 LABEL io.openshift.release.operator=true COPY --from=builder /go/src/github.com/openshift/assisted-installer/build/installer /usr/bin/installer COPY --from=builder /go/src/github.com/openshift/assisted-installer/deploy/assisted-installer-controller /assisted-installer-controller/deploy +# Create non-root user +RUN useradd -u 1001 -r -g 0 -s /sbin/nologin installer + +USER 1001 + +HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ + CMD ["/usr/bin/installer", "--health-check"] || exit 1 + ENTRYPOINT ["/usr/bin/installer"]For the builder stage at line 6:
-COPY . . +COPY go.mod go.sum ./ +COPY cmd/ cmd/ +COPY pkg/ pkg/ +COPY vendor/ vendor/ +COPY deploy/ deploy/Note: Adjust the HEALTHCHECK and COPY paths to match your actual application structure.
As per coding guidelines for container security.
🤖 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.assisted-installer.ocp` around lines 1 - 17, The Dockerfile is missing runtime security directives and copies the entire build context; update the builder stage (WORKDIR and COPY . .) to use explicit COPYs for only required sources (e.g., go.mod, go.sum, cmd/ and pkg/ paths) instead of COPY . ., keep the multi-stage COPYs (COPY --from=builder ...) intact, and add a non-root USER directive (e.g., create or switch to a dedicated uid/gid before ENTRYPOINT) plus a HEALTHCHECK instruction that probes the installer’s readiness (use a lightweight curl/wget or similar against the appropriate local port/path) so the final image includes USER and HEALTHCHECK alongside the existing ENTRYPOINT and COPY --from=builder lines.
🤖 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.assisted-installer-build`:
- Around line 13-34: The final build stage uses an insecure base and copies
entire Go directories as root; update the Dockerfile to use a UBI minimal or
distroless base instead of quay.io/centos/centos:stream9, create and switch to a
non-root user via a USER directive (add user/group and set ownership) before any
COPY operations, replace the broad COPY --from=golang $GOPATH $GOPATH and COPY
--from=golang $GOROOT $GOROOT with copying only the required binaries/artifacts
(referencing the COPY --from=golang entries), and add an appropriate HEALTHCHECK
line; ensure ENV GOROOT/GOPATH remain correct and adjust file
ownership/permissions for the non-root user.
In `@Dockerfile.assisted-installer-controller`:
- Around line 22-37: The final runtime stage in the Dockerfile for
assisted-installer-controller is missing required security directives. Update
the last stage after the COPY steps to add a non-root USER and a HEALTHCHECK
using a command supported by assisted-installer-controller so the container does
not default to root and exposes a liveness check. Keep the changes in the final
stage only, and make sure the USER and HEALTHCHECK are placed before ENTRYPOINT.
In `@Dockerfile.assisted-installer-controller.ocp`:
- Around line 9-17: The final image should run non-root and include a
healthcheck and narrow COPYs: replace any broad COPY . . usage in the
builder/final stage with explicit file copies (e.g., copying only the built
binary and required assets instead of the whole context), add a USER directive
(e.g., create or switch to a non-root account and add USER <username|uid>)
before ENTRYPOINT, and add a HEALTHCHECK instruction that exercises the service
(for example an HTTP GET or exec that checks /health) referencing the existing
ENTRYPOINT (/usr/bin/assisted-installer-controller) so the container reports
liveness; ensure file ownership/permissions allow the non-root user to execute
the binary.
---
Duplicate comments:
In `@Dockerfile.assisted-installer.ocp`:
- Around line 1-17: The Dockerfile is missing runtime security directives and
copies the entire build context; update the builder stage (WORKDIR and COPY . .)
to use explicit COPYs for only required sources (e.g., go.mod, go.sum, cmd/ and
pkg/ paths) instead of COPY . ., keep the multi-stage COPYs (COPY --from=builder
...) intact, and add a non-root USER directive (e.g., create or switch to a
dedicated uid/gid before ENTRYPOINT) plus a HEALTHCHECK instruction that probes
the installer’s readiness (use a lightweight curl/wget or similar against the
appropriate local port/path) so the final image includes USER and HEALTHCHECK
alongside the existing ENTRYPOINT and COPY --from=builder lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eccfb601-8e73-4843-a2b2-acd3789829d6
📒 Files selected for processing (11)
.ci-operator.yamlDockerfile.assisted-installerDockerfile.assisted-installer-buildDockerfile.assisted-installer-controllerDockerfile.assisted-installer-controller-downstreamDockerfile.assisted-installer-controller-mceDockerfile.assisted-installer-controller.ocpDockerfile.assisted-installer-downstreamDockerfile.assisted-installer-mceDockerfile.assisted-installer.ocpgo.mod
✅ Files skipped from review due to trivial changes (5)
- Dockerfile.assisted-installer
- Dockerfile.assisted-installer-controller-mce
- Dockerfile.assisted-installer-downstream
- Dockerfile.assisted-installer-controller-downstream
- Dockerfile.assisted-installer-mce
shay23bra
force-pushed
the
MGMT-24453-upgrade-golang-1.26
branch
from
1107df4 to
09abbe1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.assisted-installer-controller`:
- Line 1: The Dockerfile uses registry.access.redhat.com base images; update
both the builder FROM (currently
"registry.access.redhat.com/ubi9/go-toolset:1.26" in the builder stage) and the
runtime FROM (the UBI minimal image pinned by digest) to use catalog.redhat.com
equivalents instead (replace registry.access.redhat.com/... with
catalog.redhat.com/... and remove unnecessary digest pinning for the runtime
image if project policy requires tag usage), ensuring you keep the same
major/minor Go toolset and UBI minimal semantics when selecting the
catalog-hosted tags.
In `@Dockerfile.assisted-installer-mce`:
- Line 1: Update the Dockerfile's builder base image to use Red Hat's catalog
registry: replace the FROM line that references the image
registry.access.redhat.com/ubi9/go-toolset:1.26 (the builder stage aliased
"builder") so it pulls from catalog.redhat.com instead (keep the same image name
and tag and the AS builder alias); ensure the --platform and tag remain
unchanged.
🪄 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: 014ab7db-74b6-4c60-9b4d-a9d3abdf516f
📒 Files selected for processing (12)
.ci-operator.yaml.golangci.ymlDockerfile.assisted-installerDockerfile.assisted-installer-buildDockerfile.assisted-installer-controllerDockerfile.assisted-installer-controller-downstreamDockerfile.assisted-installer-controller-mceDockerfile.assisted-installer-controller.ocpDockerfile.assisted-installer-downstreamDockerfile.assisted-installer-mceDockerfile.assisted-installer.ocpgo.mod
✅ Files skipped from review due to trivial changes (4)
- Dockerfile.assisted-installer-build
- .golangci.yml
- Dockerfile.assisted-installer-controller-mce
- Dockerfile.assisted-installer-controller.ocp
| @@ -1,4 +1,4 @@ | |||
| FROM registry.access.redhat.com/ubi9/go-toolset:1.25 AS builder | |||
| FROM registry.access.redhat.com/ubi9/go-toolset:1.26 AS builder | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="."
file="Dockerfile.assisted-installer-controller"
# Confirm file exists and show FROM lines
if [ ! -f "$repo_root/$file" ]; then
echo "MISSING_FILE: $file"
exit 0
fi
echo "FILE_PATH: $file"
echo "---- First 80 lines (to capture all FROM stages) ----"
sed -n '1,80p' "$repo_root/$file" | nl -ba
echo "---- All FROM lines ----"
rg -n --fixed-string "FROM " "$repo_root/$file" || trueRepository: openshift/assisted-installer
Length of output: 1952
🌐 Web query:
Find the contents of Dockerfile.assisted-installer-controller in openshift/assisted-installer and locate the FROM line(s), especially the builder stage base image.
💡 Result:
The openshift/assisted-installer repository contains multiple Dockerfiles related to the controller, notably Dockerfile.assisted-installer-controller [1] and Dockerfile.assisted-installer-controller.ocp [2]. In the standard Dockerfile.assisted-installer-controller, the FROM lines are as follows [1]: 1. Builder stage base image: FROM registry.access.redhat.com/ubi9/go-toolset:1.25 AS builder 2. Runtime stage base image: FROM registry.access.redhat.com/ubi9/ubi-minimal:latest@sha256:6fc28bcb6776e387d7a35a2056d9d2b985dc4e26031e98a2bd35a7137cd6fd71 In the Dockerfile.assisted-installer-controller.ocp variant, the FROM lines are [2]: 1. Builder stage base image: FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.25-openshift-4.21 AS builder 2. Runtime stage base image: FROM registry.ci.openshift.org/ocp/4.21:base-rhel9
Citations:
- 1: https://github.com/openshift/assisted-installer/blob/master/Dockerfile.assisted-installer-controller
- 2: https://github.com/openshift/assisted-installer/blob/master/Dockerfile.assisted-installer-controller.ocp
Switch base images to catalog.redhat.com (builder + runtime).
Dockerfile.assisted-installer-controllerbuilder stage usesregistry.access.redhat.com/ubi9/go-toolset:1.25instead of a catalog-hosted image.Dockerfile.assisted-installer-controllerruntime stage usesregistry.access.redhat.com/ubi9/ubi-minimal:latest@sha256:6fc28bcb6776e387d7a35a2056d9d2b985dc4e26031e98a2bd35a7137cd6fd71(also notcatalog.redhat.com, and pinned by digest even though it’s a Red Hat image).
🤖 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.assisted-installer-controller` at line 1, The Dockerfile uses
registry.access.redhat.com base images; update both the builder FROM (currently
"registry.access.redhat.com/ubi9/go-toolset:1.26" in the builder stage) and the
runtime FROM (the UBI minimal image pinned by digest) to use catalog.redhat.com
equivalents instead (replace registry.access.redhat.com/... with
catalog.redhat.com/... and remove unnecessary digest pinning for the runtime
image if project policy requires tag usage), ensuring you keep the same
major/minor Go toolset and UBI minimal semantics when selecting the
catalog-hosted tags.
| @@ -1,4 +1,4 @@ | |||
| FROM --platform=$BUILDPLATFORM registry.access.redhat.com/ubi9/go-toolset:1.25 AS builder | |||
| FROM --platform=$BUILDPLATFORM registry.access.redhat.com/ubi9/go-toolset:1.26 AS builder | |||
There was a problem hiding this comment.
Update builder image source to comply with container security policy
Dockerfile.assisted-installer-mce uses a Red Hat base image from registry.access.redhat.com:
FROM --platform=$BUILDPLATFORM registry.access.redhat.com/ubi9/go-toolset:1.26 AS builderThe policy requires Red Hat base images to be sourced from catalog.redhat.com.
🤖 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.assisted-installer-mce` at line 1, Update the Dockerfile's builder
base image to use Red Hat's catalog registry: replace the FROM line that
references the image registry.access.redhat.com/ubi9/go-toolset:1.26 (the
builder stage aliased "builder") so it pulls from catalog.redhat.com instead
(keep the same image name and tag and the AS builder alias); ensure the
--platform and tag remain unchanged.
Contributor
Author
|
/retest |
|
@shay23bra: The following test 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. |
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
2 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.
Summary
https://redhat.atlassian.net/browse/MGMT-24453
Summary by CodeRabbit