Make protobuf file serde better support environment variable configs

ajesk commented 2 months ago

Issue submitter TODO list

[X] I've looked up my issue in FAQ
[X] I've searched for an already existing issues here
[X] I've tried running main-labeled docker image and the issue still persists there
[X] I'm running a supported version of the application which is listed here

Describe the bug (actual behavior)

When using environment variables to configure kafka-ui. There is a case sensitivity issue with how the topic/proto key value pairs are generated. I know that this is not the preferred way for configuring the service, but it is the most accessible way to configure when using Kubernetes.

Using an example from the documentation

protobufMessageNameForKeyByTopic:
              topic1: my.KeyType1
              topic2: my.KeyType2

becomes

PROTOBUFMESSAGENAMEFORKEYBYTOPIC_TOPIC1=my.KeyType1
PROTOBUFMESSAGENAMEFORKEYBYTOPIC_TOPIC2=my.KeyType2

In this case the topics both lose their casing. Within the code, it seems that there are no case sensitivity checks that occur so the topics included will never have access to the protos that should be assigned.

Writing

Optional<Map<String, String>> protobufMessageNameByTopic =
          properties.getMapProperty("protobufMessageNameByTopic", String.class, String.class);
      protobufMessageNameByTopic
          .ifPresent(messageNamesByTopic -> addProtobufSchemas(descriptorPaths, protobufSchemas, messageNamesByTopic));

Reading

var pattern = type == Serde.Target.KEY
          ? serdeInstance.topicKeyPattern
          : serdeInstance.topicValuePattern;
      if (pattern != null
          && pattern.matcher(topic).matches()

Expected behavior

Either the configuration structure needs to be slightly modified to support storing the case sensitive topics.

protobufMessageNameForKeyByTopic:
              - name: topic1
                proto: my.KeyType1
              - topic: topic2
                proto: my.KeyType2

Or adjust the pattern checks to ignore casing.

Your installation details

Kubernetes. Not completely applicable

Steps to reproduce

Configure protofile serdes via env variables with lowercase topic names. See that the protofiles are not accessible to the topic.

Screenshots

No response

Logs

No response

Additional context

If this seems like a reasonable fix, I am willing to make the adjustments in the preferred manner.

github-actions[bot] commented 2 months ago

Hi ajesk! 👋

Welcome, and thank you for opening your first issue in the repo!

Please wait for triaging by our maintainers.

As development is carried out in our spare time, you can support us by sponsoring our activities or even funding the development of specific issues. Sponsorship link

If you plan to raise a PR for this issue, please take a look at our contributing guide.

Haarolean commented 1 month ago

Hi @ajesk, I suggest that a pattern check ignoring the case is preferred here as it won't introduce breaking changes. Feel free to submit a PR!

luckyrider commented 1 month ago

Hello! Sorry, can we use regex for topic name in protobufMessageNameByTopic?

ajesk commented 1 month ago

There are regex patterns already in place. I performed work removing them because they were just being used as a strict less performant equals alternative.

If this isn't ideal I can just leave them there and append a case insensitive check.

On Mon, Sep 16, 2024, 18:29 luckyrider @.***> wrote:

Hello! Sorry, can we use regex for topic name in protobufMessageNameByTopic?

— Reply to this email directly, view it on GitHub https://github.com/kafbat/kafka-ui/issues/538#issuecomment-2354140712, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADAGTVPRP4YFBRWSCZYP5ETZW5LVZAVCNFSM6AAAAABNZS2RS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJUGE2DANZRGI . You are receiving this because you were assigned.Message ID: @.***>

kafbat / kafka-ui