Add error and its check for collision with JS API #3532
Conversation
server/stream.go
Outdated
| @@ -1147,6 +1147,12 @@ func (s *Server) checkStreamCfg(config *StreamConfig, acc *Account) (StreamConfi | |||
| } | |||
| } | |||
|
|
|||
| for _, subj := range cfg.Subjects { | |||
| if SubjectsCollide(subj, JSApiPrefix) { | |||
There was a problem hiding this comment.
This will help of course - we planned to add this before but with cross accounts, domains etc it’s a mine field. And of course many other subjects are problematic also.
We should for sure also stop just “>” for example also
There was a problem hiding this comment.
I guess this should stop > also though as it would over lap
There was a problem hiding this comment.
that was my first thought also, but with (possibly) exported JetStream services, I wanted to target the JS API more specifically.
Although '>' sure poses a problem every time, I can imagine a (twisted) use-case where one would want to use a stream to keep an history of $JS.EVENT.ADVISORY.> for instance.
There was a problem hiding this comment.
Yeah so we definitely shouldn’t stop advisories from being ingested that’s a good point.
And to be pedantic you CAN ingest > you just need to turn off ack on the stream.
There was a problem hiding this comment.
I do seem to recall some users having a stream on the API subject without acks for audit purposes.
There was a problem hiding this comment.
Pretty good point I'll take that into account and correct my code accordingly
There was a problem hiding this comment.
for the record a test exists to ensure events, etc are OK
nats-server/server/jetstream_test.go
Line 1085 in cb086bc
There was a problem hiding this comment.
I guess this should stop > also though as it would over lap
yes, it does. Included a test case for it to make that explicit
|
I made this pull request kind of quick, do not hesitate to reject it if you want me to do it again in a cleaner way (branched, rebased, squashed). |
There was a problem hiding this comment.
Since I would consider this a breaking change (since possibly existing streams would fail to be recovered, etc..) should this go only on the next 2.10.0 release? I have created the dev branch today that will hold the changes in main (the patches) and all PRs that are targeted for the next release. If we all agree that this is possibly breaking and should go in 2.10.0, could you alter your PR to be against the dev branch instead of main?
|
An undertone of this PR is the need to extend the authorization model transparently for JetStream primitives. That said, there may be an incremental improvement here as an opt-in (non-breaking) account limit that could guard against creating streams that bind to the JS API. |
|
Addressed via #5548 |
Resolves #NNNgit pull --rebase origin main)Resolves #3531
Changes proposed in this pull request:
/cc @nats-io/core