Skip to content

support ipv6 address validation#3158

Closed
wangjialing218 wants to merge 6 commits intoapache:masterfrom
wangjialing218:branch-ipv6
Closed

support ipv6 address validation#3158
wangjialing218 wants to merge 6 commits intoapache:masterfrom
wangjialing218:branch-ipv6

Conversation

@wangjialing218
Copy link

@wangjialing218 wangjialing218 commented Mar 30, 2022 •
edited
Loading

Motivation

current bookkeeper URI does not support ipv6 address validate.

Changes

  1. upport ipv6 address validate. mainly copy codes from [Issue 8092]support to specify multi ipv6 hosts in brokerServiceUrl pulsar#8120 (comment)
  2. change URI.create to ServiceURI.create to support multi ipv6 address of zookeeper, since URI.create does not support multi ipv6 address validation.

@hangc0276 hangc0276 mentioned this pull request Jun 24, 2022
Open
@hangc0276 hangc0276 added this to the 4.16.0 milestone Jul 25, 2022
hangc0276
hangc0276 reviewed Jul 26, 2022
View reviewed changes
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/net/ServiceURI.java
multiHosts.toArray(new String[multiHosts.size()]),
serviceURI.getServicePath(),
serviceURI.getUri());
}
Copy link
Contributor

@hangc0276 hangc0276 Jul 26, 2022

Choose a reason for hiding this comment

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

Do we need to deal with hosts.size() == 1 ?

Copy link
Author

@wangjialing218 wangjialing218 Jul 27, 2022

Choose a reason for hiding this comment

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

No, URI.create(uriStr) could handle this case, testIPv6Address() cover this test case

StevenLuMT
StevenLuMT requested changes Jul 26, 2022
View reviewed changes
Copy link
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

updating is not very versatile
I think you can add a new class named ServiceIpv6URI

@hangc0276 hangc0276 modified the milestones: 4.16.0, 4.17.0 Jul 31, 2022
@StevenLuMT
Copy link
Member

StevenLuMT commented Aug 24, 2022

fix old workflow,please see #3455 for detail

@wangjialing218 wangjialing218 reopened this Nov 4, 2022
@hangc0276 hangc0276 modified the milestones: 4.17.0, 4.16.0 Jan 11, 2023
@hangc0276 hangc0276 modified the milestones: 4.16.0, 4.17.0 Mar 13, 2023
@hangc0276
Copy link
Contributor

hangc0276 commented Mar 20, 2023

@horizonzy @zymap Please help take a look, thanks.

@hangc0276
Copy link
Contributor

hangc0276 commented Mar 20, 2023

@wangjialing218 Would you please help rebase the master? thanks.

@jiazhai
Copy link
Member

jiazhai commented Mar 20, 2023

Thanks for the help. Do we need some documents for this?

horizonzy
horizonzy reviewed Mar 20, 2023
View reviewed changes
Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

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

In org.apache.bookkeeper.net.DNS#reverseDns, it can't handle ipv6, also need to fix it.

@wangjialing218
Copy link
Author

wangjialing218 commented Apr 10, 2023

In org.apache.bookkeeper.net.DNS#reverseDns, it can't handle ipv6, also need to fix it.

@horizonzy reverseDns only execute when advertisedAddress is not set. In this case, the ipv6 address of the host will be ignored. Let's keep this behave unchanged, bookie server will advertise ipv4 address when advertisedAddress is not set.

To enable advertise ipv6 address for bookie server, we should set advertisedAddress with ipv6 address of the host.

@horizonzy
Copy link
Member

horizonzy commented Apr 13, 2023

Sorry for not getting back to you sooner. I'm reviewing it now.

horizonzy
horizonzy reviewed Apr 14, 2023
View reviewed changes
Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

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

Which case does the code cover?

@zymap
Copy link
Member

zymap commented Dec 4, 2023

Close this since it is open for a long time without any updates. Feel free to reopen it if you want to continue

@zymap zymap closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hangc0276 hangc0276 hangc0276 left review comments

@zymap zymap zymap left review comments

@StevenLuMT StevenLuMT StevenLuMT requested changes

@hezhangjian hezhangjian hezhangjian approved these changes

@dlg99 dlg99 Awaiting requested review from dlg99

@eolivelli eolivelli Awaiting requested review from eolivelli

+1 more reviewer

@horizonzy horizonzy horizonzy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wangjialing218 wangjialing218

Labels

area/bookie release/4.15.6 type/improvement

Projects

None yet

Milestone

4.17.0

Development

Successfully merging this pull request may close these issues.

7 participants

@wangjialing218 @StevenLuMT @hangc0276 @jiazhai @horizonzy @zymap @hezhangjian