Skip to content

docs: add flag config adr#1637

Merged
toddbaert merged 2 commits intomainfrom
chore/flag-config-adr
Jun 26, 2025
Merged

docs: add flag config adr#1637
toddbaert merged 2 commits intomainfrom
chore/flag-config-adr

Conversation

@toddbaert
Copy link
Member

Another "retrospective" ADR, like the cucumber one.

This explains our JSONLogic choice, shared $evaluators, as well as the the semantics of flagd's use of OpenFeature's resolution reasons.

@toddbaert toddbaert requested a review from a team June 5, 2025 19:34
@toddbaert toddbaert requested a review from a team as a code owner June 5, 2025 19:34
@netlify
Copy link

netlify bot commented Jun 5, 2025

Deploy Preview for polite-licorice-3db33c ready!

Name Link
🔨 Latest commit ebde807
🔍 Latest deploy log https://app.netlify.com/projects/polite-licorice-3db33c/deploys/6841f459facec80008a2829a
😎 Deploy Preview https://deploy-preview-1637--polite-licorice-3db33c.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 5, 2025
toddbaert added 2 commits June 5, 2025 15:47
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert force-pushed the chore/flag-config-adr branch from 89a7f50 to ebde807 Compare June 5, 2025 19:47

- **Language agnostic**: Rules must be portable across different programming languages, ideally relying on existing expression language(s)
- **Safe evaluation**: No arbitrary code execution or system access
- **Deterministic**: Same inputs must always produce same outputs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$flagd.timestamp seems to break this requirement?

Copy link
Member Author

@toddbaert toddbaert Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on whether or not you consider the timestamp an sort of implicit input... We could change it to "predictable" though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, what users provide through the evaluation API are the input, and timestamp is not there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'll change that to "predictable".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think I'll keep Deterministic (because I believe it still is), but mention the timestamp exception situation.

#### Benefits realized

- Rules can be stored in databases, transmitted over networks, shared between frontend/backend, and embedded in Kubernetes custom resources
- No eval() or code injection risks - computations are deterministic and sand-boxed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this depends on the implementation of the eval engine, and less about the language itself.
For example, a JavaScript JSON logic engine had a CVE that allows command injection: https://nvd.nist.gov/vuln/detail/CVE-2021-4329

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, that's fair. At least at the time, we saw JSONLogic as less likely to have these sort of issues.


flagd's targeting system was designed with several key requirements:

## Requirements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the requirement for the typing system back then? Is weakly typed a requirement or a result?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a result.

## Considered Options

- **Custom DSL**: Would require parsers in every language
- **JavaScript/Lua evaluation**: Security risks and language lock-in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the biggest concern for Lua?
Lua engine normally comes with a sandboxed VM, which is more lightweight and secure IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lua was something I briefly advocated for, I've had some experience with it doing nginx customization. I remember a community member being opposed to it on the basis of security concerns, and at the time it seemed like a higher bar.


### Positive

- Good, because implementations exist across languages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but also there's not much consistency guarantee across languages.

Copy link
Member Author

@toddbaert toddbaert Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there's enough at least that our test suite works in Go, Python, Java, JS, and .NET. It covers every operation and a fair few edge cases: https://github.com/open-feature/flagd-testbed/tree/main/gherkin

I acknowledge there's inconsistencies, particularly around undefineds. It's one of the reasons the JSONLogic org exists (though admittedly it's been hard to make progress with the few volunteers there). I'm sure we could use your help if you're interested in contributing to that.

Copy link
Member Author

@toddbaert toddbaert Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first alternative in this comment might be a reasonable solution to some implementation inconsistencies. Interested in your take on that; we could do it progressively and it would be non-breaking.


flagd's targeting system was designed with several key requirements:

## Requirements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly due to the JS typing system, JSONLogic's behavior on undefined values (context not provided, for example) can be very confusing. Was this considered and evaluated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't recall if that particular behavior was discussed.

@toddbaert
Copy link
Member Author

toddbaert commented Jun 6, 2025

@tangenti Thanks for the feedback.

I'd like to make it clear that personally, though I think our current JSONLogic implementation(s) are "good enough", I think there's ways to improve, and alternatives. To name a few:

  • Leverage WASM and use a single JSON logic implementation, distributed as a module, for maximum implementation coherence
  • Work on improving JSONLogic via https://github.com/json-logic/
  • Support additional targeting DSLs (Lua, CEL, others), either via WASM or natively.

I would be open to any of the above, and lend my support, if there's engineering effort to spare for these.

@toddbaert
Copy link
Member Author

Since this is historical, I'm merging this.

That doesn't mean it can't be edited or superseded, it's simply an explanation of a previous decision and reflects the current implementation.

@toddbaert toddbaert merged commit 6072909 into main Jun 26, 2025
11 checks passed
@toddbaert toddbaert deleted the chore/flag-config-adr branch June 26, 2025 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants