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.

Sign up for GitHub

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

Jump to bottom

support custom inbox prefixes #767

Merged
merged 2 commits into from
Aug 3, 2021
Merged

support custom inbox prefixes #767

merged 2 commits into from
Aug 3, 2021

Conversation

ripienaar
Copy link
Contributor

Signed-off-by: R.I.Pienaar rip@devco.net

@ripienaar
support custom inbox prefixes
559051c 
Signed-off-by: R.I.Pienaar <rip@devco.net>
@ripienaar
Copy link
Contributor Author

This has been discussed a few times, Derek mentioned he has some other ideas but that I should open this to start a discussion.

Let me know your thoughts, I think supporting this is pretty important for peoples ability to create private comms channels as it lets them make predictable inbox patterns

/cc @derekcollison @wallyqs @kozlovic

@coveralls
Copy link

coveralls commented Jun 29, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 86.898% when pulling 7d68c97 on ripienaar:custom_inbox into 274aa57 on nats-io:master.

@derekcollison
Copy link
Member

So at a very high level we want to present a way for folks to make sure only they can see any given response and not have to abandon use of nc.Request() and the like correct?

@ripienaar
Copy link
Contributor Author

Well, that might be one possible outcome? I think there are many reasons to avoid a one-size-fits-all inbox pattern.

Even if not private in many of my use cases I prefix all related traffic by some string - I want the inboxes there too. That's not about privacy as such but about just managing ACLs

now of course I think we need private reply subjects that just work and don't require special ACLs and such, but thats probably more than I wish to tackle - I think we'll have inboxes like this for a long time

kozlovic
kozlovic reviewed Jun 29, 2021
View reviewed changes
nats.go Outdated Show resolved Hide resolved
nats.go Outdated Show resolved Hide resolved
nats.go Outdated Show resolved Hide resolved
nats.go Show resolved Hide resolved
@ripienaar
review feedback
7d68c97 
Signed-off-by: R.I.Pienaar <rip@devco.net>
kozlovic
kozlovic approved these changes Jun 29, 2021
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Code LGTM, but will let Derek decide if we go with that or other approach.

@ripienaar ripienaar marked this pull request as ready for review July 7, 2021 10:33
@ripienaar
Copy link
Contributor Author

@derekcollison what are your thoughts here?

@derekcollison
Copy link
Member

I will try to find some time this week to think harder about it.

@ripienaar
Copy link
Contributor Author

@derekcollison this PR seems ok to me as is as discussed

Here's the matching feature in nats.java nats-io/nats.java@03c22e4 and .Net nats-io/nats.net#322

derekcollison
derekcollison reviewed Aug 2, 2021
View reviewed changes
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.

If I supply a custom prefix of say "derek", does my inbox look like derek.NUID.NUID or does it look like derek._INBOX.NUID.NUID?

@ripienaar
Copy link
Contributor Author

_INBOX is replaced with prefix.

@derekcollison derekcollison self-requested a review August 2, 2021 17:13
derekcollison
derekcollison reviewed Aug 2, 2021
View reviewed changes
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 - Have not tested it but if it replaces _INBOX and tests pass looks good. I could use my own prefix of "_INBOX.derek" if I wanted as well for the use case we discussed.

@derekcollison derekcollison self-requested a review August 2, 2021 17:14
derekcollison
derekcollison approved these changes Aug 2, 2021
View reviewed changes
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

@ripienaar ripienaar merged commit ceb3147 into nats-io:master Aug 3, 2021
@ripienaar ripienaar deleted the custom_inbox branch August 3, 2021 08:54
This was referenced Aug 15, 2021
Closed
Closed
Closed
kozlovic added a commit that referenced this pull request Sep 1, 2021
@kozlovic
[FIXED] Inboxes suffix would contain many zeros
eced299 
This was introduced by PR #767
when the nuidSize was used instead of replySuffixLen to just build
the reply suffix. This meant that the suffix part with 22 characters
long and had lots of zeros at the end.

Added a test to make sure we don't break that in the future.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic kozlovic mentioned this pull request Sep 1, 2021
Merged
@kozlovic kozlovic mentioned this pull request Oct 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozlovic kozlovic kozlovic approved these changes

@derekcollison derekcollison derekcollison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ripienaar @coveralls @derekcollison @kozlovic