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.

Sign up for GitHub

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

Jump to bottom

MODCLUSTER-572 Normalize hostnames in MCMP messages #246

Merged
merged 1 commit into from Feb 15, 2017
Merged

MODCLUSTER-572 Normalize hostnames in MCMP messages #246

merged 1 commit into from Feb 15, 2017

Conversation

rhusar
Copy link
Member

@rhusar rhusar commented Feb 14, 2017

pferraro
pferraro requested changes Feb 14, 2017
View reviewed changes
core/src/main/java/org/jboss/modcluster/mcmp/impl/DefaultMCMPRequestFactory.java Outdated
@@ -63,7 +63,7 @@ public MCMPRequest createConfigRequest(Engine engine, NodeConfiguration nodeConf
// If address was specified as a host name, we would prefer it
// toString() will not perform reverse dns lookup
// so send host name portion, if it exists
String address = connector.getAddress().toString();
String address = connector.getAddress().toString().toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

Should we use toLowerCase(Locale.ROOT) here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that in my first version. But couldn't think of an address where it would make a difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevertheless, we need to be deterministic. Fixed.

@Karm
Copy link
Member

Karm commented Feb 14, 2017

FQDN MUST be processed case-insensitively, but the case SHOULD be preserved, IMHO https://tools.ietf.org/html/rfc4343

e.g. NIC.CZ / nic.cz:

karm@local:~$ dig NIC.CZ
;; QUESTION SECTION:
;NIC.CZ.                IN  A
;; ANSWER SECTION:
NIC.CZ.         1800    IN  A   217.31.205.50

karm@local:~$ dig nic.cz
;; QUESTION SECTION:
;nic.cz.                IN  A
;; ANSWER SECTION:
nic.cz.         1796    IN  A   217.31.205.50

Although we might do a conscious decision to disregard it and put everything to lower case...

@jfclere jfclere merged commit adccfa2 into modcluster:master Feb 15, 2017
@rhusar rhusar deleted the MODCLUSTER-572 branch February 15, 2017 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pferraro pferraro pferraro requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rhusar @Karm @pferraro @jfclere