Fix random secret_provider

[sl4vr]

Resolves #54

• Removed region from ssm to support same interface for all secret providers
• Refactored get_secret filter to avoid using exceptions for flow control (partially)
• Refactored and expanded tests
• Fix random secret length mismatch bug

[sl4vr]

@AFriemann hopefully you don't mind if I could practice python on my spare time using k8t 😉

[AFriemann]

@effect305 absolutely not

[sl4vr]

Awesome, I'll add tests to get_secret later then.

[AFriemann]

Suggested change

	def get_secrets(key: str, default: Any = None) -> Any:
	return CONFIG.get("secrets", {}).get(key, default)
	def get_value(*args: str, default: Any = None) -> Any:
	result = copy.deepcopy(CONFIG)
	for arg in args:
	if result is None:
	break
	result.get(arg)
	return result if result is not None else default

[AFriemann]

my main issue is actually the function name, would also be fine with another suggestion

[sl4vr]

My original idea was to have some sort of safe navigation function for config, which would also support lists. Like this:

def fetch(*keys):
    return dig(CONFIG, *keys)


def dig(target, *keys):
    if len(keys) > 0:
        key = keys[0]
    else:
        return target

    if isinstance(target, dict):
        return dig(target[key], *keys[1:]) if key in target else None
    if isinstance(target, list):
        return dig(target[key], *keys[1:]) if isinstance(key, int) and key < len(target) else None
    return None

But then I considered possibility of import only get_secrets to modules that need access to secrets section of config only and end up with get_secrets function as also a much simpler solution, at least for current config usages.

[sl4vr]

If you insist to use kind of safe navigation function, I would suggest:

• definitely remove default argument as nonintuitive and return None instead. So caller could handle it on his own:

region = config.get_value("secrets", "region") or DEFAULT_REGION

• allow to navigate through lists, since yaml support them. But that's probably comes from ruby background, maybe it's redundant feature.

def get_value(*args: Union[str, int]) -> Any:
    result = CONFIG
    
    for arg in args:
        if isinstance(result, dict):
            result = result.get(arg)
        elif isinstance(result, list):
            try:
                result = result[arg]
            except (IndexError, TypeError):
                return None
        else:
            return None
    return result

Anyway while there's such limited config usage, I would stick to just get_secrets function with only 1 level of lookup as simplest option.

[AFriemann]

Removing the default makes sense, it could be confusing since you'd have to pass it as kwarg and defaults are probably handled on the module itself anyway.

The list navigation I think is unnecessary, would leave that to the caller to handle (you should be expecting a list if there is a list defined and most of the time you can't be sure about the item order anyway).

And no I definitely wouldn't insist on it; think i'd also be fine with a more clear name, it's just confusing to read get_secrets since it evokes the idea of actually getting the secret value, not configuration. How about we drop the get and just go with secrets since it reads better in context?

region = config.secrets("region") or DEFAULT_REGION

[sl4vr]

My idea of get_secrets was to provide same interface as get method of dict provides. Thus its name, it's kinda get method for secrets section of config, "get from secrets" in other words.

If it doesn't make much sense, tbh I would stick to get_value option, just without default, and maybe with better name.

[sl4vr]

Oh, I've just realized that get_secrets looks very similar to helper function itself. Maybe it's really better to go with get_value then.

[sl4vr]

@AFriemann There are still some things to decide about configuration access: type safety, default values (or doesn't suit there, because it overrides False), etc. So I've decided to use get for now as most native and simple way.

[AFriemann]

could just go with an exception here

provider_name = None

try:
  provider_name = config.CONFIG['secrets']['provider'].lower()
except KeyError:
  raise RuntimeError("Secrets provider not configured.")

[sl4vr]

Yeah, it's just kinda artifact from previous implementation.

[AFriemann]

I think it's fine for now