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

feat(cmd/flipt-edge): lean, mean flag evaluating machine #3491

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Sep 23, 2024

Lean, mean flag evaluating machine.

    _________       __      ______    __
   / ____/ (_)___  / /_    / ____/___/ /___ ____
  / /_  / / / __ \/ __/   / __/ / __  / __ ./ _ \
 / __/ / / / /_/ / /_    / /___/ /_/ / /_/ /  __/
/_/   /_/_/ .___/\__/   /_____/\__,_/\__, /\___/
         /_/                        /____/

Version: dev
Commit: 273158a316cbfff3114110a15d4df9dfe93ec4f1
Build Date: 2024-09-23T10:40:38Z
Go Version: go1.23.0
OS/Arch: darwin/arm64

Im opening this up as a draft to get it on folks radars, while I work on other related bits.

mage go:buildedge
...du -h -d 1 bin/*
118M	bin/flipt
 89M	bin/flipt-edge

This is a proposal PR containing a thinned down Flipt binary, which only supports evaluation.
You can think of it as a purely in-memory evaluation proxy.
This leaner build is more appropriate for pairing with Cloud as it does not carry around unnecessary baggage.

The following is a list of changes compared with Flipt proper:

  • No UI (purely for evaluations).
  • No management API (read-only, cannot update flag and segment configuration).
  • Only supports the declarative backends (only local, git, object and oci backends).
  • No remote cache (i.e. redis). It doesn't need one, all evaluations already served directly from memory.
  • No auditing (nothing can be changed by flipt-edge, only evaluated).
  • Minimal authn methods (only JWT for now)

Notes

Storage

This build will continue to support sourcing from declarative (file / snapshot) stores.
We're also considering some more remote backends in the near future (these will likely be supported in Flipt too):

  • Vercel Edge
  • Cloudflare KV

Authentication

We only support JWT for now as it is the only truly decentralized / stateless mechanism we have in Flipt.
Even the k8s method exchanged a service account token for a client token stored in the backing DB.

We could add support for a few more stateless authn methods like:

  • static client token (configured via env var / file / htpasswd / other secret source).
  • new k8s method (simply always send and validate the service account token (no exchange)).

Outstanding

  • Review configuration and add validation warnings and errors for non-reachable config
  • Setup e2e ITs purely running the edge binary
  • Setup publishing pipelines (purely docker/OCI/linux based for now?)
  • Documentation including columns in config docs to demonstrate what is an isn't acknowledge between the build versions

var (
rootCmd = &cobra.Command{
Use: "flipt-edge <command> <subcommand> [flags]",
Short: "Flipt edge is a modern, self-hosted, lean Flipt feature flag evaluation service",
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looking great! couple minor things so far

cmd/flipt-edge/main.go Outdated Show resolved Hide resolved

g, ctx := errgroup.WithContext(ctx)

if err := initMetaStateDir(cfg); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

also will prob want to adjust the name of this state file incase someone runs Flipt Edge and Flipt on the same machine (at different times)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking at this one too. I think actually leaving this path as-is, is both convient from a complexity standpoint, as well as being reasonably safe to do.

I think for 1, people will likely not run them on the same machine.
At-least, they will likely run these in a containerized environment. In which case, they're already isolated.

Secondly, edge consumes a subset of the full config. So it may just work.
Also, we never overwrite this config, we just read it. So I don't think we have a chance of corrupting anything.

If anything, it may just be useful that you can switch between the two without having to do much.

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 19.27928% with 448 lines in your changes missing coverage. Please review.

Project coverage is 63.98%. Comparing base (490cc12) to head (2e67c3b).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
cmd/flipt-edge/main.go 0.00% 194 Missing ⚠️
internal/cmd/edge/http.go 4.67% 102 Missing ⚠️
internal/cmd/edge/grpc.go 51.39% 73 Missing and 14 partials ⚠️
internal/cmd/edge/authn.go 23.25% 33 Missing ⚠️
cmd/flipt-edge/completion.go 0.00% 17 Missing ⚠️
cmd/flipt-edge/doc.go 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3491      +/-   ##
==========================================
- Coverage   65.74%   63.98%   -1.77%     
==========================================
  Files         169      175       +6     
  Lines       13629    14206     +577     
==========================================
+ Hits         8961     9089     +128     
- Misses       3983     4415     +432     
- Partials      685      702      +17     
Flag Coverage Δ
unittests 63.98% <19.27%> (-1.77%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/flipt-edge/main.go Outdated Show resolved Hide resolved
Signed-off-by: Mark Phelps <[email protected]>
…o gm/flipt-edge

* 'gm/flipt-edge' of https://github.com/flipt-io/flipt:
  docs: add devumesh as a contributor for code (#3512)
  feat(ext): determinism in exporting and declarative formats (#3509)
  fix: skip authz for clickhouse (#3511)
@markphelps markphelps marked this pull request as ready for review October 1, 2024 20:13
@markphelps markphelps requested a review from a team as a code owner October 1, 2024 20:13
@markphelps
Copy link
Collaborator

@GeorgeMac i setup a nightly workflow that I cant run until this is merged, to test the publishing

I also was trying to figure out how to setup ITs for just flipt-edge without redoing our entire dagger test suite.. but couldn't come up with a simple way. any ideas would be appreciated!

- name: Run UI Tests
uses: dagger/dagger-for-github@v6
with:
verb: call
version: ${{ env.DAGGER_VERSION }}
args: test --source .:default ui
cloud-token: ${{ secrets.DAGGER_CLOUD_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

i decided to remove Dagger Cloud because im not sure we're benefiting too much from it and I also don't think this free-disk-space action works incredibly well. The disk space usage is caused by Dagger Cloud downloading the entire cache history (I think) on run, which both slows down CI and blows up our disk.

@GeorgeMac
Copy link
Contributor Author

@GeorgeMac i setup a nightly workflow that I cant run until this is merged, to test the publishing

I also was trying to figure out how to setup ITs for just flipt-edge without redoing our entire dagger test suite.. but couldn't come up with a simple way. any ideas would be appreciated!

@markphelps yeah I just took a run at this and then got as far as realizing we don't expose anything under /api/v1 in edge because we don't need it. Which even half of the read-only test suites exercise.

To me the quickest option I currently see is to write evaluation specific test suites in just to exercise edge.
Better would be perhaps to split the readonly suite into separate api v1 and evaluation v1 test suites.
Also, taking the opportunity to maybe bring in a more exhausting set of eval tests would be good here.

Also, we could defer doing that and consider edge to be alpha/beta for now until we have that settled.

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

Successfully merging this pull request may close these issues.

3 participants