Allow the client to specify a custom Agent.

[cwaldbieser]

By default, use the same Agent treq always provides, but allow using a custom Agent.
Unless/until treq grows addtional features, this would allow you to customize some of the behavior.

I have been experimenting with this by adding the ability to add internal CA certs to the certificate store. This lets treq make requests to sites with public certs and sites on an internal network. I find this more desirable than adjusting the OS certificate bundle, as treq may be used in different contexts on the same server. E.g. process "A" should really only be dealing with public sites, but process "B" might need to make requests from internal sites.

[ldanielburr]

This is interesting to me, as I have a similar need to deal with client certificates issued by an internal CA. I think replacing the agent itself is perhaps doing too much work, when you could just supply a policy:

def _client(*args, **kwargs):
    policy = kwargs.get('policy', BrowserLikePolicyForHTTPS())
    reactor = default_reactor(kwargs.get('reactor'))
    pool = default_pool(reactor,
                        kwargs.get('pool'),
                        kwargs.get('persistent'))
    agent = Agent(reactor, contextFactory=policy, pool=pool)
    return HTTPClient(agent)

What do you think?

[cwaldbieser]

That seems reasonable. Are you going to create a pull request?

[ldanielburr]

Yes, but it should have tests and documentation updates, since creating a valid policy object from a cert file isn't immediately obvious.

[glyph]

The change looks okay-ish to me, but it would be good if this came with documentation as well, explaining how to specify an agent.

[glyph]

Also tests. I guess that's the same thing @ldanielburr was saying :).

[ldanielburr]

@glyph - To be clear, I was suggesting that, rather than passing in an agent, which seems like overkill in terms of solving the specific problem of self-signed SSL certs, it would be better to allow for the passing of an IPolicyForHTTPS implementer, as per my code-sketch in the comments, above.

[glyph]

@ldanielburr - I agree. Sometimes you do need to pass the whole Agent too, of course.

[ldanielburr]

I've forked the repo, and have started working on the policy-oriented change.

[ldanielburr]

I've created https://github.com/dreid/treq/pull/80 to allow the user to specify their own IPolicyForHTTPS implementation, which I think satisfies the original use-case describe in this issue.

[codecov-io]

Current coverage is 95.84%

Merging #76 into master will not affect coverage as of b46978a

@@            master     #76   diff @@
======================================
  Files           20      20       
  Stmts         1418    1418       
  Branches       113     113       
  Methods          0       0       
======================================
  Hit           1359    1359       
  Partial         19      19       
  Missed          40      40       

Review entire Coverage Diff

Powered by Codecov

[cwaldbieser]

I had rather forgot I submitted this PR a while back. I had been sitting on the fork and using it in some projects, but I'd really like to see either this PR or #80 (or both) get folded into the main project, as they seem to be the only way to allow treq to deal with one-off internal certs (short of monkeying with the global cert bundle).

I added some minimal docs with a short code example and a very basic test. To be fair, the code change was also pretty minimal.

Thanks,
Carl

[glyph]

Thanks for checking in @cwaldbieser !

[glyph]

I'm merging this, for now, since the change looks OK and it would be nice to have some way to do this.

But I don't think we should stop here.

This is an instance of an anti-pattern I've identified - let's call it the "quasiglobal" anti-pattern - where you have an API which maintains some global state, but then, if you like, you can opt out of that global state by passing a parameter along. Superficially this enables easier testing and such, but it creates an obvious division between "normal" code and "testable" code.

It's an anti-pattern because it leads to a problem where A calls B and B calls C and C calls D, and A, B, and C all take the quasiglobal parameter (in this case, agent, in Twisted's case, the other instance of this anti-pattern, reactor) but D doesn't, and so you have to modify D to actually pass things all the way through.

What I think we should do longer-term is to get rid of the treq module as an interface, and instead, make HTTPClient the interface that you interact with; making the treq module simply a global instance of that API.

@cwaldbieser - would you be up for doing any further PRs in this area?

[cwaldbieser]

Glyph,

I have the cycles to devote to it.
I need to think about what it is you are suggesting.

Are you saying that (for instance) treq might have a singleton
_HTTPCLIENT which is an instance of treq.client.HttpClient, and the
various request functions get(), post(), etc. Would refer to it?

If I am on the wrong track, a short psuedocode example might be useful.

Thanks,
Carl
On Jul 29, 2015 6:19 AM, "Glyph" [email protected] wrote:

I'm merging this, for now, since the change looks OK and it would be nice
to have some way to do this.

But I don't think we should stop here.

This is an instance of an anti-pattern I've identified - let's call it the
"quasiglobal" anti-pattern - where you have an API which maintains some
global state, but then, if you like, you can opt out of that global
state by passing a parameter along. Superficially this enables easier
testing and such, but it creates an obvious division between "normal" code
and "testable" code.

It's an anti-pattern because it leads to a problem where A calls B and B
calls C and C calls D, and A, B, and C all take the quasiglobal parameter
(in this case, agent, in Twisted's case, the other instance of this
anti-pattern, reactor) but D doesn't, and so you have to modify D to
actually pass things all the way through.

What I think we should do longer-term is to get rid of the treq module as
an interface, and instead, make HTTPClient the interface that you
interact with; making the treq module simply a global instance of that
API.

@cwaldbieser https://github.com/cwaldbieser - would you be up for doing
any further PRs in this area?

—
Reply to this email directly or view it on GitHub
#76 (comment).

[glyph]

@cwaldbieser - I'm saying that treq itself should provide the same interface as treq.client.HTTPClient, so that your application can either import treq or get passed a treq.client.HTTPClient instance that you can manipulate directly. This is almost true right now, but there are some differences - unfortunately, this PR introduces another difference, which is that HTTPClient.get doesn't take an agent parameter, but treq.get now does.

treq already does effectively have a singleton HTTPClient, it's the return value of treq.api._client. I'm just saying that if you need a custom Agent, maybe we should focus on the fact that you can instantiate your own HTTPClient rather than passing keyword parameters along to all these functions? (In fact, the more I think about this, the more I wonder if we should roll this change back out and just replace it with a documentation change.)

[cwaldbieser]

I see what you mean. So basically, I could just do something like:

from treq.client import HTTPClient

custom_agent = make_my_agent(secret_sauce)
http_client = HTTPClient(custom_agent)
...

And if I just wanted to use the default agent:

import treq as http_client

...

That makes sense.

Thanks,
Carl

On Wed, Jul 29, 2015 at 2:20 PM, Glyph [email protected] wrote:

@cwaldbieser https://github.com/cwaldbieser - I'm saying that treq
itself should provide the same interface as treq.client.HTTPClient, so
that your application can either import treq or get passed a
treq.client.HTTPClient instance that you can manipulate directly. This is
almost true right now, but there are some differences - unfortunately,
this PR introduces another difference, which is that HTTPClient.get
doesn't take an agent parameter, but treq.get now does.

treq already does effectively have a singleton HTTPClient, it's the
return value of treq.api._client. I'm just saying that if you need a
custom Agent, maybe we should focus on the fact that you can instantiate
your own HTTPClient rather than passing keyword parameters along to all
these functions? (In fact, the more I think about this, the more I wonder
if we should roll this change back out and just replace it with a
documentation change.)

—
Reply to this email directly or view it on GitHub
#76 (comment).

[glyph]

Exactly. The main problem here is that the docs don't really cover how to do this, and the fact that functions like treq.content are on the module but aren't separated out so it's not clear which ones you should call on the client and which on the module.

So this is mostly a documentation challenge.