Unleash / unleash-edge

MIT License
48 stars 8 forks source link

feat: strict behavior #474

Closed nunogois closed 3 months ago

nunogois commented 3 months ago

https://linear.app/unleash/issue/2-2370/add-closed-mode

Adds the concept of "strict" and "dynamic" behaviors when running in Edge mode.

This means no change in behavior for existing users. However we recommend edge mode is started with --strict from now on. The plan is to drop this sometime in the future and instead always default to strict mode.

Some tests assumed the "dynamic" behavior by default, so they were adapted accordingly.

github-actions[bot] commented 3 months ago

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

chriswk commented 3 months ago

We should make sure that we fail hard and exit if you start edge in closed mode with no tokens.

nunogois commented 3 months ago

We should make sure that we fail hard and exit if you start edge in closed mode with no tokens.

Agreed, that's something that came to mind after our session. What do you think of this approach? https://github.com/Unleash/unleash-edge/pull/474/commits/8459d1ab9133ce86e32e46f4f5fd005ea44c44ca

Also touches offline mode for consistency (and scouting, seems like offline mode was happy starting with no tokens?), but I'm happy to leave that out of this PR if you prefer.

FredrikOseberg commented 3 months ago

Looks like I'm a bit late to the party and without that much context. But it looks to me like we already have Edge mode and Offline mode as top level modes. These modes seem to be changing the default behavior of Edge mode if I'm reading this right? So we have two sub-modes of Edge mode? Is that the correct interpretation here?

nunogois commented 3 months ago

Looks like I'm a bit late to the party and without that much context. But it looks to me like we already have Edge mode and Offline mode as top level modes. These modes seem to be changing the default behavior of Edge mode if I'm reading this right? So we have two sub-modes of Edge mode? Is that the correct interpretation here?

I think that's spot on.

Edge and Offline modes as top level modes, just like before.

Edge behaved like the new "open" mode. We now want it to behave like the new "closed" mode by default, but leave "open" mode as an option.

The difference between the modes is explained in the PR description. Essentially "open" mode allows you to hit Edge with any token. Once validated, that token is added to the list of tokens to refresh (fetch features for). "Closed" mode will return 403 for tokens that, once validated, are not subsumed by the currently known tokens.

The idea is that e.g. you start Edge with a wildcard token for an environment, and any token that matches that environment should be OK, even if it's narrower in scope (e.g. same environment, but specific project). However tokens for other environments are denied, instead of dynamically added to the list.

FredrikOseberg commented 3 months ago

Looks like I'm a bit late to the party and without that much context. But it looks to me like we already have Edge mode and Offline mode as top level modes. These modes seem to be changing the default behavior of Edge mode if I'm reading this right? So we have two sub-modes of Edge mode? Is that the correct interpretation here?

I think that's spot on.

Edge and Offline modes as top level modes, just like before.

Edge behaved like the new "open" mode. We now want it to behave like the new "closed" mode by default, but leave "open" mode as an option.

The difference between the modes is explained in the PR description. Essentially "open" mode allows you to hit Edge with any token. Once validated, that token is added to the list of tokens to refresh (fetch features for). "Closed" mode will return 403 for tokens that, once validated, are not subsumed by the currently known tokens.

The idea is that e.g. you start Edge with a wildcard token for an environment, and any token that matches that environment should be OK, even if it's narrower in scope (e.g. same environment, but specific project). However tokens for other environments are denied, instead of dynamically added to the list.

Is it confusing to refer to it as a mode then? I would expect mode to be a state that I can start Edge in based on the current implementation of mode. For example:

Edge mode | Offline mode | Open mode | Closed mode

While in reality it seems like these are behaviors of edge mode. Would it make more sense to refer to it as Edge mote behavior: strict or dynamic behavior? I feel like we might be mixing concepts we already have here.

sighphyre commented 3 months ago

Okay I thought about this a lot more and I spoke to @gardleopard. What's our plan for open/closed (or whatever we want to call it). I'd really like to make the open mode go away medium term. I think it's a trap and my personal feelings around the flag here is to give people a window to migrate away from open mode

That aside, I think @FredrikOseberg's suggestion of dynamic/strict is a goodie.

sighphyre commented 3 months ago

Edge mode | Offline mode | Open mode | Closed mode

While in reality it seems like these are behaviors of edge mode. Would it make more sense to refer to it as Edge mote behavior: strict or dynamic behavior? I feel like we might be mixing concepts we already have here.

Okay. Also this. What makes sense to me is Edge | Offline | Closed (the names need some love maybe). But really Open/Closed can't affect offline mode

nunogois commented 3 months ago

Following the recent comments and discussions I made some changes. I recommend reading the updated PR description: https://github.com/Unleash/unleash-edge/pull/474#issue-2388193722

I think dynamic/strict behaviors are nice terms for this. Definitely better than open/closed modes. You're both correct: These are not new top-level modes. These are new behaviors that only apply to Edge mode.

I agree that we should introduce this gracefully. The default is now the "dynamic" behavior, so this means no breaking changes to existing users. However we're introducing the "strict" behavior, which is our new recommended way of starting Edge. We can have our quickstart command include --strict when we tackle the docs in a separate PR.

The idea is that sometime later we drop this in favor of always defaulting to the "strict" behavior instead.

WDYT?

gardleopard commented 3 months ago

Are we not gonna put strict as the default behaviour and deprecate but support the dynamic behaviour?