Return disabled toggles as well on the proxy endpoint

bogdanzurac commented 1 year ago

Describe the feature request

Currently, the proxy endpoint returns only toggles evaluated as enabled by the Proxy for the current user, as stated inside the docs https://docs.getunleash.io/sdks/unleash-proxy#payload. It there any particular reasoning for this design choice?

Background

In our flows we would need to know if the Proxy performed a proper evaluation to disabled OR if the Unleash instance doesn't even have the queried toggle defined.

Solution suggestions

Can this be achieved, if not through the default proxy endpoint implementation, then maybe by using either a different endpoint or a query param? Subsequently, can this be added inside the Android Proxy SDK and the rest of the SDKs (https://docs.getunleash.io/sdks/unleash-proxy#how-to-connect-to-the-proxy)?

FredrikOseberg commented 1 year ago

Hi @bogdanzurac

The reason behind this design choice is to limit the payloads going down to the clients. There are situations where an organization using unleash may have a lot of feature toggles, and it's redundant to pass down other feature flags. Most of the time, it's enough for the frontends / client applications to know what is on/off without any other context.

It would be interesting to understand your use-case a bit better, could you elaborate a bit on why you need to know whether it was evaluated or not in the client?

While it's possible to create such an implementation, it's a change that has wide-reaching implications and will require some work to implement.

bogdanzurac commented 1 year ago

We've got around 150+ feature toggles that we currently handle through another feature toggling system. We are in the process of migrating to Unleash and for some period of time we need to handle both systems at the same time + our own local features provider. Thus, we need to know when one of the feature toggles that we query Unleash for is actually defined on Unleash or it's just disabled, in order to fallback to our current feature toggling system instead.

We do appreciate that this isn't the type of change that can be performed overnight and has wider implications to other customers. Hence why I mentioned if this can be performed with a query param, so that it doesn't impact the existing implementation.

The workaround that we're currently contemplating is to rely on both the proxy call that returns only the toggles evaluated as enabled for the current user + the proxy/client/features call that returns the entire list of toggles that Unleash has defined. But that requires doing 2 API calls instead of only 1.

ivarconr commented 1 year ago

can be performed with a query param We would rather not do that, as it can allow people to discover feature toggles that should not be visible to the frontend at all.

But I am open to having this as a configuration option for the proxy itself. Should be straight forward to support this.

Would you guys be willing to look in to a PR For this?

bogdanzurac commented 1 year ago

@ivarconr How would that configuration work more precisely? What would it change on the existing proxy call?

sighphyre commented 1 year ago

Hey @bogdanzurac so right now the proxy client options look something like

export interface IProxyOption {
    unleashUrl?: string;
    unleashApiToken?: string;

    //some more options

If we can add another option, something like:

export interface IProxyOption {
    unleashUrl?: string;
    unleashApiToken?: string;
    allowDisabledToggles?: boolean; // <-- the new option, the name may need some work
    //some more options

That new boolean can control the behaviour in getEnabledToggles

I think our big concern are that this should be opt in behaviour that defaults to off so that it doesn't change existing installations

I'm not sure if that answers your question?

bogdanzurac commented 1 year ago

Hi @sighphyre I totally agree with the default being off, in order to not change the current behavior.

My only gripe with this approach is that it's not dynamic. AKA it has effect on all requests, regardless of the client that uses the API. In our implementation we will probably need to expose the disabled toggles only for our mobile clients (Android & iOS apps), while retaining the existing behavior for our backend applications. Hence why my initial thought was to introduce a query param on the existing 'proxy' API endpoint. That way, our mobile apps can use the new query param in order to also get the disabled toggles, while our backends can use the existing endpoint as is.

If the query param is not feasible, then we can try to use a different proxy server for the mobile apps with the new proxy client option to also send the disabled toggles. It wouldn't be ideal, since it would double our proxy server count, but it will indeed get the job done.

sighphyre commented 1 year ago

@bogdanzurac I think there's a workable middle ground here - I think it's okay to have the behaviour dynamic through the query parameter so long as that behaviour is only accessible when the configuration parameter is set on startup. What do you think?

bogdanzurac commented 1 year ago

@sighphyre That would be a perfect fit for our needs, if you guys think that's doable as well on your end.

On an implementation detail level, I assume that requires updates to both the proxy library in order to support the new configuration param, as well as all client libraries (such as Android) in order to support the new query param through the SDK, right?

sighphyre commented 1 year ago

@bogdanzurac Yeah, this will definitely need changes across a few projects, for the iOS and Android SDKs that's probably isolated only to those projects. If you folks want to tackle the SDKs that you care about, we can always build the remainder on an as needed basis when we get asked.

bogdanzurac commented 1 year ago

@sighphyre Yes, we can help out with PRs on the mobile SDKs side, in order to get us unblocked. But we can start on that only after the support is baked into the proxy itself or at the very least the API surface is set in stone (more specifically the query param name and type/value).

Also, I presume that in the list of toggles returned on the proxy endpoint, the disabled toggles will be returned without any variant object, like so, correct?

{
   "name": "disabledToggle",
   "enabled": false
}

This way, they will get evaluated to false by default from the existing implementation.

sighphyre commented 1 year ago

@bogdanzurac This is a new thing so I don't think I have a "true" answer for this question but yes, this looks like the correct thing to be doing here

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

thomasheartman commented 1 year ago

Hey 👋🏼 From what I can tell, this was solved by #100, so I'm gonna go ahead and close it. Feel free to reopen this issue if you think it hasn't been completely resolved or otherwise needs to be kept open.

Thanks 😄

bogdanzurac commented 1 year ago

I can confirm this works like a charm. Thanks a lot guys for the help!

bogdanzurac commented 1 year ago

Hello again. After deploying this new /proxy/all endpoint for multiple environments, we discovered that it returns all feature toggles from all projects. So if we have a project that is not available for a particular environment, the /proxy/all endpoint also returns the feature toggles from that project. They are disabled, but we would've expected them to not be returned at all.

thomasheartman commented 1 year ago

Hey! 👋🏼

That's an interesting point. Is this the correct understanding:

You have multiple projects, including Project A and Project B
You also have a number of environments, including Env E
Project A has Env E enabled, so you can freely add strategies to features in Project A
Project B does not have Env E enabled, so Env E isn't visible in Project B
When you fetch features from Env E with the proxy/all endpoint, you're also getting all features in Project B, even though Project B doesn't have Env E enabled.

Does that line up?

If so, then that's because fetaures in Unleash live across all environments. But at the same time, I can see why you find that a little surprising. It might be worth discussing, but changing it would be a longer process because it would be a breaking change.

Does this pose a problem for you or is it just unexpected?

bogdanzurac commented 1 year ago

Yes, that's the correct example. We know that feature flags live across environments and we actually want that and rely on that for our entire infrastructure. But at the same time, we would still expect that if you specifically specify that Env E isn't available at all for Project B, then Project B's feature flags shouldn't be returned for env E. Otherwise what's the point of excluding an environment from a project?

thomasheartman commented 1 year ago

Yeah, I see why it's unexpected. The point of excluding environments from a project is so that you won't be able to add any strategies in that environment and that the project's features will always be disabled in that environment. Secondarily, it also makes the admin UI easier to work with.

In general, Unleash doesn't differentiate between "disabled" and "not defined", whether it's "not defined" because it's not active in the environment, because it has no strategies, or because it doesn't have a configuration for that environment at all.

We could probably document this better, though, so I'll look into that.

Is it a problem for you that it works like this?

bogdanzurac commented 1 year ago

It poses a problem for our implementation because we use environments as individual apps and projects as features that are part of an app.

That means that we might have this scenario:

App A and B both have Features 1 and 2.
App A has both Features 1 and 2 feature toggled remotely from Unleash
App B has Feature 1 toggled from Unleash, but Feature 2 is hardcoded locally as enabled all the time, not from Unleash

Due to how our infrastructure works, for both apps we first try to check the feature toggles remotely from Unleash and if they're not defined/returned by Unleash, then the local values are taken into account instead.

That means that in the example above, Feature 2 will be returned as disabled for App B, thus we won't take into account the fact that it's actually hardcoded as enabled locally for App B.

We would've expected, like mentioned previously, that Feature 2 feature toggles to not be returned at all for App B.

thomasheartman commented 1 year ago

Sorry, I'm having a little trouble following:

You say you use environments as applications and projects as features. Later when you describe the scenario and you talk about apps and features:

are you actually talking about (Unleash's) environments and projects
or do are you talking about applications and (Unleash's) features

Anyway, you're saying that you have some sort of wrapper around Unleash that checks whether the toggle exists or not. If it doesn't exist, then you fall back to whatever you have set as the fallback?

And I'm curios now: could you explain to me why you're using environments and features in that way? What does it give you? It seems a little confusing to me, but I'm sure you must have a reason for it.

bogdanzurac commented 1 year ago

In my previous example an App == Unleash Env, a Feature == Unleash Project.

We have this setup because we have a whitelabel project deploying around 30-40 features to a total of 6 mobile apps. Some apps might have only a subset of all those 30-40 features.

All those features have feature flags that have a unique key shared across all 6 apps, so Unleash's 1 toggle per all environments fits our needs very well from this point of view. That's why an app from our perspective needs to be mapped to an environment from Unleash's perspective.

As for Unleash Projects, we use that in order to group all toggles that are used in a particular app feature, in order to not have all feature toggles as part of a single project.

thomasheartman commented 1 year ago

Okay, assuming that an "app feature" is an Unleash project:

You have 6 different apps that share 30-40 app features. Some apps only use a subset of these.

As for Unleash Projects, we use that in order to group all toggles that are used in a particular app feature, in order to not have all feature toggles as part of a single project.

One app feature can use multiple Unleash features to control it in a granular fashion. Using Unleash environments for apps would then also let you turn off all sub-features (Unleash features) that belong to that app feature with a single control.

Yeah?

And you would like apps that don't have access to a particular app feature to not be able to see any of the app features sub-features at all? In Unleash terms, if a project doesn't have an environment enabled, then none of the projects toggles should be returned in that environment?

And the reason for this is that you use a fallback mechanism where if a feature isn't defined in Unleash, then it should fall back to a default value that's encoded locally instead?

The thing is, Unleash was never designed to evaluate features into other states than true or false (unless you use variants). And using projects and environments in this way is not something we anticipated.

I think my best suggestion for now would be to apply strategies that evaluate to the fallback value in apps (Unleash environments) that you don't use but I'll see if anyone else has any suggestions and get back to you.

You could probably also use variants in some way, but I'm not sure that would be a smoother ride 🤔

bogdanzurac commented 1 year ago

And you would like apps that don't have access to a particular app feature to not be able to see any of the app features sub-features at all? In Unleash terms, if a project doesn't have an environment enabled, then none of the projects toggles should be returned in that environment?

And the reason for this is that you use a fallback mechanism where if a feature isn't defined in Unleash, then it should fall back to a default value that's encoded locally instead?

Yes, precisely.

The thing is, Unleash was never designed to evaluate features into other states than true or false (unless you use variants). And using projects and environments in this way is not something we anticipated.

I understand that, but my point is that the feature toggles shouldn't be evaluated in the first place for a particular environment for which that project isn't enabled. So if a request is made for an environment that isn't enabled for a feature toggle/project, then that toggle shouldn't even be evaluated, nor returned on the API response, since conceptually, it's not declared / made available for that particular environment.

thomasheartman commented 1 year ago

I understand that, but my point is that the feature toggles shouldn't be evaluated in the first place for a particular environment for which that project isn't enabled. So if a request is made for an environment that isn't enabled for a feature toggle/project, then that toggle shouldn't even be evaluated, nor returned on the API response, since conceptually, it's not declared / made available for that particular environment.

I think I see where you're coming from, but that not how Unleash thinks about it, I'm afraid. The toggle does exist, so it's there in all environments. Indeed, to Unleash, it doesn't matter whether an environment is available or not: it's all about whether the feature is enabled in an environment, and if so, what strategies it has.

Changing this would also require breaking changes to the client API, so it's not something we could do until the next major release, regardless. But I think you have a good point, and have brought it up for discussion internally (thanks for the input! 🙌🏼 )

Right now I can't think of a good way to solve this either with how you've set everything up. It's definitely using the tool differently to how most other folks are, so it's kinda hard to try and conceptualize everything.

bogdanzurac commented 1 year ago

I totally understand that this is not an easy change that can be done overnight, nor did we expect that to be the case. Of course it warrants a major release, I totally agree with that. And I'd like to thank you for understanding our specific use case and business needs. For now, we'll try your previous suggestion of defining the local defaults on Unleash as well, at least until this is updated and fixed in a future major release of Unleash. Thanks again for your support!

LE: Do you have an issue open with this that we can keep track of, btw?

thomasheartman commented 1 year ago

Of course, happy to help 😄

As for an open issue: no, I don't have one yet. You could open one if you want to, though? But again, I don't know when (or even if) this will be prioritized, so the issue might be closed by the bot before we get anywhere with it.

bogdanzurac commented 1 year ago

No worries. Created new issue: https://github.com/Unleash/unleash-proxy/issues/122

Unleash / unleash-proxy