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

Using net.JoinHostPort to format host:port strings #782

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mialinx
Copy link
Contributor

@mialinx mialinx commented Jan 18, 2019

IPv6 based addrs shoul look like [host]:port instead of host:port

A Pull Request should be associated with an Issue.

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.

Related issue: https://github.com/github/orchestrator/issues/0123456789

Thank you! We are open to PRs, but please understand if for technical reasons we are unable to accept each and any PR

Description

This PR [briefly explain what is does]

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • code is formatted via gofmt (please avoid goimports)
  • code is built via ./build.sh
  • code is tested via go test ./go/...

IPv6 based addrs shoul look like [host]:port instead of host:port
@shlomi-noach
Copy link
Collaborator

Thank you. This will unfortunately break a lot of things. Building the correct [host]:port is one thing, but there's a myriad of places where we assume the format is host:port and not [host]:port.

It is very unfortunate, indeed, but the change to ipv6 would need to be massive.

@shlomi-noach
Copy link
Collaborator

Hi @mialinx, I see you keep pushing changes. Would you like to perhaps explain what you are doing? Like I wrote earlier, there's a LOT to change if we want to go ipv6 that it's difficult to even find where to begin with.

@mialinx
Copy link
Contributor Author

mialinx commented Jan 22, 2019

Hi @shlomi-noach,

Yep, I'm trying to find all suspicious places like using ':', ":", "%s:" in the code, check if they are related to hostname, port handling and replace them with IPv6 aware code.

As far as I understood, there are two main points:

  • Check that all services may be conneсted using IPv6 addr. I have tested using IPv6 for specifing backend MySQL database, RAFT nodes, Zookeper store, and discovering mysql clusters (even @@hostname and show slave hosts are IPv6 addrs). And it works to me.
  • Using host:port as identifier for comparing instances. I think it should also work if we carefully change all occurrences of host:port. Anyway for IPv4 and hostnames identifiers should stay the same.

It's very likely that I missed somtheing :( What other difficulties do you see ?

@shlomi-noach
Copy link
Collaborator

Your changes are thorough, indeed. What concerns me are all the places where we parse host:port. I'm yet to consider all the cases, my focus right now is on a few other high priority issues. Meanwhile I applaud you for this undertaking.

@mialinx
Copy link
Contributor Author

mialinx commented Jan 22, 2019

It seems that integration tests flap as they depened on map keys order:
#785

@pznamensky
Copy link

@mialinx thanks for your work!
The only reason we don't use orchestrator is IPv6 related issues like this: #616
I hope this PR will be approved.

@shlomi-noach
Copy link
Collaborator

I completely dropped the ball on this PR. It'll take me time to re0review it.

@d3mash
Copy link

d3mash commented Jan 15, 2020

could you please take a look at this PR?

@mainameiz
Copy link

We really really need this PR. Please take a look 🙏

@shlomi-noach
Copy link
Collaborator

I apologize for not giving this attention. I very much appreciate the contribution and your work. It'll take a while before I've fully reviewed this. I'll meanwhile send for production testing in staging environment.

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

Successfully merging this pull request may close these issues.

None yet

5 participants