-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve ToxiProxyContainer test and docs #6065
Conversation
Current implementation exposed utilities to handle ToxiProxyClient and proxies. However, there are some issues which demonstrate that the container class is coupled to the client. Tests and docs have been updated in order to promote the usage of its own ToxiProxyClient. Also, a note about the number of ports reserved by Testcontainers is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's exactly the right thing to do, making the usage more idiomatic and also less confusing.
[Obtaining proxied host and port for connections from a different container](../../modules/toxiproxy/src/test/java/org/testcontainers/containers/ToxiproxyTest.java) inside_block:obtainProxiedHostAndPortForDifferentContainer | ||
<!--/codeinclude--> | ||
!!! note | ||
Currently, `ToxiProxyContainer` will reserve 31 ports, starting at 8666. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so opinionated 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just documenting for existing support and avoiding breaking changes.
@@ -86,6 +86,7 @@ public int getControlPort() { | |||
* @param port port number on the target service that should be proxied | |||
* @return a {@link ContainerProxy} instance | |||
*/ | |||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add to the Javadoc the deprecation comment, to use the Toxiproxy client directly instead?
Note: the MR title/release notes are a bit misleading here. Since The workaround if your code to extract some of the code that previously has been available in // Previously handled by ToxiproxyContainer, but since
// ToxiproxyContainer#getProxy(GenericContainer<?>, int) is now deprecated, we need to take care
// of the port handling ourselves. See https://github.com/testcontainers/testcontainers-java/pull/6065
// for details on the deprecation.
public static final int FIRST_PROXY_PORT = 8666;
// proxyPort should be between FIRST_PROXY_PORT and FIRST_PROXY_PORT+30
public static Proxy createProxy( ToxiproxyContainer toxiproxyContainer, String upstream, int proxyPort ) throws IOException {
ToxiproxyClient toxiproxyClient = new ToxiproxyClient( toxiproxyContainer.getHost(), toxiproxyContainer.getControlPort() );
return toxiproxyClient.createProxy(
upstream,
"0.0.0.0:" + proxyPort,
upstream
);
} And then in your calling code, do something like this in your test to add a toxic: proxy.toxics().timeout( "infinite-upstream-timeout", ToxicDirection.UPSTREAM, 0 ); Remember to tear down the proxy after your test has executed: @AfterEach
void tearDown() throws IOException {
if ( proxy != null ) {
proxy.delete();
}
} If your code relied on private static final Map<Proxy, Boolean> IS_CURRENTLY_CUT_MAP = new HashMap<>();
private static final String CUT_CONNECTION_DOWNSTREAM = "CUT_CONNECTION_DOWNSTREAM";
private static final String CUT_CONNECTION_UPSTREAM = "CUT_CONNECTION_UPSTREAM";
// Inspired by org.testcontainers.containers.ToxiproxyContainer.ContainerProxy#setConnectionCut,
// which is now deprecated.
/**
* Cuts the connection by setting bandwidth in both directions to zero.
*
* @param shouldCutConnection true if the connection should be cut, or false if it should be re-enabled
*/
public static void setConnectionCut( Proxy proxy, boolean shouldCutConnection ) {
try {
if ( shouldCutConnection ) {
proxy.toxics().bandwidth( CUT_CONNECTION_DOWNSTREAM, ToxicDirection.DOWNSTREAM, 0 );
proxy.toxics().bandwidth( CUT_CONNECTION_UPSTREAM, ToxicDirection.UPSTREAM, 0 );
IS_CURRENTLY_CUT_MAP.put( proxy, true );
}
else if ( IS_CURRENTLY_CUT_MAP.getOrDefault( proxy, false ) ) {
proxy.toxics().get( CUT_CONNECTION_DOWNSTREAM ).remove();
proxy.toxics().get( CUT_CONNECTION_UPSTREAM ).remove();
IS_CURRENTLY_CUT_MAP.put( proxy, false );
}
}
catch ( IOException e ) {
throw new RuntimeException( "Could not control proxy", e );
}
} @eddumelendez Before the deprecated method(s) are dropped completely (in 2.0?), it would perhaps be worth adding more about this migration path somewhere, to help people moving away from the deprecated methods. 👍 |
To deal with the deprecation of `ToxiproxyContainer.ContainerProxy` which got introduced with testcontainers 1.17.6 via PR testcontainers/testcontainers-java#6065
Current implementation exposed utilities to handle ToxiProxyClient
and proxies. However, there are some issues which demonstrate that
the container class is coupled to the client. Tests and docs have
been updated in order to promote the usage of its own ToxiProxyClient.
Also, a note about the number of ports reserved by Testcontainers
is added.