js: Add nats.Bind option to disable creating consumers #740

wallyqs · 2021-06-01T21:01:54Z

This allows creating subscriptions that are bound to a consumer that matches the durable name, and does not attempt to create the consumer in case it is not present.

Scenarios where this option could be used is when the consumer is bound to a JetStream instance from another account and there are limited permissions, or when a consumer is created/deleted out of band and the subscription should not attempt creation to avoid misconfigurations/configuration drift.

Signed-off-by: Waldemar Quevedo wally@synadia.com

Fixes #735

matthiashanel

LGTM

coveralls · 2021-06-01T23:35:39Z

Coverage decreased (-0.06%) to 86.919% when pulling 6237e10 on js-bind-consumer into 1267a88 on master.

derekcollison

Some comments, mostly around naming. I agree this is useful.

derekcollison · 2021-06-02T01:33:13Z

js.go

-		if err != nil && err.Error() != "nats: consumer not found" {
-			return nil, err
+		if err != nil {
+			if strings.Contains(err.Error(), "consumer not found") {


This feels like could be simplified and possibly documented just a bit better as to what is going on here.

Tried to refactor a bit this part

derekcollison · 2021-06-02T01:36:26Z

js.go

@@ -1489,6 +1500,14 @@ func BindStream(name string) SubOpt {
 	})
 }

+// BindConsumer binds a subscription to a consumer based on a durable name.
+func BindConsumer() SubOpt {


By default this happens sometimes on its own if the consumer exists. So possibly the naming might be a bit off here.

BindOnly or NoCreate. Probably not these but hopefully this gets to the point I am trying to make a bit.

Updated to use BindOnly for now

derekcollison · 2021-06-02T01:37:15Z

nats.go

@@ -149,6 +149,7 @@ var (
 	ErrConsumerConfigRequired       = errors.New("nats: consumer configuration is required")
 	ErrStreamSnapshotConfigRequired = errors.New("nats: stream snapshot configuration is required")
 	ErrDeliverSubjectRequired       = errors.New("nats: deliver subject is required")
+	ErrDurableNameRequired          = errors.New("nats: durable name of consumer is required")


If we know the name of an ephemeral can we bind to that? I am not sure how that would act today or not but we may not want to restrict to durables here per se.

We cannot right now due to some filter subject checks, but it might look something like this. First need to get the name and delivery subject of the ephemeral then use bind stream and bind only to attach to that consumer without looking up the stream.

sub, err := js.SubscribeSync("foo") if err != nil { t.Fatalf("Unexpected error: %v", err) } cinfo, err := sub.ConsumerInfo() if err != nil { t.Fatal(err) } sub, err = js.SubscribeSync(cinfo.Config.DeliverSubject, nats.Durable(cinfo.Name), nats.BindStream("foo"), nats.BindOnly(), )

Since we don't allow ephemerals, should the option be called nats.BindDurable() as that's what we currently enforce?

ConsumerInfo should work for both durables and ephemerals.

Which checks?

I meant these filter subject checks but found that it would work using the nats.Durable name for the consumer name even though it is ephemeral since that helps making the lookup of the consumer. Added examples of using BindOnly with ephemeral consumers, all these should work atm:

// Create ephemeral consumer sub1, err := js.SubscribeSync("foo") if err != nil { t.Fatalf("Unexpected error: %v", err) } cinfo, err := sub1.ConsumerInfo() if err != nil { t.Fatal(err) } // Bind to ephemeral consumer by setting the name using nats.Durable sub2, err := js.SubscribeSync("foo", nats.Durable(cinfo.Name), nats.BindStream("foo"), nats.BindOnly()) if err != nil { t.Fatalf("Unexpected error: %v", err) } sub3, err := nc.SubscribeSync(cinfo.Config.DeliverSubject) if err != nil { t.Fatalf("Unexpected error: %v", err) }

Feels off though to use Durable when its an ephemeral, at least to me.

In the call above with BindOnly, will we always need to provide the stream and consumer name? If so why not just make them args to BindOnly?

test/js_test.go

derekcollison · 2021-06-02T21:54:34Z

nats.go

@@ -149,6 +149,7 @@ var (
 	ErrConsumerConfigRequired       = errors.New("nats: consumer configuration is required")
 	ErrStreamSnapshotConfigRequired = errors.New("nats: stream snapshot configuration is required")
 	ErrDeliverSubjectRequired       = errors.New("nats: deliver subject is required")
+	ErrDurableNameRequired          = errors.New("nats: durable name of consumer is required")


Which checks?

derekcollison · 2021-06-02T21:55:16Z

test/js_test.go

-	if _, err := js.SubscribeSync("ORDERS"); err != nats.ErrJetStreamNotEnabled {
-		t.Fatalf("Expected an error of '%v', got '%v'", nats.ErrJetStreamNotEnabled, err)
+	// Cannot create ephemeral subscriber with JS context.
+	if _, err := js.SubscribeSync("ORDERS", nats.BindOnly()); err != nats.ErrDurableNameRequired {


I think we should be able to bind to any pre-existing consumer.

It is possible to bind to ephemerals as well but have to use nats.Durable to set the consumer name

derekcollison · 2021-06-03T00:28:17Z

js.go

@@ -1059,12 +1063,23 @@ func (js *js) subscribe(subj, queue string, cb MsgHandler, ch chan *Msg, isSync
 			return nil, ErrSubjectMismatch
 		}

+		// Prevent binding a subscription against incompatible consumer types.
+		if isPullMode && ccfg.DeliverSubject != _EMPTY_ {
+			return nil, fmt.Errorf("nats: cannot pull subscribe to push based consumer")


Let's make these errors constants since we don't fill anything in.

js.go

derekcollison · 2021-06-03T00:30:06Z

js.go

 	// For manual ack
 	mack bool
 	// For creating or updating.
 	cfg *ConsumerConfig
+	// For binding a subscription to a consumer without creating it.
+	cbound bool


I think just bound since subOpts.

derekcollison · 2021-06-03T00:32:28Z

nats.go

@@ -149,6 +149,7 @@ var (
 	ErrConsumerConfigRequired       = errors.New("nats: consumer configuration is required")
 	ErrStreamSnapshotConfigRequired = errors.New("nats: stream snapshot configuration is required")
 	ErrDeliverSubjectRequired       = errors.New("nats: deliver subject is required")
+	ErrDurableNameRequired          = errors.New("nats: durable name of consumer is required")


Feels off though to use Durable when its an ephemeral, at least to me.

In the call above with BindOnly, will we always need to provide the stream and consumer name? If so why not just make them args to BindOnly?

derekcollison · 2021-06-03T00:32:45Z

test/js_test.go

-	if _, err := js.SubscribeSync("ORDERS"); err != nats.ErrJetStreamNotEnabled {
-		t.Fatalf("Expected an error of '%v', got '%v'", nats.ErrJetStreamNotEnabled, err)
+	// Cannot create ephemeral subscriber with JS context.
+	if _, err := js.SubscribeSync("ORDERS", nats.BindOnly()); err != nats.ErrDurableNameRequired {


matthiashanel · 2021-06-02T18:38:52Z

nats.go

@@ -149,6 +149,7 @@ var (
 	ErrConsumerConfigRequired       = errors.New("nats: consumer configuration is required")
 	ErrStreamSnapshotConfigRequired = errors.New("nats: stream snapshot configuration is required")
 	ErrDeliverSubjectRequired       = errors.New("nats: deliver subject is required")
+	ErrDurableNameRequired          = errors.New("nats: durable name of consumer is required")


Since we don't allow ephemerals, should the option be called nats.BindDurable() as that's what we currently enforce?

js.go

derekcollison · 2021-06-03T22:45:21Z

js.go

+			return nil, err
+		}
+	default:
+		// Attempt to create consumer if not found or using BindOnly.


I think BindOnly means not create yes?

js.go

matthiashanel

LGTM

js.go

test/js_test.go

Signed-off-by: Waldemar Quevedo <wally@synadia.com>

matthiashanel

LGTM

derekcollison

LGTM

wallyqs requested review from derekcollison, matthiashanel and variadico June 1, 2021 21:02

matthiashanel approved these changes Jun 1, 2021

View reviewed changes

wallyqs force-pushed the js-bind-consumer branch from e40d303 to 76658b7 Compare June 1, 2021 23:28

derekcollison reviewed Jun 2, 2021

View reviewed changes

wallyqs force-pushed the js-bind-consumer branch 2 times, most recently from cba3d15 to 8238c79 Compare June 2, 2021 15:20

derekcollison reviewed Jun 2, 2021

View reviewed changes

wallyqs force-pushed the js-bind-consumer branch from 8238c79 to 274e7ea Compare June 3, 2021 00:05

derekcollison requested changes Jun 3, 2021

View reviewed changes

wallyqs changed the title ~~js: Add BindConsumer option~~ js: Add BindOnly option Jun 3, 2021

wallyqs force-pushed the js-bind-consumer branch 2 times, most recently from 8d78de2 to fa40115 Compare June 3, 2021 21:18

matthiashanel reviewed Jun 3, 2021

View reviewed changes

wallyqs force-pushed the js-bind-consumer branch from fa40115 to dc4a6d4 Compare June 3, 2021 22:40

derekcollison requested changes Jun 3, 2021

View reviewed changes

wallyqs force-pushed the js-bind-consumer branch from dc4a6d4 to 4d28296 Compare June 3, 2021 23:49

matthiashanel approved these changes Jun 3, 2021

View reviewed changes

wallyqs changed the title ~~js: Add BindOnly option~~ js: Add nats.Bind option to disable creating consumers Jun 3, 2021

derekcollison reviewed Jun 4, 2021

View reviewed changes

js.go Outdated Show resolved Hide resolved

js.go Outdated Show resolved Hide resolved

js.go Outdated Show resolved Hide resolved

test/js_test.go Show resolved Hide resolved

test/js_test.go Show resolved Hide resolved

js: Add Bind option to Subscribe

6237e10

Signed-off-by: Waldemar Quevedo <wally@synadia.com>

wallyqs force-pushed the js-bind-consumer branch from 4d28296 to 6237e10 Compare June 4, 2021 00:57

matthiashanel approved these changes Jun 4, 2021

View reviewed changes

derekcollison approved these changes Jun 4, 2021

View reviewed changes

wallyqs merged commit ba098b9 into master Jun 4, 2021

wallyqs deleted the js-bind-consumer branch June 4, 2021 02:12

wallyqs mentioned this pull request Jun 8, 2021

[added] option to not create durable on subscribe #734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

js: Add nats.Bind option to disable creating consumers #740

js: Add nats.Bind option to disable creating consumers #740

wallyqs commented Jun 1, 2021 •

edited

Loading

matthiashanel left a comment

coveralls commented Jun 1, 2021 •

edited

Loading

derekcollison left a comment

derekcollison Jun 2, 2021

wallyqs Jun 2, 2021

derekcollison Jun 2, 2021

wallyqs Jun 2, 2021

derekcollison Jun 2, 2021

wallyqs Jun 2, 2021 •

edited

Loading

matthiashanel Jun 2, 2021

derekcollison Jun 2, 2021

derekcollison Jun 2, 2021

wallyqs Jun 3, 2021

derekcollison Jun 3, 2021

derekcollison Jun 2, 2021

derekcollison Jun 2, 2021

wallyqs Jun 3, 2021

derekcollison Jun 3, 2021

derekcollison Jun 3, 2021

derekcollison Jun 3, 2021

derekcollison Jun 3, 2021

derekcollison Jun 3, 2021

matthiashanel Jun 2, 2021

derekcollison Jun 3, 2021

matthiashanel left a comment

matthiashanel left a comment

derekcollison left a comment

js: Add nats.Bind option to disable creating consumers #740

js: Add nats.Bind option to disable creating consumers #740

Conversation

wallyqs commented Jun 1, 2021 • edited Loading

matthiashanel left a comment

Choose a reason for hiding this comment

coveralls commented Jun 1, 2021 • edited Loading

derekcollison 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

wallyqs Jun 2, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiashanel left a comment

Choose a reason for hiding this comment

matthiashanel left a comment

Choose a reason for hiding this comment

derekcollison left a comment

Choose a reason for hiding this comment

wallyqs commented Jun 1, 2021 •

edited

Loading

coveralls commented Jun 1, 2021 •

edited

Loading

wallyqs Jun 2, 2021 •

edited

Loading