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

Added trailing 0 to ipv6 ranges that end in ":" #297

Merged
merged 6 commits into from
Feb 8, 2023

Conversation

nicklesimba
Copy link
Collaborator

@nicklesimba nicklesimba commented Jan 26, 2023

What this PR does / why we need it:
ipv6 ranges ending in a colon, such as a trail of zeroes denoted by "::" result in the storage engine failing to process the range, because ranges are expected to begin and end with alphanumerical characters to meet CRD requirements. Thus we append a 0 to any ranges that meet this criteria because it is functionally equivalent and meets the requirements.

Signed-off-by: nicklesimba simha.nikhil@gmail.com
Which issue(s) this PR fixes:
Fixes #263, fixes #293

Special notes for your reviewer (optional):
https://imgs.xkcd.com/comics/networking_problems.png

ipv6 ranges ending in a colon, such as a trail of zeroes denoted by "::"
result in the storage engine failing to process the range, because
ranges are expected to begin and end with alphanumerical characters to
meet CRD requirements. Thus we append a 0 to any ranges that meet this
criteria because it is functionally equivalent.

Signed-off-by: nicklesimba <simha.nikhil@gmail.com>
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

Totally on board with this change! Can we add a test that ends with a ::?

Add another test near this one / like this one: https://github.com/k8snetworkplumbingwg/whereabouts/blob/master/cmd/whereabouts_test.go#L223-L235

It("allocates and releases an IPv6 range that ends with zeroes with a Kubernetes backend", func() {

ipVersion := "6"
ipRange := "2001:1b74:480:603d:0304:0403:000:0000-2001:1b74:480:603d:0304:0403:0000:0004/64"
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a documentation range, so make the range start with 2001:db8

Also, add an additional test that doesn't just ends with zeros, but another one that ends with :: literally so, for example.... 2001:db8:b33f::/48 or something

Copy link
Member

Choose a reason for hiding this comment

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

@dougbtv
Copy link
Member

dougbtv commented Feb 3, 2023

Looks good, I think we just need a rebase (there's a merge commit from master in here)

nicklesimba and others added 4 commits February 3, 2023 14:22
Signed-off-by: nicklesimba <simha.nikhil@gmail.com>
Signed-off-by: Paulo Gomes <pjbgf@linux.com>
Signed-off-by: Paulo Gomes <pjbgf@linux.com>
Signed-off-by: Paulo Gomes <pjbgf@linux.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

LGTM!

@dougbtv dougbtv merged commit fd77235 into k8snetworkplumbingwg:master Feb 8, 2023
@maiqueb
Copy link
Collaborator

maiqueb commented Feb 8, 2023

wait, this merged a golang upgrade to 1.19.

Do we want to do that ?

Not saying I'm against it, but we probably should have merged #294 instead first.

@maiqueb maiqueb mentioned this pull request Feb 8, 2023
@adilGhaffarDev
Copy link

adilGhaffarDev commented Feb 9, 2023

@dougbtv can we cherry-pick this fix to v0.6 and v0.5? we dont have v0.6 branch can we have a patch release v0.6.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants