Allow the resolver to override the port setting #802
Conversation
The custom resolver couldn't override the default port even if id had been explicitly provided. The reason for that is unclear as it contradicts the open closed principle. Why: Common address resolvers should alway only return the host. If a port is being provided this has been intended. Purpose of this Patch: Access a kafka cluster behind a firewall via portforwarding on a jump host. jumphost: 9095 -> kafkahost1:9092, jumphost: 9096 -> kafkahost2:9092, ...
Bugfix for localhost-lookups which result in the setting of a default port in splitHostPortNumber
Set the default port in the resolver unless any port is set. This is done to pass the unit tests but should in my opinion not be the responsibility of this function.
Refactoring the PR for better reflect the changes Altered the Ports in one test to make it pass but kept the logical behaviour
|
Port overrides seem to have been prevented due to the default ports being provided. |
|
Hello @themulle, thanks for submitting this pull request. I left a question about the implementation, let me know what you think! |
dialer.go
Outdated
| } else if resolvedPort != port { | ||
| _, explicitSetPort, _ := net.SplitHostPort(resolved[0]) | ||
| if explicitSetPort != "" { | ||
| port = explicitSetPort | ||
| } |
There was a problem hiding this comment.
Isn't explicitSetPort the same as resolvedPort in this case? It seems we are calling net.SplitHostPort(resolved[0]) twice, so we should get the same results?
There was a problem hiding this comment.
Hi,
that's also been the confusing part for me.
In this case "kafka.splitHostPort" is being called which returns default port 9092.
net.SplitHostPort under some circumstances (s = own address) returns an empty host string, even if "s" is set.
func splitHostPort(s string) (host string, port string) {
host, port, _ = net.SplitHostPort(s)
if len(host) == 0 && len(port) == 0 {
host = s
port = "9092"
}
return
}
Example:
467 host, port := splitHostPort("mykafkahost:9095") //return mykafkahost 9095
478 resolvedHost, resolvedPort := splitHostPort("myjumphost") //return myjumphost 9092 (on "myjumphost" because net.SplitHostPort in this case returns an empty host)
-> resolvedPort!=port -> would override port setting but isn't explicitly set
because of this the Port is checked a second time
There was a problem hiding this comment.
Ok, i think this can be simplified:
`
if len(resolved) > 0 {
resolvedHost, resolvedPort, _ := net.SplitHostPort(resolved[0])
// we'll always prefer the resolved host
if len(resolvedHost) == 0 && len(resolvedPort) == 0 {
host = resolved[0]
} else {
host = resolvedHost
}
// only override the port if it's not set or if the resolver explicitly provides a port
if resolvedPort != "" {
port = resolvedPort
}
}
`
Simplified the code by directly using net.SpligHostPort
added testcase "host without port" (127.0.0.1:) restructure test to be able to run it without local kafka broker
net.SplitHostPort returns an empty host and port (plus an error) if no port is being provided now the error is properly handled and the port is explicitly checked
|
finally a good solution after digging into net/ip.go to understand the behaviour of net.SplitHostPort |
| if !runCompleteTest { | ||
| return | ||
| } |
There was a problem hiding this comment.
Should we remove this before merging?
There was a problem hiding this comment.
In my opinion the test should be split into a "real" unit test, which only checks the resolver behaviour and and a integration test which connects to kafka.
I don't know if this makes sense because allof the tests are "integration tests" and another behaviour would break the coding style.
Your decision:
- shall i remove my "uint-test-code"
- shall i completley split up the test in integration/unit-test
There was a problem hiding this comment.
Adding a separate test to validate the resolver behavior sounds fine to me, are there any downsides or blockers?
There was a problem hiding this comment.
Hello @themulle, did you get a chance to look at addressing this last suggestion?
There was a problem hiding this comment.
@themulle up? Let me know if you want us to take this over as well, no rush :)
|
hi, this commit hasn't been merged into the main branch yet(I have the same problem) |
|
Hello @chris-wangkk The original author appears to have abandoned the pull request. There were a few comments left that haven't been addressed. Would you be available to take over and complete this work? |
The custom resolver couldn't override the default port even if id had been explicitly provided.
The reason for that is unclear as it contradicts the open closed principle.
Why: Common address resolvers should alway only return the host. If a port is being provided this has been intended.
Purpose of this Patch:
Access a kafka cluster behind a firewall via portforwarding on a jump host.
jumphost: 9095 -> kafkahost1:9092, jumphost: 9096 -> kafkahost2:9092, ...