fix(ssh): don't raise exception when default ssh keys can't be found

[a-dubs]

fix(ssh): don't raise exception when default ssh keys can't be found

This commit will once again allow for setting ssh keys at runtime 
using the `Cloud.use_key()` method when default ssh keys dont exist
on the system and no ssh key paths are provided in pycloudlib.toml.

Further context:
PR #406 broke some end consumers/users of pycloudlib on systems where
1) ssh keys are not set in the pycloudlib.toml and the ssh key is later
set using the Cloud.use_key() method
2) no ssh keys exist at the default paths of '~/.ssh/id_rsa.pub' or
'~/.ssh/id_ed25519.pub'. 
This commit removes / reverts the error that was introduced in PR #406
which gets raised at the Cloud class instantation when those two cases
are true. The biggest issue with this change, is there was no way for
an end user to override/prevent pycloudlib from erroring out if those 
cases are true - besides for creating a fake ssh key at one of the two
default paths. Now, a warning is just logged instead, restoring the
flexibility pycloudlib preivously provided to end users for setting / 
using ssh keys.

If both scenarios 1 and 2 are true (ssh key is unset/doesn't exist),
a new exception type `UnsetSSHKeyError` will be raised with verbose
exception message explaining why this happened and how to fix. 

Test Steps:

[jrtknauer]

We can be more explicit by returning None and adjust _get_ssh_keys to return Optional[KeyPair] instead of a KeyPair with null attributes. Otherwise _get_ssh_keys is returning KeyPairs which are effectively invalid objects (prefer to construct complete and correct data structures) and nullity checks against the object would need to inspect each attribute.

[a-dubs]

If we return None from _get_ssh_keys, this makes for absolutely nuclear splash radius. why not add the null check in KeyPair class in one place instead of needing to add checks across dozens of call sites that rely on the ssh key being a KeyPair and not potentially none?

[TheRealFalcon]

I initially had the same thought as @jrtknauer , but you're right @a-dubs in that this'll introduce a ton of None checks across the code base. We already allow a keypair with no private key set (why???), so this doesn't seem to be all that much worse to me.

Do we know if there are any use cases of anybody needing to set a key after initializing a cloud object? I don't just mean that use_key() currently has callers, but is there a reason we couldn't rely on the class always being initialized with the key pair? If not, I think I'd prefer we nuke use_key() altogether and turn it into a constructor argument. There's no reason to carry around uninitialized or half-initialized state if it's not needed. I realize that'd be a breaking change and require a major version bump, so I'm not asking for it here, but it might be better end state to get to.

[jrtknauer]

Do we know if there are any use cases of anybody needing to set a key after initializing a cloud object? I don't just mean that use_key() currently has callers, but is there a reason we couldn't rely on the class always being initialized with the key pair? If not, I think I'd prefer we nuke use_key() altogether and turn it into a constructor argument. There's no reason to carry around uninitialized or half-initialized state if it's not needed. I realize that'd be a breaking change and require a major version bump, so I'm not asking for it here, but it might be better end state to get to

Given I encountered the issue with the current implementation in the wild and motivated this pull request:

There is no interface in the current implementation for the IBM cloud to set SSH keys on instantiation:

class IBM(BaseCloud):
    """IBM Virtual Private Cloud Class."""

    _type = "ibm"

    def __init__(
        self,
        tag: str,
        timestamp_suffix: bool = True,
        config_file: Optional[ConfigFile] = None,
        *,
        resource_group: Optional[str] = None,
        vpc: Optional[str] = None,
        api_key: Optional[str] = None,
        region: Optional[str] = None,
        zone: Optional[str] = None,
    ):
        """Initialize the connection to IBM VPC.

        Args:
            tag: string used to name and tag resources with
            timestamp_suffix: Append a timestamped suffix to the tag string.
            config_file: path to pycloudlib configuration file
        """
        super().__init__(
            tag,
            timestamp_suffix,
            config_file,
            required_values=[resource_group, api_key, region],
        )
        ...

In fact, this is an issue with all clouds. Removing use_key is a non-starter until this is addressed. But that's not the point of this pull request. All I wanted was for pycloudlib to not raise exceptions if an SSH key is not set on initialization, because the introduction of that behavior in 335c4a4d9525d78120cc127905ccbd1928046e2d now crashes applications using pycloudlib on systems which do not have the hard-coded default public SSH keys (~/.ssh/id_rsa.pub or ~/.ssh/id_ed25519.pub).

My would-like-to-haves span from:

• Not constructing objects with nothing but None attributes. However, as it's been established the existing pattern has infiltrated the entirety of the code base making it a high impact change on its own.
• Consistency in specification of SSH keys from Cloud initialization. However, this would require updating the initialization interfaces for the base Cloud class and all derived clouds.

Ultimately, my requirement is that pycloudlib is not crashing from a recoverable state (use_key exists).

[a-dubs]

thanks for the feedback @jrtknauer. I agree that we should implement a way to pass ssh key configuration to the base cloud class at runtime (enabling this across all clouds simultaneously) and then possibly explore the removal of the use_key() function. But for the time being, the changes put forward in this PR will alleviate the issue you are describing and that other members of CPC have been running into. So I will open an issue to capture this future work, which will also address both items in your nice-to-have list.

[TheRealFalcon]

A few inline comments but overall looks good.

[TheRealFalcon]

I initially had the same thought as @jrtknauer , but you're right @a-dubs in that this'll introduce a ton of None checks across the code base. We already allow a keypair with no private key set (why???), so this doesn't seem to be all that much worse to me.

Do we know if there are any use cases of anybody needing to set a key after initializing a cloud object? I don't just mean that use_key() currently has callers, but is there a reason we couldn't rely on the class always being initialized with the key pair? If not, I think I'd prefer we nuke use_key() altogether and turn it into a constructor argument. There's no reason to carry around uninitialized or half-initialized state if it's not needed. I realize that'd be a breaking change and require a major version bump, so I'm not asking for it here, but it might be better end state to get to.

[TheRealFalcon]

LGTM!

[a-dubs]

Thanks @TheRealFalcon and @jrtknauer for the reviews 🙌