Skip to content

declare dependency on ssh-import-id (SC-864) #1334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
holmanb merged 9 commits into from
Apr 19, 2022
+34 −2
Merged

declare dependency on ssh-import-id (SC-864) #1334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c266055
Make package ssh-import-id "suggested" by cloud-init
holmanb Mar 14, 2022
04abd5d
warn and exit if ssh-import-id not available
holmanb Apr 14, 2022
e935bb7
skip if bin _is not_ found
holmanb Apr 14, 2022
896e6d0
s/SSH_IMPORT_ID/SSH_IMPORT_ID_BINARY/
holmanb Apr 15, 2022
1dd6da3
don't check for ssh_import_id key
holmanb Apr 15, 2022
bec04bf
find embedded keys
holmanb Apr 18, 2022
13058fc
fmt
holmanb Apr 19, 2022
b8e30d6
early exit based on key presence, suggested by @blackboxsw
holmanb Apr 19, 2022
b7461e4
flake
blackboxsw Apr 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions cloudinit/config/cc_ssh_import_id.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
# https://launchpad.net/ssh-import-id
distros = ["ubuntu", "debian"]

SSH_IMPORT_ID_BINARY = "ssh-import-id"
MODULE_DESCRIPTION = """\
This module imports SSH keys from either a public keyserver, usually launchpad
or github using ``ssh-import-id``. Keys are referenced by the username they are
Expand Down Expand Up @@ -49,6 +50,19 @@

def handle(_name, cfg, cloud, log, args):

if not is_key_in_nested_dict(cfg, "ssh_import_id"):
log.debug(
"Skipping module named ssh-import-id, no 'ssh_import_id'"
" directives found."
)
return
elif not subp.which(SSH_IMPORT_ID_BINARY):
log.warn(
"ssh-import-id is not installed, but module ssh_import_id is "
"configured. Skipping module."
)
return

# import for "user: XXXXX"
if len(args) != 0:
user = args[0]
Expand Down Expand Up @@ -138,7 +152,7 @@ def import_ssh_ids(ids, user, log):
"--preserve-env=https_proxy",
"-Hu",
user,
"ssh-import-id",
SSH_IMPORT_ID_BINARY,
] + ids
log.debug("Importing SSH ids for user %s.", user)

Expand All @@ -149,4 +163,21 @@ def import_ssh_ids(ids, user, log):
raise exc


# vi: ts=4 expandtab
def is_key_in_nested_dict(config: dict, search_key: str) -> bool:
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable. It's probably a non-issue, but here are the costs of this generic function:

  • false positives if there are ssh_import_id keys under paths besides "users", and "user". (This implies invalid user-data schema anyway)
  • this function does more work that a specific and targeted function which only looks for the supported ssh_import_id paths. (user-data is generally small for systems and a small recursive loop across cloud-config dict really doesn't cost too much)

The only alternative I could think of here would be only looking at the places where we support this key with something like the following:

if "ssh_import_id" in config:
   return True
if "user" in config and isinstance(config["user"], dict):
   if "ssh_import_id" in config["user"]:
       return True
if "users" in config:
   if isinstance(config["users"], dict):
       for user, user_config:
           if "ssh_import_id" in user_config:
               return True
   if isinstance(config["users"], list):
       for user_config in config["users"]:
           if "ssh_import_id" in user_config:
               return True
return False

When I run timeit over the following I see the generic function is twice as long (though it's a tiny cost per run amount)

# {"ssh_import_id": "xxxx", "packages": ["sl"]}
specific  1.0499926080001387
general recursive  1.904686696000681
# d2 = {"user": {"ssh_import_id": "yep"}, "packages": ["sl"]}
specific  2.0969378739991953
general recursive  4.546597008999925

# d3 = {"users": [{"ssh_import_id": "yep"}], "packages": ["sl"]}
specific  3.2126418679999915
general recursive  6.143558423999821

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My junker timeit test

#!/usr/bin/python3

def is_ssh_import_id_set(config, key="ssh_import_id"):

   if key in config:
      return True
   if "user" in config and isinstance(config["user"], dict):
      if key in config["user"]:
          return True
   if "users" in config:
      if isinstance(config["users"], dict):
          for _username, user_config in config["users"].items():
              if key in user_config:
                  return True
      if isinstance(config["users"], list):
          for user_config in config["users"]:
              if key in user_config:
                  return True
   return False

def is_key_in_nested_dict(config: dict, search_key: str) -> bool:
    """Search for key nested in config.

    Note: A dict embedded in a list of lists will not be found walked - but in
    this case we don't need it.
    """
    for config_key in config.keys():
        if search_key == config_key:
            return True
        if isinstance(config[config_key], dict):
            return is_key_in_nested_dict(config[config_key], search_key)
        if isinstance(config[config_key], list):
            # this code could probably be generalized to walking the whole
            # config by iterating lists in search of dictionaries
            for item in config[config_key]:
                if isinstance(item, dict):
                     return is_key_in_nested_dict(item, search_key)
    return False


import timeit

d1 = {"ssh_import_id": "yep", "packages": ["sl"]}
d2 = {"user": {"ssh_import_id": "yep"}, "packages": ["sl"]}
d3 = {"users": [{"ssh_import_id": "yep"}], "packages": ["sl"]}



print("specific ", timeit.timeit('is_ssh_import_id_set(d3, "ssh_import_id")', globals=globals(), number=10000000))
print("general recursive ", timeit.timeit('is_key_in_nested_dict(d3, "ssh_import_id")', globals=globals(), number=10000000))

"""Search for key nested in config.

Note: A dict embedded in a list of lists will not be found walked - but in
this case we don't need it.
"""
for config_key in config.keys():
if search_key == config_key:
return True
if isinstance(config[config_key], dict):
return is_key_in_nested_dict(config[config_key], search_key)
if isinstance(config[config_key], list):
# this code could probably be generalized to walking the whole
# config by iterating lists in search of dictionaries
for item in config[config_key]:
if isinstance(item, dict):
return is_key_in_nested_dict(item, search_key)
return False
1 change: 1 addition & 0 deletions packages/debian/control.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Depends: ${misc:Depends},
iproute2,
isc-dhcp-client
Recommends: eatmydata, sudo, software-properties-common, gdisk
Suggests: ssh-import-id
Description: Init scripts for cloud instances
Cloud instances need special scripts to run during initialisation
to retrieve and install ssh keys and to let the user run various scripts.