Skip to content

Allow the resolver to override the port setting #802

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Allow the resolver to override the port setting #802

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1fa05a0
Allow the resolver to override the port setting
manuelmueller2 Nov 30, 2021
b849f39
Prevent usage of the fallback port in custom resolver
manuelmueller2 Nov 30, 2021
ddc2900
Set default port in resolver unless set
manuelmueller2 Nov 30, 2021
8991aea
Refactoring PR
manuelmueller2 Nov 30, 2021
4657fd1
Merge branch 'segmentio:main' into allowportoverride
themulle Feb 11, 2022
d138e8b
Simplified code
manuelmueller2 Feb 11, 2022
31372b0
Merge branch 'allowportoverride' of https://github.com/themulle/kafka…
manuelmueller2 Feb 11, 2022
6bfe0cf
Update dialer.go
achille-roussel Feb 11, 2022
e319bdc
Update dialer.go
achille-roussel Feb 11, 2022
1a01d3a
Merge branch 'segmentio:main' into allowportoverride
themulle Feb 18, 2022
d85a9f2
Merge branch 'segmentio:main' into allowportoverride
themulle Feb 21, 2022
ea3da9d
optionally run TestDialerResolver without kafka
manuelmueller2 Feb 21, 2022
17090b5
fix lookup host
manuelmueller2 Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions dialer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,10 +480,14 @@ func lookupHost(ctx context.Context, address string, resolver Resolver) (string,
// we'll always prefer the resolved host
host = resolvedHost

// in the case of port though, the provided address takes priority, and we
// only use the resolved address to set the port when not specified
// only override the port if it's not set or if the resolver explicitly provides a port
if port == "" {
port = resolvedPort
} else if resolvedPort != port {
_, explicitSetPort, _ := net.SplitHostPort(resolved[0])
if explicitSetPort != "" {
port = explicitSetPort
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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
		}
	}

`

}
}
}
Expand Down
4 changes: 2 additions & 2 deletions dialer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,9 +331,9 @@ func TestDialerResolver(t *testing.T) {
},
{
scenario: "resolve domain with port to ip with different port",
address: "example.com:9092",
address: "example.com:9093",
resolver: map[string][]string{
"example.com": {"127.0.0.1:80"},
"example.com": {"127.0.0.1:9092"},
},
},
{
Expand Down