search

confluentinc / kafka-rest

Confluent REST Proxy for Kafka
https://docs.confluent.io/current/kafka-rest/docs/index.html
Other
22 stars 641 forks source link

Misconfigured v2 consumer creation returns successfully and subsequent consume calls cause null pointer exceptions #478

Open jkahrman opened 5 years ago

jkahrman commented 5 years ago

The documentation for version 2 of the consumer creation API says that invalid consumer configuration results in error code 42204. (https://docs.confluent.io/current/kafka-rest/docs/api.html#post--consumers-(string-group_name)). However, the API appears to fail silently and successfully return with a URL for the consumer that was not created.

While trying to adjust some of the default parameters, I triggered this exception in the KafkaConsumer constructor in the Kafka Client:

  org.apache.kafka.common.KafkaException: Failed to construct kafka consumer
       caused by:
       org.apache.kafka.common.config.ConfigException: request.timeout.ms should be greater than session.timeout.ms and fetch.max.wait.ms

However this exception was caught and dropped by the following code: https://github.com/confluentinc/kafka-rest/blob/d1cbb634b3f316de5706210d84433b2eaf38e673/kafka-rest/src/main/java/io/confluent/kafkarest/v2/KafkaConsumerManager.java#L201-L210

When the client then tried to consume using the non-existent consumer, the following code threw a null pointer exception: https://github.com/confluentinc/kafka-rest/blob/d1cbb634b3f316de5706210d84433b2eaf38e673/kafka-rest/src/main/java/io/confluent/kafkarest/v2/KafkaConsumerState.java#L401-L405

It's not clear from the check-in comment of https://github.com/confluentinc/kafka-rest/commit/1039b6a2bd307953ead68627d59644ef75ee56ed#diff-d90e84976538bb813186a66c51116783L202 why the exception started being ignored but I may not be using git correctly. It's also unclear to me why the exception wrapping was removed from v2. v1 API:

https://github.com/confluentinc/kafka-rest/blob/1039b6a2bd307953ead68627d59644ef75ee56ed/src/main/java/io/confluent/kafkarest/ConsumerManager.java#L174-L183

This may be the cause of issues https://github.com/confluentinc/kafka-rest/issues/445 and https://github.com/confluentinc/kafka-rest/issues/386

stanislavkozlovski commented 5 years ago

Hi @jkahrman,

Thanks for this great, very clear report of the issue. I also independently found this out myself while implementing a feature and fixed it. It is in this PR: https://github.com/confluentinc/kafka-rest/pull/481 When it gets merged, we should throw an exception on invalid configuration outright