Hi Damien, Thanks for the KIP! Dead-letter queues are something that I think a lot of users would like.
I think there are a few points with this KIP that concern me: 1. It looks like you can only define a single, global DLQ for the entire Kafka Streams application? What about applications that would like to define different DLQs for different data flows? This is especially important when dealing with multiple source topics that have different record schemas. 2. Your DLQ payload value can either be the record value that failed, or an error string (such as "error during punctuate"). This is likely to cause problems when users try to process the records from the DLQ, as they can't guarantee the format of every record value will be the same. This is very loosely related to point 1. above. 3. You provide a ProcessorContext to both exception handlers, but state they cannot be used to forward records. In that case, I believe you should use ProcessingContext instead, which statically guarantees that it can't be used to forward records. 4. You mention the KIP-1033 ProcessingExceptionHandler, but what's the plan if KIP-1033 is not adopted, or if KIP-1034 lands before 1033? Regards, Nick On Fri, 12 Apr 2024 at 11:38, Damien Gasparina <d.gaspar...@gmail.com> wrote: > In a general way, if the user does not configure the right ACL, that > would be a security issue, but that's true for any topic. > > This KIP allows users to configure a Dead Letter Queue without writing > custom Java code in Kafka Streams, not at the topic level. > A lot of applications are already implementing this pattern, but the > required code to do it is quite painful and error prone, for example > most apps I have seen created a new KafkaProducer to send records to > their DLQ. > > As it would be disabled by default for backward compatibility, I doubt > it would generate any security concern. > If a user explicitly configures a Deal Letter Queue, it would be up to > him to configure the relevant ACLs to ensure that the right principal > can access it. > It is already the case for all internal, input and output Kafka > Streams topics (e.g. repartition, changelog topics) that also could > contain confidential data, so I do not think we should implement a > different behavior for this one. > > In this KIP, we configured the default DLQ record to have the initial > record key/value as we assume that it is the expected and wanted > behavior for most applications. > If a user does not want to have the key/value in the DLQ record for > any reason, they could still implement exception handlers to build > their own DLQ record. > > Regarding ACL, maybe something smarter could be done in Kafka Streams, > but this is out of scope for this KIP. > > On Fri, 12 Apr 2024 at 11:58, Claude Warren <cla...@xenei.com> wrote: > > > > My concern is that someone would create a dead letter queue on a > sensitive > > topic and not get the ACL correct from the start. Thus causing potential > > confidential data leak. Is there anything in the proposal that would > > prevent that from happening? If so I did not recognize it as such. > > > > On Fri, Apr 12, 2024 at 9:45 AM Damien Gasparina <d.gaspar...@gmail.com> > > wrote: > > > > > Hi Claude, > > > > > > In this KIP, the Dead Letter Queue is materialized by a standard and > > > independant topic, thus normal ACL applies to it like any other topic. > > > This should not introduce any security issues, obviously, the right > > > ACL would need to be provided to write to the DLQ if configured. > > > > > > Cheers, > > > Damien > > > > > > On Fri, 12 Apr 2024 at 08:59, Claude Warren, Jr > > > <claude.war...@aiven.io.invalid> wrote: > > > > > > > > I am new to the Kafka codebase so please excuse any ignorance on my > part. > > > > > > > > When a dead letter queue is established is there a process to ensure > that > > > > it at least is defined with the same ACL as the original queue? > Without > > > > such a guarantee at the start it seems that managing dead letter > queues > > > > will be fraught with security issues. > > > > > > > > > > > > On Wed, Apr 10, 2024 at 10:34 AM Damien Gasparina < > d.gaspar...@gmail.com > > > > > > > > wrote: > > > > > > > > > Hi everyone, > > > > > > > > > > To continue on our effort to improve Kafka Streams error handling, > we > > > > > propose a new KIP to add out of the box support for Dead Letter > Queue. > > > > > The goal of this KIP is to provide a default implementation that > > > > > should be suitable for most applications and allow users to > override > > > > > it if they have specific requirements. > > > > > > > > > > In order to build a suitable payload, some additional changes are > > > > > included in this KIP: > > > > > 1. extend the ProcessingContext to hold, when available, the > source > > > > > node raw key/value byte[] > > > > > 2. expose the ProcessingContext to the > ProductionExceptionHandler, > > > > > it is currently not available in the handle parameters. > > > > > > > > > > Regarding point 2., to expose the ProcessingContext to the > > > > > ProductionExceptionHandler, we considered two choices: > > > > > 1. exposing the ProcessingContext as a parameter in the handle() > > > > > method. That's the cleanest way IMHO, but we would need to > deprecate > > > > > the old method. > > > > > 2. exposing the ProcessingContext as an attribute in the > interface. > > > > > This way, no method is deprecated, but we would not be consistent > with > > > > > the other ExceptionHandler. > > > > > > > > > > In the KIP, we chose the 1. solution (new handle signature with old > > > > > one deprecated), but we could use other opinions on this part. > > > > > More information is available directly on the KIP. > > > > > > > > > > KIP link: > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams > > > > > > > > > > Feedbacks and suggestions are welcome, > > > > > > > > > > Cheers, > > > > > Damien, Sebastien and Loic > > > > > > > > > > > > > > -- > > LinkedIn: http://www.linkedin.com/in/claudewarren >
