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

Add proxy container configuration #44

Merged
merged 3 commits into from
Jul 14, 2022
Merged

Add proxy container configuration #44

merged 3 commits into from
Jul 14, 2022

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Jul 7, 2022

A proxy container (i.e. Toxiproxy) can be optionally configured. This container allows to create a TCP proxy between test code and Kafka broker.

Every Kafka broker request will pass through the proxy where you can simulate network conditions (i.e. connection cut, latency). For example, you can create a network partition when running a multi-node cluster.

How it works?

The StrimziKafkaContainer class uses getBootstrapServers() to build the KAFKA_ADVERTISED_LISTENERS configuration. When proxyContainer is configured, the bootstrap URL returned by getBootstrapServers() contains the proxy host and port. For this reason, Kafka clients will always pass through the proxy, even after refreshing cluster metadata.

Additional info

This commit also adds a common super-interface for containers.

I changed the project version only because it doesn't seem to be in sync with releases.

A proxy container (i.e. Toxiproxy) can be optionally configured.
This container allows to create a TCP proxy between test code and Kafka broker.

Every Kafka broker request will pass through the proxy where you can simulate
network conditions (i.e. connection cut, latency). For example, you can create
a network partition when running a multi-node cluster.

This commit also adds a common super-interface for containers.

Signed-off-by: Federico Valeri <[email protected]>
@scholzj scholzj requested review from see-quick and tombentley July 7, 2022 13:07
@scholzj scholzj added this to the 0.103.0 milestone Jul 7, 2022
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Maybe you can add to the description some more explanation as to how your changes related to the aim of the PR and why are they needed for the proxy support since it is not really clear to me how some of your changes relate to it.

It would be also good to explain how does the proxy impact the advertised hosts in Kafka since that is not clear either.

Comment on lines 90 to 93
<module name="ClassDataAbstractionCoupling">
<!-- default is 7 -->
<property name="max" value="10"/>
</module>
Copy link
Member

Choose a reason for hiding this comment

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

Please use annotation rather than increasing the global limit.

Copy link
Contributor Author

@fvaleri fvaleri Jul 7, 2022

Choose a reason for hiding this comment

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

Right, I forgot to add the implementation details.

I think it is better to also add them at the javadoc level.

Signed-off-by: Federico Valeri <[email protected]>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

I'm still not sure I follow why the abstraction was needed.

@fvaleri
Copy link
Contributor Author

fvaleri commented Jul 7, 2022

I'm still not sure I follow why the abstraction was needed.

The abstraction is not strictly needed for this PR, but I think it's useful to have a common interface when dealing with tests using both single and multi-node deployments, as it doesn't force you to use Startable. This is also something that was requested before.

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

The abstraction is not strictly needed for this PR, but I think it's useful to have a common interface when dealing with tests using both single and multi-node deployments, as it doesn't force you to use Startable. This is also something that was requested before.

I guess ideally it should have a separate PR then ;-)

@scholzj scholzj merged commit 2695e79 into strimzi:main Jul 14, 2022
@fvaleri fvaleri deleted the toxiproxy branch July 14, 2022 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants