NE-2223: Update HAProxy RPM in the router container to 3.2.19 - #792
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
gcs278
changed the title
|
FYI this is a "vanilla" 3.2 smoke test PR - just to get some green CI runs before integrating HAProxy 3.2 with the sidecar architecture. /assign @jcmoraisjr |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
gcs278
changed the title
|
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:
WalkthroughThe router Dockerfile updates the HAProxy package installed during image build from ChangesHAProxy package update
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![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
🤖 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 `@images/router/haproxy/Dockerfile.ocp`:
- Line 3: The RUN instruction that installs
haproxy32-3.2.19-1.rhaos4.23.el9.x86_64.rpm must perform package cache cleanup
in the same layer; update the RUN that currently calls "yum install -y
https://.../haproxy32-3.2.19-1.rhaos4.23.el9.x86_64.rpm" to chain the install
with cache cleanup (e.g., run yum clean all and remove yum caches) in the same
command so no package manager metadata remains in the final image.
- Line 3: The RUN line in Dockerfile.ocp that installs the RPM from a mutable
GitHub branch (the existing RUN yum install ... refs/heads/master URL) must be
replaced with an immutable, verified artifact: point to a pinned artifact URL
(digest or internal Brew/registry artifact) and verify its integrity before
install (e.g., fetch the RPM by digest/checksum or GPG signature, validate
checksum/signature, then run yum install from the verified local file). Ensure
the RPM source is not a branch URL and include checksum or signature validation
steps so the container build only consumes a pinned, trusted package.
🪄 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: d5e60100-d267-4eaa-9a00-e268af5564b5
📒 Files selected for processing (1)
images/router/haproxy/Dockerfile.ocp
There was a problem hiding this comment.
♻️ Duplicate comments (2)
images/router/haproxy/Dockerfile.ocp (2)
3-5:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftUse an immutable, verified RPM source instead of a mutable GitHub branch URL.
Line 3 installs an RPM from
refs/heads/master, which is mutable and not integrity-checked. If that artifact changes upstream, the build can silently consume untrusted content, including RPM scriptlets executed during install.🔒 Recommended hardening change
-RUN curl -fLo /tmp/haproxy32.rpm https://github.com/gcs278/openshift-hacks/raw/refs/heads/master/haproxy-builds/haproxy32-3.2.19-1.rhaos4.23.el9.x86_64.rpm && \ - yum install -y /tmp/haproxy32.rpm && \ - rm -f /tmp/haproxy32.rpm +RUN curl -fsSL -o /tmp/haproxy32.rpm https://<immutable-artifact-url>/haproxy32-3.2.19-1.rhaos4.23.el9.x86_64.rpm && \ + echo "<sha256> /tmp/haproxy32.rpm" | sha256sum -c - && \ + yum install -y /tmp/haproxy32.rpm && \ + rm -f /tmp/haproxy32.rpm && \ + yum clean allAs per coding guidelines, container artifacts should follow immutable pinning principles: "non-RH images: pin 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 `@images/router/haproxy/Dockerfile.ocp` around lines 3 - 5, The RUN step that curls an RPM from a mutable GitHub branch (the line starting with "RUN curl -fLo /tmp/haproxy32.rpm ... refs/heads/master") must be replaced with an immutable, verified artifact fetch: point the download to a pinned release URL (tag or commit SHA) or a trusted registry with a digest, and verify integrity before install (e.g., check a SHA256 checksum or GPG signature of haproxy32.rpm) instead of relying on refs/heads/master; update the RUN command to download the pinned URL, validate the file, then install and remove it.Source: Coding guidelines
3-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean yum metadata in the same layer as the RPM install.
The
yum installdoes not runyum clean allin the same RUN layer, so cache and metadata remain in that layer and bloat the final image.🧹 Proposed fix
RUN curl -fLo /tmp/haproxy32.rpm https://github.com/gcs278/openshift-hacks/raw/refs/heads/master/haproxy-builds/haproxy32-3.2.19-1.rhaos4.23.el9.x86_64.rpm && \ yum install -y /tmp/haproxy32.rpm && \ - rm -f /tmp/haproxy32.rpm + rm -f /tmp/haproxy32.rpm && \ + yum clean allAs per coding guidelines: "No package manager cache in final layer".
🤖 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 `@images/router/haproxy/Dockerfile.ocp` around lines 3 - 5, The RUN layer that downloads and installs /tmp/haproxy32.rpm leaves yum metadata behind; modify the RUN that currently runs curl && yum install -y /tmp/haproxy32.rpm && rm -f /tmp/haproxy32.rpm to also run the yum cleanup (e.g., yum clean all && rm -rf /var/cache/yum or equivalent) in the same RUN chain so package manager cache/metadata are removed before the layer is committed; target the RUN instruction that handles /tmp/haproxy32.rpm in Dockerfile.ocp to implement this change.Sources: Coding guidelines, Linters/SAST tools
🤖 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 `@images/router/haproxy/Dockerfile.ocp`:
- Around line 3-5: The RUN step that curls an RPM from a mutable GitHub branch
(the line starting with "RUN curl -fLo /tmp/haproxy32.rpm ...
refs/heads/master") must be replaced with an immutable, verified artifact fetch:
point the download to a pinned release URL (tag or commit SHA) or a trusted
registry with a digest, and verify integrity before install (e.g., check a
SHA256 checksum or GPG signature of haproxy32.rpm) instead of relying on
refs/heads/master; update the RUN command to download the pinned URL, validate
the file, then install and remove it.
- Around line 3-5: The RUN layer that downloads and installs /tmp/haproxy32.rpm
leaves yum metadata behind; modify the RUN that currently runs curl && yum
install -y /tmp/haproxy32.rpm && rm -f /tmp/haproxy32.rpm to also run the yum
cleanup (e.g., yum clean all && rm -rf /var/cache/yum or equivalent) in the same
RUN chain so package manager cache/metadata are removed before the layer is
committed; target the RUN instruction that handles /tmp/haproxy32.rpm in
Dockerfile.ocp to implement this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c88616bc-de9b-422c-a972-793668d3b076
📒 Files selected for processing (1)
images/router/haproxy/Dockerfile.ocp
|
/test ? |
|
/test e2e-metal-ipi-ovn-dualstack |
|
/test ? |
|
/test e2e-metal-ipi-ovn-dualstack |
|
/retest |
b83f1e1 to
2268fb3
Compare
|
We've typically done some aggregate E2E to expose any flakes in the past with new HAProxy versions: /payload-aggregate periodic-ci-openshift-release-master-ci-5.0-e2e-aws-ovn 5 |
|
@gcs278: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
whoops I forgot to start the perf tests: |
openshift-ci
Bot
added
the
needs-rebase
2268fb3 to
d30e595
Compare
|
/test e2e-metal-ipi-ovn-dualstack |
openshift-ci
Bot
removed
the
needs-rebase
|
/override ci/prow/okd-scos-images |
|
@Prashanth684: Overrode contexts on behalf of Prashanth684: ci/prow/okd-scos-images 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. |
|
tested on a 5.0 cluster confirmed both pods show the new HAProxy version in the router and haproxy container /verified by @rhamini3 |
openshift-ci-robot
added
the
verified
|
@rhamini3: 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. |
|
I asked claude for a assessment for the payload:
Lots of noise, but both claude and chaibot seem pretty confident there are no hard failures here. The real test if something in this noise is becoming more flakey because of haproxy32, but it's not evident that I see that. I think we can merge safely. |
|
install failure |
|
/test e2e-agnostic |
3 similar comments
|
/test e2e-agnostic |
|
/test e2e-agnostic |
|
/test e2e-agnostic |
|
not related |
|
another not related failure /test e2e-agnostic |
|
both failures (load balancer is cloud loadbalancer, not HAProxy, and DNS isn't going through HAProxy): I'm getting close to overriding - it's been a solid 2 days of trying to get this to pass - all unrelated flakes (confirm with claude and chai bot), and I have ran a payload job on this. But, it's a big change I and I guess we need to be cautious. /test e2e-agnostic |
|
I've been spinning on e2e-agnostic since Tuesday. It's been failing constantly, here's my last failures and their reason:
Summary: 4 CI infra (payload build), 1 GCP lease, 4 test flakes (SCC, DNS, CRI-O, UDP LB). Zero HAProxy/ingress/router related. We had gotten one green a couple days ago, but I feel confident enough that none of these failures are haproxy related. I had also done a full payload job out of precaution that seems acceptable. /override ci/prow/e2e-agnostic |
|
@gcs278: Overrode contexts on behalf of gcs278: ci/prow/e2e-agnostic 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. |
|
OKD is a known failure, and dependent on openshift/release#82170 merging |
|
@gcs278: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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. |
|
/test e2e-upgrade |
|
/override-sticky ci/prow/okd-scos-images |
|
@gcs278: Overrode contexts on behalf of gcs278: ci/prow/okd-scos-images These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@gcs278: 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. |
|
my override didn't stick, and e2e-agnostic failed again with a unrelated failure: Again, it's been 10 failures as documented above, none related to haproxy. /override ci/prow/e2e-agnostic |
|
@gcs278: Overrode contexts on behalf of gcs278: ci/prow/e2e-agnostic 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. |
gcs278
changed the title
5 participants
Update the default HAProxy version in the router container from 2.8 to 3.2 for OpenShift 5.0. This decouples the HAProxy 3.2 bump from the multi-version feature gate promotion, giving us earlier CI signal without waiting for the IngressControllerMultipleHAProxyVersions feature to GA.
This also affects components that use the
haproxy-routerimage directly (on-prem load balancer, MicroShift). Both teams have been notified and can fall back tohaproxy-router-haproxy28if needed. The bundled HAProxy in this image will be removed in a future release once all consumers migrate to the sidecar model.OKD: Requires a coordinating openshift/release PR (openshift/release#82170) to bump the OKD router base image to the 5.0 stream. These two PRs should be merged close together to minimize any gap in OKD CI.
Summary by CodeRabbit