feat: strict behavior #474

nunogois · 2024-07-03T09:52:58Z

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

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

Strict: The new recommended behavior. New tokens are not accepted unless they are subsumed by registered tokens.
Dynamic: The default behavior. New tokens are automatically registered and features will be fetched for them.

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 · 2024-07-03T09:55:03Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

sighphyre

Ye, it's sexy. Nothing here that I feel the need to block this for but I do feel strongly about adding the new error type over modifying the existing one. It's a personal feeling so I won't cry if no one agrees with me

server/src/http/feature_refresher.rs

server/src/error.rs

server/src/http/feature_refresher.rs

sighphyre

Meant to approve

🍌

chriswk · 2024-07-03T10:34:50Z

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

chriswk · 2024-07-03T10:36:27Z

server/src/cli.rs

@@ -174,6 +174,10 @@ pub struct EdgeArgs {
    /// Token header to use for both edge authorization and communication with the upstream server.
    #[clap(long, env, global = true, default_value = "Authorization")]
    pub token_header: TokenHeader,
+
+    /// If set to true, Edge starts in open mode. Open mode means that Edge will accept tokens outside of the scope of the startup tokens
+    #[clap(short, long, env, default_value_t = true)]


Shouldn't we default to false here?

Good point, addressed in e31c1ce

nunogois · 2024-07-03T11:08:37Z

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? 8459d1a

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.

Co-authored-by: Simon Hornby <[email protected]>

gardleopard

Closed vs Open mode. I think this is the major thing in this project for UX. I think that we should be careful on what we name the modes. Its not clear to me what closed and open means here. I think consistent mode vs eventual cosistent or deterministic vs non deterministic might be some options to consider for this. Id like us to involve @FredrikOseberg before selecting the name of this mode.

…nDenied

FredrikOseberg · 2024-07-03T11:38:31Z

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 · 2024-07-03T12:52:11Z

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 · 2024-07-03T13:05:00Z

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 · 2024-07-03T13:55:33Z

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 · 2024-07-03T14:03:43Z

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 · 2024-07-03T15:33:01Z

Following the recent comments and discussions I made some changes.
I recommend reading the updated PR description: #474 (comment)

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?

FredrikOseberg

This looks good to me. I found a small remnant of the old naming.

FredrikOseberg · 2024-07-04T06:14:07Z

server/src/error.rs

@@ -204,6 +207,7 @@ impl Display for EdgeError {
            EdgeError::NotReady => {
                write!(f, "Edge is not ready to serve requests")
            }
+            EdgeError::InvalidTokenOnClosedMode => write!(f, "Edge is running in closed mode and the token is not subsumed by any registered tokens"),


This needs to be updated to strict behavior I assume?

Yes, nice catch! 1363734

gardleopard · 2024-07-04T06:42:10Z

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

feat: closed mode

d079ec5

nunogois requested review from chriswk and sighphyre July 3, 2024 09:53

github-actions bot assigned nunogois Jul 3, 2024

sighphyre reviewed Jul 3, 2024

View reviewed changes

sighphyre approved these changes Jul 3, 2024

View reviewed changes

chriswk reviewed Jul 3, 2024

View reviewed changes

nunogois added 2 commits July 3, 2024 12:05

fix: default open mode to false

e31c1ce

fix: error when starting with no tokens in closed and offline modes

8459d1a

nunogois and others added 3 commits July 3, 2024 12:10

Update server/src/http/feature_refresher.rs

0d303e0

Co-authored-by: Simon Hornby <[email protected]>

Update server/src/http/feature_refresher.rs

9d6809e

Co-authored-by: Simon Hornby <[email protected]>

Update server/src/http/feature_refresher.rs

05459b6

Co-authored-by: Simon Hornby <[email protected]>

gardleopard suggested changes Jul 3, 2024

View reviewed changes

refactor: InvalidTokenOnClosedMode instead of kidnapping Authorizatio…

e39da39

…nDenied

refactor: prefer FeatureRefresher default spread instead of constructor

3407d2d

refactor: strict-dynamic behavior instead of open-closed mode

de8eb86

nunogois changed the title ~~feat: closed mode~~ feat: strict behavior Jul 3, 2024

FredrikOseberg reviewed Jul 4, 2024

View reviewed changes

gardleopard approved these changes Jul 4, 2024

View reviewed changes

refactor: InvalidTokenOnClosedMode -> InvalidTokenWithStrictBehavior

1363734

nunogois merged commit 9f01201 into main Jul 4, 2024
10 checks passed

nunogois deleted the feat-closed-mode branch July 4, 2024 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: strict behavior #474

feat: strict behavior #474

nunogois commented Jul 3, 2024 •

edited

Loading

github-actions bot commented Jul 3, 2024 •

edited

Loading

sighphyre left a comment

sighphyre left a comment

chriswk commented Jul 3, 2024

chriswk Jul 3, 2024

nunogois Jul 3, 2024

nunogois commented Jul 3, 2024

gardleopard left a comment

FredrikOseberg commented Jul 3, 2024

nunogois commented Jul 3, 2024 •

edited

Loading

FredrikOseberg commented Jul 3, 2024

sighphyre commented Jul 3, 2024

sighphyre commented Jul 3, 2024

nunogois commented Jul 3, 2024 •

edited

Loading

FredrikOseberg left a comment

FredrikOseberg Jul 4, 2024

nunogois Jul 4, 2024

gardleopard commented Jul 4, 2024

feat: strict behavior #474

feat: strict behavior #474

Conversation

nunogois commented Jul 3, 2024 • edited Loading

github-actions bot commented Jul 3, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

sighphyre left a comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

chriswk commented Jul 3, 2024

chriswk Jul 3, 2024

Choose a reason for hiding this comment

nunogois Jul 3, 2024

Choose a reason for hiding this comment

nunogois commented Jul 3, 2024

gardleopard left a comment

Choose a reason for hiding this comment

FredrikOseberg commented Jul 3, 2024

nunogois commented Jul 3, 2024 • edited Loading

FredrikOseberg commented Jul 3, 2024

sighphyre commented Jul 3, 2024

sighphyre commented Jul 3, 2024

nunogois commented Jul 3, 2024 • edited Loading

FredrikOseberg left a comment

Choose a reason for hiding this comment

FredrikOseberg Jul 4, 2024

Choose a reason for hiding this comment

nunogois Jul 4, 2024

Choose a reason for hiding this comment

gardleopard commented Jul 4, 2024

nunogois commented Jul 3, 2024 •

edited

Loading

github-actions bot commented Jul 3, 2024 •

edited

Loading

nunogois commented Jul 3, 2024 •

edited

Loading

nunogois commented Jul 3, 2024 •

edited

Loading