Make generic secrets importable.

[paddycarver]

This involves turning "allow_read" into "disable_read", because we have
no way to set "allow_read" to true when importing, and we can't import
if we can't read. This requires a migration and will require users to
update their config files.

[catsby]

Can you elaborate on why we can't set allow_read? I'd like to understand more before approving a breaking change.

Thanks!

[paddycarver]

So, from my digging, here's what I understand about this:

• generic_secret originally didn't allow reading of any kind. It was a write-only resource, and ignored any drift. This was to support creating things that Terraform didn't have permission to read--it could create the resource, but not view it. I assume this was done to allow for security postures.
• Feature/allow vault resource to update terraform#11776 updated this to add the allow_read field, which can be set in the config to allow the provider to read the secret back from the server. It defaulted to false.
• When importing, we have no config file to work with, and so have no option beyond weird ID hacks to tell the provider that it's ok to try and read this.
• One solution I thought about was to just attempt to read, catch the error for access denied, and not fail on it. But then legitimate errors get swallowed, and Terraform can't be told not to read the secret back even if it could. Which is a use case currently supported.
• The way I thought of around this was to switch the default; by default, the provider can read the secret. But to support the use cases that are currently supported, you can tell it not to. We just switch the default, so import doesn't need to worry about it at all.

If you have thoughts or ideas about how to solve the problem without a breaking change, or with a softer breaking change, I'm definitely not wed to this solution. It was just the cleanest way forward that I saw.

[catsby]

Sounds reasonable, my only thought is regarding "But then legitimate errors get swallowed" .. do we know what legit errors though could be? Is there anyway we can examine the error and then decide based on that? Thinking 404 vs 403 http error codes. I don't know what Vault would return code-wise for "no you can't" vs. "something went wrong" etc.

I trust your judgement here, so consider trapping if it's possible and logging that we weren't allowed to read, or otherwise merge as is 👍

[paddycarver]

That latest push updates things to just deprecate allow_read instead of hard-removing it. This means configs won't break on upgrade. Upgrade strategies:

• allow_read set to false: plan shows a disable_read change from true to false. This is because disable_read isn't set in the config. Remedy: add disable_read to the config, set to true.
• allow_read not set: plan shows a disable_read change from true to false. This is because disable_read isn't set in the config. Remedy: add disable_read to the config, set to true.
• allow_read set to true: plan shows no change.

In all these scenarios, a deprecation warning is shown. So to get the behaviour they want, users still need to make a config change, but I don't see any way around that, and this at least is a better UX than just breaking outright.

@catsby, if you think this is still good, I'll go ahead and merge it.

Updated strategy for upgrades.