Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 4, 2021

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Jun 1, 2021

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

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coveralls
Copy link

coveralls commented Jun 1, 2021

Coverage Status

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

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

js.go Outdated
if err != nil && err.Error() != "nats: consumer not found" {
return nil, err
if err != nil {
if strings.Contains(err.Error(), "consumer not found") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to refactor a bit this part

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use BindOnly for now

nats.go Outdated
@@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@wallyqs wallyqs Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(),
	)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConsumerInfo should work for both durables and ephemerals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which checks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
	}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@wallyqs wallyqs force-pushed the js-bind-consumer branch 2 times, most recently from cba3d15 to 8238c79 Compare June 2, 2021 15:20
nats.go Outdated
@@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which checks?

test/js_test.go Outdated
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

js.go Outdated
@@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

js.go Outdated Show resolved Hide resolved
js.go Outdated
// For manual ack
mack bool
// For creating or updating.
cfg *ConsumerConfig
// For binding a subscription to a consumer without creating it.
cbound bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just bound since subOpts.

nats.go Outdated
@@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

@wallyqs wallyqs changed the title js: Add BindConsumer option js: Add BindOnly option Jun 3, 2021
@wallyqs wallyqs force-pushed the js-bind-consumer branch 2 times, most recently from 8d78de2 to fa40115 Compare June 3, 2021 21:18
nats.go Outdated
@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

js.go Outdated Show resolved Hide resolved
js.go Outdated
return nil, err
}
default:
// Attempt to create consumer if not found or using BindOnly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think BindOnly means not create yes?

js.go Outdated Show resolved Hide resolved
js.go Outdated Show resolved Hide resolved
Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wallyqs wallyqs changed the title js: Add BindOnly option js: Add nats.Bind option to disable creating consumers Jun 3, 2021
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
Signed-off-by: Waldemar Quevedo <wally@synadia.com>
Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wallyqs wallyqs merged commit ba098b9 into master Jun 4, 2021
@wallyqs wallyqs deleted the js-bind-consumer branch June 4, 2021 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to turn off auto consumer create with PullSub
4 participants