Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Initial2 #16

Merged
merged 14 commits into from
May 6, 2022
Merged

Initial2 #16

merged 14 commits into from
May 6, 2022

Conversation

chris-giblin
Copy link
Contributor

This PR consolidates changes made during the discussion in PR #9 which this PR supersedes

Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
@chris-giblin chris-giblin requested a review from tombentley April 25, 2022 09:39
@chris-giblin chris-giblin mentioned this pull request Apr 25, 2022
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor points, then let's get this merged!

# Getting started

Requirements:
- a Kafka instance, version 2.8.0 or older, which you can configure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the 2.8.0 requirement come from? Or is this really a statement about what you've tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kafka 3.1.0 introduces topic id's to FETCH requests (KIP 516). This version of topic encryption does not support topic id's, rather it performs string-matching to identify topic partitions in protocol messages.
Actually Kafka 3.0 will also work with this release, but 2.8.0 is what is tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created an issue to address topic IDs: #17

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

encmod/pom.xml Outdated
Comment on lines 18 to 29
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these can be test scoped in this module, and then let the vertx-proxy module pull them in as runtime dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<dependency>
<groupId>io.strimzi</groupId>
<artifactId>encmod</artifactId>
<version>0.0.1-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be ${project.version}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I just committed it. Thanks.

@chris-giblin chris-giblin merged commit ef7812b into main May 6, 2022
@chris-giblin chris-giblin deleted the initial2 branch May 6, 2022 09:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants