launchdarkly / node-server-sdk

LaunchDarkly Server-side SDK for Node
Other
79 stars 65 forks source link

Method `LaunchDarkly.init` blows up when the user provides an invalid key #288

Open jpinho opened 7 months ago

jpinho commented 7 months ago

Describe the bug The method LaunchDarkly.init blows up when the user provides an invalid key.

To reproduce Call LaunchDarkly.init with an invalid key.

Expected behavior As a FF SDK I would expect the SDK to never blow up. Instead the init method should return a client with a failure state, that can be check unattended. Example below:

const client = LaunchDarkly.init()

client.initialized() => false
client.getStatus() => { initialized: false, initError: 'invalid SDK key', connectionEstablished: true }

I can provide a contribution, if we have an agreement here.

Logs

    error: [LaunchDarkly] Authentication failed. Double check your SDK key.
    error: [LaunchDarkly] Received error 401 (invalid SDK key) for streaming request - giving up permanently

SDK version

"launchdarkly-node-server-sdk": "^7.0.3",

Language version, developer tools

OS/platform

kinyoklion commented 7 months ago

Hello @jpinho,

To clarify here, you don't mean an invalid SDK, but calling the init function without any parameters?

If you provide parameters, but they are invalid, the behavior is different. If you provide a key, but it is empty, then it would not encounter this situation. This is specifically the only condition under which the SDK will throw an error such as this during initialization (at least very intentionally). (Unless you are offline as well, in which case it will continue). It typically represents a programming error that we want to communicate quickly.

I know I reminded on the other issue, but this is not the current repository. Especially if you are configuring a new project you should use the 9.x version. @launchdarkly/node-server-sdk.

Thank you, Ryan

jpinho commented 6 months ago

Hey @kinyoklion thanks for your answer and for suggesting me the right SDK! I got it from your website, see this link and the image attached: https://launchdarkly.com/feature-flags-node-js

Screenshot 2024-02-14 at 12 15 52

Regarding the initialisation, I'm not convinced, I see your point of wanting to fail fast and giving immediate feedback to the user. But specially being this a FF client which is never mission critical, throwing runtime errors is very unsafe. See the example below:

    darklyClientInstance = initDarkly(config.LAUNCHDARKLY_SDK_KEY, {
      timeout: LAUNCH_DARKLY_CONN_TIMEOUT_SECONDS,
      logger: Log,
    });

In the case above, I'm passing a config.LAUNCHDARKLY_SDK_KEY, due to an error loading the SDK Key config, I may pass you undefined. Therefore, your SDK has no way to know if I'm actually passing something that evals to undefined or if I'm not passing the argument at all.

Now imagine a critical service running in production that fails to load a FF SDK key and fails live customer request... that doesn't look good. Of course, resiliency can be done on the service side, but this for us Launch Darkly customers means we have to run custom code to add a bunch of resiliency when dealing with LaunchDarkly client. And now multiply this effort for +10 or more micro-services... suddenly we need a shared-library 🙈 . If LD client could be just super resilient and never throw exception and instead log errors that would be way nicer.

kinyoklion commented 6 months ago

The general thought is if you don't provide a key, then it can never work, and we should let you know. This example here does assume that you are going to define your key.

If you really want to allow that behavior, than it would be:

process.env.LAUNCHDARKLY_SDK_KEY ?? "invalid"

This will never throw because you are providing a key, even if you are intentionally providing a bad one. You will later get a 401 and we will log that, and subsequently evaluations will always report the default variation.

You don't have to provide a valid key, but you do have to provide something in order to not hit that condition.

kinyoklion commented 6 months ago

For this issue it requires some internal discussion. Generally speaking configuration errors we have treated as usage errors. Basically code that isn't correct and will never become correct on its own. For instance not including the SDK key is something you may encounter during dev, but once it was in production wouldn't stop at runtime.

I see your case is a little different and does merit discussion.

jpinho commented 6 months ago

Shall I move these issues under the proper repo @kinyoklion?

kinyoklion commented 6 months ago

That would be great. Thank you!

jpinho commented 6 months ago

Ok, I will take care of the clean up for us then 😉!

Thanks for the support.

On 14 Feb 2024, at 17:58, Ryan Lamb @.***> wrote:

That would be great. Thank you!

— Reply to this email directly, view it on GitHub https://github.com/launchdarkly/node-server-sdk/issues/288#issuecomment-1944329239, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDQPZ5JE6NAYKXQ77U3NFTYTT3OBAVCNFSM6AAAAABCPKSRZGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBUGMZDSMRTHE. You are receiving this because you were mentioned.