launchdarkly / dotnet-client-sdk

LaunchDarkly Client-side SDK for .NET
Other
7 stars 8 forks source link

Support Paths in Configuration.*Uri #26

Closed borfig closed 3 years ago

borfig commented 3 years ago

Is your feature request related to a problem? Please describe. Due to regulation, there are scenarios when our mobile clients are in a closed network and are not allowed to connect directly to *.launchdarkly.com. However, it is allowed to deploy a proxy server in the DMZ for each external FQDN. [1]

Describe the solution you'd like To reduce changes to the internal network to an absolute minimum, it is desired to proxy launchdarkly requests along with our own requests on a single FQDN. The LaunchDarkly client will be initialized with:

var config = LdConfiguration.Builder(settingsLaunchDarkly.MobileKey)
    .BaseUri(new Uri("https://proxy.example.internal/launchdarkly/api"))
    .EventsUri(new Uri("https://proxy.example.internal/launchdarkly/mobile"))
    .StreamUri(new Uri("https://proxy.example.internal/launchdarkly/stream"))
    .Build();

The proxy would strip the prefix and forward the request as-is to the correct LaunchDarkly server directly. The LaunchDarkly client configuration must not strip the paths provided by the configuration, in order to make the above example usable.

Describe alternatives you've considered If the *Uri Paths are discarded as in v1.2.0, we would need to deploy 3 additional proxy servers (or 3 virtual hosts) for the base, events, and stream URIs, in addition to our own existing one. Such a change also requires changing the internal DNS to add 3 additional records. The LaunchDarkly client will be initialized with:

var config = LdConfiguration.Builder(settingsLaunchDarkly.MobileKey)
    .BaseUri(new Uri("https://api.launchdarkly.example.internal/"))
    .EventsUri(new Uri("https://mobile.launchdarkly.example.internal/"))
    .StreamUri(new Uri("https://clientstream.launchdarkly.example.internal/"))
    .Build();

This is not ideal for us, and it generates unnecessary friction.

Additional context For example, the line at [2] is an example of a logic that discards any .Path value, and should be replaced with .Path += path or equivalent.

[1] https://www.hysolate.com/blog/hysolate-in-a-closed-network-with-nginx/ [2] https://github.com/launchdarkly/xamarin-client-sdk/blob/625397741e51a90efe88cc8d430fe0636a5f2263/src/LaunchDarkly.XamarinSdk/FeatureFlagRequestor.cs#L72

eli-darkly commented 3 years ago

Yes, we should fix that. It wasn't our intention to discard the base path, and I know that we do retain the base path in most other SDKs, though there may be others where we made a similar mistake to this.

I do have one question, not directly relevant to this issue but something I wanted to make sure wasn't a sign of some misunderstanding. ~In your example code, is there a particular reason that you're using the path /mobile for the events base URI? There's nothing mobile-specific about that host, even though one of the endpoints on the host is used for mobile events. Again, this probably isn't of any real significance but I just wanted to clarify.~

eli-darkly commented 3 years ago

Ah, strike that last part— I forgot that we do in fact use a hostname mobile.launchdarkly.com in this SDK. The underlying service is the same as events.launchdarkly.com, but that explains why you were calling it that. Not important.

borfig commented 3 years ago

Yes, you can replace /mobile with /events in the example above.

eli-darkly commented 3 years ago

We'll release a patch for this as soon as possible. The patch itself is straightforward, but we hadn't done a Xamarin SDK release in a while and we found that the build tools have changed enough to break our release process, so we need to resolve that first. Thanks again for reporting the bug.

eli-darkly commented 3 years ago

Fixed in the 1.2.1 release. Sorry this took a while.