Re: [PATCH 4/8] tests/vm: Add configuration to basevm.py

[Robert Foley]

On Mon, 27 Jan 2020 at 07:26, Alex Bennée <address@hidden> wrote:
> > -SSH_KEY = open(os.path.join(os.path.dirname(__file__),
> > -               "..", "keys", "id_rsa")).read()
> > -SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__),
> > -                   "..", "keys", "id_rsa.pub")).read()
> > -
> > +SSH_KEY_FILE = os.path.join(os.path.dirname(__file__),
> > +               "..", "keys", "id_rsa")
> > +SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__),
> > +                   "..", "keys", "id_rsa.pub")
> > +SSH_KEY = open(SSH_KEY_FILE).read()
> > +SSH_PUB_KEY = open(SSH_PUB_KEY_FILE).read()
>
> Why are we tracking more information about the keyfile than we used to
> now? Is this because it's harder to embed keys over paths in the config?
We now allow the user to override the ssh keys.  Because of this we
need to track
the filename separately from the contents of the file, so that we can
put this default
filename into the DEFAULT_CONFIG below.
We also keep the original SSH_PUB_KEY since it is still
used by some pre-existing VM scripts, and we did not want to break backwards
compatibility for those scripts.
Actually upon further inspection, it looks like we can delete SSH_KEY,
this is no longer needed. :)
>
> > +
> > +# This is the standard configuration.
> > +# Any or all of these can be overridden by
> > +# passing in a config argument to the VM constructor.
> > +DEFAULT_CONFIG = {
> > +    'cpu'             : "max",
> > +    'machine'         : 'pc',
> > +    'guest_user'      : "qemu",
> > +    'guest_pass'      : "qemupass",
> > +    'root_pass'       : "qemupass",
> > +    'ssh_key_file'    : SSH_KEY_FILE,
> > +    'ssh_pub_key_file': SSH_PUB_KEY_FILE,
> > +    'memory'          : "4G",
> > +    'extra_args'      : [],
> > +    'dns'             : "",
> > +    'ssh_port'        : 0,
> > +    'install_cmds'    : "",
> > +    'boot_dev_type'   : "block",
> > +    'ssh_timeout'     : 1,
> > +}
> > +BOOT_DEVICE = {
> > +    'block' :  "-drive file={},if=none,id=drive0,cache=writeback "\
> > +               "-device virtio-blk,drive=drive0,bootindex=0",
> > +    'scsi'  :  "-device virtio-scsi-device,id=scsi "\
> > +               "-drive file={},format=raw,if=none,id=hd0 "\
> > +               "-device scsi-hd,drive=hd0,bootindex=0",
> > +}
> >  class BaseVM(object):
> > -    GUEST_USER = "qemu"
> > -    GUEST_PASS = "qemupass"
> > -    ROOT_PASS = "qemupass"
>
> Don't we need these?
We don't need these since we moved them up to the new DEFAULT_CONFIG.
These are essentially the default values now since the user
can now override these.
We also handle the cases where these are accessed by existing scripts,
and ensuring backwards compatibility by referencing these values in the
_config (see the code in __getattr__).

This actually brings up a good point that I wanted to mention.
Our initial plan was to leave the existing VM scripts unchanged.
We were thinking that it would also clarify things for a later patch to
simply change references to ROOT_PASS, GUEST_USER/ PASS,
and SSH_PUB_KEY, within the existing VM scripts (centos, openbsd, etc)
to use _config, and then we could get rid of the __getattr__ change entirely.
Actually, we could even put it at the end of this series too.
I think I will add this change to the next version of this patch unless you
think we should keep it separate?
> >
> >      envvars = [
> >          "https_proxy",
> > @@ -59,19 +84,26 @@ class BaseVM(object):
> >      poweroff = "poweroff"
> >      # enable IPv6 networking
> >      ipv6 = True
> > -    def __init__(self, debug=False, vcpus=None):
> > +    def __init__(self, debug=False, vcpus=None, config=None):
> >          self._guest = None
> > +        # Allow input config to override defaults.
> > +        self._config = DEFAULT_CONFIG.copy()
> > +        if config != None:
> > +            self._config.update(config)
> >          self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-",
> >                                                           suffix=".tmp",
> >                                                           dir="."))
> >          atexit.register(shutil.rmtree, self._tmpdir)
> > -
> > +        self._config['ssh_key'] = \
> > +            open(self._config['ssh_key_file']).read().rstrip()
> > +        self._config['ssh_pub_key'] = \
> > +            open(self._config['ssh_pub_key_file']).read().rstrip()
> >          self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa")
> > -        open(self._ssh_key_file, "w").write(SSH_KEY)
> > +        open(self._ssh_key_file, "w").write(self._config['ssh_key'])
> >          subprocess.check_call(["chmod", "600", self._ssh_key_file])
> >
> >          self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub")
> > -        open(self._ssh_pub_key_file, "w").write(SSH_PUB_KEY)
> > +        open(self._ssh_pub_key_file,
> >          "w").write(self._config['ssh_pub_key'])
>
> Read as a block I find this confusing:
>
>         self._config['ssh_key'] = \
>             open(self._config['ssh_key_file']).read().rstrip()
>         self._config['ssh_pub_key'] = \
>             open(self._config['ssh_pub_key_file']).read().rstrip()
>         self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa")
>         open(self._ssh_key_file, "w").write(self._config['ssh_key'])
>         subprocess.check_call(["chmod", "600", self._ssh_key_file])
>
>         self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub")
>         open(self._ssh_pub_key_file, "w").write(self._config['ssh_pub_key'])
>
> We read config['ssh_key_file'] but write out _ssh_pub_key_file which
> doesn't seem related.
I agree we can clarify this. :) Most of this logic was here previously,
we're just adding the parameterization of the keys.
This is the current flow:
1) copy the key file (config['ssh_key_file']) to a temporary file
(_ssh_pub_key_file)
2) chmod the key file so that ssh is happy with the permissions.
Without this chmod ssh will refuse to use the key file.
It seems to make sense to add a comment here to clarify all this.
It also seems like we could change the name _ssh_pub_key_file to
_ssh_tmp_pub_key_file
to make it more clear it is a temp file.
What do you think, would that be enough to clarify things?

Thanks & Regards,
-Rob
> <snip>

>
> --
> Alex Bennée