Flag locally created logs as direct #374

carsonfarmer · 2020-06-10T23:36:12Z

Fixes #373, and a better alternative to #372.

The approach here is to simply take advantage of the existing metadata store on the logstore to set a flag as to whether a given log is "managed?" by the local peer. Directly "managed" logs are those logs created or directly added by the local peer. Otherwise, unmanaged logs are those from other peers in a given thread. Related, "owned" logs are those logs for which the peer has the private key. There can only be one "owned" log, versus potentially many "managed" logs.

This should support multiple use cases:

The local peer is the client peer. One "owned" log is used to interact with a given thread (single-tenant, single-writer setup)
The local peer is (also) a relay peer. Maybe multiple "managed" logs on behalf of other clients, and zero or one "owned" log to also participate in the thread on its own (multi-tenant, multi-writer setup)
The local peer is (also) a proxy-writer. Maybe multiple "managed" logs on behalf of other clients, and one "owned" log that is written to on behalf of other 'agents' (multi-tenant, single-writer setup)

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

core/logstore/logstore.go

core/thread/id.go

logstore/logstore.go

net/net.go

net/api/pb/threadsnet.proto

core/thread/id.go

net/net.go

sanderpick

Looking good! Some comments and questions...

core/logstore/logstore.go

core/thread/id.go

logstore/logstore.go

net/net.go

carsonfarmer

Thanks for the comments @sanderpick. I'm going to address these in the AM, but I think nothing outstanding, all comments and suggestions were clear and helpful 👍

core/thread/id.go

logstore/logstore.go

net/api/pb/threadsnet.proto

net/net.go

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

core/logstore/logstore.go

core/thread/id.go

logstore/logstore.go

carsonfarmer · 2020-06-11T19:46:30Z

net/net.go

@@ -218,6 +218,7 @@ func (n *net) CreateThread(_ context.Context, id thread.ID, opts ...core.NewThre
 	if !info.Key.Defined() {
 		info.Key = thread.NewRandomKey()
 	}
+	// This may overwrite ThreadKey information if there is a mismatch


Note that some existing tests relied on the ability to overwrite read and service keys, so no check or error is thrown when this happens. This seems ok, because it would support things like key rotation etc... but I've added this comment as a reminder and perhaps this should be documented in the public APIs?

What about only adding new keys if they don't already exists? So, modifying logstore.AddThread to not blindly overwrite?

So silently ignore if new/mismatched keys are used? That's an option for sure, would it lead to unexpected results?

Maybe just error if something would be overwritten?

Yeh that's what I had, but we have some existing tests that rely on being able to overwrite. Hence my comment above... unless I'm missing something here?

In particular, this test: https://github.com/textileio/go-threads/blob/master/db/db_test.go#L94

We could modify NewDB to allow passing in ThreadOptions, so that you could specify the same keys again, and if you didn't, return an error.

That's what I went with.

net/net.go

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

sanderpick

Couple more comments and questions!

net/net.go

sanderpick · 2020-06-11T20:18:45Z

net/net.go

@@ -218,6 +218,7 @@ func (n *net) CreateThread(_ context.Context, id thread.ID, opts ...core.NewThre
 	if !info.Key.Defined() {
 		info.Key = thread.NewRandomKey()
 	}
+	// This may overwrite ThreadKey information if there is a mismatch


What about only adding new keys if they don't already exists? So, modifying logstore.AddThread to not blindly overwrite?

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer · 2020-06-12T18:05:39Z

Ok, @sanderpick, this ended up touching a lot more places than I had initially anticipated, but it should now cover all our bases. Checks for all keys types have been added, and errors thrown for any mismatches. One 'own' log per thread is enforced, but multiple 'managed' logs are possible. There are also tests for the expected behavior, and peers can even replicate managed logs which is important for our JS use-cases.

carsonfarmer · 2020-06-12T18:06:16Z

db/options.go

@@ -68,6 +69,13 @@ func WithNewToken(t thread.Token) NewOption {
 	}
 }

+// WithNewThreadKey provides control over thread keys to use with a db.
+func WithNewThreadKey(key thread.Key) NewOption {


This was required to be able to get some tests passing again, but is also useful/required.

carsonfarmer · 2020-06-12T18:07:29Z

logstore/logstore.go

@@ -89,13 +92,35 @@ func (ls *logstore) AddThread(info thread.Info) error {
 	if info.Key.Service() == nil {
 		return fmt.Errorf("a service-key is required to add a thread")
 	}
-	if err := ls.AddServiceKey(info.ID, info.Key.Service()); err != nil {
+	sk, err := ls.ServiceKey(info.ID)


Keeping these checks in the high-level AddThread function rather than in the individual stores because it might still be useful to override this behavior in certain cases. So seems like the most common operation should fail, but if you're digging into internals, its your fault if something get's messed up.

carsonfarmer · 2020-06-12T18:08:38Z

net/net.go

-	_, err := n.store.GetThread(id)
-	if err == nil {
-		return lstore.ErrThreadExists
+func (n *net) ensureUnique(id thread.ID, key crypto.Key) error {


This is now a lot more verbose, as there are multiple different possible scenarios here, but I think the early outs help.

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer · 2020-06-12T18:11:15Z

net/net.go

+	if err != nil {
+		return err
+	}
+	// Early out if no log key required.


If we don't care about log keys, then we'll end up creating a random one downstream, with negligible chance of a collision.

carsonfarmer · 2020-06-12T18:12:00Z

net/net.go

+		return nil
+	}
+	// Ensure we don't already have this log in our store.
+	if _, ok := key.(crypto.PrivKey); ok {


If this is a private key, and we don't already have a private key for this thread, we're good to go, otherwise, we fail 'early' here.

carsonfarmer · 2020-06-12T18:14:00Z

net/net.go

+		}
+		return nil
+	}
+	pubKey, ok := key.(crypto.PubKey)


If this is a public key, just make sure we don't already have a log for this key.

sanderpick

Awesome, looks great! This is a great improvement... thanks for hanging in with the back and forth 💥

net/net_test.go

carsonfarmer · 2020-06-12T18:33:13Z

Thank you @sanderpick!

feat: own vs managed log concepts and tooling Directly "managed" logs are those logs created or directly added by the local peer. Otherwise, unmanaged logs are those from other peers in a given thread. Related, "owned" logs are those logs for which the peer has the private key. There can only be one "owned" log, versus potentially many "managed" logs.

feat: flag locally created logs as direct

adf3ad0

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer added bug Something isn't working enhancement New feature or request labels Jun 10, 2020

carsonfarmer added this to the Sprint 38 milestone Jun 10, 2020

carsonfarmer requested a review from sanderpick June 10, 2020 23:36

carsonfarmer self-assigned this Jun 10, 2020

carsonfarmer commented Jun 10, 2020

View reviewed changes

core/thread/id.go Show resolved Hide resolved

carsonfarmer commented Jun 10, 2020

View reviewed changes

net/net.go Show resolved Hide resolved

sanderpick reviewed Jun 11, 2020

View reviewed changes

carsonfarmer commented Jun 11, 2020

View reviewed changes

fix: address review comments + tests

2cd69fa

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer commented Jun 11, 2020

View reviewed changes

feat: bail after first failed push

8a8d1f2

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer requested a review from sanderpick June 11, 2020 19:56

sanderpick reviewed Jun 11, 2020

View reviewed changes

feat: net tests + edge case fixes + key checks

eccf8dc

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer requested a review from sanderpick June 12, 2020 18:03

carsonfarmer commented Jun 12, 2020

View reviewed changes

fix: early out for existing log

722d903

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

carsonfarmer commented Jun 12, 2020

View reviewed changes

sanderpick approved these changes Jun 12, 2020

View reviewed changes

net/net_test.go Show resolved Hide resolved

carsonfarmer merged commit a8477ff into master Jun 12, 2020

carsonfarmer deleted the carson/log-ownership branch June 12, 2020 18:34

carsonfarmer mentioned this pull request Jul 10, 2020

Unable to push records to Replicators when using client-side Networking layer textileio/js-threads#273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flag locally created logs as direct #374

Flag locally created logs as direct #374

carsonfarmer commented Jun 10, 2020 •

edited

Loading

sanderpick left a comment

carsonfarmer left a comment

carsonfarmer Jun 11, 2020

sanderpick Jun 11, 2020

carsonfarmer Jun 11, 2020

sanderpick Jun 11, 2020

carsonfarmer Jun 11, 2020

carsonfarmer Jun 11, 2020

carsonfarmer Jun 11, 2020

carsonfarmer Jun 12, 2020

sanderpick left a comment

sanderpick Jun 11, 2020

carsonfarmer commented Jun 12, 2020

carsonfarmer Jun 12, 2020

carsonfarmer Jun 12, 2020

carsonfarmer Jun 12, 2020

carsonfarmer Jun 12, 2020

carsonfarmer Jun 12, 2020

carsonfarmer Jun 12, 2020

sanderpick left a comment

carsonfarmer commented Jun 12, 2020

Flag locally created logs as direct #374

Flag locally created logs as direct #374

Conversation

carsonfarmer commented Jun 10, 2020 • edited Loading

sanderpick left a comment

Choose a reason for hiding this comment

carsonfarmer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderpick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonfarmer commented Jun 12, 2020

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

sanderpick left a comment

Choose a reason for hiding this comment

carsonfarmer commented Jun 12, 2020

carsonfarmer commented Jun 10, 2020 •

edited

Loading