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

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

Merged
merged 3 commits into from Nov 10, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
94 changes: 81 additions & 13 deletions src/txkube/test/test_authentication.py
Expand Up @@ -8,12 +8,16 @@

import pem

import attr

from pyrsistent import InvariantException

from fixtures import TempDir

from testtools.matchers import AfterPreprocessing, Equals, Contains, IsInstance, raises

from testtools import ExpectedException
from testtools.matchers import (
AfterPreprocessing, Equals, Contains, IsInstance, raises
)
from cryptography.hazmat.primitives.asymmetric import rsa
from cryptography.x509.oid import NameOID
from cryptography.hazmat.primitives.hashes import SHA256
Expand All @@ -28,7 +32,13 @@
)
from cryptography.hazmat.backends import default_backend

from zope.interface import implementer

from twisted.python.filepath import FilePath
from twisted.internet import defer
from twisted.internet.address import IPv4Address
from twisted.internet.error import DNSLookupError
from twisted.internet.interfaces import IHostResolution, IReactorPluggableNameResolver
from twisted.internet.protocol import Factory
from twisted.web.http_headers import Headers
from twisted.test.iosim import ConnectionCompleter
Expand Down Expand Up @@ -68,11 +78,51 @@
-----END CERTIFICATE-----
"""

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

def create_reactor():
"""
Twisted 17.1.0 and higher requires a reactor which implements
``IReactorPluggableNameResolver``.
"""

@implementer(IHostResolution)
@attr.s
class Resolution(object):
name = attr.ib()

class _FakeResolver(object):

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

resolutionReceiver.resolutionBegan(r)
if hostName in HOST_MAP:
resolutionReceiver.addressResolved(
IPv4Address('TCP', HOST_MAP[hostName], portNumber))
resolutionReceiver.resolutionComplete()
return r
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


@implementer(IReactorPluggableNameResolver)
class _ResolvingMemoryClockReactor(MemoryReactorClock):
nameResolver = _FakeResolver()

return _ResolvingMemoryClockReactor()



class AuthenticateWithServiceAccountTests(TestCase):
"""
Tests for ``authenticate_with_serviceaccount``.
"""
def _authorized_request(self, token, headers):
@defer.inlineCallbacks
def _authorized_request(self, token, headers,
kubernetes_host=b"example.invalid."):
"""
Get an agent using ``authenticate_with_serviceaccount`` and issue a
request with it.
Expand All @@ -83,7 +133,7 @@ def _authorized_request(self, token, headers):
factory = Factory.forProtocol(lambda: server)
factory.protocolConnectionMade = None

reactor = MemoryReactorClock()
reactor = create_reactor()
reactor.listenTCP(80, factory)

t = FilePath(self.useFixture(TempDir()).join(b""))
Expand All @@ -95,34 +145,36 @@ def _authorized_request(self, token, headers):

self.patch(
os, "environ", {
b"KUBERNETES_SERVICE_HOST": b"example.invalid.",
b"KUBERNETES_SERVICE_HOST": kubernetes_host,
b"KUBERNETES_SERVICE_PORT": b"443",
},
)

agent = authenticate_with_serviceaccount(
reactor, path=serviceaccount.path,
)
agent.request(b"GET", b"http://example.invalid.", headers)

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

[(host, port, factory, _, _)] = reactor.tcpClients

self.expectThat((host, port), Equals((b"example.invalid.", 80)))
addr = HOST_MAP.get(kubernetes_host.decode("ascii"), None)
self.expectThat((host, port), Equals((addr, 80)))

pump = ConnectionCompleter(reactor).succeedOnce()
pump.pump()

return server.data
defer.returnValue(server.data)


@defer.inlineCallbacks
def test_http_bearer_token_authorization(self):
"""
The ``IAgent`` returned adds an *Authorization* header to each request it
issues. The header includes the bearer token from the service account
file. This works over HTTP.
"""
token = bytes(uuid4())
request_bytes = self._authorized_request(token=token, headers=None)
request_bytes = yield self._authorized_request(token=token, headers=None)

# Sure would be nice to have an HTTP parser.
self.assertThat(
Expand All @@ -143,7 +195,7 @@ def test_https_bearer_token_authorization(self):
factory = Factory.forProtocol(lambda: server)
factory.protocolConnectionMade = None

reactor = MemoryReactorClock()
reactor = create_reactor()
reactor.listenTCP(443, factory)

token = bytes(uuid4())
Expand Down Expand Up @@ -184,14 +236,30 @@ def test_https_bearer_token_authorization(self):
)


@defer.inlineCallbacks
def test_hostname_does_not_resolve(self):
"""
Specifying a hostname which cannot be resolved to an
IP address will result in an ``DNSLookupError``.
"""
with ExpectedException(DNSLookupError, "DNS lookup failed: no results "
"for hostname lookup: doesnotresolve."):
yield self._authorized_request(
token="test",
headers=Headers({}),
kubernetes_host=b"doesnotresolve"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.



@defer.inlineCallbacks
def test_other_headers_preserved(self):
"""
Other headers passed to the ``IAgent.request`` implementation are also
sent in the request.
"""
token = bytes(uuid4())
headers = Headers({u"foo": [u"bar"]})
request_bytes = self._authorized_request(token=token, headers=headers)
request_bytes = yield self._authorized_request(token=token, headers=headers)
self.expectThat(
request_bytes,
Contains(u"Authorization: Bearer {}".format(token).encode("ascii")),
Expand Down Expand Up @@ -223,7 +291,7 @@ def test_missing_ca_certificate(self):

self.assertThat(
lambda: authenticate_with_serviceaccount(
MemoryReactorClock(), path=serviceaccount.path,
create_reactor(), path=serviceaccount.path,
),
raises(ValueError("No certificate authority certificate found.")),
)
Expand Down Expand Up @@ -254,7 +322,7 @@ def test_bad_ca_certificate(self):

self.assertThat(
lambda: authenticate_with_serviceaccount(
MemoryReactorClock(), path=serviceaccount.path,
create_reactor(), path=serviceaccount.path,
),
raises(ValueError(
"Invalid certificate authority certificate found.",
Expand Down