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

ipv6 support for modified-URL-like #1403

Merged

Conversation

imavroukakis
Copy link
Contributor

Closes #1223

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

Nice 👍

if (serverName.startsWith("[")) {
portLoc = serverName.indexOf("]:") + 1;
} else {
serverName = "[" + serverName + "]";
Copy link
Contributor

Choose a reason for hiding this comment

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

There cannot be port in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't one, but because the shortUrl contains the port number, it needs to be formatted that way

shortUrl example sqlserver://[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:1433.

I'm now thinking though, whether this is not appropriate for the host attribute i.e. [2001:0db8:85a3:0000:0000:8a2e:0370:7334]. What do you think?

By the way, the mariadb test is partially incorrect because it includes the port as part of the shortUrl

EDIT
For

"jdbc:mariadb:loadbalance://[2001:0660:7401:0200:0000:0000:0edf:bdd7]:33,mdb.host/mdbdb"

shortUrl becomes

"mariadb:loadbalance://2001:0660:7401:0200:0000:0000:0edf:bdd7:33"

Copy link
Contributor

Choose a reason for hiding this comment

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

You lost me :) I am not proficient with ip6 formats. I just see:

if (serverName.startsWith("[")) {
   portLoc = XXX
} else {
  serverName = "[" + serverName + "]";
}

and wonder, why if serverName does not start with [, then it means there is no port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I went neck-deep into IPv6 land. Yes, since : is a delimiter for the IPv6 parts, the only way to define a port in a URL/URI-like manner, is to wrap the address in [-] and then append the port with a :

Does this make sense, especially with regards to my point about the MariaDB tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarification, when I say

the only way to define a port in a URL/URI-like manner

, I should have said instead, "the only unambigious way to define a port in a URL/URI-like manner"

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I have left a comment in #1421, but that is a separate issue.

@iNikem iNikem merged commit df2b663 into open-telemetry:master Oct 19, 2020
@imavroukakis imavroukakis deleted the modified-url-like-ipv6-support branch October 20, 2020 20:27
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.

JDBCConnectionUrlParser parse sqlserver IPv6 addresses error
3 participants