Skip to content

Fix HTTP CONNECT proxy options not passing to core #336

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

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

adeal
Copy link
Contributor

@adeal adeal commented Aug 24, 2024

What was changed

Why?

Follow-up fix to #318.

The proxy settings were not being pushed down to core. Although the ClientHttpConnectProxyOptions were properly translated to core's HttpConnectProxyOptions, the TryFrom trait on the CoreClientOptions was not updated to include the new ClientHttpConnectProxyOptions.

Checklist

  1. Closes
    N/A

  2. How was this tested:

Unfortunately, there still is no viable path for automated test coverage as-is. However, I validated this change by configuring a temporal app to point to an invalid proxy location and verified that a connection was not successful. Previously, no error would occur since the connection was not being routed to the proxy target.

var client = await TemporalClient.ConnectAsync(new TemporalClientConnectOptions()
{
    TargetHost = "<valid_host>",
    HttpConnectProxy = new HttpConnectProxyOptions("localhost:9392"), // Arbitrary invalid location
});
Unhandled exception. System.InvalidOperationException: Connection failed: Server connection error: tonic::transport::Error(Transport, ConnectError(client error (Connect)

Caused by:
    Connection refused (os error 61)))
   at Temporalio.Bridge.Client.ConnectAsync(Runtime runtime, TemporalConnectionOptions options)
   at Temporalio.Client.TemporalConnection.GetBridgeClientAsync()
   at Temporalio.Client.TemporalConnection.ConnectAsync(TemporalConnectionOptions options)
   at Temporalio.Client.TemporalClient.ConnectAsync(TemporalClientConnectOptions options)
   at Program.<Main>$(String[] args) in /Users/adeal/src/samples-dotnet/src/ActivitySimple/Program.cs:line 15
   at Program.<Main>(String[] args)

  1. Any docs updates needed?

@adeal adeal force-pushed the dev/adeal/http_connect_fix branch from 8a1ed05 to 6e501e8 Compare August 25, 2024 06:40
@cretz
Copy link
Member

cretz commented Aug 26, 2024

We will prioritize working on temporalio/features#521

@cretz cretz merged commit aa3edef into temporalio:main Aug 26, 2024
8 checks passed
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.

2 participants