Fix a case where mounts could be duplicated

[jefferai]

When unmounting, the router entry would be tainted, preventing routing.
However, we would then unmount the router before clearing storage, so if
an error occurred the router would have forgotten the path. For auth
mounts this isn't a problem since they had a secondary check, but
regular mounts didn't (not sure why, but this is true back to at least
0.2.0). This meant you could then create a duplicate mount using the
same path which would then not conflict in the router until postUnseal.

This adds the extra check to regular mounts, and also moves the location
of the router unmount.

This also ensures that on the next router.Mount, tainted is set to the
mount entry's tainted status.

Fixes #6769

[jefferai]

Note for reviewers: The test case in external_tests is just for completeness. The main test is the modifications to the one in mount_test.go. I've exercised these individually to test that they fail without the changes and don't fail with by running this test with the rest of it git stashed and that both checks work by commenting out the first addition to mount.go and ensuring it still fails.

[mgritter]

I spend some time reading this; LGTM.

The placement of Unmount is after writes to both "view"and to the MountTable. I think it should therefore handle errors in either place, but the unit test exercises only the first write failing.

[jefferai]

@mgritter Yeah that's what I was getting at in my note above. I tested that manually by commenting out the code in the first check and it still guarded as intended. But without adding test flags or something pretty ugly there isn't a great way in tests to bypass that initial check.

[tstoermer]

@jefferai please let me know if you appreciate some retests on our side with the fixes on this branch. I could check to get a test environment prepared.

[jefferai]

Sure, that would be great!

[tstoermer]

Hi @jefferai
I did a retest with docker image from your branch and it was successful.

The vault server log shows now:

2019-05-29T07:22:27.090Z [ERROR] core: failed to clear view for path being unmounted: error="list failed at path "": Get https://127.0.0.1:8500/v1/kv/vault/logical/a7a696af-1f11-19e7-4059-c03b351baf39/?keys=&separator=%2F: context canceled" path=pki-tsr-1/
2019-05-29T07:22:27.090Z [ERROR] secrets.system.system_ca91d9d4: unmount failed: path=pki-tsr-1/ error="list failed at path "": Get https://127.0.0.1:8500/v1/kv/vault/logical/a7a696af-1f11-19e7-4059-c03b351baf39/?keys=&separator=%2F: context canceled"
2019-05-29T07:22:27.094Z [INFO]  expiration: revoked lease: lease_id=pki-tsr-1/issue/tsr-client/zzyzJh3MB1CWV3TqzJeI4yo8
2019-05-29T07:22:59.408Z [INFO]  core: successfully unmounted: path=pki-tsr-1/ namespace=

... [perform recreate pki]
2019-05-29T07:23:43.105Z [INFO]  core: successful mount: namespace= path=pki-tsr-1/ type=pki

The mount gets cleaned up properly. After recreating same PKI the restart of vault server is also successful (no duplicate mount error).

Just one thing I forgot to mention in the ticket: terraform triggers multiple delete operations (I assume because of request timeout). I assume from vault perspective, proper timeouts should be used in order to prevent multiple delete calls?

Thanks for the fix!

[jefferai]

@tstoermer Thanks for checking back! I think in the normal case the TF thing won't be a problem, just happened because you were in a weird state.

[mjarmy]

I reviewed these changes, and they LGTM