DNS doesn't allow individual TXT records larger than 255 characters, but it does allow you to split up a TXT record into multiple strings. For example:

foo.com. IN TXT "grpc_config=[{\"serviceConfig\":{" "\"loadBalancingPolicy\":\"round_robin\"}}]"

I don't know the protocol details, but I do know that DNS can somehow represent the difference between multiple independent TXT records and a single split-up TXT record. For example, if I add a second TXT record to the zone file in addition to the above, dig shows the following:

roth@segfault:~/grpc> dig @127.0.0.1 -p 1234 foo.com. txt
; <<>> DiG 9.9.5-3ubuntu0.15-Ubuntu <<>> @127.0.0.1 -p 1234 foo.com. txt
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 60627
;; flags: qr aa rd ra; QUERY: 1, ANSWER: 2, AUTHORITY: 1, ADDITIONAL: 2
;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;foo.com.                       IN      TXT
;; ANSWER SECTION:
foo.com.                3600    IN      TXT     "a=12345678901234567890123456789
01234567890123456789012345678901234567890123456789012345678901234567890123456789
01234567890123456789012345678901234567890123456789012345678901234567890123456789
0123456789012345678901234567890123456789012345678901234567890"
foo.com.                3600    IN      TXT     "grpc_config=[{\"serviceConfig\"
:{" "\"loadBalancingPolicy\":\"round_robin\"}}]"
;; AUTHORITY SECTION:
foo.com.                3600    IN      NS      ns.foo.com.
;; ADDITIONAL SECTION:
ns.foo.com.             3600    IN      A       127.0.0.1
;; Query time: 0 msec
;; SERVER: 127.0.0.1#1234(127.0.0.1)
;; WHEN: Thu Aug 17 08:09:58 PDT 2017
;; MSG SIZE  rcvd: 417

Note that the two parts of the service config string are shown as a single split-up entry, but the other TXT record is shown as an independent record.

Note that because the split-up record appears differently at the protocol level, the c-ares code has special handling for this case. So we do need to find a way to test this.

From a quick glance at the dnslib code, I'm not sure it can handle this right now. If not, perhaps we can fix it to do that?

We should also make sure to test the case where the sum total of the TXT records is larger than 512 bytes, because that will cause DNS to fall back from UDP to TCP. (I was having some problems testing this with the service config code, so this may not work right yet.)

marked the UDP -> TCP test case as a TODO.

from offline chat, this needs some change to the ares wrapper code to work.

The problem that's currently happening is:

1. c-ares receives a truncated DNS response and re-sends the query over TCP
2. c-ares now signals interest in the TCP socket and the previous UDP socket (sometimes needed), and we still register notify_on_read on the UDP socket
3. fd_orphan fails because the callback is still pending

I think we need to "maybe cancel" the callback on all sockets to cleanup - cc @zyc

I think you meant to tag @y-zeng here.

Yuchen, do you have time to look at this? We don't need to block this PR on it, but I think we will need to fix this before we can make c-ares the default DNS resolver.

In C++, instead of using static, it's cleaner to put things in an anonymous namespace.

done, moved everything into an anonymous namespace

This is closing the testing namespace, not the anonymous namespace.

s/e a/expected addresses/

This can return bool.

got rid of the whole matches_any function

Checking that the value is present in expected_addrs isn't quite precise enough. If the expected addresses are (A, B) and the actual addresses are (A, A), the test will not fail as it should.

Since we're using C++ here, can we use some of the GMock constructs to make this more readable? For example, if we can get both the expected and actual addresses into vectors of grpc_lb_address objects, perhaps we can use something like UnorderedElementsAreArray to concisely check the result?

Had some trouble getting the gmock construct to work - I'm not certain why but I think think that it's because this test isn't following a needed framework (running as an ordinary binary, and not with RUN_ALL_TESTS and TEST_ test cases).

Can look into this further if we want to use that though

I don't see any reason why we can't structure this like any other C++ test, where the individual tests are defined using the TEST() macros and invoked via RUN_ALL_TESTS().

If the service config specifies the LB policy, then we should also check that the resolver has set GRPC_ARG_LB_POLICY_NAME.

done. I'd like to check offline though that the expected lb policy names that I have set (which are the ones appearing), are correct. E.g., my understanding is that if an SRV address is resolved, then grpclb should be used - but it appears that the policy name channel arg is actually null in that case

That's correct. The resolver's behavior is that it will only set GRPC_ARG_LB_POLICY_NAME if the service config explicitly specifies an LB policy, otherwise it is left unset. When the resolver returns its results to the client_channel code, that's where we check whether any of the addresses are balancer addresses and if so, force the use of grpclb regardless of what policy (if any) was present in the service config.

https://github.com/grpc/grpc/blob/master/src/core/ext/filters/client_channel/client_channel.c#L389

I don't think we want case-insensitive comparison here -- the JSON data is compared in a case-sensitive way.

done

This is not actually an end-to-end test. Can we rename it to something like resolver_test? (Please update function names, file names, etc).

Renamed this to resolver_component_test...

I wanted make it more unique than resolver_test, for namespacing/regex matching purposes, but can change if we want to

Could just say return test_case['expected_lb_policy'] or ''.

done

No need for the ? true : false part.

whoops, done

We should probably use C++ style names in this test -- e.g., ConvertStringToBool() instead of convert_string_to_bool().

Converted some but not all, I was a little unsure here. Kept types used by C funcs (e.g., args_struct), and functions called back from C, as C-style, and converted the top-level to C++

This test is written in C++, so I think it's fine to use C++ style even for things that are used as C callbacks.

Please declare variables right before they are used, rather than declaring them all at once at the top of a function.

done

Suggest calling this is_balancer.

done

That's correct. The resolver's behavior is that it will only set GRPC_ARG_LB_POLICY_NAME if the service config explicitly specifies an LB policy, otherwise it is left unset. When the resolver returns its results to the client_channel code, that's where we check whether any of the addresses are balancer addresses and if so, force the use of grpclb regardless of what policy (if any) was present in the service config.

https://github.com/grpc/grpc/blob/master/src/core/ext/filters/client_channel/client_channel.c#L389

Instead of having an expected_config_index attribute, I suggest having a expected_service_config attribute that contains the expected string. This is because when the TXT record contains multiple choices, the resolver is responsible for picking one of those choices. The service config it returns should include only the selected choice -- i.e., it's a subset of the TXT record.

For example, one of the tests you've defined has the following TXT record:

grpc_config=[
  {"percentage":0,
   "serviceConfig":{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"NeverPickedService","waitForReady":true}]}]}},
  {"percentage":100,
   "serviceConfig":{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"AlwaysPickedService","waitForReady":true}]}]}}]

In this case, the resolver should return only the second service config:

{"loadBalancingPolicy":"round_robin","methodConfig":[{"name":[{"method":"Foo","service":"AlwaysPickedService","waitForReady":true}]}]}}

done - previously string found in python, that change simplified the code

I don't see any reason why we can't structure this like any other C++ test, where the individual tests are defined using the TEST() macros and invoked via RUN_ALL_TESTS().

Looks good. The format might be a bit less ambiguous if we used a different delimiter character for the (address,is_balancer) pairs than between the pairs. But this is fine as-is too.

I think you meant to tag @y-zeng here.

Yuchen, do you have time to look at this? We don't need to block this PR on it, but I think we will need to fix this before we can make c-ares the default DNS resolver.

I think port_server.py also starting a DNS server as a side effect is a pretty dirty solution (no one would expect such behavior from something called port_server). Also, port server is started in other circumstances - like in performance benchmarks, where you don't need the DNS server at all. I think overall this solution will lead to problems in the future.

@nicolasnoble recommended this solution, since we need a hermetic DNS server in the tests. I suppose we could have the tests run yet another binary, but that seems unnecessarily complicated.

References should only be used for const parameters.

https://google.github.io/styleguide/cppguide.html#Reference_Arguments

This doesn't say how the is_balancer bit is encoded.

Also, the parens seem a little cumbersome. I was thinking of something more like "address0,is_balancer0;address1,is_balancer1;..." -- i.e., use comma between the two elements of each pair and semicolon between the pairs. Or perhaps "address0=is_balancer0,address1=is_balancer1,...".

Please eliminate unnecessary blank lines within functions.

https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

In general, I find that if you really need to separate code into logical "chunks", it's better to use a comment line documenting the purpose of the next section than a blank line.

Still need to address this.

If you save this as a C++ string instead of a C string, then you can change check_service_config_result_locked() to just use EXPECT_EQ().

(Actually, even if you keep it as a C string, you can use EXPECT_STREQ().)

Same here.

@nicolasnoble recommended this solution, since we need a hermetic DNS server in the tests. I suppose we could have the tests run yet another binary, but that seems unnecessarily complicated.

s/of/or/

Still need to address this.

s/:/,/

s/Cant/Can't/

Suggest moving the flag checking down into main().

Maybe also log the pid, if there's an easy way to get at it?

I couldn't see an easy way to do this. Even with gpr_subprocess directly, the pid info appears to be platform-specific and not exposed. Using fork/exec directly I could, but I'd lean towards gpr_subprocess and its SubProcess (I think this wrapper is more portable too)

No need for == true.

No need for == true.

Still need to remove the == true here.

Suggest moving this down into the main logic below.

Let's put the rest of this in a main() function, and invoke it only if __name__ == '__main__'.