PR#2: Lookaside improvements - fedpkg

fedpkg

#2 Lookaside improvements

Merged 9 years ago by ausil. Opened 9 years ago by bochecha.

Unknown source lookaside into master

lookaside: Use our new download path

Mathieu Bridon • 9 years ago

9f5468d

Simplify handling of our custom certificates

Mathieu Bridon • 9 years ago

bb5c278

Drop obsolete functions

Mathieu Bridon • 9 years ago

36db0d6

src/fedpkg/__init__.py

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 9 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 9 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"""`

src/fedpkg/lookaside.py

file added

+29

		`@@ -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

bochecha commented 9 years ago

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.

pbabinca commented 9 years ago

:thumbsup: code changes looks good.

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

without this pr:
$ cd /tmp
$ fedpkg co fedpkg
$ cd ./fedpkg
$ fedpkg switch-branch master
$ fedpkg -v sources
Creating repo object from /tmp/fedpkg
Downloading fedpkg-1.20.tar.bz2
Full url: http://pkgs.fedoraproject.org/repo/pkgs/fedpkg/fedpkg-1.20.tar.bz2/8ada82dca8bd21cf8cf3d47e2ae5ed14/fedpkg-1.20.tar.bz2
######################################################################## 100.0%
$ rm -rf /tmp/fedpkg
with this pr:
$ cd /tmp
$ fedpkg co fedpkg
$ cd ./fedpkg
$ fedpkg switch-branch master
$ fedpkg -v sources
Creating repo object from /tmp/fedpkg
Downloading fedpkg-1.20.tar.bz2
Full url: http://pkgs.fedoraproject.org/repo/pkgs/fedpkg/fedpkg-1.20.tar.bz2/md5/8ada82dca8bd21cf8cf3d47e2ae5ed14/fedpkg-1.20.tar.bz2
######################################################################## 100.0%
$ rm -rf /tmp/fedpkg

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

bochecha commented 9 years ago

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.

bochecha commented 9 years ago

@ausil: any comment?

ausil commented on line 40 of src/fedpkg/__init__.py 9 years ago

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

ausil commented on line 54 of src/fedpkg/__init__.py 9 years ago

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

ausil commented 9 years ago

Other than the config deatails it looks okay