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

Allow explicitly disabling actors #514

Open
jnunemaker opened this issue Apr 20, 2021 · 6 comments
Open

Allow explicitly disabling actors #514

jnunemaker opened this issue Apr 20, 2021 · 6 comments
Assignees

Comments

@jnunemaker
Copy link
Collaborator

This has come up a few times. It would be great if we could come up with a good way of explicitly enabling a feature with exceptions. For example, this feature is enabled for everyone but john. Or this feature is enabled for everyone but customers on the basic tier.

The main problem currently is that flipper is about gates and letting people through the gates. Flipper.disable is about clearing those enablements, not explicitly disabling the gate for the actor which is different.

#82 (comment) talks more about this. Storage of disabled is easy but the interface changes could cause quite a ripple for existing flipper users so we'd want to be careful.

  • enable - enables feature for gate
  • un-enable - not disable, just removes the enabled gate if it is enabled else nothing, if actor enabled then this would remove actor, if group is enabled, it would remove group, if percentage is set it would unset it
  • disable - disables feature for gate, disable actor would store actor id in set of disabled actors, disable group would store group in set of disabled groups, flipper feature enabled? checks would likely need throw catch or some other mechanism to explicitly say "even though this feature is enabled for this gate, it is explicitly disabled for this actor or group or other gate and should return false

Workarounds

  • Create a negative feature. If feature is :redesign, you create new feature :redesign_disabled. Each code point needs to check Flipper.enabled?(:redesign, actor) && !Flipper.enabled?(:redesign_disabled, actor). A bit painful but can be wrapped up in another application method (e.g. Redesign.enabled?(actor) which can check both and return value). Con: Involves checking two features. Also involves knowing what/how the negative feature works in this context.
  • Use group + application storage. If you have group and group checks attribute you can enable the group and have that group check an attribute on the actor. :redesign_enabled group, checks if redesign_enabled? (some attribute stored on actor). If that is false it returns false for group. If that is true it returns true for group. Then you just need to avoid using other gates. Con: Would break if someone fully enabled. Also involves application storage instead of all baked into flipper.
  • Use application storage + pre-feature flag check. Check something with application logic in addition to Flipper feature check. Can also do this via Redesign.enabled?(actor) or something. That can check return false if actor.redesign_disabled? or whatever. Con: Involves you storing stuff on your own instead of baked into flipper.
@jnunemaker
Copy link
Collaborator Author

A concrete example of how this would be cool. For flipper cloud, say we have an api feature. Its always enabled.

If we made it possible to disable for individual actors or groups, then when we got a bad actor using too many resources or whatever we could just Flipper.disable(:api, bad_actor).

No code changes. No additional software other than flipper. Now the bad actor can no longer do things.

@bkeepers
Copy link
Collaborator

Would it work to add deny as another state for gates (or maybe there's a better word)? Unlike disable, which will proceed to the next gate, deny would stop checking gates and immediately return false.

Flipper.deny_actor :api, bad_actor
Flipper.deny_group :api, :abusers

Implementation wise, it could use throw :denied to abort checks.

@jnunemaker
Copy link
Collaborator Author

Yeah that could work. We’d still need to adjust api, ui, adapters and Feature but that verbiage could help make it all more clear.

We’d also still need a way to remove someone from the deny list. I guess we could use allow/deny. Part of me wonders if that is confusing, like what is the difference between allow and enable.

@nickvanw
Copy link
Contributor

I'll definitely second/third this feature - we ❤️ this gem, and this is one of the things that we've found ourselves working around a number of times - either when testing a regression, or some sort of back-compatibility after we've released a feature to everyone.

I definitely think that a deny gate would be how we'd want this to work - much like there are a handful of other types, we'd love to add a group or individual actor to never return true for a particular flag - basically like adding an "inverse" flag that we would logically && with when checking.

Let me know if there's anything we can do to help this along.

@Haegin
Copy link

Haegin commented Mar 2, 2022

Somewhat related to this, it'd be useful to be able to specify a default when checking if a feature is enabled. For example, if I run a hypothetical SaaS blogging platform and everyone has comments, but then I sell to a customer who doesn't want comments it'd be nice to be able to add a comments feature, disable it for that new user but default the code to enabled for everyone else.

Presumably the changes you're talking about here would support this because you could globally enable comments, then disable it for that particular user. I'm currently looking at trying to build this on top of our use of Flipper at the moment.

As for the terminology, disable or deny both sound good but I agree that allow vs enable wouldn't be clear. How about clear to just wipe out any current settings for a given actor/group?

@brandondrew
Copy link

brandondrew commented Dec 6, 2023

For the terminology, I think block is just a bit clearer than deny—implying that you're on a list of blocked users.

Let's think of "real-world" analogies. If you try to enter an event, but you don't have a ticket, you'll be denied entry. Not because you're on a list of blocked people, but because you're not on the list of invited people (based on the credentialing system of tickets). But block as a word in English seems to imply a more active prevention of access rather than just avoiding the granting of access.

Also, since deny and disable begin with the same letter, it's easier to occasionally confuse them. (Especially since their meaning is also more similar (IMO) than block and disable.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants