Add a reverse dns processor #7927

andrewkroh · 2018-08-09T04:21:15Z

Package dns implements a processor that can perform DNS lookups by sending
a DNS request over UDP to a recursive nameserver. Each instance of the
processor is independent (no shared cache) so it's best to only define one
instance of the processor.

It caches DNS results in memory and honors the record's TTL. It also caches
failures for the configured failure TTL.

This filter, like all filters, only processes 1 event at a time, so the use
of this plugin can significantly slow down your pipeline's throughput if you
have a high latency network. By way of example, if each DNS lookup takes 2
milliseconds, the maximum throughput you can achieve with a single filter
worker is 500 events per second (1000 milliseconds / 2 milliseconds).

Simple config example:

processors:
- dns:
    type: reverse
    fields:
      source.ip: source.hostname
      destination.ip: destination.hostname

Full config example:

processors:
- dns:
    type: reverse
    action: append
    fields:
      server.ip: server.hostname
      client.ip: client.hostname
    success_cache:
      capacity.initial: 1000
      capacity.max: 10000
    failure_cache:
      capacity.initial: 1000
      capacity.max: 10000
      ttl: 1m
    nameservers: ['192.0.2.1', '203.0.113.1']
    timeout: 500ms
    tag_on_failure: [_dns_reverse_lookup_failed]

Closes #7770

ruflin · 2018-08-09T06:57:36Z

This is great. @andrewvc This could also be interesting for heartbeat.

andrewkroh · 2018-08-09T12:33:17Z

Other than needing documentation added this is ready for review. I wanted to wait on docs in case there are changes to the config.

andrewvc

Started my review, looking very nice, very clean and readable.

Left a partial review of the cache code. Will continue tomorrow with the rest.

andrewvc · 2018-08-12T16:44:01Z

libbeat/processors/dns/cache.go

+
+func (c *ptrCache) evict() {
+	var key string
+	for k := range c.data {


This could use a comment, maybe it's because I'm a go noob, I'd appreciate a comment on this function:

// Evict removes a random key from the cache

Also, not something that belongs in an initial impl, but it might be nice to add a real LRU at some point.

I agree that a better cache implementation would be nice. I tested a few different cache libs and none of them seemed all that great or efficient. So opted to keep is simple (this was also the approach I found in the coredns cache).

In addition to improving on the algorithm, optimizing for the Go garbage collector would be nice too. When you get above 100k objects in a map the GC times start to grow. I looked at the bigcache which addresses this issue, and it worked very well, but it doesn't implement LRU.

But let's see how this gets used before optimizing. I plan to mark this as beta in the docs for the first release.

andrewvc · 2018-08-12T22:16:48Z

libbeat/processors/dns/cache.go

+
+// Logger logs debug messages.
+type Logger interface {
+	Debugw(msg string, keysAndValues ...interface{})


Why do we define a new type vs. using Logger from libbeat/logp?

I wanted use an interface in my method signatures rather than accepting a concrete type to decouple the implementation from the choice of logging libraries (facilitate reuse). But then I later injected a *monitoring.Registry so it's pretty tightly couple to libbeat stuff.

But now I have done some refactoring and it no longer uses a logger (instead it returns detailed errors).

andrewvc · 2018-08-12T22:19:18Z

libbeat/processors/dns/cache.go

+	}
+	c.stats.Miss.Inc()
+
+	ptr, err = c.resolver.LookupPTR(ip)


Might be nice to have a stat for actual DNS queries sent to help users determine if their cache size is appropriate. They could check if increasing the cache size diminishes the number of queries sent.

Wouldn't the number of cache misses be the metric to look when doing this testing? You'd see if the miss number decreased, right?

Additionally there are metrics reported by the resolver that could indicate the total number of lookups if you summed success and failures across each DNS server.

Yes, cache misses is an even better statistic.

I think I missed that we report those stats already, but it does make sense that cache misses would be a good stat.

andrewvc · 2018-08-12T22:22:07Z

libbeat/processors/dns/cache.go

+	c := &PTRLookupCache{
+		success: &ptrCache{
+			data:    make(map[string]ptrRecord, conf.SuccessCache.InitialCapacity),
+			maxSize: max(100, max(conf.SuccessCache.InitialCapacity, conf.SuccessCache.MaxCapacity)),


This behavior is a little weird. It essentially says you can't set a cache size < 100. I'm not so certain we want to get into the business of overriding user preferences silently. Shouldn't we fail to start and log a warning?

I agree. I'll change this to do validation that the maxSize is >0.

andrewvc · 2018-08-12T22:29:38Z

libbeat/processors/dns/cache.go

+	delete(c.data, key)
+}
+
+func (c *failureCache) get(now time.Time, key string) error {


My read here is that the tradeoff is between:

Dual cache impl, one per type

'Generic' cache impl, with entries of (key,success|failure) (probably using two pointers, only one of which is non-null

where the first option is more memory efficient at the cost of duplicated code, while the second is a little less memory efficient but more DRY approach (though there is some extra complexity in methods since they have to determine the type they're working with).

I'm neutral on the choice. It's a tradeoff, and the difference in terms of both isn't very large since 1. the code is simple. 2. finding the hash code of a short string is fast, but I thought I'd just bring this up because I'm curious if there's general guidance on this issue.

In previous PL's I've used professionally the answer is generics. Not having them forces this choice, so I'd like to hear from more experience go programmers how this is handled.

I chose what I thought was a more memory efficient implementation (1) at the cost of some duplication. This was mainly because there could be many of these items in memory, and secondly because the code was simple.

I don't have any general advice 🤷‍♂️ other than to weigh the pros and cons for the choice like you have done here.

Makes sense, thanks for the feedback.

webmat · 2018-08-13T15:26:36Z

Can this new processor be used with when? This will be important, in order to help preserve throughput, if users know they only want to do reverse DNS on a subset of their traffic (only private, or only public).

andrewkroh · 2018-08-13T15:38:01Z

libbeat/processors/dns/resolver.go

+			}
+		}
+
+		return nil, &dnsError{"no PTR record was found the response"}


s/the/in the/

andrewkroh · 2018-08-13T15:39:21Z

libbeat/processors/dns/resolver_test.go

+	s.Handler = h
+	go s.ActivateAndServe()
+	return s.Shutdown, s.PacketConn.LocalAddr().String(), err
+


Remove this newline.

andrewvc

Excellent impl, left some notes for discussion.

This will need more asciidoc in processors_using.asciidoc etc. before it is mergeable of course.

Worked well when tested manually at the CLI:

❰andrewcholakian❙~/golang/src/github.com/elastic/beats/filebeat(git✱feature/libbeat/dns-processor)❱✔≻ ./filebeat
8.8.8.8
{
  "@timestamp": "2018-08-13T16:20:20.782Z",
  "@metadata": {
    "beat": "filebeat",
    "type": "doc",
    "version": "7.0.0-alpha1"
  },
  "prospector": {
    "type": "stdin"
  },
  "input": {
    "type": "stdin"
  },
  "beat": {
    "version": "7.0.0-alpha1",
    "name": "MacBook-Pro.scoutworkshop.com",
    "hostname": "MacBook-Pro.scoutworkshop.com"
  },
  "host": {
    "name": "MacBook-Pro.scoutworkshop.com"
  },
  "resolved": "google-public-dns-a.google.com",
  "message": "8.8.8.8",
  "source": "",
  "offset": 0
}
1.2.3
{
  "@timestamp": "2018-08-13T16:20:27.705Z",
  "@metadata": {
    "beat": "filebeat",
    "type": "doc",
    "version": "7.0.0-alpha1"
  },
  "beat": {
    "name": "MacBook-Pro.scoutworkshop.com",
    "hostname": "MacBook-Pro.scoutworkshop.com",
    "version": "7.0.0-alpha1"
  },
  "host": {
    "name": "MacBook-Pro.scoutworkshop.com"
  },
  "source": "",
  "offset": 0,
  "message": "1.2.3",
  "prospector": {
    "type": "stdin"
  },
  "input": {
    "type": "stdin"
  }
}

andrewvc · 2018-08-13T14:44:08Z

libbeat/monitoring/adapter/go-metrics-wrapper.go

+	vs.OnFloat(w.h.StdDev())
+	vs.OnKey("median")
+	vs.OnFloat(ps[0])
+	vs.OnKey("p75")


Why are we returning these constant values? I assume this was intended to be the derived values at those percentiles from the actual histogram data?

The percentiles are derived from samples stored in the histogram. The p75 is the key name used to hold the 75th percentile which is obtained up on line 106 and stored in ps, the percentiles slice.

Ah, I misread that code. Thanks for the clarification.

andrewvc · 2018-08-13T15:05:55Z

libbeat/processors/dns/cache.go

+	ptr, err = c.resolver.LookupPTR(ip)
+	if err != nil {
+		c.log.Debugw("Reverse DNS lookup failed.", "error", err, "ip", ip)
+		if _, cacheable := err.(*dnsError); cacheable {


Timeouts with DNS enrichment are a tricky problem. I think we want timeouts to be cacheable by default. Even a small number of timeouts can cause a large number of real-world problems. See logstash-plugins/logstash-filter-dns#13 for what we found in the field with Logstash.

Good point. I think the simplest answer is to cache all failures. I will do that here.

I think a future improvement would be to have the resolver keep state information about each nameserver. If a server is non-responsive it could be taken out of the rotation temporarily. And if one server is much faster than another then it could be preferred.

andrewvc · 2018-08-13T15:09:58Z

CHANGELOG.asciidoc

@@ -73,6 +73,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
  internal monitoring data. {issue}7807[7807]
 - Allow for cloud-id to specify a custom port. This makes cloud-id work in ECE contexts. {pull}7887[7887]
 - Add support to grow or shrink an existing spool file between restarts. {pull}7859[7859]
+- Add DNS processor with support for performing reverse lookups on IP addresses. {issue}7770[7770]


Thoughts on forward lookups? Wouldn't have to be in this PR, but those are really useful for log lines where a hostname is specified, but an IP is not.

I can see this being useful for certain use cases where the forward lookup (A or AAAA) has a deterministic response. I could see this being used to resolve hostnames from syslog events against your internal DNS. I don't plan to add this now but the config takes this into account with the lookup type field.

Someone might also want to lookup TXT records, where the query is derived from a field value. For example a lookup like %{[source][ip]}.origin.asn.cymru.com (for ip-to-asn lookup), but this is probably best suited to a centralized enrichment process.

andrewvc · 2018-08-13T15:14:29Z

libbeat/processors/dns/config.go

+	for _, l := range c.Lookup {
+		l.Type = strings.ToLower(l.Type)
+		switch l.Type {
+		case "reverse", "ptr":


Do we really want two names for the same thing? Since there's no backward compatibility requirement here maybe we could just pick one and go with it.

I'll go with only reverse.

andrewvc · 2018-08-13T15:34:29Z

libbeat/processors/dns/dns.go

+		return nil
+	}
+
+	old, err := event.PutValue(target, name.Host)


This is a bit weird to read, where we overwrite the old value, but retain it, then, optionally overwrite that later with an array if an append behavior is specified.

There are lots of ways to implement this (maybe even a func pointer passed in on startup), but I suspect perf-wise they are all a wash.

It feels like a higher level switch statement would be the most clear, we need the conditional branch regardless, and I think it's the most defensive, so something like:

switch action { case ActionReplace: ... case ActionAppend: ... default: panic("Unexpected action type encountered. This should never happen") }

andrewvc · 2018-08-13T15:47:20Z

libbeat/processors/dns/cache_test.go

+	"github.com/elastic/beats/libbeat/monitoring"
+)
+
+const (


These consts are used in dns_test.go as well. Would be nice to extract to constants_test.go or similar IMHO.

andrewvc · 2018-08-13T15:50:20Z

libbeat/processors/dns/doc.go

+//
+// This filter, like all filters, only processes 1 event at a time, so the use
+// of this plugin can significantly slow down your pipeline’s throughput if you
+// have a high latency network. By way of example, if each DNS lookup takes 2


Maybe make a note about caching and perf? The calculation here would be heavily affected by that.

andrewvc · 2018-08-13T15:59:31Z

libbeat/processors/dns/resolver.go

+
+	// Try the nameservers until we get a response.
+	var rtnErr error
+	for _, server := range res.servers {


Might be nice to alternate requests among servers instead of hitting the first one preferentially.

Based on man /etc/resolv.conf I think honoring the order in which the user listed the servers is the way to go.

If there are multiple servers, the resolver library queries them in the order listed.

Oh, well, then nevermind!

andrewvc · 2018-08-13T16:37:46Z

libbeat/processors/dns/config.go

+)
+
+// Config defines the configuration options for the DNS processor.
+type Config struct {


This is a very complete config structure, but its quite clunky to write.

processors: - dns: lookup: - type: reverse fields: message: resolved

This could be improved IMHO. Maybe to:

dns: type: reverse fields: message: resolved

If you need multiple types of lookup you'd use different processors. An added bonus here is that you'd separate caches by type (forward vs. reverse).

SGTM. I'll make the changes.

andrewvc · 2018-08-13T16:41:14Z

libbeat/processors/dns/dns.go

+
+	name, err := p.resolver.LookupPTR(maybeIP)
+	if err != nil {
+		return nil


It might be nice to actually set an error field. I think most users are expecting this to always do something. If there's a bad record that goes in (say 1.shoe.2), the user might want to find those records in ES. They might be able to do it searching for a missing field, but it would be nicer if we could just tag the event with a user-definable tag.

The only alternative would be parsing the log output of the beat to check for errors.

I added a tag_on_failure configuration option.

Package dns implements a processor that can perform DNS lookups by sending a DNS request over UDP to a recursive nameserver. Each instance of the processor is independent (no shared cache) so it's best to only define one instance of the processor. It caches DNS results in memory and honors the record's TTL. It also caches failures for the configured failure TTL. This filter, like all filters, only processes 1 event at a time, so the use of this plugin can significantly slow down your pipeline’s throughput if you have a high latency network. By way of example, if each DNS lookup takes 2 milliseconds, the maximum throughput you can achieve with a single filter worker is 500 events per second (1000 milliseconds / 2 milliseconds). Simple config example: ``` processors: - dns.lookup: - type: reverse action: append fields: ip: server.hostname client_ip: client.hostname ``` Full config example: ``` processors: - dns: lookup: - type: reverse action: append fields: ip: hostname client_ip: client_hostname timeout: 500ms success_cache: capacity.initial: 1000 capacity.max: 10000 failure_cache: capacity.initial: 1000 capacity.max: 10000 ttl: 1m nameservers: ['1.1.1.1', '8.8.8.8'] ``` Closes elastic#7770

houndci-bot · 2018-08-17T01:36:11Z

libbeat/processors/dns/config.go

+	return nil
+}
+
+func (c *CacheConfig) Validate() error {


exported method CacheConfig.Validate should have comment or be unexported

andrewkroh

Thanks for the review @andrewvc . Your feedback was very helpful.

I've made the requested changes and also added documentation. Please have another look.

andrewkroh · 2018-08-15T19:23:19Z

CHANGELOG.asciidoc

@@ -73,6 +73,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
  internal monitoring data. {issue}7807[7807]
 - Allow for cloud-id to specify a custom port. This makes cloud-id work in ECE contexts. {pull}7887[7887]
 - Add support to grow or shrink an existing spool file between restarts. {pull}7859[7859]
+- Add DNS processor with support for performing reverse lookups on IP addresses. {issue}7770[7770]


I can see this being useful for certain use cases where the forward lookup (A or AAAA) has a deterministic response. I could see this being used to resolve hostnames from syslog events against your internal DNS. I don't plan to add this now but the config takes this into account with the lookup type field.

Someone might also want to lookup TXT records, where the query is derived from a field value. For example a lookup like %{[source][ip]}.origin.asn.cymru.com (for ip-to-asn lookup), but this is probably best suited to a centralized enrichment process.

andrewkroh · 2018-08-15T19:26:04Z

libbeat/monitoring/adapter/go-metrics-wrapper.go

+	vs.OnFloat(w.h.StdDev())
+	vs.OnKey("median")
+	vs.OnFloat(ps[0])
+	vs.OnKey("p75")


The percentiles are derived from samples stored in the histogram. The p75 is the key name used to hold the 75th percentile which is obtained up on line 106 and stored in ps, the percentiles slice.

andrewkroh · 2018-08-15T19:38:14Z

libbeat/processors/dns/cache.go

+
+func (c *ptrCache) evict() {
+	var key string
+	for k := range c.data {


I agree that a better cache implementation would be nice. I tested a few different cache libs and none of them seemed all that great or efficient. So opted to keep is simple (this was also the approach I found in the coredns cache).

In addition to improving on the algorithm, optimizing for the Go garbage collector would be nice too. When you get above 100k objects in a map the GC times start to grow. I looked at the bigcache which addresses this issue, and it worked very well, but it doesn't implement LRU.

But let's see how this gets used before optimizing. I plan to mark this as beta in the docs for the first release.

andrewkroh · 2018-08-15T19:48:47Z

libbeat/processors/dns/cache.go

+	delete(c.data, key)
+}
+
+func (c *failureCache) get(now time.Time, key string) error {


I chose what I thought was a more memory efficient implementation (1) at the cost of some duplication. This was mainly because there could be many of these items in memory, and secondly because the code was simple.

I don't have any general advice 🤷‍♂️ other than to weigh the pros and cons for the choice like you have done here.

andrewkroh · 2018-08-15T19:58:56Z

libbeat/processors/dns/cache.go

+
+// Logger logs debug messages.
+type Logger interface {
+	Debugw(msg string, keysAndValues ...interface{})


I wanted use an interface in my method signatures rather than accepting a concrete type to decouple the implementation from the choice of logging libraries (facilitate reuse). But then I later injected a *monitoring.Registry so it's pretty tightly couple to libbeat stuff.

But now I have done some refactoring and it no longer uses a logger (instead it returns detailed errors).

andrewkroh · 2018-08-15T20:18:06Z

libbeat/processors/dns/cache.go

+	ptr, err = c.resolver.LookupPTR(ip)
+	if err != nil {
+		c.log.Debugw("Reverse DNS lookup failed.", "error", err, "ip", ip)
+		if _, cacheable := err.(*dnsError); cacheable {


Good point. I think the simplest answer is to cache all failures. I will do that here.

I think a future improvement would be to have the resolver keep state information about each nameserver. If a server is non-responsive it could be taken out of the rotation temporarily. And if one server is much faster than another then it could be preferred.

andrewkroh · 2018-08-15T20:19:27Z

libbeat/processors/dns/config.go

+)
+
+// Config defines the configuration options for the DNS processor.
+type Config struct {


SGTM. I'll make the changes.

andrewkroh · 2018-08-15T20:22:29Z

libbeat/processors/dns/config.go

+	for _, l := range c.Lookup {
+		l.Type = strings.ToLower(l.Type)
+		switch l.Type {
+		case "reverse", "ptr":


I'll go with only reverse.

andrewkroh · 2018-08-15T20:34:18Z

libbeat/processors/dns/dns.go

+
+	name, err := p.resolver.LookupPTR(maybeIP)
+	if err != nil {
+		return nil


I added a tag_on_failure configuration option.

andrewkroh · 2018-08-15T21:43:48Z

libbeat/processors/dns/resolver.go

+
+	// Try the nameservers until we get a response.
+	var rtnErr error
+	for _, server := range res.servers {


Based on man /etc/resolv.conf I think honoring the order in which the user listed the servers is the way to go.

If there are multiple servers, the resolver library queries them in the order listed.

andrewvc

Changes look great. LGTM!

andrewvc · 2018-08-20T20:59:04Z

Waiting on a likely unrelated travis ci failure, respinning

Package dns implements a processor that can perform DNS lookups by sending a DNS request over UDP to a recursive nameserver. Each instance of the processor is independent (no shared cache) so it's best to only define one instance of the processor. It caches DNS results in memory and honors the record's TTL. It also caches failures for the configured failure TTL. This filter, like all filters, only processes 1 event at a time, so the use of this plugin can significantly slow down your pipeline’s throughput if you have a high latency network. By way of example, if each DNS lookup takes 2 milliseconds, the maximum throughput you can achieve with a single filter worker is 500 events per second (1000 milliseconds / 2 milliseconds). Simple config example: ``` processors: - dns: - type: reverse action: append fields: ip: server.hostname client_ip: client.hostname ``` Full config example: ``` processors: - dns: - type: reverse action: append fields: ip: hostname client_ip: client_hostname timeout: 500ms success_cache: capacity.initial: 1000 capacity.max: 10000 failure_cache: capacity.initial: 1000 capacity.max: 10000 ttl: 1m nameservers: ['1.1.1.1', '8.8.8.8'] ``` Closes elastic#7770 This also updates golang/x/net to let us build correctly on netbsd/arm, which was failing with some of the new includes this requires. (cherry picked from commit 3b89051)

Package dns implements a processor that can perform DNS lookups by sending a DNS request over UDP to a recursive nameserver. Each instance of the processor is independent (no shared cache) so it's best to only define one instance of the processor. It caches DNS results in memory and honors the record's TTL. It also caches failures for the configured failure TTL. This filter, like all filters, only processes 1 event at a time, so the use of this plugin can significantly slow down your pipeline’s throughput if you have a high latency network. By way of example, if each DNS lookup takes 2 milliseconds, the maximum throughput you can achieve with a single filter worker is 500 events per second (1000 milliseconds / 2 milliseconds). Simple config example: ``` processors: - dns: - type: reverse action: append fields: ip: server.hostname client_ip: client.hostname ``` Full config example: ``` processors: - dns: - type: reverse action: append fields: ip: hostname client_ip: client_hostname timeout: 500ms success_cache: capacity.initial: 1000 capacity.max: 10000 failure_cache: capacity.initial: 1000 capacity.max: 10000 ttl: 1m nameservers: ['1.1.1.1', '8.8.8.8'] ``` Closes #7770 This also updates golang/x/net to let us build correctly on netbsd/arm, which was failing with some of the new includes this requires. (cherry picked from commit 3b89051)

andrewkroh added in progress Pull request is currently in progress. review libbeat :Processors labels Aug 9, 2018

andrewkroh removed the in progress Pull request is currently in progress. label Aug 9, 2018

andrewvc self-requested a review August 10, 2018 19:31

andrewvc reviewed Aug 12, 2018

View reviewed changes

andrewkroh commented Aug 13, 2018

View reviewed changes

andrewvc requested changes Aug 13, 2018

View reviewed changes

andrewkroh added 3 commits August 16, 2018 21:35

Address review items

a80bcc8

Add dns processor documentation

2e56736

andrewkroh force-pushed the feature/libbeat/dns-processor branch from 9107649 to 2e56736 Compare August 17, 2018 01:36

houndci-bot reviewed Aug 17, 2018

View reviewed changes

andrewkroh commented Aug 17, 2018

View reviewed changes

Add godoc to exported Validate method

8ad2e5a

andrewvc approved these changes Aug 20, 2018

View reviewed changes

Update golang/x/net

74b6c86

andrewvc merged commit 3b89051 into elastic:master Aug 21, 2018

andrewkroh added the needs_backport PR is waiting to be backported to other branches. label Aug 29, 2018

andrewkroh mentioned this pull request Aug 29, 2018

Cherry-pick #7865 to 6.x: Update to github.com/miekg/dns v1.0.8 #8148

Merged

andrewkroh removed the needs_backport PR is waiting to be backported to other branches. label Aug 29, 2018

andrewkroh mentioned this pull request Aug 29, 2018

Cherry-pick #7927 to 6.x: Add a reverse dns processor #8150

Merged

andrewkroh added the v6.5.0 label Aug 29, 2018

andrewkroh deleted the feature/libbeat/dns-processor branch November 8, 2018 20:23

Add a reverse dns processor #7927

Add a reverse dns processor #7927

Conversation

andrewkroh commented Aug 9, 2018 • edited Loading

ruflin commented Aug 9, 2018

andrewkroh commented Aug 9, 2018

andrewvc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmat commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc left a comment

Choose a reason for hiding this comment

andrewvc commented Aug 20, 2018

andrewkroh commented Aug 9, 2018 •

edited

Loading