On Thu, 2011-10-06 at 14:37 +0000, James Tunnicliffe wrote:
> Hi Guilherme,
> 
> Thanks for another review. Seem to be getting close now.

Indeed, if it wasn't for the permission issue we just seem to have
uncovered... :(

> On 6 October 2011 14:28, Guilherme Salgado <email address hidden> wrote:
> > On Wed, 2011-10-05 at 16:25 +0000, James Tunnicliffe wrote:
> >> James Tunnicliffe has proposed merging lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh into lp:~linaro-infrastructure/offspring/linaro.
> 
> >> This merge address the comments raised about the initial one. It also
> >> updates the web UI so the admin interface allows the user to change
> >> the ssh user and key, but can't get access to the stored key.
> >
> > I was expecting the ssh user/key to be editable on the project's edit
> > page (the project_edit() view), but I guess that wouldn't work because
> > people with the change_project permission would then be able to change
> > that for all projects, right?
> >
> > Having to use the admin UI to change a project's ssh user/key means the
> > people in charge of every project will have to send the ssh key to
> > somebody with admin rights to set.  I think we should check with Vicky
> > that this is ok.
> 
> Ah, sorry, that comment was wrong - anyone can paste a new SSH key
> into the project page, it isn't just the admin page.
> 
> Am I right in thinking that the code you are putting in will restrict
> access to all the project pages to specified users? If so, I think
> this is OK.

Only access to private projects will be restricted to specified users.

> >> === modified file 'lib/offspring/slave/slave.py'
> >> -    def startBuild(self, projectName, configBranchURL):
> >> +    def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):
> >> +        # check for lp: format URLs. Convert these to bzr+ssh ones
> >> +        configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
> >> +                                 configBranchURL)
> >
> > I thought you were going to use 'bzr lp-login' as it's not a problem if
> > we fail to cleanup ~/.bazaar/authentication.conf as long as we make sure
> > to kill the ssh agent.  And if we fail to kill the ssh agent then using
> > lp-login is not much worse than it is now.
> 
> The last comment James W made about home directories was that they may
> be shared between builders, so we don't want to transport the
> launchpad user using ~/.bazaar/authentication.conf to the build script
> because it could change before it is used.

My understanding is that they are only shared between builds on a
certain builder, which is not a problem as they're serialized and we're
killing ssh-agent between builds.

> >> +    def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
> >> +
> >> +        if lpUser:
> >> +            # We don't want to pass the user separately, we want it to be
> >> +            # integrated into the configBranchURL.
> >> +            configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
> >> +
> >>          self.currentBuild = ProjectBuildProcess(
> >> -            self, projectName, configBranchURL)
> >> +            self, projectName, configBranchURL, lpSSHKey)
> >>          logging.info("Starting build: %s %s %s" % (
> >>              self.currentBuild.getCommandPath(), projectName, configBranchURL))
> >>          self.currentBuild.start()
> >> @@ -70,9 +94,12 @@
> >>              if self.currentBuild.process is not None:
> >>                  if self.currentBuild.process.poll() is None:
> >>                      self.currentBuild.process.terminate()
> >> -            # Give it 5 seconds before sending kill.
> >> -            time.sleep(int(config.slave("child_wait_timeout")))
> >> -            self.currentBuild.process.kill()
> >> +                    if self.currentBuild.process.poll() is None:
> >> +                        # Give it 5 seconds before sending kill.
> >> +                        time.sleep(int(config.slave("child_wait_timeout")))
> >> +                        if self.currentBuild.process.poll() is None:
> >> +                            self.currentBuild.process.kill()
> >
> > Why is this needed?
> 
> I found when running tests that since I was creating and destroying
> the slave, but not starting a build process, the old code would just
> call self.currentBuild.process.kill() and self.currentBuild.process
> didn't exist, causing an error. This code only does exactly the same
> as the old code (ask process to end gracefully, wait 5 seconds, send
> kill signal), but only performing each step as needed.

Ok, I asked because it seemed to be there just to please the tests but
on second thought I think it makes sense because we don't want to call
self.currentBuild.process.kill() unconditionally, as the if block above
it implies it may be None.

> 
> >> === added directory 'lib/offspring/slave/tests'
> >> === added file 'lib/offspring/slave/tests/test_private_ssh_keys.py'
> >> --- lib/offspring/slave/tests/test_private_ssh_keys.py        1970-01-01 00:00:00 +0000
> >> +++ lib/offspring/slave/tests/test_private_ssh_keys.py        2011-10-05 16:24:23 +0000
> >> @@ -0,0 +1,88 @@
> >> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> >> +# GNU Affero General Public License version 3 (see the file LICENSE).
> >> +
> >> +import os
> >> +import subprocess
> >> +import offspring.slave
> >> +import unittest
> >> +import xmlrpclib
> >> +import re
> >> +import os.path
> >> +
> >> +class TestLexbuilderSlavePrivateSSHKeys(unittest.TestCase):
> >> +
> >> +    def nothing(self):
> >> +        pass
> >> +
> >> +    def setUp(self):
> >> +        self.slave = offspring.slave.LexbuilderSlave(('localhost', 8760))
> >> +        self.slave.start()
> >> +        self.x = xmlrpclib.ServerProxy("http://admin:password@localhost:8760")
> >
> > Why do we need a ServerProxy?
> 
> <snip>
> 
> > This test is showing that startBuild() does the right thing when given a
> > user and key, but I think it should do so via XML-RPC because that is
> > how the user/key are going to be passed in from the master to the slave.
> > What do you think?
> 
> OK. the ServerProxy stays. I have modified the tests to use it where
> they can and commented as to why in the places they can't.

Cool!

> 
> >> +
> >> +        passed_args = self.slave.currentBuild.args
> >
> > Instead of storing the passed arguments as an instance variable in
> > ProjectBuildProcess, you could just mock its __init__() here and check
> > that it was passed the correct arguments. That way we don't need to
> > pollute production code with stuff that only exists to aid testing
> > (.args).  It's not a big deal, but it'd be good to at least have a
> > comment there stating that .args is there to make testing easier.
> 
> ProjectBuildProcess.args is used locally in __init__ to allow us to
> pass the SSH key location to the build script. It could just be a
> variable local to __init__ if I mocked up MonitoredProcess.__init__ -
> will do that.

Right, that was what I thought. :)

> >> === modified file 'lib/offspring/web/queuemanager/forms.py'
> >> --- lib/offspring/web/queuemanager/forms.py   2011-03-03 00:06:58 +0000
> >> +++ lib/offspring/web/queuemanager/forms.py   2011-10-05 16:24:23 +0000
> <snip>
> >>  class ProjectBaseForm(ModelForm):
> >>      status = fields.CharField(max_length=200,
> >>          widget=fields.Select(choices=Project.STATUS_CHOICES), required=True)
> >>
> >> +    ssh_help = ("Enter a private SSH ASCII key block, complete with begin "+
> >> +                "and end markers. Saved keys are not shown. If you wish to "+
> >> +                "erase a saved key (rather than replacing it) enter the "+
> >> +                "string \"ERASE\".")
> >> +
> >> +    lp_ssh_key_input = forms.CharField(label="Launchpad User's SSH key",
> >> +                                       required=False,
> >> +                                       widget=forms.Textarea(
> >> +                                               attrs={'cols': 73, 'rows' : 4}),
> >> +                                       help_text=ssh_help)
> >
> > Does this mean it's also possible to change a project's ssh key on its
> > edit view?  IIUC that means anybody with change_project rights can then
> > do so for all projects?  I guess that's why you've made it so that an
> > existing ssh key is not displayed?
> 
> Yes, anyone who can change the project can change the SSH key. Should
> be simple enough to keep it just on the admin page though.

The problem is that there are no per-project permissions in Offspring;
if a user can change one project they can change all of them (modulo the
private ones, after our work is complete).

If we keep it only in the admin UI we'd have to give the ssh keys to
someone with admin rights, and this someone would then be given the ssh
keys of all landing teams.

Or maybe we could have one member of each LT as an admin in Offspring so
that they can do it themselves.  With your changes to not show the
existing ssh key that means they'd only be able to enter new ssh keys
for projects but not see existing ones.  I don't think it is a good idea
to have one person from each LT with admin rights though.


-- 
Guilherme Salgado <https://launchpad.net/~salgado>