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

netutil: consistently format ipv6 addresses #15153

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

Kidsan
Copy link
Contributor

@Kidsan Kidsan commented Jan 20, 2023

This formats ipv6 addresses to ensure they can be compared safely

Fixes #15127

@Kidsan Kidsan marked this pull request as draft January 20, 2023 15:41
@Kidsan Kidsan marked this pull request as ready for review January 20, 2023 18:41
pkg/netutil/netutil.go Outdated Show resolved Hide resolved
This formats ipv6 addresses to ensure they can be compared safely

Signed-off-by: kidsan <8798449+Kidsan@users.noreply.github.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @Kidsan

@ahrtr
Copy link
Member

ahrtr commented Jan 26, 2023

Could you backport this PR to 3.4 and 3.5 as well?

@serathius
Copy link
Member

Should we add a e2e test for IPv6?

@ahrtr
Copy link
Member

ahrtr commented Jan 26, 2023

Should we add a e2e test for IPv6?

Adding e2e test is good, but I think it can be a separate PR.

@serathius
Copy link
Member

serathius commented Jan 27, 2023

Makes sense, however let's posmone backporting until we ensure that overall issue of IPv6 is fixed and tested. Please remember that we are backporting to fixes to issues, not just backporting PRs for sake of backporting.

@ahrtr
Copy link
Member

ahrtr commented Jan 27, 2023

Makes sense, however let's posmone backporting until we ensure that overall issue of IPv6 is fixed and tested. Please remember that we are backporting to fixes to issues, not just backporting PRs for sake of backporting.

Do not understand the concern.

  1. Do not literally compare IPv6 addresses #15127 is a real issue to me, please let me know if you don't think it's an issue. The contributor fixes the issue and backport it to 3.5 and 3.4. It makes perfect sense to me. Thx @Kidsan
  2. We can add whatever e2e test for IPv6 in separate PRs. If we see any other issues, we can just continue fix them. etcd shouldn't care about the IP type (IPv4 or IPv6), so I don't worry about any big changes due to any new issues caused by IPv6.

@serathius
Copy link
Member

For me a feature like IPv6 support cannot exist without tests. If IPv6 formatting didn't work, means that our tests were insufficient, ergo there might be other things that are in out blind spot.

Discovering and fixing one detail after another is insufficient to make sufficient progress. We need more holistic approach. My thinking is that if IPv6 had such a rudimental bug like this one, it was never properly tested. We should revisit this feature and add sufficient testing to bring up the quality of this area.

As agreed in https://github.com/etcd-io/etcd/blob/main/Documentation/contributor-guide/features.md all stable features should have a proper testing. If we don't think that IPv6 support is mature enough we should mark it as experimental.

@ahrtr
Copy link
Member

ahrtr commented Jan 28, 2023

I do not see any strong reason to reject this minor bug fix.

@sanmai-NL
Copy link

I think you can both merge this PR and open/request an issue or PR for the tests.

@serathius
Copy link
Member

FYI I'm not blocking merging of this PR, just backporting before whole issue is resolved.

@ahrtr ahrtr merged commit 82243d0 into etcd-io:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Do not literally compare IPv6 addresses
4 participants