Reject TTL messages when not enabled, strip TTL header from sourcing/mirroring #6298
Conversation
| @@ -2418,6 +2418,12 @@ func (mset *stream) processInboundMirrorMsg(m *inMsg) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // If the TTL header is set, remove it. | |||
| hdr := m.hdr | |||
| if len(hdr) > 0 { | |||
There was a problem hiding this comment.
I think instead of removing we flag that the messages is sourced to this function and if so we ignore this plus a bunch of others..
There was a problem hiding this comment.
Until we have a good story for how TTLs should work across sources/mirrors, it seems like a better approach to remove the headers. That way there is no possible way during, i.e. a recovery scenario, that we can apply TTLs in the wrong places.
There was a problem hiding this comment.
I don't prefer removing things from the original messages though myself.
| @@ -3447,8 +3453,10 @@ func (mset *stream) processInboundSourceMsg(si *sourceInfo, m *inMsg) bool { | |||
| hdr, msg := m.hdr, m.msg | |||
|
|
|||
| // If we are daisy chained here make sure to remove the original one. | |||
| // If the TTL header is set, remove it. | |||
There was a problem hiding this comment.
Let's rework this logic to not remove but ignore.
There was a problem hiding this comment.
Is this wise? If we have to rebuild the mirroring/sourcing stream, how do we know whether or not to recover the TTLs? Do we just never recover the TTLs if there are mirrors/sources present?
| @@ -4780,6 +4789,19 @@ func (mset *stream) processJetStreamMsg(subject, reply string, hdr, msg []byte, | |||
| } | |||
| } | |||
|
|
|||
| // TTL'd messages are rejected entirely if TTLs are not enabled on the stream. | |||
| if ttl, _ := getMessageTTL(hdr); ttl != 0 && !mset.cfg.AllowMsgTTL { | |||
| mset.mu.Unlock() | |||
There was a problem hiding this comment.
We can do this before proposing to the NRG so no need to bump CLFS etc.
There was a problem hiding this comment.
This was specific to the downstream stream not having per msg ttls enabled correct?
There was a problem hiding this comment.
Have added code to do the check before proposal in clustered mode too as well as a unit test to check CLFS wasn't bumped. We still need it in processJetStreamMsg for single server setups though.
neilalexander
force-pushed
the
neil/ttl
branch
from
4d4187b to
6ea9fe0
Compare
…mirroring Signed-off-by: Neil Twigg <neil@nats.io>
neilalexander
force-pushed
the
neil/ttlsourcing
branch
from
71216b1 to
5a4bbea
Compare
Now that sourcing/mirroring strips the TTL header, it is safe to reject messages with the
Nats-TTLheader ifAllowMsgTTLis not set on the stream.Sourcing/mirroring tests are both clustered and non-clustered as there are different codepaths for sourcing/mirroring when clustered.
Signed-off-by: Neil Twigg neil@nats.io