OCPBUGS-79056: update INFW to 4.22 version#707
OCPBUGS-79056: update INFW to 4.22 version#707raphaelvrosa wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-79056, which is invalid:
Comment 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. |
|
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:
WalkthroughBumps operator/project version 4.21.0→4.22.0 across manifests and constants, upgrades Go/toolchain references from 1.24→1.25 in CI/build files and go.mod, updates Makefile build tooling and envtest handling, consolidates RBAC rules for ingressnodefirewall resources, and fixes a test UDP field. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-79056, which is valid. The bug has been moved to the POST state. 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. |
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-79056, 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Makefile (1)
284-284: Makesetup-envtestpinning deterministic across local environments.Line 284 only checks file existence, so an older local
setup-envtestbinary can bypass the intendedrelease-0.23pin.♻️ Suggested change
- test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) GOFLAGS="" go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.23 + GOBIN=$(LOCALBIN) GOFLAGS="" go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.23
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 2: The Dockerfile's builder image uses "FROM golang:1.22" which does not
match go.mod's "go 1.25.0"; update the builder base image to "golang:1.25" (same
as Dockerfile.daemon) so module resolution and builds align with go.mod—modify
the FROM line in the builder stage accordingly.
In `@Makefile`:
- Line 129: The Makefile uses $(shell ...) (and was changed to $$(...)) to set
KUBEBUILDER_ASSETS via the ENVTEST/ENVTEST_K8S_VERSION command, but both
approaches will silently succeed with empty/invalid values if the command fails;
update the recipe to run the envtest resolution at recipe execution time and
explicitly detect failures: invoke the envtest command inside the recipe (using
$$(...) or plain shell) with shell error handling (e.g., set -e or checking exit
status with && or ||) or, if you require a Make-level check, use GNU make 4.2+
$(.SHELLSTATUS) after invoking $(shell ...) to fail the build when the command
exits non‑zero. Target the KUBEBUILDER_ASSETS assignment and the test recipe
that runs `go test ./... -coverprofile cover.out` to ensure the build aborts on
envtest resolution errors.
In `@manifests/art.yaml`:
- Around line 9-10: The search pattern for olm.skipRange in the replacement rule
is wrong and won't match the current CSV; update the search string
"olm.skipRange: '>=4.22.0-0 <{MAJOR}.{MINOR}.0'" to "olm.skipRange: '>=4.11.0
<4.22.0'" so it matches the existing CSV value and the replace rule
("olm.skipRange: '>=4.22.0-0 <{FULL_VER}'") will be applied correctly; look for
the exact literal search string used in the diff and change its leading version
to >=4.11.0 to match the stable CSV entry.
🪄 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: Pro Plus
Run ID: 9ac1e0d1-f8ad-481e-a862-1e987cc472ef
📒 Files selected for processing (22)
.ci-operator.yamlDockerfileDockerfile.daemonDockerfile.daemon.openshiftDockerfile.openshiftMakefileREADME.mdbundle/manifests/ingress-node-firewall.clusterserviceversion.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewallconfigs.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewallnodestates.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewalls.yamlconfig/manifests/bases/ingress-node-firewall.clusterserviceversion.yamlconfig/olm-install/install-resources.yamlconfig/rbac/role.yamlcontrollers/ingressnodefirewall_controller_rules_test.gogo.modhack/generators.Dockerfilemanifests/art.yamlmanifests/ingress-node-firewall.package.yamlmanifests/stable/ingress-node-firewall.clusterserviceversion.yamlopenshift-ci/wait_for_csv.shpkg/version/version.go
danwinship
left a comment
danwinship
left a comment
There was a problem hiding this comment.
you need to address/explain the coderabbit comments
| ProtocolConfig: infv1alpha1.IngressNodeProtocolConfig{ | ||
| Protocol: infv1alpha1.ProtocolTypeUDP, | ||
| TCP: &infv1alpha1.IngressNodeFirewallProtoRule{ | ||
| UDP: &infv1alpha1.IngressNodeFirewallProtoRule{ |
There was a problem hiding this comment.
What is this? Was there a bug before that is fixed now?
There was a problem hiding this comment.
It appears so, it was specified a merge of two settings with different protocols (TCP and UDP).
The first is TCP, and the second UDP, but it was not defined correctly (Protocol: infv1alpha1.ProtocolTypeUDP,).
| @@ -1,5 +1,5 @@ | |||
| # Build the manager binary | |||
| FROM golang:1.22 as builder | |||
| FROM golang:1.25 as builder | |||
There was a problem hiding this comment.
please squash the second commit in with the first
raphaelvrosa
force-pushed
the
ocpbugs-79056
branch
from
bea0084 to
5611293
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 (1)
Dockerfile.daemon (1)
7-10:⚠️ Potential issue | 🟠 MajorHarden runtime image by setting a non-root user.
The runtime stage does not declare
USER, so the container defaults to root. Add a non-root user (UID 1001 or equivalent per your platform policy) to ensure secure operation. The copied binaries have executable permissions for all users and will run correctly with the suggested user.Suggested hardening diff
FROM quay.io/centos/centos:stream8 COPY --from=builder /go/src/github.com/openshift/ingress-node-firewall/bin/daemon /usr/bin/ COPY --from=builder /go/src/github.com/openshift/ingress-node-firewall/bin/syslog /usr/bin/ +USER 1001 CMD ["/usr/bin/daemon"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.daemon` around lines 7 - 10, Add a non-root user and switch the runtime to it: create or ensure a user with UID 1001 (or your platform's non-root UID), chown the copied binaries (/usr/bin/daemon and /usr/bin/syslog) to that UID (and appropriate group), and set USER 1001 before the CMD so the container does not run as root; keep CMD ["/usr/bin/daemon"] unchanged and verify the binaries remain executable by that user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile.daemon`:
- Around line 7-10: Add a non-root user and switch the runtime to it: create or
ensure a user with UID 1001 (or your platform's non-root UID), chown the copied
binaries (/usr/bin/daemon and /usr/bin/syslog) to that UID (and appropriate
group), and set USER 1001 before the CMD so the container does not run as root;
keep CMD ["/usr/bin/daemon"] unchanged and verify the binaries remain executable
by that user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de6dcc58-0023-4534-8bca-cae373b3947a
📒 Files selected for processing (22)
.ci-operator.yamlDockerfileDockerfile.daemonDockerfile.daemon.openshiftDockerfile.openshiftMakefileREADME.mdbundle/manifests/ingress-node-firewall.clusterserviceversion.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewallconfigs.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewallnodestates.yamlconfig/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewalls.yamlconfig/manifests/bases/ingress-node-firewall.clusterserviceversion.yamlconfig/olm-install/install-resources.yamlconfig/rbac/role.yamlcontrollers/ingressnodefirewall_controller_rules_test.gogo.modhack/generators.Dockerfilemanifests/art.yamlmanifests/ingress-node-firewall.package.yamlmanifests/stable/ingress-node-firewall.clusterserviceversion.yamlopenshift-ci/wait_for_csv.shpkg/version/version.go
✅ Files skipped from review due to trivial changes (13)
- manifests/ingress-node-firewall.package.yaml
- hack/generators.Dockerfile
- config/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewalls.yaml
- controllers/ingressnodefirewall_controller_rules_test.go
- config/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewallnodestates.yaml
- openshift-ci/wait_for_csv.sh
- Dockerfile.openshift
- .ci-operator.yaml
- go.mod
- manifests/art.yaml
- Dockerfile
- config/olm-install/install-resources.yaml
- config/manifests/bases/ingress-node-firewall.clusterserviceversion.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/version/version.go
- README.md
- Dockerfile.daemon.openshift
- config/crd/bases/ingressnodefirewall.openshift.io_ingressnodefirewallconfigs.yaml
- bundle/manifests/ingress-node-firewall.clusterserviceversion.yaml
- manifests/stable/ingress-node-firewall.clusterserviceversion.yaml
- Makefile
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 (1)
Makefile (1)
72-74:⚠️ Potential issue | 🟡 MinorStale comment references removed
setup-envtest.sh.The comment mentions
setup-envtest.shas a requirement, but this script is no longer used. Line 288 now installssetup-envtestdirectly viago install. Consider updating the comment to reflect the current setup.📝 Suggested update
# Setting SHELL to bash allows bash commands to be executed by recipes. -# This is a requirement for 'setup-envtest.sh' in the test target. +# This is a requirement for shell command substitution and error handling in recipes. # Options are set to exit when a recipe line exits non-zero or a piped command fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 72 - 74, The comment referencing setup-envtest.sh is stale; update the Makefile comment that currently mentions "setup-envtest.sh" to reflect that the project now installs setup-envtest directly via go install (the Makefile invokes go install for setup-envtest), remove the obsolete requirement text, and keep the rest of the note about SHELL being bash and the -e/o pipefail behavior; search for the string "setup-envtest.sh" and the go install invocation for "setup-envtest" to find and edit the comment and replace it with a concise note stating setup-envtest is installed via go install.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 288: Update the Makefile's setup-envtest install command to use the
release-0.20 tag so it matches the project's controller-runtime v0.20.4; locate
the line that invokes "go install
sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.23" and change the
version suffix from `@release-0.23` to `@release-0.20` to ensure compatibility with
controller-runtime v0.20.4.
- Around line 129-137: The Makefile sets KUBEBUILDER_ASSETS as a shell variable
before running go test in the test and test-race targets so child processes
don’t see it; change the assignment so the variable is exported to the
environment (or use inline prefix assignment) so go test inherits
KUBEBUILDER_ASSETS—update the lines that set KUBEBUILDER_ASSETS (referencing
KUBEBUILDER_ASSETS, ENVTEST, ENVTEST_K8S_VERSION, and the test/test-race
targets) to either export the variable into the environment or put it inline on
the same command as go test.
---
Outside diff comments:
In `@Makefile`:
- Around line 72-74: The comment referencing setup-envtest.sh is stale; update
the Makefile comment that currently mentions "setup-envtest.sh" to reflect that
the project now installs setup-envtest directly via go install (the Makefile
invokes go install for setup-envtest), remove the obsolete requirement text, and
keep the rest of the note about SHELL being bash and the -e/o pipefail behavior;
search for the string "setup-envtest.sh" and the go install invocation for
"setup-envtest" to find and edit the comment and replace it with a concise note
stating setup-envtest is installed via go install.
🪄 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: Pro Plus
Run ID: dacc86f3-2a11-421b-a0fd-0a403f5d73c2
📒 Files selected for processing (1)
Makefile
|
Again, please squash the fixes. Each commit should be correct by itself without needing later fixups. |
raphaelvrosa
force-pushed
the
ocpbugs-79056
branch
from
9b2bad3 to
e0ded37
Compare
|
/test unit-test |
|
/lgtm |
tssurya
left a comment
tssurya
left a comment
There was a problem hiding this comment.
/approve
thanks for taking over the PR and working on this
For next time - very important - I prefer PRs to always always have describe's populated and commit message must NEVER have empty body unless its a trivial fix,
example provide a list of things you are doing in the PR via commit message that makes it easier for the reviewer
cause there are random things throughout - rbac consolidation, unit test bug for udp, sdk version update, golang update but not k8s update i think, controller-gen as well updated,
Also we need to cherry-pick this into 4.22 release if I'm not mistaken but at this point given all of this is just alpha API I'm fine to let 4.22 GA before we do the pick
| metadata: | ||
| annotations: | ||
| controller-gen.kubebuilder.io/version: v0.14.0 | ||
| controller-gen.kubebuilder.io/version: v0.20.1 |
There was a problem hiding this comment.
| build_root_image: | ||
| name: release | ||
| namespace: openshift | ||
| tag: rhel-9-release-golang-1.24-openshift-4.22 |
There was a problem hiding this comment.
these are supposed to be done by art bot automatically but ok at least doing this in one go helps since we need the new golang version
There was a problem hiding this comment.
It is mandatory to upgrade golang.
From the commit message of the original PR:
Bump Go version from 1.24.0 to 1.25.0 to satisfy controller-gen@v0.20.1
requirement (requires Go >= 1.25.0). Update go.mod and all Dockerfiles
to use Go 1.25 for consistency.
Fix test case that was using TCP field with UDP protocol type. The
updated controller-gen enforces stricter validation where protocol type
must match the field used in protocolConfig.
There was a problem hiding this comment.
great should have kept this description in the commit message :D
There was a problem hiding this comment.
Fixed, the commit is done with the messages now.
| DAEMON_IMG ?= quay.io/openshift/origin-ingress-node-firewall-daemon:latest | ||
| # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary. | ||
| ENVTEST_K8S_VERSION = 1.25.2 | ||
| ENVTEST_K8S_VERSION = 1.32.x |
There was a problem hiding this comment.
I don't really understand this version definition "what kubebuilder assets" are we talking about? I guess its not the same as the actual kube version right?
There was a problem hiding this comment.
From the previous commit message of the original PR:
Update Makefile to use modern setup-envtest Go binary instead of
deprecated shell script that was failing with 403 errors from Google
Cloud Storage. Align versions with project dependencies:
- ENVTEST_K8S_VERSION: 1.25.2 → 1.32.x (matches k8s.io/* v0.32.3)
- CONTROLLER_TOOLS_VERSION: v0.14.0 → v0.20.1 (matches controller-runtime v0.20.4)
- setup-envtest pinned to v0.20.4 (matches controller-runtime v0.20.4)
- Add GOFLAGS="" to setup-envtest install to fix -mod=vendor CI errors
Regenerate CRDs and RBAC with updated controller-gen.
There was a problem hiding this comment.
great should have kept this description in the commit message :D
There was a problem hiding this comment.
Fixed, the commit is done with the messages now.
| You need to install the following packages: | ||
|
|
||
| operator-sdk 1.22.0 | ||
| operator-sdk 1.33.0 |
There was a problem hiding this comment.
No, it's quite old. Not sure why it's kept this way, is there any dependency?
The newest one is 1.42, see https://github.com/operator-framework/operator-sdk/releases
@tssurya should it be updated to latest?
There was a problem hiding this comment.
it can be done in the future PR for 5.0 bump - there is another assignee for 5.0 for this work
openshift-ci
Bot
added
the
approved
| - apiGroups: | ||
| - ingressnodefirewall.openshift.io | ||
| resources: | ||
| - ingressnodefirewallconfigs/finalizers |
There was a problem hiding this comment.
Also, unrelated to this PR cause you are just moving things around - finalizers aren't really a seperate sub resource - so technically this expression does nothing. what's helping is the fact that ingressnodefirewallconfigs is totally updatable with above RBAC
Bump Go version from 1.24.0 to 1.25.0 to satisfy controller-gen@v0.20.1 requirement (requires Go >= 1.25.0). Update go.mod and all Dockerfiles to use Go 1.25 for consistency. Fix test case that was using TCP field with UDP protocol type. The updated controller-gen enforces stricter validation where protocol type must match the field used in protocolConfig. Update Makefile to use modern setup-envtest Go binary instead of deprecated shell script that was failing with 403 errors from Google Cloud Storage. Align versions with project dependencies: ENVTEST_K8S_VERSION: 1.25.2 → 1.32.x (matches k8s.io/* v0.32.3) CONTROLLER_TOOLS_VERSION: v0.14.0 → v0.20.1 (matches controller-runtime v0.20.4) setup-envtest pinned to v0.20.4 (matches controller-runtime v0.20.4) Add GOFLAGS="" to setup-envtest install to fix -mod=vendor CI errors Regenerate CRDs and RBAC with updated controller-gen. Signed-off-by: Raphael Rosa <raprosa@redhat.com>
raphaelvrosa
force-pushed
the
ocpbugs-79056
branch
from
e0ded37 to
2778643
Compare
|
aaah! I had already lgmt-ed approved it haha :D I didn't mean for you to repush it , I meant "for next time".. :D |
|
@raphaelvrosa: all tests passed! 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raphaelvrosa, tssurya The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Upgrades INFW version to 4.22.
Copy/Replace of: #705
Bump Go version from 1.24.0 to 1.25.0 to satisfy controller-gen@v0.20.1
requirement (requires Go >= 1.25.0). Update go.mod and all Dockerfiles
to use Go 1.25 for consistency.
Fix test case that was using TCP field with UDP protocol type. The
updated controller-gen enforces stricter validation where protocol type
must match the field used in protocolConfig.
Update Makefile to use modern setup-envtest Go binary instead of
deprecated shell script that was failing with 403 errors from Google
Cloud Storage. Align versions with project dependencies:
Regenerate CRDs and RBAC with updated controller-gen.
Summary by CodeRabbit
New Features
Chores
Bug Fixes
Docs