googleads / googleads-python-lib

The Python client library for Google's Ads APIs
Apache License 2.0
681 stars 974 forks source link

Calling ReportService through a managed NAT resulting in closed connections #250

Closed oaustegard closed 6 years ago

oaustegard commented 6 years ago

We have attempted to utilize both this and the Java lib to download DFP reports from the ReportService through a managed NAT service, whose default timeout is 350 seconds. The absence of a Keep-Alive header on the http call forces the NAT to drop our connection prematurely.

https://github.com/googleads/googleads-python-lib/blob/master/googleads/common.py#L681 sets the socket timeout of the call to 1 hr, and this allows us to download the report from a public subnet, but our process must run behind the NAT and we're thus stymied with the current code base.

Is it possible to either a) add a Keep-Alive header with a similar timeout as in L681, or b) allow users to pass in the needed headers as part of the call? We'd hate to have to edit and keep synchronized changes to the library.

Thanks! Oskar

grivescorbett commented 6 years ago

Hey Oskar!

Thanks for writing in, I haven't heard this use case before. Just to be sure I fully understand, you'd like to be able to insert a custom http header in the request to the report download URL that's generated here? Do you have a similar desire to do so with the soap request http headers set here?

Best, Gabe

oaustegard commented 6 years ago

Gabe, thanks for the response. To be honest I tried following the logic in Github between suds and the various googleads classes and got a bit turned around*. Rather than proposing a solution at this point (we may get to that later) I want to simply clarify the problem: while our socket timeout is adequate, the absence of a keep-alive header forces the NAT gateway to drop the connection after 350 seconds. As such we would want the ability to add the keep-alive headers to any connection we find timing out. To date that's happening while asking for a specific report to be generated, and for the download of said report.

*One thought that did occur to me was that perhaps the change in code could happen on the suds code itself in https://github.com/googleads/googleads-python-lib/blob/master/googleads/util.py rather than in the common.py codebase. If we do end up having to monkeypatch the library, that may be the approach we take, but we'd obviously prefer a clean solution.


From: Gabe Rives-Corbett notifications@github.com Sent: Thursday, February 8, 2018 11:32 AM To: googleads/googleads-python-lib Cc: Austegard, Oskar; Author Subject: Re: [googleads/googleads-python-lib] Calling ReportService through a managed NAT resulting in closed connections (#250)

Hey Oskar!

Thanks for writing in, I haven't heard this use case before. Just to be sure I fully understand, you'd like to be able to insert a custom http header in the request to the report download URL that's generated herehttps://github.com/googleads/googleads-python-lib/blob/master/googleads/dfp.py#L804? Do you have a similar desire to do so with the soap request http headers set herehttps://github.com/googleads/googleads-python-lib/blob/master/googleads/dfp.py#L361?

Best, Gabe

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/googleads/googleads-python-lib/issues/250#issuecomment-364168678, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ADHggxPmet2g-UKODMNd18-ltMIpFxPoks5tSyG2gaJpZM4R9iDK.

oaustegard commented 6 years ago

An update on this - while we would still like the library to handle keep-alives (or allowing us to specify it as a header parameter, without the need for us to alter the library code), one of our intrepid platform engineers came up with a workaround:

import httplib

orig_connect = httplib.HTTPConnection.connect
def monkey_connect(self):
    orig_connect(self)
    self.sock.setsockopt(socket.SOL_SOCKET, socket.SO_KEEPALIVE, 1)
    self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPIDLE, 30)
    self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPINTVL, 10)
    self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPCNT, 6)
httplib.HTTPConnection.connect = monkey_connect

Basically we're keeping the NAT Gateway open by monkey-patching the httplib.HTTPConnection.connect function with the appropriate header applied to the socket directly. A hack, but appears to work.

grivescorbett commented 6 years ago

Hey Oskar,

Apologies for the delay, but I'm glad you're unblocked. This doesn't look the same as setting an HTTP header to me though, is this really what you were after? Exposing the raw socket is pretty different to exposing support for custom headers.

oaustegard commented 6 years ago

Well, as I say above, rather than prescribing a specific solution I wanted a way to successfully download the data through a gateway that enforces a time-out. This lower level hack appears to do the work, but as I also state, ideally this would be settable by the consumer of the SDK. Monkey patching anything is never ideal.

-- Oskar 301.675.2831


From: Gabe Rives-Corbett notifications@github.com Sent: Wednesday, February 14, 2018 9:32:34 AM To: googleads/googleads-python-lib Cc: Austegard, Oskar; Author Subject: Re: [googleads/googleads-python-lib] Calling ReportService through a managed NAT resulting in closed connections (#250)

Hey Oskar,

Apologies for the delay, but I'm glad you're unblocked. This doesn't look the same as setting an HTTP header to me though, is this really what you were after? Exposing the raw socket is pretty different to exposing support for custom headers.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/googleads/googleads-python-lib/issues/250#issuecomment-365624576, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ADHgg22-ZKyBvTXpH5mBivdo3MZ9tAODks5tUu6CgaJpZM4R9iDK.

grivescorbett commented 6 years ago

Hey Oskar,

I hear you, I did not know that you could specify socket flags like that using HTTP headers. Again I'm happy that you're unblocked, and we'll explore building this properly into the library.

grivescorbett commented 6 years ago

Hi Oskar,

Quick update: we've agreed on an API and I've begun working on this feature.

Cheers! Gabe

oaustegard commented 6 years ago

Excellent! I look forward to being able to strip our hack and replace with the formal library upgrade!

msaniscalchi commented 6 years ago

This was fixed in the last release.