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

reverse resolutions for remote side of connections. #404

Merged
merged 3 commits into from
Sep 7, 2015
Merged

Conversation

inercia
Copy link
Contributor

@inercia inercia commented Aug 27, 2015

Async reverse dns lookups in probe that creates nodes for the remote side of connections.

It's part of the solution for #364

TODO:

  • unit test
  • update detailed_node.go to display the remote name

if err == gcache.NotFoundKeyError {
// we trigger a asynchronous reverse resolution when not cached, but we return
// immediately so the client does not block...
r.addresses <- address

This comment was marked as abuse.

This comment was marked as abuse.

@inercia inercia force-pushed the scope-364 branch 4 times, most recently from b1852fc to 917a48a Compare August 28, 2015 10:17
@inercia inercia self-assigned this Aug 28, 2015
@tomwilkie
Copy link
Contributor

#411 is merged, so you should now be able to put the reverse dns lookups on pseudo nodes and plumb this all the way through to the UI.

Addr: c.RemoteAddress.String(),
})
}
}

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie
Copy link
Contributor

We'll also need unit tests for the resolver.

@@ -104,7 +104,8 @@ func main() {
}

var (
endpointReporter = endpoint.NewReporter(hostID, hostName, *spyProcs, *useConntrack)
revResolver = endpoint.NewReverseResolver(endpoint.RAddrCacheLen)
endpointReporter = endpoint.NewReporter(hostID, hostName, *spyProcs, *useConntrack, revResolver)

This comment was marked as abuse.

This comment was marked as abuse.

@peterbourgon
Copy link
Contributor

Just waiting for the WIP prefix to go away, and those two checkboxes to be checked :)

throttle := time.Tick(time.Second / 10)
for address := range r.addresses {
<-throttle // rate limit our DNS resolutions
names, err := net.LookupAddr(address)

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@inercia
Copy link
Contributor Author

inercia commented Sep 3, 2015

@tomwilkie what should I do with the reverse address in detailed_node.go? should I replace IPs by names when available? or maybe should I show those names in some other way?

@tomwilkie
Copy link
Contributor

@inercia yes I'd try and replace IP addresses in the connections table, in connectionDetailsRows()

https://github.com/weaveworks/scope/blob/master/render/detailed_node.go#L206

@inercia
Copy link
Contributor Author

inercia commented Sep 3, 2015

@tomwilkie the problem with doing a s/IP/name/ is that some names are rather long (eg, db3msgr5011705.gateway.messenger.live.com., c2-54-163-163-128.compute-1.amazonaws.com.) so I'm concerned about the result in the UI...

@tomwilkie
Copy link
Contributor

The UI will truncate them and offer tooltips (recent change just went in, you'll need to rebase).

I wouldn't be too worries about it.

for address := range r.addresses {
<-throttle // rate limit our DNS resolutions
_, err := r.cache.Get(address) // and check if the answer is already in the cache
if err == nil {

This comment was marked as abuse.

This comment was marked as abuse.

@peterbourgon peterbourgon assigned inercia and unassigned peterbourgon Sep 3, 2015
@inercia inercia force-pushed the scope-364 branch 3 times, most recently from 5509503 to 8c817d1 Compare September 4, 2015 15:00
Add nodes for the remote side of connections iff we have a DNS reverse resolution for the IP.
Unit test for the resolver
// we trigger a asynchronous reverse resolution when not cached
select {
case r.addresses <- request:
if wait {

This comment was marked as abuse.

@tomwilkie
Copy link
Contributor

I just put in a few small changes to address some of my own review feedback...

func TestReverseResolver(t *testing.T) {
tests := map[string]string{
"1.2.3.4": "test.domain.name",
"4.3.2.1": "im.a.little.tea.pot",

This comment was marked as abuse.

@peterbourgon
Copy link
Contributor

LGTM!

@peterbourgon peterbourgon assigned inercia and unassigned peterbourgon Sep 7, 2015
inercia added a commit that referenced this pull request Sep 7, 2015
reverse resolutions for remote side of connections.
@inercia inercia merged commit d3c7138 into master Sep 7, 2015
@peterbourgon peterbourgon deleted the scope-364 branch September 7, 2015 08:58
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.

4 participants