Support a flag configured to passthrough to env var

[djeebus]

Previously, flags had 2 possibilities:

• if flag is configured in launchdarkly, use that value
• if flag is unconfigured in launchdarkly, use fallback value

Now flags can opt-in to a third possibility

• if flag is configured in launchdarkly and returns a default value (empty string, zero, etc), use the fallback value

This allows us to create flags that should return no value, and allow it to fallback to the local configuration.

A concrete example:

• the env var DEFAULT_PERSISTENT_VOLUME_TYPE sets the default
• the launch darkly flag default-persistent-volume-type can be set to customize this default per team

If we don't want to customize some teams, but fall through for others, it currently takes extra code to support it. After this PR, our feature flag client supports it natively.

Old usage:

var volumeTypeFlag = newStringFlag("default-persistent-volume-type", "")

func getVolumeType() string {
	volumeType := client.StringFlag(volumeTypeFlag)

	// could be empty from launch darkly, could be empty b/c launchdarkly is offline or failed
	if volumeType == "" {  
    	return os.Getenv("DEFAULT_PERSISTENT_VOLUME_TYPE")
	}
	return volumeType
}

New usage:

var volumeTypeFlag = newStringFlagFallbackOnEmptyString(
	"default-persistent-volume-type", 
	os.Getenv("DEFAULT_PERSISTENT_VOLUME_TYPE"),
)

func getVolumeType() string {
	return client.StringFlag(volumeTypeFlag)
}

[cursor]

PR Summary

Medium Risk
Changes core feature-flag evaluation semantics (via reflection-based zero checks), which could alter runtime behavior for any flags that opt into FallbackOnZero. Risk is limited by being opt-in, but misconfiguration could mask intentionally-empty flag values.

Overview
Adds an opt-in FallbackOnZero behavior to the feature-flag client so selected flags return their configured fallback when LaunchDarkly evaluates to a type’s zero value (e.g., empty string/0/null). This is used to simplify “passthrough to env” flags: DEFAULT_PERSISTENT_VOLUME_TYPE is now sourced directly via the DefaultPersistentVolumeType flag fallback (and the API handler/config no longer implement a manual env fallback), and BuildFirecrackerVersion similarly treats empty-string evaluations as fallback-worthy.

^{Reviewed by Cursor Bugbot for commit 0b70a21. Bugbot is set up for automated code reviews on this repo. Configure here.}

[claude]

LGTM — the core feature (StringFlag fallback on empty string) is correctly implemented and the flag system refactoring is clean.

Extended reasoning...

Overview

The PR adds a third flag evaluation mode to the feature flags client: when LaunchDarkly returns a zero/empty value, the client can now fall back to the configured fallback (e.g. an env var). Four files are touched: the flag client generic logic, the flag type definitions, a config struct (removing a now-redundant field), and a handler that was inlined.

Security risks

None. This is purely internal feature flag plumbing with no auth, crypto, or external trust boundary changes.

Level of scrutiny

Low. The changes are mechanical and self-contained. The isZeroValue reflection helper follows standard Go idioms. The StringFlag path is the only one that actually exercises the new code path, and it does so correctly via newStringFlagFallbackOnEmptyString. Removing DefaultPersistentVolumeType from the Config struct is safe because its value is now supplied directly to the flag as its fallback.

Other factors

A nit was flagged in the inline comments: JSONFlag and IntFlag gain a fallbackOnZero field and FallbackOnZero()\ method but no constructor ever sets the field to true, making it unreachable dead code. This is a minor inconsistency (compared to BoolFlagwhich hard-codesreturn false`) but has zero runtime impact today. The inline comment already surfaces this to the author.

[claude]

🟡 Both JSONFlag and IntFlag gain a fallbackOnZero field and FallbackOnZero() method in this PR, but neither type has a constructor (e.g. newJSONFlagFallbackOnZero / newIntFlagFallbackOnZero) that ever sets the field to true. Since the field is unexported, it is permanently false, making it dead code. Either add the missing constructors to make the feature usable, or remove the fields and hard-code return false as BoolFlag already does.

Extended reasoning...

What the bug is and how it manifests

Both JSONFlag (lines 35–43) and IntFlag (lines 132–140) in flags.go were given a fallbackOnZero bool field and a corresponding FallbackOnZero() bool method that returns it. The intent is clear: the getFlag generic in client.go checks flag.FallbackOnZero() and, when true, substitutes the fallback if LaunchDarkly returns a zero value. However, neither flag type has a constructor that ever sets fallbackOnZero = true.

The specific code path that triggers it

Every JSONFlag is created exclusively through newJSONFlag(), which leaves fallbackOnZero at its Go zero value (false). Every IntFlag is created exclusively through newIntFlag(), which does the same. Because both fields are unexported (fallbackOnZero, not FallbackOnZero), callers outside the featureflags package cannot set them either. The method FallbackOnZero() therefore always returns false for every JSONFlag and IntFlag in the codebase.

Why existing code doesn't prevent it

The compiler happily accepts the struct fields and method — there is no static check that an unexported field must be reachable. The interface typedFlag[T] requires FallbackOnZero() bool, and both types satisfy it, so no build or vet error is raised. The dead code is silently valid.

What the impact would be

No runtime behavior is broken today because no current flag uses the feature. The danger is forward-looking: a developer looking at JSONFlag or IntFlag will see the fallbackOnZero field and FallbackOnZero() method, assume they can enable the passthrough behaviour by providing a suitable constructor, and then wonder why the feature never fires — or add a constructor without noticing the existing FallbackOnZero method already exists. It also creates an inconsistency: StringFlag correctly ships both the field and newStringFlagFallbackOnEmptyString(), while BoolFlag correctly hard-codes return false (no field at all). JSONFlag and IntFlag fall into a confusing middle ground.

How to fix it

Option A (add the missing constructors, matching the StringFlag pattern):

func newJSONFlagFallbackOnZero(name string, fallback ldvalue.Value) JSONFlag {
    flag := newJSONFlag(name, fallback)
    flag.fallbackOnZero = true
    return flag
}

func newIntFlagFallbackOnZero(name string, fallback int) IntFlag {
    flag := newIntFlag(name, fallback)
    flag.fallbackOnZero = true
    return flag
}

Option B (remove the dead fields, matching the BoolFlag pattern):

// JSONFlag — remove fallbackOnZero field; change method to:
func (f JSONFlag) FallbackOnZero() bool { return false }

// IntFlag — same
func (f IntFlag) FallbackOnZero() bool { return false }

Step-by-step proof

1. A caller writes newJSONFlag("my-flag", ldvalue.Null()). JSONFlag.fallbackOnZero is false.
2. There is no newJSONFlagFallbackOnZero to call instead.
3. flag.fallbackOnZero is unexported — the caller cannot write f.fallbackOnZero = true.
4. At evaluation time, getFlag calls flag.FallbackOnZero() → false.
5. The if flag.FallbackOnZero() && isZeroValue(value) branch is never entered for any JSONFlag or IntFlag.
6. The feature is inaccessible. The field exists but can never influence behaviour.

[levb]

I can add it after compression, it would compose the fallback bag from ENV, just like the strings but with code, and use it for compression config.