These func UndeleteSecret(path string, versions []int) methods should return an error for better audit tracking

[v0lkan]

No description provided.

[sahinakyol]

@v0lkan assign

[v0lkan]

@sahinakyol let me do some cleanup and then add more context here and then I'll assign :).

Thanks for your help 🙏

[v0lkan]

Some more context:

There is a key-value store implemented here: app/nexus/internal/state/store

The issue with that is, its methods are inconsistent.

For example Undelete returns an error when a secret is not found, but Delete does not return such an error.

We have a custom ErrSecretNotFound (that Undelete uses but other methods don't)

Here is the full list of methods:

• func (kv *KV) Delete(path string, versions []int)
• func (kv *KV) Get(path string, version int) (map[string]string, bool)
• func (kv *KV) List() []string
• func (kv *KV) Put(path string, values map[string]string)
• func (kv *KV) Undelete(path string, versions []int) error

For consistency, the signatures could better be as follows:

• func (kv *KV) Delete(path string, versions []int) error // err out if there is no secret to delete.
• func (kv *KV) Get(path string, version int) (map[string]string, error) // return ErrSecretNotFound if there is no secret to get; return ErrSecretSoftDeleted if the secret is marked as "deleted"
• func (kv *KV) List() []string // no change
• func (kv *KV) Put(path string, values map[string]string) // no change
• func (kv *KV) Undelete(path string, versions []int) error // no change.

That's part one.

Part two is the methods that consume these functions.

For example

func UndeleteSecret(path string, versions []int) {
	kvMu.Lock()
	err := kv.Undelete(path, versions)
	kvMu.Unlock()

	if err != nil {
		return
	}

	persist.AsyncPersistSecret(kv, path)
}

^ it would be nice if this (and other similar functions) return the error instead...

So that this can catch the error and instead of this

	state.UndeleteSecret(path, versions)
	log.Log().Info("routeUndeleteSecret", "msg", "Secret undeleted")

it could say:

	err := state.UndeleteSecret(path, versions)
	if err.equals(ErrSecretNotFound) {
		log.Log().Warn("routeUndeleteSecret", "msg", "Secret not found")
	} else {
		log.Log().Info("routeUndeleteSecret", "msg", "Secret undeleted")
	}