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

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

Closed
1 task done
ben-r opened this issue Mar 4, 2024 · 6 comments
Closed
1 task done
Labels
acknowledged The issue has been acknowledged and being looked into. Further details will follow. bug

Comments

@ben-r
Copy link

ben-r commented Mar 4, 2024

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

@ben-r ben-r added the bug label Mar 4, 2024
@jaeopt jaeopt added the acknowledged The issue has been acknowledged and being looked into. Further details will follow. label Mar 4, 2024
@jaeopt
Copy link
Contributor

jaeopt commented Mar 4, 2024

@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
Copy link
Author

ben-r commented Mar 5, 2024

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

@ben-r
Copy link
Author

ben-r commented Mar 18, 2024

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

@jaeopt
Copy link
Contributor

jaeopt commented Mar 18, 2024

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

@jaeopt
Copy link
Contributor

jaeopt commented Apr 12, 2024

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

@jaeopt jaeopt closed this as completed Apr 12, 2024
@ben-r
Copy link
Author

ben-r commented Apr 29, 2024

Thanks @jaeopt , I can confirm the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged The issue has been acknowledged and being looked into. Further details will follow. bug
Projects
None yet
Development

No branches or pull requests

2 participants