`NotaryRepository` refactor proposal

[cyli]

NotaryRepository is sort of one big monolithic object with a lot of things on it right now, mostly made together to serve the CLI and may not necessarily make sense for something that uses NotaryRepository as a library. Also, the fact that everything is bound to a single config directory, and a URL, makes testing slightly more inconvenient.

The proposal is to break up NotaryRepository into a two or three composite pieces, and to clean up the package tests to try to inject more in-memory objects as opposed to creating a temp directory, running a whole server so the repo can update, etc. (the integration tests for the CLI should of course continue to do so).

We could keep the function NewNotaryRepository to conveniently create a single repo given a config directory, and possibly with the roundtripper, etc. as well. But it'd be good to have NotaryRepository that can use other implementations of cache/remote stores etc.

Update: This would also address/supersede #932 and #380.

NotaryRepositoryChanger object

I am completely uncommitted to the name, just the first thing that springs to mind. Basically this wraps a changelist.Changelist object and the following functions will be attached to it:

• AddTarget
• RemoveTarget
• GetChangelist (this function maybe can just be removed, because it doesn't make sense)
• AddDelegation
• AddDelegationRoleAndKeys
• AddDelegationPaths
• RemoveDelegationKeysAndPaths
• RemoveDelegationRole
• RemoveDelegationPaths
• RemoveDelegationKeys
• ClearDelegationPaths

As far as I can tell, none of these functions require anything from NotaryRepository except the tufRepoPath field to tell it where to create a new FileChangelist.

In addition #700 suggests that Initialize should also create a set of changes to be applied, as opposed to create some initial metadata.

Proposal 1: completely break up NotaryRepository into 2 objects

1. A NotaryRepositoryInfo object (again, I am completely uncommitted to the name) - this just wraps a tuf.Repo object and just gets random useful information on it. The following functions will be attached to it (and they will stop calling Update - the user should explicitly call something to obtain the latest on the repo first, and then we can get all sorts of information from the repo without having to re-download new data every time):

   • ListTargets
   • GetTargetByName
   • GetAllTargetMetadataByName
   • ListRoles
   • GetDelegationRoles

   A constructor probably only take a tuf.Repo, and maybe a GUN just for informational purposes.

2. A NotaryRepositoryWriter object (can you tell I'm terrible at naming yet?) - this object is responsible for getting the latest tuf.Repo object from a remote store, applying a changelist to it, and then publishing. The following functions will be attached:

   • RotateKey
   • Publish (which should now take a changelist.Changelist object as an argument; the unexported private publish function already does)
   • DeleteTrustData

   A constructor should probably only take a store.MetadataStore to use as a cache, store.RemoteStore to use to update the tuf.Repo object, a signed.CryptoService for signing, and maybe a GUN for informational purposes.

Other changes:

• bootstrapClient and Update: I think can these be rolled into TUFClient, and TUFClient can be a helper object that will download you the latest version of a tuf.Repo, which can be used to construct a NotaryRepositoryInfo object or used byNotaryRepositoryWriter. Alternately, maybe TUFClient and NotaryRepositoryInfo can be the same object, as the comment for it suggests it might be.
• Initialize: If we don't want to move this to NotaryRepositoryChanger, maybe it can be a standalone function that takes a store.MetadataStore to write the initialized metadata files to and a signed.CryptoService for generating keys and for signing? It can return a NotaryRepositoryWriter object, since if it's empty, there's probably no point in reading anything from it.

  Proposal 2: keep the NotaryRepository object, but change function signatures

The NotaryRepository object will remain an object with both read and write functionality, and without all the NotaryRepositoryChanger functions.

However, the constructor will only take a store.MetadataStore to use as a cache, and a GUN for informational purposes.

The following function signatures would change:

• Initialize should also take a store.RemoteStore and a signed.CryptoService, if we don't want to move it to NotaryRepositoryChanger
• RotateKey should also take a store.RemoteStore and a signed.CryptoService
• Publish should also take a changelist.Changelist, a store.RemoteStore and a signed.CryptoService
• DeleteTrustData should also take a store.RemoteStore

I think it would still be awesome to roll bootstrapClient and Update into TUFClient, and TUFClient.

[cyli]

cc @endophage @riyazdf @HuKeping @ecordell

[HuKeping]

also see https://github.com/docker/notary/issues/932

[cyli]

@HuKeping Ah yes, thanks, I was looking for that one and for some reason my eyes kept skipping over it.

[cyli]

Related, but not part of this particular proposal: I just wanted to leave a breadcrumb here since the previous issues about this were already closed.

@ecordell made the awesome suggestion of making some of the parts of notary more "batteries removable", such as the x509 requirements: https://github.com/docker/notary/issues/794#issuecomment-231818928, https://github.com/docker/notary/issues/977#issuecomment-251482246

Also, https://github.com/theupdateframework/taps provides some nice augmentation proposals that will maybe make it into TUF, but it would be nice to have cleaner interfaces for library writers to plug in extra features that they might want to support.

I don't really have any concrete proposals about this yet, so I didn't open a new issue, but since we're discussing some of the refactoring I thought I'd mention it as well in case anyone had any good ideas.