#2 Lookaside improvements
Merged 8 years ago by ausil. Opened 8 years ago by bochecha.
Unknown source lookaside  into  master

file modified
+23 -52
@@ -14,10 +14,12 @@

  import cli

  import git

  import re

- import pycurl

  import fedora_cert

  import platform

  

+ from .lookaside import FedoraLookasideCache

+ from pyrpkg.utils import cached_property

+ 

  

  class Commands(pyrpkg.Commands):

  
@@ -57,8 +59,6 @@

  

          # New properties

          self._kojiconfig = None

-         self._cert_file = None

-         self._ca_cert = None

          # Store this for later

          self._orig_kojiconfig = kojiconfig

  
@@ -96,27 +96,33 @@

                  return

          self._kojiconfig = self._orig_kojiconfig

  

-     @property

+     @cached_property

      def cert_file(self):

-         """This property ensures the cert_file attribute"""

+         """A client-side certificate for SSL authentication

  

-         if not self._cert_file:

-             self.load_cert_files()

-         return self._cert_file

+         We override this from pyrpkg because we actually need a client-side

+         certificate.

+         """

+         return os.path.expanduser('~/.fedora.cert')
ausil commented 8 years ago

This needs to be read from the koji config and not hardcoded. if we change the location everything would break

  

-     @property

+     @cached_property

      def ca_cert(self):

-         """This property ensures the ca_cert attribute"""

+         """A CA certificate to authenticate the server in SSL connections

  

-         if not self._ca_cert:

-             self.load_cert_files()

-         return self._ca_cert

+         We override this from pyrpkg because we actually need a custom

+         CA certificate.

+         """

+         return os.path.expanduser('~/.fedora-server-ca.cert')
ausil commented 8 years ago

Again must be read from the config, I do plan to actually change this location

  

-     def load_cert_files(self):

-         """This loads the cert_file attribute"""

+     @cached_property

+     def lookasidecache(self):

+         """A helper to interact with the lookaside cache

  

-         self._cert_file = os.path.expanduser('~/.fedora.cert')

-         self._ca_cert = os.path.expanduser('~/.fedora-server-ca.cert')

+         We override this because we need a different download path.

+         """

+         return FedoraLookasideCache(

+             self.lookasidehash, self.lookaside, self.lookaside_cgi,

+             client_cert=self.cert_file, ca_cert=self.ca_cert)

  

      # Overloaded property loaders

      def load_rpmdefines(self):
@@ -196,41 +202,6 @@

              super(Commands, self).load_user()

  

      # New functionality

-     def _create_curl(self):

-         """Common curl setup options used for all requests to lookaside."""

- 

-         # Overloaded to add cert files to curl objects

-         # Call the super class

-         curl = super(Commands, self)._create_curl()

- 

-         # Set the users Fedora certificate:

-         if os.path.exists(self.cert_file):

-             curl.setopt(pycurl.SSLCERT, self.cert_file)

-         else:

-             self.log.warn("Missing certificate: %s" % self.cert_file)

- 

-         # Set the Fedora CA certificate:

-         if os.path.exists(self.ca_cert):

-             curl.setopt(pycurl.CAINFO, self.ca_cert)

-         else:

-             self.log.warn("Missing certificate: %s" % self.ca_cert)

- 

-         return curl

- 

-     def _do_curl(self, file_hash, file):

-         """Use curl manually to upload a file"""

- 

-         # This is overloaded to add in the fedora user's cert

-         cmd = ['curl', '-k', '--cert', self.cert_file, '--fail', '-o',

-                '/dev/null', '--show-error', '--progress-bar', '-F',

-                'name=%s' % self.module_name,

-                '-F', '%ssum=%s' % (self.lookasidehash, file_hash),

-                '-F', 'file=@%s' % file]

-         if self.quiet:

-             cmd.append('-s')

-         cmd.append(self.lookaside_cgi)

-         self._run_command(cmd)

- 

      def _findmasterbranch(self):

          """Find the right "fedora" for master"""

  

@@ -0,0 +1,29 @@

+ # Copyright (c) 2015 - Red Hat Inc.

+ #

+ # This program is free software; you can redistribute it and/or modify it

+ # under the terms of the GNU General Public License as published by the

+ # Free Software Foundation; either version 2 of the License, or (at your

+ # option) any later version.  See http://www.gnu.org/copyleft/gpl.html for

+ # the full text of the license.

+ 

+ 

+ """Interact with the Fedora lookaside cache

+ 

+ We need to override the pyrpkg.lookasidecache module to handle our custom

+ download path.

+ """

+ 

+ 

+ from pyrpkg.errors import DownloadError

+ from pyrpkg.lookaside import CGILookasideCache

+ 

+ 

+ class FedoraLookasideCache(CGILookasideCache):

+     def __init__(self, hashtype, download_url, upload_url,

+                  client_cert=None, ca_cert=None):

+         super(FedoraLookasideCache, self).__init__(

+             hashtype, download_url, upload_url, client_cert=client_cert,

+             ca_cert=ca_cert)

+ 

+         self.download_path = (

+             '%(name)s/%(filename)s/%(hashtype)s/%(hash)s/%(filename)s')

no initial comment

If anybody wants to try this out, I have a copr with a fedpkg build that includes these patches:

http://copr.fedoraproject.org/coprs/bochecha/pyrpkg-tests

It requires pyrpkg 1.35, which has been pushed to stable in all Fedora/EPEL branches for a while, now.

:thumbsup: code changes looks good.

:thumbsup: new download path works as advertised (tested on fedpkg repo):

@ausil: if you are ok with this change please either ack and I'll merge the changes or merge it by yourself.

Just to make it clear one more time: this is not a breaking change that needs to be part of the "flag day" (when we decide to break old clients and everybody needs to upgrade)

The change in this pull request could be released to users, and both the new and old clients would continue working just fine.

This needs to be read from the koji config and not hardcoded. if we change the location everything would break

Again must be read from the config, I do plan to actually change this location

Other than the config deatails it looks okay