cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
> index bb24d57..3e5ba4c 100755
> --- a/cloudinit/config/cc_set_passwords.py
> +++ b/cloudinit/config/cc_set_passwords.py
> @@ -68,16 +68,49 @@ import re
>  import sys
>  
>  from cloudinit.distros import ug_util
> -from cloudinit import ssh_util
> +from cloudinit import log as logging
> +from cloudinit.ssh_util import update_ssh_config
>  from cloudinit import util
>  
>  from string import ascii_letters, digits
>  
> +LOG = logging.getLogger(__name__)
> +
>  # We are removing certain 'painful' letters/numbers
>  PW_SET = (''.join([x for x in ascii_letters + digits
>                     if x not in 'loLOI01']))
>  
>  
> +def handle_ssh_pwauth(pw_auth, service_cmd=None, service_name="ssh"):

ack. done

> +    cfg_name = "PasswordAuthentication"
> +    if service_cmd is None:
> +        service_cmd = ["service"]
> +
> +    if util.is_true(pw_auth):
> +        cfg_val = 'yes'
> +    elif util.is_false(pw_auth):
> +        cfg_val = 'no'
> +    else:
> +        bmsg = "Leaving ssh config '%s' unchanged." % cfg_name
> +        if pw_auth is None or pw_auth.lower() == 'unchanged':
> +            LOG.debug("%s ssh_pwauth=%s", bmsg, pw_auth)
> +        else:
> +            LOG.warning("%s Unrecognized value: ssh_pwauth=%s", bmsg, pw_auth)
> +        return
> +
> +    updated = update_ssh_config({cfg_name: cfg_val})
> +    if not updated:
> +        LOG.debug("No need to restart ssh service, %s not updated.", cfg_name)

i think i'd rather see the "nothing to do here" message.
i just think the logs then are more clear.

> +        return
> +
> +    if 'systemctl' in service_cmd:
> +        cmd = list(service_cmd) + ["restart", service_name]
> +    else:
> +        cmd = list(service_cmd) + [service_name, "restart"]
> +    util.subp(cmd)

well previously we just swallowed (logged and swallowed) the error.
now it would get raised, and the stack will know there is an error rather than just a warning getting written to the log.

I considered try/catch and adding to 'errors' in handle as before, but I think this is better.  this way the ProcessExecutionError willg o up to the caller.

> +    LOG.debug("Restarted the ssh daemon.")
> +
> +
>  def handle(_name, cfg, cloud, log, args):
>      if len(args) != 0:
>          # if run from command line, and give args, wipe the chpasswd['list']
> diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py
> index 882517f..3cb235b 100644
> --- a/cloudinit/ssh_util.py
> +++ b/cloudinit/ssh_util.py
> @@ -310,4 +314,55 @@ def parse_ssh_config_map(fname):
>          ret[line.key] = line.value
>      return ret
>  
> +
> +def update_ssh_config(updates, fname=DEF_SSHD_CFG):
> +    """Read fname, and update if changes are necessary.
> +

ack. done.

> +    @return: boolean indicating if an update was done."""
> +    lines = parse_ssh_config(fname)
> +    changed = update_ssh_config_lines(lines=lines, updates=updates)
> +    if changed:
> +        util.write_file(
> +            fname, "\n".join([str(l) for l in lines]) + "\n", copy_mode=True)
> +    return bool(changed)

len(changed) vs bool(changed)
I dont have a strong feeling.  len(changed) is possibly more explicit, catching a accidental None.
but if we change that here, then we need to make the same change above (if changed).
then we have 2 calls to 'len(changed)'

> +
> +
> +def update_ssh_config_lines(lines, updates):
> +    """Update the ssh config lines per updates.
> +
> +    @param lines: array of SshdConfigLine.  This array is updated in place.
> +    @param update: dictionary of desired values {Option: value}

done

> +    @return: A list of keys in updates that were changed."""
> +    found = set()
> +    changed = []
> +
> +    # Keywords are case-insensitive and arguments are case-sensitive
> +    casemap = dict([(k.lower(), k) for k in updates.keys()])
> +
> +    for (i, line) in enumerate(lines, start=1):
> +        if not line.key:
> +            continue
> +        if line.key in casemap:
> +            key = casemap[line.key]
> +            value = updates[key]
> +            found.add(key)

no. they're added to the found to know that we wont append them at the end.

> +            if line.value == value:
> +                LOG.debug("line %d: option %s already set to %s",
> +                          i, key, value)
> +            else:
> +                changed.append(key)
> +                LOG.debug("line %d: option %s updated %s -> %s", i,
> +                          key, line.value, value)
> +                line.value = value
> +
> +    if len(found) != len(updates):
> +        for key, value in updates.items():
> +            if key in found:
> +                continue
> +            changed.append(key)
> +            lines.append(SshdConfigLine('', key, value))
> +            LOG.debug("line %d: option %s added with %s",
> +                      len(lines), key, value)
> +    return changed
> +
>  # vi: ts=4 expandtab


-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/343246
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/1677205-eol-on-sshd_config into cloud-init:master.