replace iptree on the repairip controller #122888
Conversation
k8s-ci-robot
added
release-note-none
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
k8s-ci-robot
added
the
needs-priority
|
/assign @tnqn @danwinship |
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) |
There was a problem hiding this comment.
NamespaceIndex shouldn't be required here as it's non namespaced resource?
| @@ -135,7 +130,6 @@ func NewRepairIPAddress(interval time.Duration, | |||
| cidrQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "servicecidrs"), | |||
There was a problem hiding this comment.
I suppose cidrQueue should be removed too?
pkg/util/servicecidr/servicecidr.go
Outdated
| // is not the network address and also, if IPv4, is not the broadcast address. | ||
| // This is required because the Kubernetes allocators reserve these addresses | ||
| // so IPAddresses can not block deletion of this ranges. | ||
| func PrefixContainIP(prefix netip.Prefix, ip netip.Addr) bool { |
There was a problem hiding this comment.
| func PrefixContainIP(prefix netip.Prefix, ip netip.Addr) bool { | |
| func PrefixContainsIP(prefix netip.Prefix, ip netip.Addr) bool { |
And I feel like it should be ServiceCIDRContainsIP or something, though I guess that would be wrong since we already have servicecidr in the package name... But still, it would be good to make it more obvious that this is not just a generic utility that just happens to be in the servicecidr package (like BroadcastAddress below), but rather, it is a utility that is in the servicecidr package because it's doing something non-generic and ServiceCIDR-specific.
ContainsAllocatableIP?
There was a problem hiding this comment.
(Also, this function does not need to be exported in this commit... does it get used from outside the package later?)
There was a problem hiding this comment.
yes, I split a large PR, but I will make this unexported now and export it later, once it gets used
pkg/util/servicecidr/servicecidr.go
Outdated
| base := subnet.Masked().Addr() | ||
| bytes := base.AsSlice() | ||
| // get all the host bits from the subnet | ||
| n := 8*len(bytes) - subnet.Bits() |
There was a problem hiding this comment.
| n := 8*len(bytes) - subnet.Bits() | |
| n := base.BitLen() - subnet.Bits() |
pkg/util/servicecidr/servicecidr.go
Outdated
| bytes[i] |= mask | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
If you made this function IPv4-specific you could do
bits := binary.BigEndian.Uint32(bytes)
bits |= math.MaxUint32 >> subnet.Bits()
binary.BigEndian.PutUint32(bytes, bits)
(not tested)
| // isIPOutOfRange returns false if the IP is not contained in any of the ServiceCIDRs | ||
| func (r *RepairIPAddress) isIPOutOfRange(ip net.IP) bool { | ||
| address := servicecidr.IPToAddr(ip) | ||
| return len(servicecidr.ContainsAddress(r.serviceCIDRLister, address)) == 0 |
There was a problem hiding this comment.
you could just have servicecidr.ContainsAddress take a net.IP (for now)
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aojea The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
removed
the
needs-rebase
|
@aojea: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
/close Aggregated all the commits in one PR to remove the iptree all at once #122047 or it will be hard to understand the changes on the helpers functions |
|
@aojea: Closed this PR. In 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. |
/kind cleanup
This is part of #122047 but splitting it to facilitate the review, this is the most straightforward as it just needs to validate if the IP is contained within the existing ServiceCIDR ranges
Since these helpers have two be used in 3 different controllers I'm moving them to a common location, bear in mind that the iptree will be removed lately.
/sig network