HYPERFLEET-1057 - fix: Update e2e dockerfile for prow job#125
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDockerfile isolates Helm home directory via dedicated Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile (3)
32-33:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCWE-250, CWE-732: Runtime stage runs as root.
The runtime stage switches to
USER root(line 32) and never drops privileges. The container executes as UID 0, violating the hardening guideline: "Do not run as root — use USER with a non-root UID." A compromised process gains full container privileges.Add a
USER <non-root-uid>directive before the ENTRYPOINT (after line 86) to drop privileges.🤖 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 32 - 33, The Dockerfile currently sets USER root and never drops privileges; before the ENTRYPOINT directive add a non-root USER (e.g., USER 1000 or a named user) to run the runtime stage as a non-root UID, ensure the chosen UID has ownership/permissions on app files and any needed directories (use chown/chmod during build or create a user via groupadd/useradd in the Dockerfile), and verify ENTRYPOINT and any startup scripts are executable by that user so the container no longer runs as root.Source: Coding guidelines
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBase images not pinned by digest.
Lines 7, 10, 26, and 29 reference base images by tag (including
:lateston line 26), violating guideline#3: "Pin base images by digest (@sha256:...) not just tag." Tags are mutable; a registry compromise or tag retargeting injects a backdoor into the build.Pin each base image to a SHA256 digest:
- Line 7:
registry.access.redhat.com/ubi9/go-toolset@sha256:...- Line 10:
golang:1.25@sha256:...- Line 26:
registry.ci.openshift.org/.../hyperfleet-credential-provider@sha256:...Also applies to: 10-10, 26-26, 29-29
🤖 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` at line 7, Update the Dockerfile to pin all base images by SHA256 digest instead of mutable tags: replace the ARG BASE_IMAGE value and any FROM/image references (e.g., ARG BASE_IMAGE, the golang image reference, and the hyperfleet-credential-provider image) so they use the registry@sha256:... form rather than a tag or :latest; ensure you obtain the correct sha256 digests from the registries and substitute them in each referenced symbol (BASE_IMAGE and the explicit golang and hyperfleet image references) so every base image is immutably pinned.Source: Coding guidelines
36-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winkubectl binary downloaded without signature verification.
Lines 36-37 download the kubectl binary from
dl.k8s.iowithout verifying a checksum or GPG signature. A compromised CDN or MITM attack injects a backdoored binary.Add checksum verification using the
.sha256file published alongside each kubectl release, or verify the binary's GPG signature before installation.🤖 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 36 - 37, The Dockerfile RUN step that downloads the kubectl binary (the RUN ... kubectl && chmod +x /usr/local/bin/kubectl line) lacks integrity verification; update this step to fetch the corresponding .sha256 (or GPG signature) for the release (use the same release string from https://dl.k8s.io/release/stable.txt), verify the downloaded kubectl against the checksum (or verify the signed checksum using the Kubernetes release key), and abort (non-zero exit) if verification fails before running chmod +x; ensure the verification uses the exact same URL/version used to download the binary so the process is atomic and secure.
🤖 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`:
- Line 43: The Dockerfile currently makes /tmp/helm-home world-writable with
"chmod 777 /tmp/helm-home"; change this to use a more secure permission and
ownership: replace the chmod 777 with "chmod 755 /tmp/helm-home" and add a chown
to the non-root test user (e.g., "chown <UID>:<GID> /tmp/helm-home" or use the
existing user name) so only that user can write, or remove explicit chmod and
create the directory after switching to the non-root user; update both
occurrences that touch /tmp/helm-home to follow this pattern.
- Line 40: Replace the unsafe "RUN curl -fsSL
https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 | bash"
pattern with a pinned, verifiable Helm install: choose a specific Helm version,
download the corresponding release tarball or binary directly (not a remote
install script), fetch its published SHA256 (or signature) from the official
release assets, verify the checksum/signature before extracting, and then move
the verified helm binary into the target PATH (or install from a pinned OS
package/RPM/container layer). Update the Dockerfile step that currently uses the
piped script (the RUN curl -fsSL ... | bash line) to perform these explicit
download, verify, extract, and clean-up actions instead.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 32-33: The Dockerfile currently sets USER root and never drops
privileges; before the ENTRYPOINT directive add a non-root USER (e.g., USER 1000
or a named user) to run the runtime stage as a non-root UID, ensure the chosen
UID has ownership/permissions on app files and any needed directories (use
chown/chmod during build or create a user via groupadd/useradd in the
Dockerfile), and verify ENTRYPOINT and any startup scripts are executable by
that user so the container no longer runs as root.
- Line 7: Update the Dockerfile to pin all base images by SHA256 digest instead
of mutable tags: replace the ARG BASE_IMAGE value and any FROM/image references
(e.g., ARG BASE_IMAGE, the golang image reference, and the
hyperfleet-credential-provider image) so they use the registry@sha256:... form
rather than a tag or :latest; ensure you obtain the correct sha256 digests from
the registries and substitute them in each referenced symbol (BASE_IMAGE and the
explicit golang and hyperfleet image references) so every base image is
immutably pinned.
- Around line 36-37: The Dockerfile RUN step that downloads the kubectl binary
(the RUN ... kubectl && chmod +x /usr/local/bin/kubectl line) lacks integrity
verification; update this step to fetch the corresponding .sha256 (or GPG
signature) for the release (use the same release string from
https://dl.k8s.io/release/stable.txt), verify the downloaded kubectl against the
checksum (or verify the signed checksum using the Kubernetes release key), and
abort (non-zero exit) if verification fails before running chmod +x; ensure the
verification uses the exact same URL/version used to download the binary so the
process is atomic and secure.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1743b232-a653-40b6-a8c1-ffc24640b1f5
📒 Files selected for processing (1)
Dockerfile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
from
75e826c to
ed1ba57
Compare
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 (2)
Dockerfile (2)
32-86:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winContainer still runs as root by default (CWE-250).
USER rootis set and never dropped in the final runtime stage. This violates the hardening requirement and expands blast radius for any compromised tool/plugin/script in the image.As per coding guidelines, “Do not run as root — use USER with a non-root UID.”
🤖 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 32 - 86, The image currently sets USER root and never drops privileges; create a non-root runtime user (e.g., UID 1000, group wheel or nobody) and switch to it before the final ENTRYPOINT so the container does not run as root. Ensure all files and directories used at runtime (references: /usr/local/bin/hyperfleet-e2e, /usr/local/bin/hyperfleet-credential-provider, /e2e, /e2e/configs, /e2e/testdata, /e2e/deploy-scripts, /e2e/env, /e2e/scripts) are owned or accessible by that user (chown/chmod in the Dockerfile during image build) and then add USER <nonroot> right before ENTRYPOINT to enforce the non-root runtime. Maintain existing ENTRYPOINT and CMD but run them under the non-root account.Source: Coding guidelines
7-7:⚠️ Potential issue | 🟠 MajorPin Docker base images to immutable digests (CWE-494 supply-chain risk)
Dockerfile:FROM golang:1.25,FROM registry.ci.openshift.org/ci/hyperfleet-credential-provider:latest,ARG BASE_IMAGE=registry.access.redhat.com/ubi9/go-toolset+FROM ${BASE_IMAGE}images/Dockerfile.platform:FROM registry.access.redhat.com/ubi9/ubi:latestNone are digest-pinned (
@sha256), so rebuilds can silently pull different contents. Pin eachFROMimage by digest.🤖 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` at line 7, Pin the base images to immutable digests: replace each floating tag with the corresponding `@sha256` digest (e.g., change "FROM golang:1.25", "FROM registry.ci.openshift.org/ci/hyperfleet-credential-provider:latest", the ARG/default "ARG BASE_IMAGE=registry.access.redhat.com/ubi9/go-toolset" and the later "FROM ${BASE_IMAGE}", and "FROM registry.access.redhat.com/ubi9/ubi:latest" in images/Dockerfile.platform) so the Dockerfile and images/Dockerfile.platform reference concrete digests instead of tags; update ARG BASE_IMAGE to its digest form (or hardcode the digest in the FROM) and verify digests match your CI registry and rebuild/test images.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.
Inline comments:
In `@Dockerfile`:
- Around line 43-47: The /tmp/helm-home directory is created root-owned with
chmod 755 so a non-root prow UID can't write to Helm state; update the
Dockerfile so that after creating /tmp/helm-home (the RUN mkdir -p ... && chmod
755 ... block) you chown it to the intended non-root user/UID (or create a
dedicated helm user and chown), and ensure you switch to that non-root USER
before runtime; keep the HELM_* env vars (HELM_CACHE_HOME, HELM_CONFIG_HOME,
HELM_DATA_HOME, HELM_PLUGINS) but make sure /tmp/helm-home and its
.cache/.config/.local/share subdirs are owned by the non-root UID (or are
group-writable to that UID) so Helm can write state at runtime.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 32-86: The image currently sets USER root and never drops
privileges; create a non-root runtime user (e.g., UID 1000, group wheel or
nobody) and switch to it before the final ENTRYPOINT so the container does not
run as root. Ensure all files and directories used at runtime (references:
/usr/local/bin/hyperfleet-e2e, /usr/local/bin/hyperfleet-credential-provider,
/e2e, /e2e/configs, /e2e/testdata, /e2e/deploy-scripts, /e2e/env, /e2e/scripts)
are owned or accessible by that user (chown/chmod in the Dockerfile during image
build) and then add USER <nonroot> right before ENTRYPOINT to enforce the
non-root runtime. Maintain existing ENTRYPOINT and CMD but run them under the
non-root account.
- Line 7: Pin the base images to immutable digests: replace each floating tag
with the corresponding `@sha256` digest (e.g., change "FROM golang:1.25", "FROM
registry.ci.openshift.org/ci/hyperfleet-credential-provider:latest", the
ARG/default "ARG BASE_IMAGE=registry.access.redhat.com/ubi9/go-toolset" and the
later "FROM ${BASE_IMAGE}", and "FROM
registry.access.redhat.com/ubi9/ubi:latest" in images/Dockerfile.platform) so
the Dockerfile and images/Dockerfile.platform reference concrete digests instead
of tags; update ARG BASE_IMAGE to its digest form (or hardcode the digest in the
FROM) and verify digests match your CI registry and rebuild/test images.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d2e24a0c-9472-425c-b5d5-ff67acf818ea
📒 Files selected for processing (1)
Dockerfile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
|
/retest |
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
from
ed1ba57 to
2f0bc84
Compare
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 `@scripts/cleanup-pubsub-resources.sh`:
- Around line 176-179: The script uses return code 1 for both "no
topics/subscriptions found" and actual gcloud API failures, which causes real
errors to be masked as successful cleanups. Fix this by using distinct return
codes: use a specific code (e.g., 2) when no resources are found, and keep 1 for
actual failures. Then update the error-handling logic in the cleanup sections to
differentiate between these cases. Specifically, in
scripts/cleanup-pubsub-resources.sh at lines 176-179 (topics discovery) and
237-240 (subscriptions discovery), return a different code when the resource
arrays are empty. At lines 306-309 and 338-341 in the cleanup logic, check for
the specific "no resources found" code and only return success in that case;
allow other non-zero codes to propagate as actual errors.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 53584461-7681-4f8a-b8e9-7a9fb6853c8e
📒 Files selected for processing (2)
Dockerfilescripts/cleanup-pubsub-resources.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
from
2f0bc84 to
447ff8b
Compare
Commit type mismatch in PR title (nit)The PR title uses Per the commit standard, Suggestion: Rename the PR title to: |
| # Build with commit: podman build --build-arg GIT_COMMIT=$(git rev-parse HEAD) -t quay.io/hyperfleet/hyperfleet-e2e:latest . | ||
| # Run: podman run --rm -e HYPERFLEET_API_URL=<url> quay.io/hyperfleet/hyperfleet-e2e:latest test | ||
|
|
||
| # HyperFleet E2E Testing Framework |
There was a problem hiding this comment.
nit (Improvement): Duplicate comment — # HyperFleet E2E Testing Framework already exists at line 1 (file header). Remove this copy.
| # HyperFleet E2E Testing Framework |
| # Build with commit: podman build --build-arg GIT_COMMIT=$(git rev-parse HEAD) -t quay.io/hyperfleet/hyperfleet-e2e:latest . | ||
| # Run: podman run --rm -e HYPERFLEET_API_URL=<url> quay.io/hyperfleet/hyperfleet-e2e:latest test | ||
|
|
||
| # HyperFleet E2E Testing Framework |
There was a problem hiding this comment.
This is a duplicate of line 1. Should be removed.
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
from
447ff8b to
0f7fadb
Compare
ma-hill
changed the title
ma-hill
changed the title
| RUN mkdir -p /tmp/helm-home/.cache/helm \ | ||
| /tmp/helm-home/.config/helm \ | ||
| /usr/local/share/helm/plugins && \ | ||
| chmod -R 777 /tmp/helm-home && \ |
There was a problem hiding this comment.
Consider adding a comment explaining why 777 is necessary here, so it doesn't get "fixed" later and break CI.
There was a problem hiding this comment.
@pnguyen44 Added a comment explaining the reasoning
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Dockerfile (1)
42-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove world-writable Helm state permissions (CWE-732).
Line 46 sets
/tmp/helm-hometo777recursively, which allows arbitrary write/tamper of Helm cache/config state at runtime. Replace with least-privilege group-based access compatible with OpenShift/Prow arbitrary UID.Suggested patch
RUN mkdir -p /tmp/helm-home/.cache/helm \ /tmp/helm-home/.config/helm \ /usr/local/share/helm/plugins && \ - chmod -R 777 /tmp/helm-home && \ + chgrp -R 0 /tmp/helm-home && \ + chmod -R g=u /tmp/helm-home && \ chmod 755 /usr/local/share/helm/pluginsAs per coding guidelines, "
**/{Dockerfile,Containerfile}*: CONTAINER HARDENING (CWE-250, CWE-732)`" and hardening rule to avoid insecure permissions.🤖 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 42 - 47, The chmod command on line 46 sets /tmp/helm-home to 777 permissions recursively, which makes it world-writable and violates CWE-732 secure permission guidelines. Replace the chmod -R 777 /tmp/helm-home line with a more restrictive permission that provides least-privilege group-based access (typically 770 or similar) to ensure the Helm cache and config directories remain writable by the container process while preventing arbitrary users from tampering with the Helm state. This approach maintains compatibility with OpenShift/Prow arbitrary UID execution while addressing the security hardening requirement.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.
Duplicate comments:
In `@Dockerfile`:
- Around line 42-47: The chmod command on line 46 sets /tmp/helm-home to 777
permissions recursively, which makes it world-writable and violates CWE-732
secure permission guidelines. Replace the chmod -R 777 /tmp/helm-home line with
a more restrictive permission that provides least-privilege group-based access
(typically 770 or similar) to ensure the Helm cache and config directories
remain writable by the container process while preventing arbitrary users from
tampering with the Helm state. This approach maintains compatibility with
OpenShift/Prow arbitrary UID execution while addressing the security hardening
requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2c95b9f9-d413-4f40-a4d3-5c4939dab0eb
📒 Files selected for processing (2)
Dockerfilescripts/cleanup-pubsub-resources.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/cleanup-pubsub-resources.sh
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
2 times, most recently
from
f2f560c to
2d3508e
Compare
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
from
2d3508e to
dad9eed
Compare
| RUN mkdir -p /tmp/helm-home/.cache/helm \ | ||
| /tmpl/helm-home/.config/helm \ | ||
| /usr/local//share/helm/plugins && \ |
There was a problem hiding this comment.
Typos in directory paths
Line 45 has /tmpl/helm-home/.config/helm (should be /tmp/) and line 46 has /usr/local//share/helm/plugins (double slash).
| RUN mkdir -p /tmp/helm-home/.cache/helm \ | |
| /tmpl/helm-home/.config/helm \ | |
| /usr/local//share/helm/plugins && \ | |
| RUN mkdir -p /tmp/helm-home/.cache/helm \ | |
| /tmp/helm-home/.config/helm \ | |
| /usr/local/share/helm/plugins && \ |
… plugins for prow job
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
from
dad9eed to
6272ad0
Compare
4 participants
Summary
Update e2e dockerfile to override default helm default directories. Seems like another limitation of prow since we can't write to
$HOMEso overriding the default helm directories.Changes
Setting these values:
Fixing cleanup script bug
Relates to HYPERFLEET-1057
Test Plan