Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lbipam: Fix off-by-one error in rangeFromPrefix #29425

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Nov 28, 2023

There's an off-by-one error in rangeFromPrefix that miscalculates the end of the allocatable IP range when the prefix length is not byte-aligned. We couldn't catch this bug with the test because the expected value in the test was wrong.

netipx package provides a convenient function netipx.PrefixLastIP which does what we want. Rely on that instead of calculating it by ourselves.

Fixes: #29410
Fixes: 32feef5
Fixes: 27322f3

lbipam: Fix off-by-one error in LBIPAM range allocation

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 28, 2023
@YutaroHayakawa YutaroHayakawa added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Nov 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 28, 2023
@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review November 28, 2023 04:01
@YutaroHayakawa YutaroHayakawa requested a review from a team as a code owner November 28, 2023 04:01
@YutaroHayakawa
Copy link
Member Author

/test

Copy link
Contributor

@aspsk aspsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

@YutaroHayakawa
Copy link
Member Author

Rebased on the latest main.

@YutaroHayakawa
Copy link
Member Author

/test

@tklauser
Copy link
Member

Looks like this needs a rebase to pull in a2694fc to fix the failing runtime tests.

There's an off-by-one error in rangeFromPrefix that miscalculates the
end of the allocatable IP range when the prefix length is not
byte-aligned. We couldn't catch this bug with the test because the
expected value in the test was wrong.

netipx package provides a convenient function netipx.PrefixLastIP which
does what we want. Rely on that instead of calculating it by ourselves.

Fixes: cilium#29410
Fixes: 32feef5
Fixes: 27322f3

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
Copy link
Member Author

Thanks! Rebased...

@tklauser
Copy link
Member

/test

@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Nov 29, 2023

Hmm the image build failure is very mysterious (thanks Tobias for retrying). Now it's succeeding, so it's an intermittent error. Sort of rate limiting I guess...

#25 ERROR: failed to push quay.io/cilium/operator-aws-ci:664342a3665098fa63a378ae7bd28b55497350bb: failed to copy: unexpected status from PUT request to https://quay.io/v2/cilium/operator-aws-ci/blobs/uploads/c685ce25-72e4-4667-89f6-68fc6d8edbc1?digest=sha256%3Ab91a0ae35e54eba53655d1f10a01f0b6caa632482615eb0e9be2a22aedd7bd5d: 413 Request Entity Too Large

@YutaroHayakawa
Copy link
Member Author

Retriggered all tests failed with timeout waiting for the image build.

@YutaroHayakawa
Copy link
Member Author

Cilium E2E: #29476

@tklauser tklauser added this pull request to the merge queue Nov 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 29, 2023
Merged via the queue into cilium:main with commit fd29df3 Nov 29, 2023
61 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/lb-ipam ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LB IPAM allocates IPs improperly from a subnet
3 participants