[BUG] 3005 vault.read_secret changes are slightly problematic

[Oloremo]

Description

vault modules were updated in 3005 to change their default behavior of the... default parameter: https://github.com/saltstack/salt/pull/61089

There are several issues with that change:

1. Change is not documented. Nothing states that if the default is None it will lead to the error raise.
2. Changing the defaults should be done very carefully and usually with some kind of deprecation policy. People could rely on defaults. Like we used default=None as a secret default for the vault! It just makes sense to us.

Expected behavior I do not fully understand the issue that was attempted to be fixed here so I can't really suggest the change here.

[ITJamie]

+1 on this. None is a sane fallback incase a vault item cant be read. It allows us to easily gate out code in states as we can easily do {% if vault_item_var %}

+1 on needing a deprecation warning. This is a common usecase (at least from my experience across a few companies that use salt + vault)

[Oloremo]

@OrangeDog technically it's not only about the vault, pillars.get affected as well.

[OrangeDog]

@Oloremo the linked PR didn't change the pillar behaviour. It's still KeyError by default.

[Oloremo]

It's not, it became None by default with None leading to KeyError, same as with vault modules.

So you now can't do:

pillar.get nonexisting_key default=None

And you can do it in 3004

[OrangeDog]

I see what you mean. You are correct - that is a breaking change.

[twangboy]

The reason this was changed is because we were passing KeyError as the default to the function. This was causing a serialization error as you can't serialize type objects. So, we set it to None and if it is None, then the default becomes KeyError.

I don't understand your example saying that you can't do pillar.get nonexisting_key default=None. Since the default is None I don't see why that is a breaking change. Maybe I'm misunderstanding something.

[OrangeDog]

Because then it will raise KeyError instead of returning None.

[waynew]

nice catch!

I see what the problem is, and it was a case that we hadn't really considered - namely that folks would want a default=None. On the subject of deprecation for default args - under normal circumstance I'd agree, but the existing defaults are actually bugs.

In this case what we probably want to do is have a salt.common.SPECIAL_DEFAULT or something that we can use that's literally just SPECIAL_DEFAULT = object(). I think that would solve the serializable issue as well as give us the correct fallback behavior.

[Ch3LL]

Closed by https://github.com/saltstack/salt/pull/62458