Implement a fake resolver, for compatibility with Twisted 17.1.0 and higher #150

rodrigc · 2017-09-05T01:03:28Z

This is a forward port of part of the patch in #135 .

I couldn't find any existing test classes in Twisted which worked to get this test working.

rodrigc · 2017-09-21T13:49:44Z

See: twisted/twisted@cd75dd8 , which is part of this pull request to Twisted: twisted/twisted#624

rodrigc · 2017-09-21T13:50:25Z

exarkun

Thanks. This basically looks good to me. A few small changes requested inline, along with one slightly more significant request to ensure we retain good test coverage. Sorry about the review delay here, I've been entirely occupied offline recently. Thanks again.

exarkun · 2017-10-11T17:44:49Z

src/txkube/test/test_authentication.py

+        def resolveHostName(self, resolutionReceiver, hostName, *args,  **kwargs):
+            portNumber = kwargs.pop('portNumber')
+            r = Resolution()
+            r.name = hostName


How about Resolution(name=hostName)? Your choice of pyrsistent or attrs for defining Resolution to make this possible. :)

exarkun · 2017-10-11T17:45:27Z

src/txkube/test/test_authentication.py

 from twisted.internet.protocol import Factory
 from twisted.web.http_headers import Headers
 from twisted.test.iosim import ConnectionCompleter
 from twisted.test.proto_helpers import AccumulatingProtocol, MemoryReactorClock

+from zope.interface import implementer


Can you move this up above the Twisted imports? (because Twisted depends on zope.interface)

exarkun · 2017-10-11T17:48:11Z

src/txkube/test/test_authentication.py

+            r.name = hostName
+
+            resolutionReceiver.resolutionBegan(r)
+            resolutionReceiver.addressResolved(IPv4Address('TCP', 'example.invalid.', portNumber))


I suppose that since there's now an explicit resolution step going on (whereas previously the hostname would be passed to connectTCP), the result here should be an IP address instead of a hostname? The hostname and the IP address are both sloppily represented as native strings but they're pretty distinctly different things.

Pick an address from one of the TEST-NET ranges, probably: https://en.wikipedia.org/wiki/IPv4#Special-use_addresses.

Also, there should be some kind of assertion made about the value of hostName somewhere. It is currently essentially ignored, as far as I can tell. Previously, each test asserted that "example.invalid." was being connected to (a value taken from an environment variable). Now no matter what hostname is passed in, it will be resolved to the hard-coded value passed to this addressResolved method. So the coverage of the hostname being selected properly is being removed.

A well-behaved fake resolver, I suppose, would know about just the relevant fake hostnames and return a resolution error for any others (in whatever way is correct for the resolver interface now, I haven't checked, but I guess there's some other method).

So resolveHostname https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces.IHostnameResolver.html#resolveHostName returns an IResolutionReceiver:
https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces.IResolutionReceiver.html
not an IP address.

I'm new to all these interfaces, but let me see what I can do to clean this up.

exarkun · 2017-10-11T17:51:17Z

setup.py

@@ -32,7 +32,7 @@
        "incremental",
        # See https://github.com/twisted/treq/issues/167
        # And https://twistedmatrix.com/trac/ticket/9032
-        "twisted[tls]!=17.1.0",
+        "twisted[tls]>=17.9.0",


Do you want this change here or in #151? Also, I'm not sure the comments above are now relevant. The constraint is now primarily due to the name resolver behavior.

I want the change in #151 . I just pulled it into this branch because it was interrelated. I wanted to get your feedback separately on bumping the Twisted version in #151 because at the time I wasn't sure how many versions of Twisted you wanted to maintain compatibility with. We had a short conversion on IRC later where you mentioned you were OK with bumping the Twisted version.

If you can, please review #151 separately,and once we beat that into shape, I will pull that into this branch.

rodrigc · 2017-10-12T09:17:32Z

I've improved the _FakeResolver, please review.

exarkun

Thanks! This looks really good now. Just a couple very minor additional changes and then I'm happy to see it merged.

exarkun · 2017-10-12T10:53:32Z

src/txkube/test/test_authentication.py

+                str(result.value),
+                Equals('DNS lookup failed: no results for '
+                       'hostname lookup: doesnotresolve.')
+            )


This doesn't necessarily assert that the Deferred fails. It might just never fire. It's a good idea to use failureResultOf for this reason.

exarkun · 2017-10-12T12:10:49Z

src/txkube/test/test_authentication.py

@@ -68,11 +76,48 @@
 -----END CERTIFICATE-----
 """

+# Let hostname u"example.invalid" map to an
+# IPv4 address in the TEST-NET range.
+_hostMap = { u"example.invalid." : "192.0.2.2" }


There's some extra whitespace inside this dict. Also, as a global, it should probably be something more like HOST_MAP. It probably doesn't need a leading underscore because test modules are automatically not public APIs.

exarkun · 2017-10-12T13:43:02Z

src/txkube/test/test_authentication.py

+                resolutionReceiver.addressResolved(
+                    IPv4Address('TCP', _hostMap[hostName], portNumber))
+            resolutionReceiver.resolutionComplete()
+            return r


Cool. This seems to be the right way to get the desired behavior from the new resolver interface. I'm not quite sure I entirely understand how the interface means to deal with errors - but not calling addressResolved seems to be how _resolver.py in Twisted deals with errors, so it makes sense to do the same here.

rodrigc · 2017-10-13T20:51:37Z

@exarkun I've updated the patch, please look.

exarkun

Thanks! A few more minor comments, sorry I didn't notice them earlier. But more importantly, I misled you with the failureResultOf suggestion. Correct idea in a comment inline. Sorry about that. Thanks again for your efforts here.

exarkun · 2017-10-17T14:55:33Z

src/txkube/test/test_authentication.py

+        """
+        d = self._authorized_request(token="test", headers=Headers({}),
+                                     kubernetesHost=b"doesnotresolve")
+        result = SynchronousTestCase().failureResultOf(d, DNSLookupError)


Oops. :( I forgot that txkube tests don't use trial's TestCase. I don't think instantiating SynchronousTestCase to call methods on it is a great idea. It probably won't break but normal usage of is not for application code to instantiate this kind of thing.

So, instead, what I should have asked for in my previous review was use of testtools.twistedsupport.failed. I think it'll look something like this:

has_lookup_failure = failed( AfterPreprocessing(lambda f: str(f.value), Equals("DNS lookup failed: ..."), ) self.assertThat(some_deferred, has_lookup_failure)

Sorry about steering you wrong with failureResultOf.

exarkun · 2017-10-17T14:57:55Z

src/txkube/test/test_authentication.py

-
+        d = agent.request(b"GET", b"http://" + kubernetesHost, headers)
+        if isinstance(d.result, Failure):
+            return d


I think this should use the testtools.twistedsupport.succeeded (and testtools.matchers.Always) matcher:

self.assertThat(d, succeeded(Always())

The Always() just matches any result after succeeded matches on a successful deferred. This has the benefit of the nice reporting that you get from testtools matches and avoiding using Deferred.result directly (which looks public but it's really a bad idea to ever use it directly).

I tried self.assertThat(d, succeeded(Always())) and got:

=============================================================================== [FAIL] Traceback (most recent call last): Failure: testtools.testresult.real._StringException: Traceback (most recent call last): File "/Users/crodrigues/txkube2/src/txkube/test/test_authentication.py", line 262, in test_other_headers_preserved request_bytes = self._authorized_request(token=token, headers=headers) File "/Users/crodrigues/txkube2/src/txkube/test/test_authentication.py", line 159, in _authorized_request self.assertThat(d, succeeded(Always())) File "/Users/crodrigues/venv-2.7/lib/python2.7/site-packages/testtools/testcase.py", line 498, in assertThat raise mismatch_error testtools.matchers._impl.MismatchError: Success result expected on <Deferred at 0x1120f98c0 waiting on Deferred at 0x1120f9b00>, found no result instead

Oh, huh. I guess the point of the code is that if the Deferred has failed, fail quickly. But expected behavior is for it to have no result at all yet.

I wonder if assertThat(d, Not(failed(Always()))) would work... I expect probably not. Peeking at an in-progress Deferred is kind of a sneaky thing. I'm going to sleep on this, maybe I'll remember some better trick.

exarkun · 2017-10-17T14:58:29Z

src/txkube/test/test_authentication.py

 class AuthenticateWithServiceAccountTests(TestCase):
    """
    Tests for ``authenticate_with_serviceaccount``.
    """
-    def _authorized_request(self, token, headers):
+    def _authorized_request(self, token, headers,
+                            kubernetesHost=b"example.invalid."):


I guess this should be kubernetes_host.

exarkun · 2017-10-17T14:59:31Z

src/txkube/test/test_authentication.py

+    u"example.invalid.": "192.0.2.2"
+}
+
+def createReactor():


I guess this should be create_reactor.

…higher

rodrigc · 2017-10-28T07:38:24Z

@exarkun please take a look, I've updated the patch. I used @defer.inlineCallbacks. I hope you don't mind.

exarkun · 2017-11-09T18:31:57Z

src/txkube/test/test_authentication.py


+        yield agent.request(b"GET", b"http://" + kubernetes_host, headers)


I don't understand this change. The agent isn't doing real network operations, is it? Shouldn't it return an already-fired Deferred? If so, self.successResultOf seems more appropriate - and there's no need to turn _authorized_request into a Deferred-returning API.

This change was the easiest way for me to test the failure path by trying to resolve an invalid host against the FakeResolver, and the success path.
I didn't have good luck with successResultOf.

Use assertNoResult instead of inlineCallbacks

exarkun suggested changes Oct 11, 2017

View reviewed changes

This was referenced Oct 11, 2017

drop the version constraint #129

Closed

Use Twisted 17.9.0 or higher #151

Merged

exarkun suggested changes Oct 12, 2017

View reviewed changes

exarkun suggested changes Oct 17, 2017

View reviewed changes

Implement a fake resolver, for compatibility with Twisted 17.1.0 and …

ad1dcc6

…higher

exarkun reviewed Nov 9, 2017

View reviewed changes

exarkun and others added 2 commits November 10, 2017 09:03

Use assertNoResult instead of inlineCallbacks

03fb1da

Merge pull request #1 from LeastAuthority/fakeresolver

ff51f11

Use assertNoResult instead of inlineCallbacks

exarkun approved these changes Nov 10, 2017

View reviewed changes

exarkun merged commit 1f3582a into LeastAuthority:master Nov 10, 2017

rodrigc deleted the fakeresolver branch November 10, 2017 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a fake resolver, for compatibility with Twisted 17.1.0 and higher #150

Implement a fake resolver, for compatibility with Twisted 17.1.0 and higher #150

rodrigc commented Sep 5, 2017

rodrigc commented Sep 21, 2017

rodrigc commented Sep 21, 2017

exarkun left a comment

exarkun Oct 11, 2017

exarkun Oct 11, 2017

exarkun Oct 11, 2017

rodrigc Oct 12, 2017

exarkun Oct 11, 2017

rodrigc Oct 11, 2017

rodrigc commented Oct 12, 2017

exarkun left a comment

exarkun Oct 12, 2017

exarkun Oct 12, 2017

exarkun Oct 12, 2017

rodrigc commented Oct 13, 2017

exarkun left a comment

exarkun Oct 17, 2017

rodrigc Oct 18, 2017

exarkun Oct 17, 2017

rodrigc Oct 18, 2017

exarkun Oct 18, 2017

exarkun Oct 17, 2017

rodrigc Oct 18, 2017

exarkun Oct 17, 2017

rodrigc Oct 18, 2017

rodrigc commented Oct 28, 2017

exarkun Nov 9, 2017

rodrigc Nov 9, 2017


		yield agent.request(b"GET", b"http://" + kubernetes_host, headers)

Implement a fake resolver, for compatibility with Twisted 17.1.0 and higher #150

Implement a fake resolver, for compatibility with Twisted 17.1.0 and higher #150

Conversation

rodrigc commented Sep 5, 2017

rodrigc commented Sep 21, 2017

rodrigc commented Sep 21, 2017

exarkun 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

rodrigc commented Oct 12, 2017

exarkun 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

rodrigc commented Oct 13, 2017

exarkun 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

rodrigc commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment