Skip to content

Support a flag configured to passthrough to env var #2432

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.

Sign up for GitHub

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

Draft
djeebus wants to merge 1 commit into
base: main
Choose a base branch
from
+47 −20
Draft

Support a flag configured to passthrough to env var #2432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/api/internal/cfg/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,6 @@ type Config struct {

DefaultKernelVersion string `env:"DEFAULT_KERNEL_VERSION"`

DefaultPersistentVolumeType string `env:"DEFAULT_PERSISTENT_VOLUME_TYPE"`

// SandboxStorageBackend selects the sandbox storage implementation.
// "redis" uses Redis directly; "populate_redis" uses in-memory with Redis shadow writes.
SandboxStorageBackend string `env:"SANDBOX_STORAGE_BACKEND" envDefault:"memory"`
Expand Down
11 changes: 1 addition & 10 deletions packages/api/internal/handlers/volume_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (a *APIStore) PostVolumes(c *gin.Context) {

ctx = featureflags.AddToContext(ctx, featureflags.VolumeContext(body.Name))

volumeType := a.getVolumeType(ctx)
volumeType := a.featureFlags.StringFlag(ctx, featureflags.DefaultPersistentVolumeType)
if volumeType == "" {
a.sendAPIStoreError(c, http.StatusInternalServerError, "No persistent volume type is configured")
telemetry.ReportCriticalError(ctx, "default persistent volume type is not configured", nil)
Expand Down Expand Up @@ -165,15 +165,6 @@ func (a *APIStore) PostVolumes(c *gin.Context) {
c.JSON(http.StatusCreated, result)
}

func (a *APIStore) getVolumeType(ctx context.Context) string {
volumeType := a.featureFlags.StringFlag(ctx, featureflags.DefaultPersistentVolumeType)
if volumeType == "" {
volumeType = a.config.DefaultPersistentVolumeType
}

return volumeType
}

var validVolumeNameRegex = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)

func isValidVolumeName(name string) bool {
Expand Down
11 changes: 11 additions & 0 deletions packages/shared/pkg/featureflags/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package featureflags
import (
"context"
"os"
"reflect"
"time"

"github.com/launchdarkly/go-sdk-common/v3/ldcontext"
Expand Down Expand Up @@ -108,6 +109,7 @@ func (c *Client) StringFlag(ctx context.Context, flag StringFlag, contexts ...ld
type typedFlag[T any] interface {
Key() string
Fallback() T
FallbackOnZero() bool
}

func getFlag[T any](
Expand All @@ -128,9 +130,18 @@ func getFlag[T any](
logger.L().Warn(ctx, "error evaluating flag", zap.Error(err), zap.String("flag", flag.Key()))
}

if flag.FallbackOnZero() && isZeroValue(value) {
return flag.Fallback()
}

return value
}

// isZeroValue returns true if the value is the zero value for its type.
func isZeroValue[T any](v T) bool {
return reflect.ValueOf(&v).Elem().IsZero()
}

func (c *Client) Close(ctx context.Context) error {
if c.ld == nil {
return nil
Expand Down
43 changes: 35 additions & 8 deletions packages/shared/pkg/featureflags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"context"
"fmt"
"os"
"strings"

"github.com/launchdarkly/go-sdk-common/v3/ldcontext"
Expand Down Expand Up @@ -31,14 +32,15 @@
OrchestratorCommitAttribute string = "commit"
)

// All flags must be defined here: https://app.launchdarkly.com/projects/default/flags/

type JSONFlag struct {
name string
fallback ldvalue.Value
name string
fallback ldvalue.Value
fallbackOnZero bool
}

func (f JSONFlag) Key() string {

Check warning on line 43 in packages/shared/pkg/featureflags/flags.go

View check run for this annotation

Claude / Claude Code Review

Dead-code fallbackOnZero field on JSONFlag and IntFlag (no constructor ever enables it) 

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.
Comment on lines 35 to 43
Copy link
Copy Markdown

@claude claude Bot Apr 17, 2026

Choose a reason for hiding this comment

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

🟡 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.

Copy link
Copy Markdown
Contributor

@levb levb Apr 17, 2026

Choose a reason for hiding this comment

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

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.

return f.name
}

Expand All @@ -50,6 +52,10 @@
return f.fallback
}

func (f JSONFlag) FallbackOnZero() bool {
return f.fallbackOnZero
}

func newJSONFlag(name string, fallback ldvalue.Value) JSONFlag {
flag := JSONFlag{name: name, fallback: fallback}
builder := launchDarklyOfflineStore.Flag(flag.name).ValueForAll(fallback)
Expand Down Expand Up @@ -88,6 +94,10 @@
return f.fallback
}

func (f BoolFlag) FallbackOnZero() bool {
return false
}

func newBoolFlag(name string, fallback bool) BoolFlag {
flag := BoolFlag{name: name, fallback: fallback}
builder := launchDarklyOfflineStore.Flag(flag.name).VariationForAll(fallback)
Expand Down Expand Up @@ -122,8 +132,9 @@
)

type IntFlag struct {
name string
fallback int
name string
fallback int
fallbackOnZero bool
}

func (f IntFlag) Key() string {
Expand All @@ -138,6 +149,10 @@
return f.fallback
}

func (f IntFlag) FallbackOnZero() bool {
return f.fallbackOnZero
}

func newIntFlag(name string, fallback int) IntFlag {
flag := IntFlag{name: name, fallback: fallback}
builder := launchDarklyOfflineStore.Flag(flag.name).ValueForAll(ldvalue.Int(fallback))
Expand Down Expand Up @@ -207,8 +222,9 @@
)

type StringFlag struct {
name string
fallback string
name string
fallback string
fallbackOnEmptyString bool
}

func (f StringFlag) Key() string {
Expand All @@ -223,6 +239,10 @@
return f.fallback
}

func (f StringFlag) FallbackOnZero() bool {
return f.fallbackOnEmptyString
}

func newStringFlag(name string, fallback string) StringFlag {
flag := StringFlag{name: name, fallback: fallback}
builder := launchDarklyOfflineStore.Flag(flag.name).ValueForAll(ldvalue.String(fallback))
Expand All @@ -231,6 +251,13 @@
return flag
}

func newStringFlagFallbackOnEmptyString(name string, fallback string) StringFlag {
flag := newStringFlag(name, fallback)
flag.fallbackOnEmptyString = true

return flag
}

// This is currently not configurable via feature flags.
const (
DefaultKernelVersion = "vmlinux-6.1.158"
Expand All @@ -251,9 +278,9 @@

// BuildIoEngine Sync is used by default as there seems to be a bad interaction between Async and a lot of io operations.
var (
BuildFirecrackerVersion = newStringFlag("build-firecracker-version", env.GetEnv("DEFAULT_FIRECRACKER_VERSION", DefaultFirecrackerVersion))
BuildFirecrackerVersion = newStringFlagFallbackOnEmptyString("build-firecracker-version", env.GetEnv("DEFAULT_FIRECRACKER_VERSION", DefaultFirecrackerVersion))
BuildIoEngine = newStringFlag("build-io-engine", "Sync")
DefaultPersistentVolumeType = newStringFlag("default-persistent-volume-type", "")
DefaultPersistentVolumeType = newStringFlagFallbackOnEmptyString("default-persistent-volume-type", os.Getenv("DEFAULT_PERSISTENT_VOLUME_TYPE"))
BuildNodeInfo = newJSONFlag("preferred-build-node", ldvalue.Null())
FirecrackerVersions = newJSONFlag("firecracker-versions", ldvalue.FromJSONMarshal(FirecrackerVersionMap))
)
Expand Down
Loading