#2619 cli: use main requests.Session in download_file()
Closed by ktdreyer. Opened by ktdreyer.
ktdreyer/koji download-file-session  into  master

Download 2619.patch

Prior to this change, we used new requests.Session objects in the download_file() method. This meant that we did not respect the serverca profile setting (for example), and we would inefficiently recreate a new HTTP session for every file we downloaded.

Update the CLI code to pass the main Koji session's rsession (requests Session) through to download_file(), so we can re-use our HTTP settings and connection.

Fixes: https://pagure.io/koji/issue/2618

maybe try a new topurl_ca option then fallback to serverca?

My purpose is to make Koji easier for newcomers.
Adding another configuration option is shifting the complexity to users. I would rather make "serverca" apply to all HTTPS connections that the Koji client performs.

Using session.rsession feels wrong here. This is an implementation detail of ClientSession and not really meant to be used this way. There is nowhere else in the code where this has been done.

The parameter name session is potentially confusing in the context of Koji, where "session" normally means a ClientSession instance. However, I really don't think that passing in a requests session is the right thing here. There is no cached session that it makes sense to use.

maybe try a new topurl_ca option then fallback to serverca?

I concur with Yu Ming here. Our download connections are fundamentally distinct from our xmlrpc connections. While using the same server for both is certainly a common case, it is not the only case and we need to handle the general case here.

Adding another configuration option is shifting the complexity to users.

I don't believe that is the case here. If the default for an unset topurl_ca is to fall back to the serverca value, then the new user with a simple set up only has to set the one value.

The name "topurl_ca" is a good example of how hard this will be for users and admins: it has an underscore, when "serverca" does not.

There are already dozens of configuration options in a Koji deployment, and I want to avoid adding more complexity to Koji configuration. If we don't support non-system-wide CAs for topurl, I think it's better to simply keep the feature out of Koji altogether and require that users trust the CA system-wide.

I don't think it's worth falling back here because we would need to catch the specific error from requests and then parse the error string from the exception to make sure that it was the exact CA error that we wanted to handle. That is some incredible complexity for QE to test in an integration test.

What is the resolution here? (I'm in favour of just documenting this as @ktdreyer writes here. I think that situation when system-wide CAs don't trust download server is not that common. Workaround with REQUESTS_CA_BUNDLE is ok to me, but not sure if it is a majority opinion.

Yeah, I'll close this, and we'll document it.

kojid has similar problems with HTTPS and non-system-wide certs. kojid uses koji.openRemoteFile(), and that method uses a temporary requests session as well. It's not possible to use an HTTPS topurl with kojid if the builder host does not trust the CA system-wide.

Pull-Request has been closed by ktdreyer

Metadata