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

Improve ToxiProxyContainer test and docs #6065

Merged
merged 2 commits into from Nov 4, 2022
Merged

Improve ToxiProxyContainer test and docs #6065

merged 2 commits into from Nov 4, 2022

Conversation

eddumelendez
Copy link
Member

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.

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.
@eddumelendez eddumelendez requested a review from a team as a code owner October 26, 2022 18:33
kiview
kiview previously approved these changes Oct 27, 2022
Copy link
Member

@kiview kiview left a 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.
Copy link
Member

Choose a reason for hiding this comment

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

This is so opinionated 😅

Copy link
Member Author

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

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?

@eddumelendez eddumelendez added this to the next milestone Nov 1, 2022
@perlun
Copy link
Contributor

perlun commented Apr 13, 2023

Note: the MR title/release notes are a bit misleading here. Since ToxyproxyContainer.getProxy() was now deprecated this might actually break the build for projects relying on -Werror (treat warnings as errors).

The workaround if your code to extract some of the code that previously has been available in ToxiproxyContainer into your own code (perhaps as static helper methods). Something like this:

// 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 ToxyproxyContainer.setConnectionCut(), things are a bit complex but copy the following snippet to your helper class and you should be covered:

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. 👍

magro added a commit to tomorrow-one/transactional-outbox that referenced this pull request Dec 27, 2023
To deal with the deprecation of `ToxiproxyContainer.ContainerProxy`
which got introduced with testcontainers 1.17.6 via PR testcontainers/testcontainers-java#6065
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants