Add dependency injection of remote store for NotaryRepository initial… #1094

n4ss · 2017-02-10T17:22:21Z

…ization

Signed-off-by: Nassim 'Nass' Eddequiouaq [email protected]

cyli · 2017-02-10T18:59:05Z

client/client.go

+
+// NewNotaryRepository is the base method that returns a new notary repository.
+// It takes the base directory under where all the trust files will be stored
+// (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling


Non-blocking: Perhaps these should be listed as examples rather than normal defaults, since in this particular function there is no default for the baseDir (these are just defaults for the CLI tool that uses it and in docker, which uses it)

(Apologies, I know this wasn't your change, I just hadn't noticed it before)

cyli · 2017-02-10T19:04:45Z

client/client.go

+// (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling
+// docker content trust).
+// It expects initiated remote store and cache.
+func NewNotaryRepository(baseDir, gun, baseURL string, remoteStore store.RemoteStore, rt http.RoundTripper, cache store.MetadataStore,


I think maybe the RoundTripper object may no longer be necessary when store.RemoteStore is passed. NotaryRepository.roundTrip only seems to be passed to the getRemoteStore, getRemoteKey, and rotateRemoteKey helper functions. getRemoteKey and rotateRemoteKey just use it in order to call getRemoteStore, so they can probably just be passed the remote store instead?

Had this in my todo list and skipped it somehow.. will fix.

cyli · 2017-02-10T19:08:43Z

client/client.go

-	if err != nil {
-		return err
-	}
+	remote := r.remoteStore


Since this is only used in the line below, would it make sense to just call r.remoteStore.SetMulti(updatedFiles)?

Actually, wondering if we should check if this is nil, first, and error if so? Sometimes we do not require the client to be online (if we're just reading from cache, for example), but write operations on the remote store definitely require us to be online.

Wait never mind, ignore me - I think we use OfflineStore in this case. Ok, that was why I think we used getRemoteStore everywhere - if there was an error, we return OfflineStore + the error, and in some cases, we fail the operation if there was an error.

In the case of downloading new data, we ignore the error and just use the OfflineStore because we can maybe read from cache.

This means that https://github.com/docker/notary/pull/1094/files#diff-1cd41a2611da49ce63630d5a51bd82c5R78 may not be the right behavior. However, if we don't propagate the error, we'd lose it.

Every operation from OfflineStore will result in a generic offline error, though, so wondering if we should update OfflineStore to take an error it can wrap, and propagate the original error that way?

cc @endophage

good catch @cyli: I agree we need to wrap the OfflineStore and also have a check for nil here

Changes I've suggested to @n4ss:

In NewNotaryRepository check if remoteStore is nil and instantiate an OfflineStore if it is.

Create a getter for remoteStore that will do the same check and also initialize an OfflineStore if necessary.

re. the behaviour at https://github.com/docker/notary/pull/1094/files#diff-1cd41a2611da49ce63630d5a51bd82c5R78, I believe this is correct. The pre-existing behaviour was to directly return the error if getRemoteStore errored (i.e. https://github.com/docker/notary/pull/1094/files#diff-1cd41a2611da49ce63630d5a51bd82c5R78`), we're now simply detecting and aborting sooner.

@endophage: that sounds good 👍

The pre-existing behaviour was to directly return the error if getRemoteStore errored...

That is true of these two previous calls to getRemoteStore:

https://github.com/docker/notary/pull/1094/files#diff-1cd41a2611da49ce63630d5a51bd82c5L689

https://github.com/docker/notary/pull/1094/files#diff-1cd41a2611da49ce63630d5a51bd82c5L1149

But this one just logged the error and proceeded: https://github.com/docker/notary/pull/1094/files#diff-1cd41a2611da49ce63630d5a51bd82c5L973

cyli · 2017-02-10T19:18:36Z

client/client.go

-		if err != nil {
-			return err
-		}
+		remote := r.remoteStore


Similarly to above, should we check that r.remoteStore isn't nil first, and then if it isn't, we can also just call r.remoteStore.RemoveAll()?

riyazdf · 2017-02-10T19:42:13Z

client/client.go

+// It takes the base directory under where all the trust files will be stored
+// (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling
+// docker content trust).
+// It expects initiated remote store and cache.


s/initiated/initialized

Following on from the points that @cyli mentioned - even if we expect the initialized store/cache, we should check the values in case someone accidentally calls this with nils. Would be great to have unit tests for these cases.

Also, maybe this maybe shouldn't have to be exported anymore? cc @endophage

riyazdf · 2017-02-10T19:43:36Z

client/client.go

+// It expects initiated remote store and cache.
+func NewNotaryRepository(baseDir, gun, baseURL string, remoteStore store.RemoteStore, rt http.RoundTripper, cache store.MetadataStore,
+	trustPinning trustpinning.TrustPinConfig, cryptoService signed.CryptoService) (
+	*NotaryRepository, error) {


super nit: this signature is kind of hard to read - not sure if you chose it or if gofmt imposed it to be this way. Maybe we could move the return values up a line such that we have 2 lines instead of 3?

riyazdf · 2017-02-10T19:44:51Z

client/client.go

-	if remoteErr != nil {
-		logrus.Error(remoteErr)
-	} else if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized {
+	remote := r.remoteStore


same as @cyli's comment below:

Similarly to above, should we check that r.remoteStore isn't nil first, and then if it isn't, we can also just call r.remoteStore.RemoveAll()?

riyazdf · 2017-02-10T23:50:18Z

client/client.go

-	if err != nil {
-		return err
-	}
+	remote := r.remoteStore


good catch @cyli: I agree we need to wrap the OfflineStore and also have a check for nil here

riyazdf · 2017-02-10T23:52:40Z

client/client.go

@@ -17,7 +17,6 @@ import (
 	"github.com/docker/notary/client/changelist"
 	"github.com/docker/notary/cryptoservice"
 	store "github.com/docker/notary/storage"
-	"github.com/docker/notary/trustmanager"


wonder how this passed the gofmt before

trustmanager was in use at line 92 prior to this PR.

n4ss · 2017-02-13T19:00:12Z

Sorry, I pushed only the first commit by mistake - pushing the rest soon.
Please wait before reviewing again as all as everything is not fixed yet.

endophage · 2017-02-19T18:45:22Z

Made a couple of updates:

changelists are now injectable (it's still fine to create an in memory changelist within specific operations but the file based changelist is injected).
DeleteTrustData is no longer defined on the repo, it's just a function now. There were errors we didn't care about but still had to deal with by initializing a repo for this operation.
The trust pinning test that triggers an error no longer covers the delete operation as delete no longer requires trust pinning (this is one of the errors we didn't care about but still had to deal with).

endophage · 2017-02-19T18:46:44Z

Also, I'd like to remove baseDir off the NotaryRepository but the tests rely on it heavily. I have an idea how to fix it without massive changes to the tests but it's not in scope for this PR.

The idea is create the following struct and use it at a few key points in the tests to wrap NotaryRepository:

type TestNotaryRepository struct {
    NotaryRepository
    baseDir string
}```

endophage · 2017-02-19T18:48:27Z

@riyazdf @cyli thought on the remoteStore nil check. What about if we use a getter for remoteStore and have the getter always return an offline store if no other store was set?

riyazdf · 2017-02-21T17:50:59Z

@endophage - I think that logic for a remoteStore getter makes sense, would that getter be defined on the NotaryRepository?

cyli · 2017-02-21T18:06:21Z

@endophage That would be fine - would we care about propagating the error that caused the remote store to be nil? In some cases we want to error right away, and propagating why we couldn't get a remote store would be nice. Passing just nil as the remote store to the constructor would not propagate this error.

endophage · 2017-02-21T18:56:03Z

@cyli I feel like as long as we log the error visibly, not just when running in debug, that's probably sufficient. A user doesn't necessarily care when in the flow an error gets emitted as long as there's sufficient information for them to understand what went wrong.

If that seems like a reasonable assumption, then we don't need to add the complexity of propagating the error until we might want to expose it.

endophage · 2017-02-21T18:56:56Z

@riyazdf yeah, I was thinking it would be defined on NotaryRepository. It would just check the field and if it isn't nil, return the value, if it is nil, return an OfflineStore

cyli · 2017-02-21T19:04:19Z

@endophage That seems reasonable - would we still need a remote store getter in that case? There is only one place where we need an offline store - in the other two cases we just seem to return an error if the remote store is nil.

endophage · 2017-02-21T22:21:08Z

@cyli on the basis that somebody could directly initialize a NotaryRepository without using a constructor function, it's probably still a good practice to use a getter.

cyli · 2017-02-21T23:19:10Z

@endophage Oh I mean returning OfflineStore won't be useful for 2 of the cases where we want to get the remote store - nil should error. Only in one case would we actually want the OfflineStore. If the user initializes a NotaryRepository directly without the constructor function, that just means there's no error to propagate/log, but the behavior of NotaryRepository with and without an actual remote store should be the same.

endophage · 2017-02-22T19:33:30Z

@cyli I figured the OfflineStore would cause the desired behaviour by erroring when we need it to, rather than having additional preemptive checks on whether the NotaryRepository.remoteStore was nil.

cyli · 2017-02-22T22:23:25Z

@endophage Ah good point, you're right. For some reason I was thinking we'd return a different error like "invalid remote" but "offline" would be fine so long as that other error was logged.

cyli

LGTM

riyazdf

LGTM

riyazdf · 2017-02-23T23:35:43Z

client/client.go

 	"github.com/docker/notary/trustpinning"
 	"github.com/docker/notary/tuf"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
 	"github.com/docker/notary/tuf/utils"
+	"os"


super nit: weird that this got formatted to here in the import list

riyazdf · 2017-02-23T23:38:35Z

client/client.go

-	if err != nil {
-		return err
-	}
+	remote := r.remoteStore

 	return remote.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles))


nit: return r.remoteStore.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles))

(no need for the extra remote var)

cyli

Ack, sorry, I totally forgot about @endophage's suggestion about the getter and logging the remote store error.

…ization Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

…tiated Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

… or an OfflineStore Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

cyli · 2017-02-27T19:35:24Z

client/client.go

-		keyStores, trustPinning)
+	cryptoService := cryptoservice.NewCryptoService(keyStores...)
+
+	remoteStore, err := getRemoteStore(baseURL, gun, rt)


Since we still seem to be using this initializer in the CLI for both reading notary data and publishing/initializing notary data, would it make sense to log this error instead? That way later the actual NotaryRepository object itself can decide whether to fail if it's offline (in the case of publishing and initializing) or continue reading from cache?

It makes sense to me, will fix!

I actually added the error logging directly inside getRemoteStore without being too sure about this decision.. Should we let caller decide on wether to logging instead? It seems more reasonable.

Yeah, let the caller decide on logging.

(but in this specific case, always log the error)

@cyli actually we should probably log when we get an OfflineStore only from getRemoteStore I think. If we get an err from getRemoteStore and hence from NewHTTPStore, it means that baseURL was incorrect, not that it was unreachable.

From your comment in slack:

We have the following behavior for NewHTTPStore:

if we have an invalid url from baseURL+gun then we get a nil store and a non-nil err -> this should error in callers, not switched to an OfflineStore

That makes sense to change this (from master, it seem sot already do so in this PR) to error on actual invalid values.

if we have an invalid http.roundtrip, we get an OfflineStore and a nil err -> this should be logged in callers

If we were looking just at NewHTTPStore, I'm wondering if we should actually convert a nil http.RoundTrip to a http.DefaultTransport, or count it as invalid input and return an error (I don't feel strongly either way).

But in our usage of this function, we seem to set the roundtrip to nil when doing offline operations, namely updating the changelist, so we don't want it to error or probably care about it logging. I'd argue that eventually we should probably just directly call NewNotaryRepository instead with a nil remote store or something. I think it doesn't even need the cache, or trust pinning, or a cryptoservice, so those could probably also all be nil.

In the meantime (unless we wanted to make that change here), since our usage is specifically "a nil http.Roundtrip means this is intended to be an offline operation", maybe we should explicitly add a parameter to denote that, or just ignore invalid roundtrips?

we have a valid HTTPStore -> all good

We don't check if the remote is actually reachable

👍

cyli · 2017-02-27T19:36:17Z

client/client.go

+
+// GetRemoteStore returns the remoteStore of a repository if valid or
+// or an OfflineStore otherwise
+func (r *NotaryRepository) GetRemoteStore() (store.RemoteStore, error) {


This function doesn't seem to need to return an error.

Also non-blocking nit: I'm not sure this needs to be exported, but if it does, would just RemoteStore() make sense as kind of an accessor function?

I agree on returning error, if it's not necessary, don't do it. Every unnecessary error return demands 3 lines of code to check it :-P

Also agree on exporting. If it doesn't need to be part of the interface, don't export it because then somebody might rely on it and we could break them if we change it in the future.

Good point!

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

…it on error or not Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

endophage

LGTM!!!

endophage · 2017-03-01T01:11:26Z

@cyli when you get a chance, can you do final review please. Don't want to merge while you're still an X but I think everything is now resolved.

cyli · 2017-03-01T03:17:52Z

client/client.go

+	remoteStore, err := getRemoteStore(baseURL, gun, rt)
+	// baseURL is syntactically invalid
+	if err != nil {
+		logrus.Error(err)


Non-blocking: I'm not sure we have to log as well as returning the error.

Also it looks like in the CLI, tuf.go's tokenAuth function also parses the URL and checks for validity, which is probably a better validity check than NewHTTPStore's, since it looks like a url like http:asdga gets through the url.Parse + checking for the existence of a scheme?:

$ bin/notary -D -s https:asdgadg list repo DEBU[0000] Configuration file not found, using defaults DEBU[0000] Using the following trust directory: /Users/cyli/.notary ERRO[0000] could not reach https:asdgadg: Get https:///v2/: http: no Host in request URL INFO[0000] continuing in offline mode DEBU[0000] No yubikey found, using alternative key storage: loaded library /usr/local/lib/libykcs11.dylib, but no HSM slots found DEBU[0000] Making dir path: /Users/cyli/.notary/tuf/repo/changelist * fatal: client is offline

We can probably fix that in a later PR, though.

Just fixed it right now in a last small commit

Awesome, thanks! That was my only comment! This otherwise looks good to merge on green!

cyli

Other than a non-blocking nitpick about a log and returning an error both, this LGTM!

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

endophage · 2017-03-01T18:43:32Z

I'm not waiting on codecov to get worked out on this. It was OK on test coverage before and I don't see any updates that should significantly change that.

n4ss requested review from cyli, endophage and riyazdf February 10, 2017 17:22

GordonTheTurtle added the dco/no label Feb 10, 2017

n4ss force-pushed the cli-grpc-refactor branch from 82679a8 to 2ae0cf0 Compare February 10, 2017 18:33

GordonTheTurtle removed the dco/no label Feb 10, 2017

cyli reviewed Feb 10, 2017

View reviewed changes

riyazdf reviewed Feb 10, 2017

View reviewed changes

endophage force-pushed the cli-grpc-refactor branch 2 times, most recently from 7927e96 to 7921e31 Compare February 19, 2017 19:09

endophage force-pushed the cli-grpc-refactor branch from 7921e31 to 136a8cf Compare February 22, 2017 21:22

endophage added the Next-To-Merge label Feb 23, 2017

cyli approved these changes Feb 23, 2017

View reviewed changes

riyazdf approved these changes Feb 23, 2017

View reviewed changes

cyli suggested changes Feb 24, 2017

View reviewed changes

n4ss mentioned this pull request Feb 24, 2017

Notary repository lazy initialization #1105

Merged

n4ss and others added 7 commits February 25, 2017 03:05

Add dependency injection of remote store for NotaryRepository initial…

ddfe727

…ization Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Remove helper and test for changing remote store base URL once instan…

e4cacd0

…tiated Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Remove the RoundTripper object out of NotaryRepository

ae43610

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

making changelist injectable

7d8ec9f

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

fixing an ineffassign error

1070d54

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

Minor fixes from reviews

e8d8eae

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Add a getter for RemoteStore of a repo returning a valid remote store…

144a9ad

… or an OfflineStore Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

n4ss force-pushed the cli-grpc-refactor branch from 136a8cf to 144a9ad Compare February 25, 2017 02:17

Minor gofmt fix

1d03a50

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

cyli reviewed Feb 27, 2017

View reviewed changes

n4ss added 4 commits February 28, 2017 01:58

Remove unncesserary error return value from repo's remoteStore getter

9b5aeb7

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Let getRemoteStore helper function caller decide wether to log and ex…

10577ed

…it on error or not Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Remove export of repo's remoteStore getter

1afae72

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

Error on invalid baseURL for a remote store

0b3f3cb

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

endophage approved these changes Mar 1, 2017

View reviewed changes

cyli reviewed Mar 1, 2017

View reviewed changes

cyli approved these changes Mar 1, 2017

View reviewed changes

Remove unnecessary error logging on invalid repo baseURL

b70d671

Signed-off-by: Nassim 'Nass' Eddequiouaq <[email protected]>

endophage merged commit be4f33e into notaryproject:master Mar 1, 2017

riyazdf removed the Next-To-Merge label Mar 1, 2017

endophage mentioned this pull request Mar 2, 2017

Improve Dependency Injection for NotaryRepository initialization #932

Closed

Add dependency injection of remote store for NotaryRepository initial… #1094

Add dependency injection of remote store for NotaryRepository initial… #1094

Conversation

n4ss commented Feb 10, 2017

cyli Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n4ss commented Feb 13, 2017

endophage commented Feb 19, 2017

endophage commented Feb 19, 2017 • edited Loading

endophage commented Feb 19, 2017

riyazdf commented Feb 21, 2017

cyli commented Feb 21, 2017

endophage commented Feb 21, 2017

endophage commented Feb 21, 2017

cyli commented Feb 21, 2017

endophage commented Feb 21, 2017

cyli commented Feb 21, 2017

endophage commented Feb 22, 2017

cyli commented Feb 22, 2017

cyli left a comment

Choose a reason for hiding this comment

riyazdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

cyli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endophage left a comment

Choose a reason for hiding this comment

endophage commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli left a comment

Choose a reason for hiding this comment

endophage commented Mar 1, 2017

cyli Feb 10, 2017 •

edited

Loading

riyazdf Feb 10, 2017 •

edited

Loading

endophage commented Feb 19, 2017 •

edited

Loading

riyazdf Feb 23, 2017 •

edited

Loading

cyli Feb 28, 2017 •

edited

Loading