optimizely/java-sdk

[BUG] Setting a proxy via OptimizelyFactory creates proxy-less clients

ben-r opened this issue · 6 comments

ben-r commented

Is there an existing issue for this?

  • I have searched the existing issues

SDK Version

4.0.0

Current Behavior

Currently the only way to set an dedicated proxy (disregarding environment settings) is via OptimizelyFactory.newDefaultInstance() (#330).

Optimizely instances created this way are able to use the proxy e.g. for fetching the data file (via cdn.optimizely.com), but both, AsyncEventHandler and the newly added ODPApiManager disregard the proxy setting by creating their own http clients further down the constructor chain.

public static Optimizely newDefaultInstance(String sdkKey, String fallback, String datafileAccessToken, CloseableHttpClient customHttpClient) {
        NotificationCenter notificationCenter = new NotificationCenter();
        OptimizelyHttpClient optimizelyHttpClient = new OptimizelyHttpClient(customHttpClient);
        HttpProjectConfigManager.Builder builder;
        builder = HttpProjectConfigManager.builder()
            .withDatafile(fallback)
            .withNotificationCenter(notificationCenter)
            .withOptimizelyHttpClient(customHttpClient == null ? null : optimizelyHttpClient) <-- uses correct client
            .withSdkKey(sdkKey);

        if (datafileAccessToken != null) {
            builder.withDatafileAccessToken(datafileAccessToken);
        }

        return newDefaultInstance(builder.build(), notificationCenter);
}
public static Optimizely newDefaultInstance(ProjectConfigManager configManager, NotificationCenter notificationCenter) {
        EventHandler eventHandler = AsyncEventHandler.builder().build(); <-- creates own client
        return newDefaultInstance(configManager, notificationCenter, eventHandler);
}
public static Optimizely newDefaultInstance(ProjectConfigManager configManager, NotificationCenter notificationCenter, EventHandler eventHandler) {
        if (notificationCenter == null) {
            notificationCenter = new NotificationCenter();
        }

        BatchEventProcessor eventProcessor = BatchEventProcessor.builder()
            .withEventHandler(eventHandler)
            .withNotificationCenter(notificationCenter)
            .build();

        ODPApiManager defaultODPApiManager = new DefaultODPApiManager(); <-- creates own clients
        ODPManager odpManager = ODPManager.builder()
            .withApiManager(defaultODPApiManager)
            .build();

        return Optimizely.builder()
            .withEventProcessor(eventProcessor)
            .withConfigManager(configManager)
            .withNotificationCenter(notificationCenter)
            .withODPManager(odpManager)
            .build();
}

Expected Behavior

Setting the proxy once should do it for every SDK-internal client.

Configuring the proxy is unnecessarily complicated at the moment. It should be part of the Optimizily.Builder API

Steps To Reproduce

  1. Create Optimizely instance via
import com.optimizely.ab.Optimizely;
import com.optimizely.ab.OptimizelyFactory;
import org.apache.http.HttpHost;
import org.apache.http.impl.client.HttpClients;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

@Configuration
public class OptimizelyConfig {

    @Bean
    ProxySettings externalProxy(
            @Value("${proxy.config.host:}") String host,
            @Value("${proxy.config.port:}") Integer port) {
        return new ProxySettings(host, port);
    }

    @Bean
    Optimizely createOptimizely(
            @Value("${optimizely.fullstack.sdk.key}") String optimizelySdkKey,
            @Qualifier("externalProxy") ProxySettings proxySettings) {

        var clientBuilder = HttpClients.custom();
        if (proxySettings.isEnabled()) {
            clientBuilder.setProxy(new HttpHost(proxySettings.getHost(), proxySettings.getPort()));
        }

        return OptimizelyFactory.newDefaultInstance(optimizelySdkKey, null, null, clientBuilder.build());
    }

    public record ProxySettings(String host, Integer port) {
        public boolean isEnabled() {
            return host != null && !host.isBlank() && port != null;
        }
    }
}
  1. Start local proxy, e.g. squid
  2. Fetch the data file via optimizely.getOptimizelyConfig().getDatafile() and verify it's not empty. You should see requests to cdn.optimizely.com in the proxy logs.
  3. Create an event via optimizely.track(). You should see that requests to logx.optimizely.com don't appear in the proxy logs

Java Version

21

Link

No response

Logs

org.apache.http.conn.ConnectTimeoutException: Connect to logx.optimizely.com:443 [logx.optimizely.com/34.111.140.246] failed: Connect timed out
at org.apache.http.impl.conn.DefaultHttpClientConnectionOperator.connect(DefaultHttpClientConnectionOperator.java:151)
at org.apache.http.impl.conn.PoolingHttpClientConnectionManager.connect(PoolingHttpClientConnectionManager.java:376)
at org.apache.http.impl.execchain.MainClientExec.establishRoute(MainClientExec.java:393)
at org.apache.http.impl.execchain.MainClientExec.execute(MainClientExec.java:236)
at org.apache.http.impl.execchain.ProtocolExec.execute(ProtocolExec.java:186)
at org.apache.http.impl.execchain.RetryExec.execute(RetryExec.java:89)
at org.apache.http.impl.execchain.RedirectExec.execute(RedirectExec.java:110)
at org.apache.http.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:185)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:72)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:221)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:165)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:140)
at com.optimizely.ab.OptimizelyHttpClient.execute(OptimizelyHttpClient.java:62)
at com.optimizely.ab.event.AsyncEventHandler$EventDispatcher.run(AsyncEventHandler.java:229)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.net.SocketTimeoutException: Connect timed out
at java.base/sun.nio.ch.NioSocketImpl.timedFinishConnect(NioSocketImpl.java:551)
at java.base/sun.nio.ch.NioSocketImpl.connect(NioSocketImpl.java:602)
at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327)
at java.base/java.net.Socket.connect(Socket.java:633)
at org.apache.http.conn.ssl.SSLConnectionSocketFactory.connectSocket(SSLConnectionSocketFactory.java:368)
at org.apache.http.impl.conn.DefaultHttpClientConnectionOperator.connect(DefaultHttpClientConnectionOperator.java:142)
... 16 more

Severity

Blocking development

Workaround/Solution

No response

Recent Change

No response

Conflicts

No response

jaeopt commented

@ben-r Thanks for reportiing. I see the limitation. We'll look into it for a solution as suggested.
Setting proxy with system properties is not a solution for you?

ben-r commented

Thanks for the quick response @jaeopt. Unfortunately, system properties are not feasible for us, as we have no maintained nonProxyHosts list.

  • 👍1
ben-r commented

Hi @jaeopt can you estimate when a fixed version is going to be released?

jaeopt commented

@ben-r we're currently at full capacity. expected to have in 1-2 months roughly.

jaeopt commented

@ben-r A new release (4.1.0) includes the fix this issue. Let us know if it works good for you.

ben-r commented

Thanks @jaeopt , I can confirm the fix

  • 👍1