Test suite for storage implementations

[aidansteele]

Hi,

I would love to write a DynamoDb certmagic.Storage implementation (as per #41) - and have done so. What I'm not confident about is whether it correctly implements the interface, e.g. in terms of respective the recursive parameter for List() etc.

Is there a suite of tests that I can run against my implementation to make sure it is correct?

Thank you for such a wonderful package.

EDIT: The work in progress is here. I haven't implemented locking, etc yet. But might be helpful for discussion purposes https://github.com/glassechidna/awscertmagic/blob/master/dynamodb.go

[mholt]

This is awesome! It's something a lot of people have requested.

There is no test suite that you're asking for currently, but that is a good idea! I will see if I can get around to making one, but anyone else is welcomed to contribute one too. 👍

When you're done with your implementation, let's get it added to the wiki and make sure it can be used in Caddy, too.

[DisposaBoy]

I've implemented a badger-based Storage at https://github.com/oyato/certmagic-storage-badger. I'll add it to the wiki once I've deployed it to production.

Since I couldn't find any tests for the storage, I've put ours in another repo at https://github.com/oyato/certmagic-storage-tests. Maybe it's useful as a base for a dedicated certmagic test suite.

For now, the semantics are based on what the FileSystem Storage does as opposed to what Storage documents because it seems inconsistent and/or imprecise.

e.g. for Delete it just says Delete deletes key. but it doesn't mention that a non-existent key should return an error. This one in particular is important because in some implementations it means there needs to be an additional lookup first.

Also the Size field on KeyInfo. From the docs I understand that it's optional, but there are no hints about what it's used for. In the case of badger, it has a fast-path to get an estimated size, so I've used it instead of loading the value (from disk) since it appears that certmagic only uses it for sorting display information and nothing critical.

[DisposaBoy]

@mholt I've improved the tests to the point where certmagic-storage-badger has 100% test coverage and added a test for FileStorage... but FileStorage is failing for this case for List:

Store(dir/k1)
Store(dir/k/2)
List(dir, false) returns [dir/k1, dir/k]

My understanding is that it should only return [dir/k1] i.e. it returns all names in the directory, including sub-directories. I expected it to return only files esp. since I didn't set a key for dir/k.

[mholt]

@DisposaBoy Great, thanks for the update, and the questions!

e.g. for Delete it just says Delete deletes key. but it doesn't mention that a non-existent key should return an error. This one in particular is important because in some implementations it means there needs to be an additional lookup first.

Good point. I suppose the semantics of Delete imply that if the function returns without error, the key should be gone. Doesn't matter if it already existed, but it should just be gone afterward regardless. How's that sound?

Also the Size field on KeyInfo. From the docs I understand that it's optional, but there are no hints about what it's used for. In the case of badger, it has a fast-path to get an estimated size, so I've used it instead of loading the value (from disk) since it appears that certmagic only uses it for sorting display information and nothing critical.

Stat() and the resulting info struct was used primarily for the LastModified field:

• So lockfiles that were placed could be considered stale if they existed for too long. Recently, locks were upgraded to use "active locking" where the age of the file is irrelevant, and the timestamp of last update is stored inside the file, so LastModified on the key/file itself is no longer used for this purpose, I think.
• To get the most recent among keys in a "directory" (non-terminal key, or prefix). This is still used, but is not very critical, for example: if you don't provide an email address, CertMagic will use the "most recent" one, but if that's not important, then this field doesn't matter much either.

Those are the two that come to mind off the top of my head... so Size is probably not used. If it is not easily available, I wouldn't bother setting it. Maybe just make a note in the docs that your implementation doesn't fill in the Size.

Maybe we could even remove most of that info, I dunno. I figured starting with it was fine and then we could remove it later if we wanted to, before stable 1.0.

I've improved the tests to the point where certmagic-storage-badger has 100% test coverage and added a test for FileStorage...

Nice!

but FileStorage is failing for this case for List:

That might be expected, since you listed dir instead of dir/ -- i.e. "foo" is a prefix of "foobar" as well as "foo/bar".

esp. since I didn't set a key for dir/k.

This is a good point, but I suppose is one of the consequences of using a hierarchical storage system. The presence of dir/k indicate that something exists within that prefix, I think that's good info to know.

[DisposaBoy]

@DisposaBoy Great, thanks for the update, and the questions!

e.g. for Delete it just says Delete deletes key. but it doesn't mention that a non-existent key should return an error. This one in particular is important because in some implementations it means there needs to be an additional lookup first.

Good point. I suppose the semantics of Delete imply that if the function returns without error, the key should be gone. Doesn't matter if it already existed, but it should just be gone afterward regardless. How's that sound?

SGTM; I've relaxed that requirement in the tests and removed it from the badger implementation.

Also the Size field on KeyInfo. From the docs I understand that it's optional, but there are no hints about what it's used for. In the case of badger, it has a fast-path to get an estimated size, so I've used it instead of loading the value (from disk) since it appears that certmagic only uses it for sorting display information and nothing critical.

Stat() and the resulting info struct was used primarily for the LastModified field:

* So lockfiles that were placed could be considered stale if they existed for too long. Recently, locks were upgraded to use "active locking" where the age of the file is irrelevant, and the timestamp of last update is stored inside the file, so LastModified on the key/file itself is no longer used for this purpose, I think.

Great! nothing more for me to do then.

* To get the most recent among keys in a "directory" (non-terminal key, or prefix). This is still used, but is not very critical, for example: if you don't provide an email address, CertMagic will use the "most recent" one, but if that's not important, then this field doesn't matter much either.

Those are the two that come to mind off the top of my head... so Size is probably not used. If it is not easily available, I wouldn't bother setting it. Maybe just make a note in the docs that your implementation doesn't fill in the Size.

Maybe we could even remove most of that info, I dunno. I figured starting with it was fine and then we could remove it later if we wanted to, before stable 1.0.

For now I've left it as-is with the estimated size. If I observe any issues I'll just get the real size - the DB is relatively small so it's probably all cached in memory anyway so it's not like it's slow or anything (FileStorage in /tmp i.e. in memory completes all tests in 1 second while in-memory badger completes in 11 milliseconds)

I've improved the tests to the point where certmagic-storage-badger has 100% test coverage and added a test for FileStorage...

Nice!

but FileStorage is failing for this case for List:

That might be expected, since you listed dir instead of dir/ -- i.e. "foo" is a prefix of "foobar" as well as "foo/bar".

esp. since I didn't set a key for dir/k.

This is a good point, but I suppose is one of the consequences of using a hierarchical storage system. The presence of dir/k indicate that something exists within that prefix, I think that's good info to know.

Makes sense. I've relaxed this requirement in the tests and badger implementation. I'll report if I observe any issues. The previous version's been solid over the last few days.