OCPBUGS-86886: modernize dependency of k8s imports#92
Open
rikatz wants to merge 5 commits into
Open
Conversation
openshift-ci-robot
added
jira/severity-low
Contributor
|
@rikatz: This pull request references Jira Issue OCPBUGS-86886, 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. |
Member
Author
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
Contributor
|
@rikatz: This pull request references Jira Issue OCPBUGS-86886, 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. |
WalkthroughRemoves legacy k8s and container-image indirect deps from go.mod, migrates ingress controllers to client-go scheme, replaces controller.KeyFunc usage with cache.DeletionHandlingMetaNamespaceKeyFunc in ingressip controller, and adds a new in-memory CIDR-backed IP allocator with README and tests. ChangesLegacy Kubernetes API cleanup and modern scheme migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz 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 |
openshift-ci
Bot
added
the
approved
Contributor
|
@rikatz: This pull request references Jira Issue OCPBUGS-86886, 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. |
|
I think this PR takes care of everything in |
![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 `@pkg/utils/ipallocator/ipallocator_test.go`:
- Line 45: Replace the ignored error returns from NewInMemory(...) and
r.Allocate(...) in the ipallocator tests so failures surface immediately: after
calling NewInMemory(...) (symbol NewInMemory) check the returned error and fail
the test (e.g., t.Fatalf or require.NoError) instead of using “_”, and likewise
check the error returned by Allocate (symbol Allocate) and fail the test on
error; apply this change to every test that currently discards those errors so
setup/allocation errors break the test immediately.
In `@pkg/utils/ipallocator/ipallocator.go`:
- Around line 168-184: The contains() logic currently truncates the big.Int
delta to int64/int via calculateIPOffset, which can overflow for large IPv6
ranges; change calculateIPOffset to return the full *big.Int delta
(netutils.BigForIP(ip).Sub(base)) and update Range.contains to compare that
big.Int directly: check delta.Sign() < 0 or
delta.Cmp(big.NewInt(0).SetInt64(int64(r.max))) >= 0 to decide out-of-range, and
only after those checks convert the delta to a Go int (ensuring it fits, e.g.
via delta.IsInt64() and bounds against runtime.MaxInt) before returning the
offset; update references to calculateIPOffset and the return types accordingly
to avoid any Int64()/int truncation.
🪄 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: ce8607bd-6377-4d23-bd20-e8cdebaba694
⛔ Files ignored due to path filters (140)
go.sumis excluded by!**/*.sumvendor/github.com/distribution/reference/.gitattributesis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/CODE-OF-CONDUCT.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/GOVERNANCE.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/distribution-logo.svgis excluded by!**/*.svg,!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/normalize.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/reference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/regexp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/distribution/reference/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/.mailmapis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/.pullapprove.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/LICENSE.docsis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/algorithm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/digest.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/digester.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/opencontainers/go-digest/verifiers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/chain.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/constantdelay.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/cron.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/logger.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/option.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/robfig/cron/v3/spec.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apiextensions-apiserver/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/features/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/features/kube_features.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/util/retry/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/util/retry/util.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/node/util/sysctl/namespace.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/node/util/sysctl/sysctl.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/resource/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/resource/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/scheduling/corev1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/scheduling/corev1/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/controller-manager/pkg/features/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/controller-manager/pkg/features/kube_features.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubelet/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubelet/pkg/apis/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubelet/pkg/apis/well_known_labels.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/api/legacyscheme/scheme.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/api/service/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/api/service/util.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/api/service/warnings.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/api/servicecidr/servicecidr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/api/v1/pod/util.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/api/v1/service/util.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/apps/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/apps/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/apps/register.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/apps/types.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/apps/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kubernetes/pkg/apis/autoscaling/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/autoscaling/annotations.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/autoscaling/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/autoscaling/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/autoscaling/register.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/autoscaling/types.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/autoscaling/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kubernetes/pkg/apis/certificates/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/certificates/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/certificates/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/certificates/register.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/certificates/types.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/certificates/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kubernetes/pkg/apis/core/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/annotation_key_constants.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/helper/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/helper/qos/qos.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/install/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/install/install.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/json.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/objectreference.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/pods/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/register.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/resource.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/taint.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/toleration.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/types.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/v1/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/v1/conversion.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/v1/defaults.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/v1/helper/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/v1/zz_generated.conversion.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kubernetes/pkg/apis/core/v1/zz_generated.defaults.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kubernetes/pkg/apis/core/v1/zz_generated.validations.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kubernetes/pkg/apis/core/validation/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/validation/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/validation/events.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/validation/names.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/validation/validation.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/apis/core/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kubernetes/pkg/capabilities/capabilities.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/capabilities/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/controller/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/controller/controller_ref_manager.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/controller/controller_utils.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/controller/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/features/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/features/client_adapter.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/features/kube_features.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/fieldpath/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/fieldpath/fieldpath.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/allocator/bitmap.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/allocator/interfaces.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/allocator/utils.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator/bitmap.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator/cidrallocator.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator/interfaces.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator/ipallocator.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/util/hash/hash.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/util/parsers/parsers.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kubernetes/pkg/util/taints/taints.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (8)
go.modpkg/route/ingress/ingress.gopkg/route/ingressip/service_ingressip_controller.gopkg/route/ingressip/service_ingressip_controller_test.gopkg/utils/ipallocator/README.mdpkg/utils/ipallocator/allocator.gopkg/utils/ipallocator/ipallocator.gopkg/utils/ipallocator/ipallocator_test.go
💤 Files with no reviewable changes (1)
- go.mod
✅ Files skipped from review due to trivial changes (1)
- pkg/utils/ipallocator/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/route/ingress/ingress.go
- pkg/route/ingressip/service_ingressip_controller.go
| } | ||
|
|
||
| func TestAllocateSpecific(t *testing.T) { | ||
| r, _ := NewInMemory(parseCIDR(t, "172.16.0.0/28")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/utils/ipallocator/ipallocator_test.go"
if [ ! -f "$file" ]; then
echo "Missing file: $file" >&2
exit 1
fi
echo "== Lines 1-220: $file =="
nl -ba "$file" | sed -n '1,220p'
echo
echo "== Lines containing 'NewInMemory' / 'Allocate(' =="
rg -n "NewInMemory\\(|Allocate\\(" "$file" || true
echo
echo "== Lines where errors are assigned to '_' =="
rg -n ":=\\s*.*_,\\s*\\w|_,\\s*_\\s*[:=]|\\s*_,\\s*_\\s*:=" "$file" || trueRepository: openshift/route-controller-manager
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/utils/ipallocator/ipallocator_test.go"
sed -n '1,1p' "$file" >/dev/null 2>&1 || { echo "Missing/unreadable: $file" >&2; exit 1; }
echo "== Relevant line ranges (1-220): $file =="
cat -n "$file" | sed -n '1,220p'
echo
echo "== Lines containing 'NewInMemory(' or 'Allocate(' =="
rg -n "NewInMemory\\(|Allocate\\(" "$file" || true
echo
echo "== Lines where an error is discarded with '_' (common patterns) =="
rg -n "NewInMemory\\(.*\\)\\s*$|\\:=\\s*[^\\n]*_,\\s*[^\\n]*$|\\:=\\s*[^\\n]*\\_,\\s*[^\\n]*$|\\,\\s*_\\s*[:=]|\\b_\\s*[:=]" "$file" || trueRepository: openshift/route-controller-manager
Length of output: 6994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/utils/ipallocator/ipallocator_test.go"
echo "== Exact discarded-constructor error patterns =="
rg -n "r, _ := NewInMemory" "$file" || true
echo
echo "== Allocate calls that ignore returned error (r.Allocate(…)) =="
rg -n "^\s*r\.Allocate\(" "$file" || true
echo
echo "== Allocate calls with error checks (looking for := or if err :=) =="
rg -n "Allocate\\(.*\\)\\s*\\{|if .*Allocate|:= .*Allocate|err := .*Allocate|_=.*Allocate" "$file" || true
echo
echo "== Show around each match for context =="
for m in $(rg -n "r, _ := NewInMemory|^\s*r\.Allocate\(" "$file" | cut -d: -f1 | sort -n | uniq); do
echo "--- Context around line $m ---"
start=$((m-3)); end=$((m+3))
sed -n "${start},${end}p" "$file" | cat -n | head -n 10
doneRepository: openshift/route-controller-manager
Length of output: 4315
Stop discarding errors from NewInMemory() and Allocate() in ipallocator tests.
In pkg/utils/ipallocator/ipallocator_test.go, multiple tests ignore constructor errors (r, _ := NewInMemory(...) at lines 45, 57, 76, 94, 101, 113, 125, 139, 146, 153) and also ignore the returned error from allocation (r.Allocate(ip) at lines 79 and 116). These can hide allocator regressions and leave later assertions validating the wrong state—fail the test on every setup/allocation error instead of using _.
🤖 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 `@pkg/utils/ipallocator/ipallocator_test.go` at line 45, Replace the ignored
error returns from NewInMemory(...) and r.Allocate(...) in the ipallocator tests
so failures surface immediately: after calling NewInMemory(...) (symbol
NewInMemory) check the returned error and fail the test (e.g., t.Fatalf or
require.NoError) instead of using “_”, and likewise check the error returned by
Allocate (symbol Allocate) and fail the test on error; apply this change to
every test that currently discards those errors so setup/allocation errors break
the test immediately.
Comment on lines
+168
to
+184
| func (r *Range) contains(ip net.IP) (bool, int) { | ||
| if !r.net.Contains(ip) { | ||
| return false, 0 | ||
| } | ||
|
|
||
| offset := calculateIPOffset(r.base, ip) | ||
| if offset < 0 || offset >= r.max { | ||
| return false, 0 | ||
| } | ||
| return true, offset | ||
| } | ||
|
|
||
| // calculateIPOffset calculates the integer offset of ip from base such that | ||
| // base + offset = ip. It requires ip >= base. | ||
| func calculateIPOffset(base *big.Int, ip net.IP) int { | ||
| return int(big.NewInt(0).Sub(netutils.BigForIP(ip), base).Int64()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and print the relevant sections
echo "=== pkg/utils/ipallocator/ipallocator.go (contains + calculateIPOffset + Range fields) ==="
sed -n '1,260p' pkg/utils/ipallocator/ipallocator.go | nl -ba | sed -n '130,230p'
echo "=== pkg/utils/ipallocator/ipallocator.go (NewInMemory and IPv6 capping) ==="
sed -n '1,260p' pkg/utils/ipallocator/ipallocator.go | nl -ba | sed -n '1,150p'
echo "=== Downstream usage: service_ingressip_controller.go around 315-341 ==="
sed -n '280,380p' pkg/route/ingressip/service_ingressip_controller.go | nl -ba | sed -n '20,140p'
echo "=== Find where ErrNotInRange is produced/propagated in ipallocator ==="
rg -n "ErrNotInRange|NotInRange" pkg/utils/ipallocator/ipallocator.go pkg/utils/ipallocator -S
echo "=== Ensure Allocate/Release/Has use contains() as claimed ==="
rg -n "contains\\(" pkg/utils/ipallocator/ipallocator.go -SRepository: openshift/route-controller-manager
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== File existence ==="
ls -la pkg/utils/ipallocator/ipallocator.go
ls -la pkg/route/ingressip/service_ingressip_controller.go
echo "=== pkg/utils/ipallocator/ipallocator.go (around contains/calculateIPOffset) ==="
python3 - <<'PY'
import itertools
path="pkg/utils/ipallocator/ipallocator.go"
start, end = 120, 220
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:5d}: {line.rstrip()}")
if i > end:
break
PY
echo "=== pkg/utils/ipallocator/ipallocator.go (around NewInMemory) ==="
python3 - <<'PY'
import itertools
path="pkg/utils/ipallocator/ipallocator.go"
start, end = 1, 160
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:5d}: {line.rstrip()}")
if i > end:
break
PY
echo "=== Downstream usage: service_ingressip_controller.go around 315-341 ==="
python3 - <<'PY'
path="pkg/route/ingressip/service_ingressip_controller.go"
start, end = 280, 380
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:5d}: {line.rstrip()}")
if i > end:
break
PY
echo "=== Search for ErrNotInRange production ==="
rg -n "ErrNotInRange" pkg/utils/ipallocator -S
echo "=== Search for contains() usage in ipallocator ==="
rg -n "contains\\(" pkg/utils/ipallocator/ipallocator.go -SRepository: openshift/route-controller-manager
Length of output: 13671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pkg/utils/ipallocator/ipallocator_test.go (around existing tests) ==="
python3 - <<'PY'
path="pkg/utils/ipallocator/ipallocator_test.go"
start, end = 1, 220
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:5d}: {line.rstrip()}")
if i > end:
break
PY
echo "=== allocation map implementation: search for newAllocationMapWithOffset ==="
rg -n "newAllocationMapWithOffset|type allocation|allocatorInterface|Allocate\\(offset\\)" pkg/utils/ipallocator -S
echo "=== show file(s) containing newAllocationMapWithOffset and allocatorInterface ==="
fd -a "ipallocator" pkg/utils/ipallocator 2>/dev/null || true
files=$(rg -l "newAllocationMapWithOffset" pkg/utils/ipallocator -S || true)
echo "Files: $files"
for f in $files; do
echo "---- $f (relevant lines) ----"
python3 - <<PY
path="$f"
start,end=1,260
with open(path,'r',encoding='utf-8') as fh:
for i,line in enumerate(fh, start=1):
if i>=1 and i<=260:
pass
PY
done
# Print the whole relevant files but capped by size using sed line ranges
for f in $files; do
echo "---- $f (first 260 lines with numbers) ----"
python3 - <<'PY'
import sys
path=sys.argv[1]
start=1; end=260
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:5d}: {line.rstrip()}")
if i>end:
break
PY "$f"
doneRepository: openshift/route-controller-manager
Length of output: 7432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pkg/utils/ipallocator/allocator.go (around allocator + bitmap Allocate/Release) ==="
python3 - <<'PY'
path="pkg/utils/ipallocator/allocator.go"
# Print key sections; adjust ranges if needed.
ranges=[(1,220),(220,420)]
for start,end in ranges:
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:5d}: {line.rstrip()}")
if i>end:
break
PY
echo "=== pkg/utils/ipallocator/allocator.go: search for bounds checks in Allocate/Release/Has ==="
rg -n "func \\(.*\\) (Allocate|Release|Has)\\(" pkg/utils/ipallocator/allocator.go -S || true
rg -n "offset|max" pkg/utils/ipallocator/allocator.go -S | head -n 120Repository: openshift/route-controller-manager
Length of output: 8790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "calculateIPOffset" pkg/utils/ipallocator -SRepository: openshift/route-controller-manager
Length of output: 374
Avoid big.Int truncation in IPv6 offset bounds checks.
pkg/utils/ipallocator/ipallocator.go computes the offset using Int64()/int (calculateIPOffset), then uses that truncated value for the offset < 0 || offset >= r.max check in (*Range).contains(). For large IPv6 CIDRs, the big.Int delta can overflow int64 and wrap into a small in-range int, causing contains() to return ok=true and making Allocate() succeed instead of returning *ErrNotInRange (which service_ingressip_controller.go relies on to trigger reallocation).
Suggested fix
func (r *Range) contains(ip net.IP) (bool, int) {
if !r.net.Contains(ip) {
return false, 0
}
- offset := calculateIPOffset(r.base, ip)
- if offset < 0 || offset >= r.max {
+ offset := big.NewInt(0).Sub(netutils.BigForIP(ip), r.base)
+ if offset.Sign() < 0 || offset.Cmp(big.NewInt(int64(r.max))) >= 0 {
return false, 0
}
- return true, offset
-}
-
-// calculateIPOffset calculates the integer offset of ip from base such that
-// base + offset = ip. It requires ip >= base.
-func calculateIPOffset(base *big.Int, ip net.IP) int {
- return int(big.NewInt(0).Sub(netutils.BigForIP(ip), base).Int64())
+ return true, int(offset.Int64())
}🤖 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 `@pkg/utils/ipallocator/ipallocator.go` around lines 168 - 184, The contains()
logic currently truncates the big.Int delta to int64/int via calculateIPOffset,
which can overflow for large IPv6 ranges; change calculateIPOffset to return the
full *big.Int delta (netutils.BigForIP(ip).Sub(base)) and update Range.contains
to compare that big.Int directly: check delta.Sign() < 0 or
delta.Cmp(big.NewInt(0).SetInt64(int64(r.max))) >= 0 to decide out-of-range, and
only after those checks convert the delta to a Go int (ensuring it fits, e.g.
via delta.IsInt64() and bounds against runtime.MaxInt) before returning the
offset; update references to calculateIPOffset and the return types accordingly
to avoid any Int64()/int truncation.
Member
Author
There was a problem hiding this comment.
@coderabbitai this code was copied from the dropped vendor vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator and I don't wanna change any behavior
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve my own review comment on the PR
gh api graphql -f query='
{
repository(owner: "openshift", name: "route-controller-manager") {
pullRequest(number: 92) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}'Length of output: 12923
🏁 Script executed:
#!/bin/bash
# Resolve my own review thread about big.Int truncation
gh api graphql -f query='
mutation {
resolveReviewThread(input: { threadId: "PRRT_kwDOHzQul86GhoJ5" }) {
thread {
id
isResolved
}
}
}'Length of output: 735
@rikatz, understood — I'll defer to the upstream behavior since this is a faithful copy of vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator. I wasn't able to resolve the comment programmatically due to permissions, but you can dismiss it manually.
✏️ Learnings added
Learnt from: rikatz
Repo: openshift/route-controller-manager PR: 92
File: pkg/utils/ipallocator/ipallocator.go:168-184
Timestamp: 2026-06-02T18:25:50.708Z
Learning: In `pkg/utils/ipallocator/ipallocator.go` and `pkg/utils/ipallocator/allocator.go` (repository: openshift/route-controller-manager), the IP allocator implementation was intentionally copied verbatim from the dropped vendor path `vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator`. The maintainer does not want to diverge from upstream behavior, so review suggestions that alter logic (e.g., `big.Int` truncation in `calculateIPOffset`/`contains()`) should not be raised against this code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
rikatz
commented
Jun 2, 2026
| "k8s.io/kubernetes/pkg/registry/core/service/allocator" | ||
| "k8s.io/kubernetes/pkg/registry/core/service/ipallocator" | ||
|
|
||
| "github.com/openshift/route-controller-manager/pkg/utils/ipallocator" |
Member
Author
There was a problem hiding this comment.
this could be github.com/cilium/ipam which is also a copy, we can consider it
Member
Author
|
/retest-required |
Member
Author
|
/retest prow/e2e-gcp |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== pkg/utils/ipallocator/ipallocator.go ==="
cat pkg/utils/ipallocator/ipallocator.go
echo ""
echo "=== pkg/utils/ipallocator/allocator.go ==="
cat pkg/utils/ipallocator/allocator.go
echo ""
echo "=== pkg/utils/ipallocator/ipallocator_test.go ==="
cat pkg/utils/ipallocator/ipallocator_test.go
echo ""
echo "=== pkg/utils/ipallocator/README.md ==="
cat pkg/utils/ipallocator/README.md
echo ""
echo "=== pkg/route/ingressip/service_ingressip_controller.go ==="
cat pkg/route/ingressip/service_ingressip_controller.go
echo ""
echo "=== pkg/route/ingressip/service_ingressip_controller_test.go ==="
cat pkg/route/ingressip/service_ingressip_controller_test.go
echo ""
echo "=== Searching for any remaining k8s.io/kubernetes imports ==="
rg "k8s.io/kubernetes" --type=go -n
echo ""
echo "=== Searching for ipallocator usages in the codebase ==="
rg "ipallocator" --type=go -n -A2 -B2
echo ""
echo "=== Check if there's a vendor copy of the original ==="
find . -path "*/registry/core/service/ipallocator*" 2>/dev/null | head -20
echo ""
echo "=== Check go.mod for remaining k8s.io/kubernetes ==="
cat go.mod | grep -E "k8s.io/kubernetes|ipallocator"Length of output: 63945
Dropped dependency verification
|
| Step | New code | Upstream logic | Equivalent? |
|---|---|---|---|
| Compute range size | netutils.RangeSize(cidr) |
same call | ✅ |
| Compute numeric base | netutils.BigForIP(cidr.IP) |
same call | ✅ |
| IPv4 broadcast skip | max-- |
max-- |
✅ |
| IPv6 cap | if max > 65536 { max = 65536 } |
same cap | ✅ |
Skip .0 network address |
base.Add(base, big.NewInt(1)); max-- |
same | ✅ |
| Clamp negative max | if max < 0 { max = 0 } |
same guard | ✅ |
| Allocation map offset | calculateRangeOffset(cidr) |
same function | ✅ |
calculateRangeOffset
Constants min=16, max=256, step=16 are byte-for-byte identical to the upstream. Formula min(max(min, cidrSize/step), max), returns 0 when cidrSize <= min. ✅
contains(ip)
net.Contains(ip) → calculateIPOffset(base, ip) → bounds [0, r.max)
Same logic as upstream. ✅
calculateIPOffset
int(big.NewInt(0).Sub(netutils.BigForIP(ip), base).Int64())Uses big.Int arithmetic then converts to int64 → int. Safe for all practical CIDR ranges (IPv6 capped at 65536; IPv4 never exceeds ~4B which fits in int64). Verbatim from upstream. ✅
Public methods — error contract
| Method | Returned errors | Controller checks for | Contract preserved? |
|---|---|---|---|
Allocate(ip) |
*ErrNotInRange, ErrAllocated |
err.(*ipallocator.ErrNotInRange) type-assert |
✅ |
AllocateNext() |
ErrFull |
err == ipallocator.ErrFull |
✅ |
Release(ip) |
nil if out-of-range; underlying bitmap error |
controller logs any non-nil | ✅ |
Has(ip) |
false if out-of-range |
direct bool use | ✅ |
Free() |
int |
direct int use | ✅ |
pkg/utils/ipallocator/allocator.go — semantic analysis
allocationBitmap
| Method | Behaviour | Notes |
|---|---|---|
Allocate(offset) |
bounds check → check bit → set bit + count++ → return (true, nil) or (false, nil) |
No error on double-alloc, upper layer (ipallocator.go) converts (false, nil) → ErrAllocated. ✅ |
AllocateNext() |
delegates to strategy.AllocateBit → set bit + count++ → (offset, true, nil) or (0, false, nil) |
Upper layer converts false → ErrFull. ✅ |
Release(offset) |
no-op if bit=0; otherwise clear bit + count-- | ✅ |
Has(offset) |
returns allocated.Bit(offset) == 1 |
✅ |
Free() |
max - count |
✅ |
randomScanStrategyWithOffset.AllocateBit
- Returns
(0, false)immediately whencount >= max(full). - Computes
subrangeMax = max - offset(the "dynamic" upper block). - Picks random start in
[0, subrangeMax), scans forward wrapping within upper block. - If upper block exhausted, falls back to lower block
[0, offset). - Guards
rand.Intn(0)panic withif rss.offset > 0before the fallback scan. ✅
This preserves the upstream semantics: IPs in the "static allocation" zone (lower block) are withheld from AllocateNext until the upper block is full, matching the original randomScanStrategyWithOffset.
Controller changes (service_ingressip_controller.go)
1. ipallocator.NewInMemory(ipNet) → no breaking change
Provides identical in-memory allocation over the same *net.IPNet. The returned *ipallocator.Range exposes the same method set used by the controller. ✅
2. cache.DeletionHandlingMetaNamespaceKeyFunc replaces controller.KeyFunc
In k8s.io/kubernetes/pkg/controller, KeyFunc is literally defined as:
var KeyFunc = cache.DeletionHandlingMetaNamespaceKeyFuncThe new code calls cache.DeletionHandlingMetaNamespaceKeyFunc directly — same function, different import path. Used in 3 places: enqueueChange, processInitialSync (pending services loop), clearOldAllocation. All semantically identical. ✅
3. scheme.Scheme (k8s.io/client-go/kubernetes/scheme) replaces legacyscheme.Scheme
The legacy scheme was the full monorepo scheme. client-go/kubernetes/scheme registers all core API types including v1.Service and v1.EventSource, which is all the event recorder needs. No functional change for IngressIP controller event recording. ✅
Test file change
// Before
informers.NewSharedInformerFactory(client, controller.NoResyncPeriodFunc())
// After
informers.NewSharedInformerFactory(client, 0)controller.NoResyncPeriodFunc() is defined as returning 0 (a zero time.Duration). Passing 0 directly is byte-for-byte the same value. ✅
Removed features (README accuracy check)
All 6 listed removals (metrics, dry-run, snapshot/restore, IPFamily, factory pattern, unused methods) are confirmed absent from the new code and are not referenced anywhere in the codebase. The controller only ever called NewInMemory/Allocate/AllocateNext/Release/Has/Free — all of which are preserved. ✅
Verdict
This is not a breaking change. The copied code is semantically equivalent to the upstream k8s.io/kubernetes ipallocator for every code path exercised by the IngressIP controller. The public API contract (error sentinels, method signatures, range boundary logic) is fully preserved, and the dependency on the Kubernetes monorepo is cleanly eliminated.
🧠 Learnings used
Learnt from: rikatz
Repo: openshift/route-controller-manager PR: 92
File: pkg/utils/ipallocator/ipallocator.go:168-184
Timestamp: 2026-06-02T18:25:54.551Z
Learning: In `pkg/utils/ipallocator/ipallocator.go` and `pkg/utils/ipallocator/allocator.go` (repository: openshift/route-controller-manager), the IP allocator implementation was intentionally copied verbatim from the dropped vendor path `vendor/k8s.io/kubernetes/pkg/registry/core/service/ipallocator`. The maintainer does not want to diverge from upstream behavior, so review suggestions that alter logic (e.g., `big.Int` truncation in `calculateIPOffset`/`contains()`) should not be raised against this code.
Contributor
|
@rikatz: The The following commands are available to trigger optional jobs: 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. |
Contributor
|
@rikatz: 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. |
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
4 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.
This PR removes dependency of imports that were once necessary but were moved to a proper package, reducing the surface of import of libraries on route-controller-manager
Summary by CodeRabbit