Enabled Vault clients to revoke leases using the revoke-prefix endpoint

spring-projects / spring-vault

Provides familiar Spring abstractions for HashiCorp Vault

https://spring.io/projects/spring-vault

Apache License 2.0

283 stars 186 forks source link

Enabled Vault clients to revoke leases using the revoke-prefix endpoint #590

Closed ThomasKasene closed 3 years ago

ThomasKasene commented 4 years ago

Changes made to LeaseEndpoints:

Added LeasesRevokedByPrefix which uses the /sys/leases/revoke-prefix/{prefix} endpoint when revoking leases
Added Leases which work just like SysLeases, ie. it uses the /sys/leases/revoke endpoint when revoking leases
Deprecated SysLeases in favour of Leases
Did various refactoring to reduce the amount of code duplication

Closes #588.

mp911de commented 4 years ago

The pull request looks pretty solid. I'd like to keep the implementations as-is meaning not extracting common bits into methods to keep things simple and more readable (and together). It would also make sense to extend the tests, specifically for SecretLeaseContainerUnitTests since we're quite lenient in the tests right now.

ThomasKasene commented 4 years ago

Hmm, SecretLeaseContainerUnitTests mocks out everything at the moment though. Should I change all that or just try to make a LeaseEndpointsUnitTests instead?

I can put the code back to its original state, but does that also include the implementation for SysLeases, or should that still use the implementation from Leases to emphasize that it's the same thing?

By the way, I noticed the argument list for the old revocation implementation includes lease.getLeaseId() but that shouldn't be necessary, should it? The lease ID will be passed into the PUT request body:

operations.exchange(endpoint, HttpMethod.PUT, LeaseEndpoints.getLeaseRevocationBody(lease), Map.class,
        lease.getLeaseId());

ThomasKasene commented 4 years ago

I gambled and added LeaseEndpointsUnitTests - it might be useful regardless of whatever should be done about SecretLeaseContainerUnitTests, if anything.

While I looked around in SecretLeaseContainerUnitTests I took the liberty of replacing the usages of Mockito.verifyZeroInteractions with Mockito.verifyNoInteractions because the former is deprecated. I still don't know how to improve this test class with regards to the changes made to LeaseEndpoints, though.

I also reverted the refactoring in LeaseEndpoints and assumed you wanted me to copy the contents from Leases into SysLeases rather than calling Leases.renew and Leases.revoke.

mp911de commented 3 years ago

Thank you for your contribution. That's merged and polished now.