nats-io / nats-architecture-and-design

Architecture and Design Docs
Apache License 2.0
177 stars 20 forks source link

ADR-6 specify consumer name #151

Closed scottf closed 1 year ago

tbeets commented 1 year ago

We don't allow slash or backslash in stream names and consumer names in 2.9.

scottf commented 1 year ago

We don't allow slash or backslash in stream names and consumer names in 2.9.

So what about other filename unsafe characters? Maybe we should switch

stream-name        = term
durable-name       = term
consumer-name      = term
filename-safe      = (printable except dot, asterisk, lt, gt, colon, double-quote, fwd-slash, backslash, pipe, question-mark, ampersand)
...
stream-name        = filename-safe 
durable-name       = filename-safe 
consumer-name      = filename-safe 
tbeets commented 1 year ago

@derekcollison Which way do you want to go? We should minimally match 2.9 validations. If we make filename-safe then the 2.9 server allows illegal characters and implication is we should add the restrictions in a subsequent release.

Current stream and consumer validation on create is essentially (term except fwd-slash, backslash)

scottf commented 1 year ago

@derekcollison @tbeets On windows, stream creation fails for these allowed characters

Failed creating stream: 'lessthan<' io.nats.client.JetStreamApiException: error creating store for stream [10049]
Failed creating stream: 'colon:' io.nats.client.JetStreamApiException: error creating store for stream [10049]
Failed creating stream: 'doublequote"' io.nats.client.JetStreamApiException: error creating store for stream [10049]
Failed creating stream: 'pipe|' io.nats.client.JetStreamApiException: error creating store for stream [10049]
Failed creating stream: 'question?' io.nats.client.JetStreamApiException: error creating store for stream [10049]

On ubuntu and a mac:

Failed creating stream: 'fwdslash/' io.nats.client.JetStreamApiException: Stream name can not contain path separators [10128]
Failed creating stream: 'backslash\' io.nats.client.JetStreamApiException: Stream name can not contain path separators [10128]
Created stream: 'lessthan<'
Created stream: 'colon:'
Created stream: 'doublequote"'
Created stream: 'pipe|'
Created stream: 'question?'
Created stream: 'ampersand&'
derekcollison commented 1 year ago

We could add client support to check them if we feel that is a better experience. Can add in additional checks to server as well.

scottf commented 1 year ago

Will most installations have only one type of server? Probably, and most often linux servers, so as is it's probably not that much of a problem. I only wonder if other symbols that are used in shells like pipe | could be security issues. That's why it and ampersand & is on the filename-safe list. What do you think @philpennock ?

So the options are:

  1. It's fine how it is in 2.9.0
  2. Make it stricter on windows
  3. Make it stricter everywhere

How many installations are actually using <, :, ", | or ? in stream names? That answer might help make the decision.

scottf commented 1 year ago

These changes simply try to reflect the current state of the server as of v2.9.0