OCPBUGS-87352: Updating coredns-container image to be consistent with ART for 5.0#189
OCPBUGS-87352: Updating coredns-container image to be consistent with ART for 5.0#189aswinsuryan wants to merge 2 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-moderate
|
@aswinsuryan: This pull request references Jira Issue OCPBUGS-87352, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
[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 |
WalkthroughGo toolchain is bumped from 1.25.0/1.24.6 to 1.26.3, and OpenShift base images are upgraded from 4.22 to 5.0 across configuration files. Test assertion formatting is corrected from string-style to numeric formatting for DNS and SOA integer values. ChangesGo 1.26 / OpenShift 5.0 Toolchain Upgrade
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile.ocp (1)
1-13:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd USER directive for non-root execution and refine COPY pattern; define HEALTHCHECK.
The Dockerfile has three security/hardening gaps per container security guidelines:
- No USER directive (line 9): Container will run as root. CoreDNS should run as a non-root user for security hardening.
- Overly broad COPY (line 3):
COPY . .copies the entire build context including test files, docs, and VCS metadata. Use selective COPY.- Missing HEALTHCHECK: Guidelines require HEALTHCHECK to be defined for container orchestration health monitoring.
🛡️ Proposed fixes for container hardening
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.26-openshift-5.0 AS builder WORKDIR /go/src/github.com/coredns/coredns -COPY . . +COPY go.mod go.sum ./ +COPY . . RUN GO111MODULE=on GOFLAGS=-mod=vendor go build -o coredns . FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 COPY --from=builder /go/src/github.com/coredns/coredns/coredns /usr/bin/ +USER 1001 ENTRYPOINT ["/usr/bin/coredns"] +HEALTHCHECK --interval=10s --timeout=2s --start-period=5s --retries=3 \ + CMD exec nslookup localhost 127.0.0.1:53 || exit 1 + LABEL io.k8s.display-name="CoreDNS" \Note: If the base image does not include
nslookupordig, use a simpler check like a TCP probe or adjust the UID to match the base image's expected non-root user. Verify the UID 1001 is appropriate for the runtime 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.ocp` around lines 1 - 13, The Dockerfile has three security hardening issues to address. First, replace the overly broad COPY . . command in the builder stage with a more selective pattern that copies only essential files needed for the build, excluding test files, documentation, and VCS metadata directories. Second, add a USER directive in the runtime stage after the COPY --from=builder command to ensure CoreDNS runs as a non-root user (verify the appropriate UID with your base image, commonly 1001 or similar). Third, add a HEALTHCHECK directive after the ENTRYPOINT to enable container orchestration health monitoring; use an appropriate health check command such as a TCP probe or DNS query depending on what tools are available in the base image.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 `@go.mod`:
- Line 5: The Go version specified in the go.mod file is set to 1.26.0, which
contains known security vulnerabilities. Update the version number in the line
go 1.26.0 from 1.26.0 to 1.26.4 (the current stable release) or at minimum to
1.26.1 to receive the necessary security patches that address the known CVEs.
---
Outside diff comments:
In `@Dockerfile.ocp`:
- Around line 1-13: The Dockerfile has three security hardening issues to
address. First, replace the overly broad COPY . . command in the builder stage
with a more selective pattern that copies only essential files needed for the
build, excluding test files, documentation, and VCS metadata directories.
Second, add a USER directive in the runtime stage after the COPY --from=builder
command to ensure CoreDNS runs as a non-root user (verify the appropriate UID
with your base image, commonly 1001 or similar). Third, add a HEALTHCHECK
directive after the ENTRYPOINT to enable container orchestration health
monitoring; use an appropriate health check command such as a TCP probe or DNS
query depending on what tools are available in the base image.
🪄 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: 5160c812-d564-437e-935e-4db656c6176b
📒 Files selected for processing (4)
.ci-operator.yaml.go-versionDockerfile.ocpgo.mod
aswinsuryan
force-pushed
the
OCPBUGS-87352-go-1.26
branch
2 times, most recently
from
7fa85fc to
d07a143
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Update .go-version and go.mod to match Go 1.26.3 builder images. Signed-off-by: Aswin Suryanarayanan <asuryana@redhat.com>
aswinsuryan
force-pushed
the
OCPBUGS-87352-go-1.26
branch
from
d07a143 to
62c0e7e
Compare
|
@aswinsuryan: 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. |
|
/assign @Thealisyed |
Thealisyed
left a comment
Thealisyed
left a comment
There was a problem hiding this comment.
@coderabbitai you can also do review on this PR or answer/ pushback / agree with my comments!
| // Note this minimum version requirement. CoreDNS supports the last two | ||
| // Go versions. This follows the upstream Go project support. | ||
| go 1.25.0 | ||
| go 1.26.3 |
There was a problem hiding this comment.
The go directive is set to 1.26.3 rather than 1.26.0. Was that intentional?
Typically the go directive tracks the minimum language version needed and pinning to a patch forces all consumers to have at least that patch. The .go-version file at 1.26.3 is fine since that controls the local toolchain but go.mod could arguably stay at 1.26.0?
There was a problem hiding this comment.
1.26.0 flagged by coderabbit [1] due to security vulnerabilities hence updated to 1.26.3
[1] #189 (comment)
| actual := rec.Msg | ||
| if actual.Rcode != rc { | ||
| t.Fatalf("ServeDNS should return real result code %q != %q", actual.Rcode, rc) | ||
| t.Fatalf("ServeDNS should return real result code %d != %d", actual.Rcode, rc) |
There was a problem hiding this comment.
Were these flagged by go vet under 1.26 or caught manually? Just want to confirm the build actually fails without these on the new toolchain.
There was a problem hiding this comment.
yes, that is right the build was failing and these where flagged by go vet.
4 participants
Description:
Updates for OpenShift 5.0 ART alignment:
registry.ci.openshift.org/ocp/4.22:base-rhel9→registry.ci.openshift.org/ocp/5.0:base-rhel9rhel-9-golang-1.25-openshift-4.22→rhel-9-golang-1.26-openshift-5.0.go-version: 1.24.6 → 1.26.0go.mod: 1.25.0 → 1.26.0Updates : #187
1. Why is this pull request needed and what does it do?
2. Which issues (if any) are related?
3. Which documentation changes (if any) need to be made?
4. Does this introduce a backward incompatible change or deprecation?
Summary by CodeRabbit