[ADDED] Account Support #755

derekcollison · 2018-09-24T13:44:07Z

This PR introduces accounts and account isolation. Users are assigned to one account per connection and that defines their subject space.

We also introduce streams and services. These are ways to share messages across accounts with the proper permissions. Streams are pub/sub and Services are usually req/reply semantics.

We allow explicit permissions to services to also generate implicit permissions in realtime for one time use responses.

We also have support for configuration parsing.

/cc @nats-io/core

derekcollison · 2018-09-24T13:45:32Z

Note this work is backwards compatible, but only works for a single server. The cluster updates will be a separate PR.

coveralls · 2018-09-24T13:48:29Z

Coverage decreased (-0.5%) to 92.007% when pulling dc745f0 on accounts into d5ceade on master.

kozlovic

Few comments so far. Will continue review.

server/client.go

kozlovic · 2018-09-25T16:15:30Z

server/client.go

+	for _, im := range ims {
+		// We have a match for a local subscription with an import from another account.
+		// We will create a shadow subscription.
+		nsub := *sub // copy


should we have a clone() here? This copy does not copy much ;-), the []byte will still point to sub, so are the pointers.

I think that is ok, I just wanted to not overwrite anything in the main sub when doing subject or reply rewriting.

kozlovic · 2018-09-25T16:52:02Z

server/client.go

+	c.mu.Unlock()
+
+	for sid, sub := range subs {
+		if c.sl != nil && !c.canSubscribe(sub.subject) {


canSubscribe() needs locking. During a reload, the client's perms may be changed (under the client's lock).

Will take a look.

kozlovic · 2018-09-25T17:26:50Z

server/client.go

@@ -143,6 +144,8 @@ type client struct {
 	ncs   string
 	out   outbound
 	srv   *Server
+	acc   *Account
+	sl    *Sublist


At first it looked like a client had its own sublist, but I think this is just a reference to server's sublist. Did you do that so that you don't have to do c.srv.sl?

This brings the point that client should always have c.srv != nil, and we would not have many places where we check for that. It's probably only in few tests that we create client directly, maybe it would be worth "fixing" those instead.

Since sl has it's own locking did not want to have to potentially lock c.acc in performance paths.

kozlovic · 2018-09-25T17:31:50Z

server/server.go

+	return s.opts.AllowNewAccounts
+}
+
+func (s *Server) LookupOrRegisterAccount(name string) (*Account, bool) {


For this and below: do we need them to be exported?

Also, I know that you don't like to name returned parameters (and me too somehow), but then we need to document them ;-) (here the boolean is true if did not exist and now registered, but could have been the opposite).

I think if people embed the server and want to configure without a config file they might, but agree we should double check. No problem on name parameters in some cases, agree here.

Added comment and named params.

kozlovic · 2018-09-25T18:03:52Z

server/client_test.go

 	c.parse(op)
 	for cp := range c.pcd {
 		cp.mu.Lock()
+		cp.flags.clear(flushOutbound)


I would like to understand why we need this? The only place this flag is set is in flushOutbound() itself and is cleared when this function returns. The purpose of the flag was to prevent re-entrant calls since flushOutbound() releases client's lock when doing actual write to socket.

These tests do not run a flush Go routine, and by default data (msgs) are expected to be sent via that Go routine, so we need to flush the pcd list in the test, which we do. What I was finding was that the flag was set every once in while so the manual flush was failing, so I clear the flag to force it in place.

kozlovic

Some more comments...

kozlovic · 2018-09-25T22:09:12Z

server/accounts_test.go

+)
+
+func simpleAccountServer(t *testing.T) (*Server, *Account, *Account) {
+	opts := defaultServerOptions


make this a t.Helper()

ok will do.

kozlovic · 2018-09-25T22:09:48Z

server/accounts_test.go

+
+func TestAccountIsolation(t *testing.T) {
+	s, fooAcc, barAcc := simpleAccountServer(t)
+	if fooAcc == nil || barAcc == nil {


Not needed since simpleAccountServer() would have failed if those were not properly registered.

kozlovic · 2018-09-25T22:18:48Z

server/accounts_test.go

+	if err != nil {
+		t.Fatalf("Error for client 'bar' from server: %v", err)
+	}
+	if !strings.HasPrefix(l, "PONG\r\n") {


Not sure this proves it. You first did a go cbar.parse([]byte("SUB foo 1\r\nPING\r\nPING\r\n")) which sent 2 PINGs, then consumed one before doing foo work. Ideally, you would then send the second PING after that, not prior, and then getting PONG would better prove that nothing went to bar.

Will take a look.

kozlovic · 2018-09-25T22:21:44Z

server/accounts_test.go

+	s = New(&opts)
+
+	c, _, _ = newClientForServer(s)
+	err := c.parse(connectOp)


should you add a PING after the connect and get the PONG to show that there was no issue?

Will take a look.

kozlovic · 2018-09-25T22:29:42Z

server/accounts_test.go

+		t.Fatalf("Expected to see 2 accounts in opts, got %d", la)
+	}
+
+	if lu := len(opts.Users); lu != 4 {


That brings a point: you can't have same user name across multiple accounts then?

Correct, was discussed in the DD specifically. Do you feel we should allow?

kozlovic · 2018-09-25T22:50:11Z

server/accounts_test.go

+	if err != nil {
+		t.Fatalf("Error for client 'bar' from server: %v", err)
+	}
+


should we consume PONG here?

We do with ReadString.

kozlovic · 2018-09-25T22:52:14Z

server/accounts_test.go

+		t.Fatalf("Did not get correct sid: '%s'", matches[SID_INDEX])
+	}
+	checkPayload(crBar, []byte("hello\r\n"), t)
+}


Should we test that since bar only import *, it would not get say foo.bar (even though foo exports ">")? Or there may be another test checking that already.

Will take a look.

Tested elsewhere.

kozlovic · 2018-09-25T22:52:33Z

server/accounts_test.go

+		t.Fatalf("Error registering client with 'bar' account: %v", err)
+	}
+
+	if err := cfoo.acc.addStreamExport(">", []*Account{barAcc}); err != nil { // Public with no accounts defined.


Wrong comment again..

Same as above :-)

kozlovic · 2018-09-25T22:54:22Z

server/accounts_test.go

+	if err := cfoo.acc.addStreamExport(">", []*Account{barAcc}); err != nil { // Public with no accounts defined.
+		t.Fatalf("Error adding account export to client foo: %v", err)
+	}
+	if err := cbar.acc.addStreamImport(fooAcc, "*", "pub.imports."); err != nil {


addStreamImport() checks if prefix's last character is . or not, and if not, adds it. We have tests without the . and here with it, which is good. Maybe a comment above this line to say that . is added automatically but testing that it works if explicitly specified?

kozlovic · 2018-09-25T22:55:07Z

server/accounts_test.go

+		t.Fatalf("Error registering client with 'bar' account: %v", err)
+	}
+
+	if err := cfoo.acc.addStreamExport(">", []*Account{barAcc}); err != nil { // Public with no accounts defined.


Fixed, when I started the work originally we just had import and export. Once stream and service became evident I should have updated the original tests. Thanks..

Again, I meant the // Public ... comment

derekcollison

I will take a look in a bit the others that you mentioned. Thanks.

kozlovic

Getting close...

kozlovic · 2018-09-26T18:34:14Z

server/accounts_test.go

+		t.Fatalf("Error registering client with 'bar' account: %v", err)
+	}
+
+	if err := cfoo.acc.addStreamExport(">", []*Account{barAcc}); err != nil { // Public with no accounts defined.


Sorry, what I actually meant was the presence of // Public with no accounts defined. is wrong since you provide an account.

kozlovic · 2018-09-26T18:34:27Z

server/accounts_test.go

+		t.Fatalf("Error registering client with 'bar' account: %v", err)
+	}
+
+	if err := cfoo.acc.addStreamExport(">", []*Account{barAcc}); err != nil { // Public with no accounts defined.


Same as above :-)

kozlovic · 2018-09-26T18:37:46Z

server/client.go

+	c.mu.Unlock()
+
+	for _, sub := range removed {
+		c.sl.Remove(sub)


Or could skip this and before or after the loop call c.sl.RemoveBatch(removed)

Good call.. Will change..

kozlovic · 2018-09-26T18:38:28Z

server/accounts_test.go

+		t.Fatalf("Error registering client with 'bar' account: %v", err)
+	}
+
+	if err := cfoo.acc.addStreamExport(">", []*Account{barAcc}); err != nil { // Public with no accounts defined.


Again, I meant the // Public ... comment

kozlovic · 2018-09-26T22:19:37Z

server/server.go

@@ -769,13 +833,14 @@ func (s *Server) copyInfo() Info {

 func (s *Server) createClient(conn net.Conn) *client {
 	// Snapshot server options.
+	// TODO(dlc) - This can get expensive.


No it is not. There is no copy/clone. s.getOpts() is just returning the pointer to the options under the optsMu lock, so that it is safe with the reload code which switch s.opts with new options pointer under that same lock.

Ah ok, much better then ;)

kozlovic · 2018-09-28T18:13:50Z

server/accounts_test.go

+	}
+}
+
+func TestImportExportConfigFailures(t *testing.T) {


In this test, please check that returned error contains string that you expect. I noticed that some errors are just malformed config (say expect map got string) and not at all what is the expected error.

ok will take a look. This errors from the config parsing?

wallyqs · 2018-09-28T20:55:44Z

server/accounts_test.go

+	cf = createConfFile(t, []byte(`
+    accounts {
+      nats.io {
+        exports = [{service: {account: nats.io, subject:"foo.*"}]


There is a missing bracket in this one (so right now the error is a parsing error)

diff --git a/server/accounts_test.go b/server/accounts_test.go index eee29b3..dda4a25 100644 --- a/server/accounts_test.go +++ b/server/accounts_test.go @@ -423,13 +423,15 @@ func TestImportExportConfigFailures(t *testing.T) { cf = createConfFile(t, []byte(` accounts { nats.io { - exports = [{service: {account: nats.io, subject:"foo.*"}] + exports = [{service: {account: nats.io, subject:"foo.*"}}] } } `))

kozlovic · 2018-09-28T21:18:09Z

server/client.go

@@ -312,22 +320,33 @@ func (c *client) initClient() {
 	}
 }

+// RegisterWithAccount will register the given user with a specific
+// account. This will change the subject namespace.
+func (c *client) registerWithAccount(acc *Account) error {


I see that this is called only if a client lib would send the account name in CONNECT. So how does this work if one defined an account in the server and list users there. I would have imagined that if a regular client connects with that user name, it would be associated with the account, which does not seem to be the case. Could you clarify?

Yes this was for programmatic not file parsing. But this has been fixed.

kozlovic · 2018-09-28T23:35:38Z

server/client.go

 	if shouldForward {
 		c.srv.broadcastSubscribe(sub)
 	}

 	return nil
 }

+// Check to see if we need to create a shadow subscription due to imports
+// in other accounts.
+// Assume lock is held


Actually not, this function grabs the lock.

Signed-off-by: Derek Collison <[email protected]>

kozlovic

Just few comments

server/accounts_test.go

server/client.go

Signed-off-by: Derek Collison <[email protected]>

server/client.go

derekcollison requested review from kozlovic and wallyqs September 24, 2018 13:44

kozlovic reviewed Sep 25, 2018

View reviewed changes

derekcollison commented Sep 26, 2018

View reviewed changes

kozlovic reviewed Sep 26, 2018

View reviewed changes

kozlovic changed the title ~~Account Support~~ [ADDED] Account Support Sep 26, 2018

kozlovic reviewed Sep 26, 2018

View reviewed changes

kozlovic requested changes Sep 28, 2018

View reviewed changes

wallyqs reviewed Sep 28, 2018

View reviewed changes

kozlovic reviewed Sep 28, 2018

View reviewed changes

derekcollison added 6 commits September 29, 2018 13:04

Basic account support

1cbfbfa

Signed-off-by: Derek Collison <[email protected]>

Basic account mapping via import and export

620e1d7

Signed-off-by: Derek Collison <[email protected]>

First pass req/reply across accounts

f6cb706

Signed-off-by: Derek Collison <[email protected]>

Map anonymous reply subjects

c4bcbf6

Signed-off-by: Derek Collison <[email protected]>

API changes to match config for account mappings

ae21fa2

Signed-off-by: Derek Collison <[email protected]>

Added import and export parsing for configs

9f8330b

Signed-off-by: Derek Collison <[email protected]>

derekcollison force-pushed the accounts branch from 79f78ac to b453133 Compare September 30, 2018 00:47

Updates from comments

14cdda8

Signed-off-by: Derek Collison <[email protected]>

derekcollison force-pushed the accounts branch from a732ede to 14cdda8 Compare September 30, 2018 16:36

kozlovic approved these changes Oct 1, 2018

View reviewed changes

server/accounts_test.go Outdated Show resolved Hide resolved

server/client.go Show resolved Hide resolved

Remove helper

dc745f0

Signed-off-by: Derek Collison <[email protected]>

derekcollison merged commit fd3e5fd into master Oct 1, 2018

derekcollison deleted the accounts branch October 1, 2018 15:58

wallyqs reviewed Oct 2, 2018

View reviewed changes

server/client.go Show resolved Hide resolved

[ADDED] Account Support #755

[ADDED] Account Support #755

Conversation

derekcollison commented Sep 24, 2018

derekcollison commented Sep 24, 2018

coveralls commented Sep 24, 2018 • edited Loading

kozlovic 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

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

kozlovic 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

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

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

derekcollison left a comment

Choose a reason for hiding this comment

kozlovic 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

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

kozlovic left a comment

Choose a reason for hiding this comment

coveralls commented Sep 24, 2018 •

edited

Loading