Can you expand on the usage of this resource? It strikes me as odd that it has an optional backend. The usage in the test config imply "use this roletag blacklist on this backend", which I see as "I have a backend, I would like to also have this roletag blacklist attached to this specific backend". The default / optional value here makes me think that this resource could be used without a backend declared at all. Could the two resources be created like so?:

resource "vault_auth_backend" "aws" {
  path = "aws123"
  type = "aws"
}

resource "vault_aws_auth_backend_roletag_blacklist" "test" {
  backend = "aws123"
  safety_buffer = 8600
  disable_periodic_tidy = true
}

Is this config alone valid?

resource "vault_aws_auth_backend_roletag_blacklist" "test" {
  backend = "aws123"
  safety_buffer = 8600
  disable_periodic_tidy = true
}

The optional + default for backend implies to me this resource can stand alone and function without a vault_auth_backend to operate on, but reading the Vault docs I felt otherwise. What am I missing?

That first config is the correct usage. The second config is invalid unless you've already configured the auth backend outside of Terraform. I originally went with the optional+default because that's how the Vault API functions when mounting a backend; it just assumes a default value. And in my head, for whatever reason, carrying that behaviour over here made sense, even though now that we're talking about it, it seems much more intuitive that it be required to be explicit. TL;DR: I agree with you, should be Required instead of Optional+Default.

I'm going to update it here, at the very least. My preference is to also update it everywhere else backend is used as a field, to be consistent. Do you have a strong preference either way?

Does Vault provide defaults for disable_periodic_tidy or safety_buffer? Assuming these are the docs:

• https://www.vaultproject.io/docs/auth/aws.html

It seems periodic tidying is on by default, and safety_buffer is by default 72. My concern here is that without defaults, once enabled or added to the configuration, removing them will have no effect if those values were set to something besides the default, e.g. disable_periodic_tidy is false by default in this context (because periodic tidying is on by default), so if a user were to mark it true in the configuration, apply it, then later change it to false, there would be no change detected by Terraform.

It does provide defaults, and disable_periodic_tidy defaults to false and safety_buffer defaults to 72h, as you've noted. So yeah, this is a bug, good catch.

I'll add a default value to both fields. This does mean overriding the Vault default value if it ever changes, but in thinking it through, I think it's a more predictable user experience than GetOkExists+Computed.

disable_periodic_tidy needs a default value specified, or we need to use GetOkExists here otherwise disable_periodic_tidy can never be set to false, and we'll never enter this block

I feel like we should check the error first, and then call d.SetId if the error is nil

if the error returned from awsAuthBackendRoleTagBlacklistBackendFromPath is "no path found", should we remove this item from state?

I can see an argument either way on this one, and it really depends on how we think the ID is more likely to get into an invalid state--from a coding/server error, or from user error during import. If the former, I think having the value remain in state gives us options for workarounds (even though it means manual edits), but the more I think about it, import support means better workarounds are available if we just drop it from state.

TL;DR: after further thought, I agree.

Will Delete return an error that tells us this resource already no longer exists? If so we can catch that error and simply log that and remove from state without this error. This probably isn't 100% required though because you've implemented the Exists method though... I suppose a follow up refresh/plan would remove it from that 🤔

I just tried it with curl, and it appears DELETE is nilpotent. No matter how many times I call it--even if I never configured the blacklist--as long as the backend is actually enabled, the request succeeds.

Unfortunately, catching the 404 returned if the backend doesn't exist would be... tricky. Vault's error returns are basically big fmt.Errorf statements printing everything into human-readable formats. And while we could try to parse out the error, it's going to be brittle.

That said, Exists won't catch this, unfortunately, as it'll run into the same problem. If it tries to check if the configuration exists, that works, but the way the Vault client works, I can't seem to find any way to detect when the backend is missing without resorting to string parsing.

Won't this require a DiffSuppressFunc as well if slashes are included? I don't see a configuration that explicitly tests this condition, but I think it will cause a perpetual diff.

StateFunc should make DiffSuppressFunc unnecessary:

// StateFunc is a function called to change the value of this before
// storing it in the state (and likewise before comparing for diffs).
// The use for this is for example with large strings, you may want
// to simply store the hash of it.

Nitpick: We can reduce the number of these test configurations if we parameterize some of these items like safety_buffer (it also makes it obvious why the resource.TestCheckResourceAttr() values are what they are)

Nitpick: I know at least in AWS provider-land, the backslashes are no longer required for the documentation header

Typo for Optional and it might be worth mentioning the time unit here, e.g. extra time (in seconds)