Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make protobuf file serde better support environment variable configs #538

Open
4 tasks done
ajesk opened this issue Sep 7, 2024 · 4 comments
Open
4 tasks done

Make protobuf file serde better support environment variable configs #538

ajesk opened this issue Sep 7, 2024 · 4 comments
Assignees
Labels
area/serde Serialization & Deserialization (plugins) good first issue Up for grabs scope/backend Related to backend changes status/triage/completed Automatic triage completed type/bug Something isn't working type/enhancement En enhancement/improvement to an already existing feature

Comments

@ajesk
Copy link

ajesk commented Sep 7, 2024

Issue submitter TODO list

  • I've looked up my issue in FAQ
  • I've searched for an already existing issues here
  • I've tried running main-labeled docker image and the issue still persists there
  • 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.

@ajesk ajesk added status/triage Issues pending maintainers triage type/bug Something isn't working labels Sep 7, 2024
Copy link

github-actions bot commented Sep 7, 2024

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
Copy link
Member

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!

@Haarolean Haarolean added good first issue Up for grabs type/enhancement En enhancement/improvement to an already existing feature scope/backend Related to backend changes area/serde Serialization & Deserialization (plugins) and removed status/triage Issues pending maintainers triage labels Sep 10, 2024
@Haarolean Haarolean added the status/triage/completed Automatic triage completed label Sep 10, 2024
@luckyrider
Copy link

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

@ajesk
Copy link
Author

ajesk commented Sep 17, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/serde Serialization & Deserialization (plugins) good first issue Up for grabs scope/backend Related to backend changes status/triage/completed Automatic triage completed type/bug Something isn't working type/enhancement En enhancement/improvement to an already existing feature
Projects
Status: No status
Development

No branches or pull requests

3 participants