flagd icon indicating copy to clipboard operation
flagd copied to clipboard

[FEATURE] provider option to reject flag set with invalid rules

Open cupofcat opened this issue 1 year ago • 3 comments

Moved to FEATURE as discussed below.

Observed behavior

When using a flag config that does not conform to the schema, flagd shows the following warning in the logs but accepts the config nevertheless without error: 2024-12-19T10:26:53.980+0100 warn evaluator/json.go:114 flag definition does not conform to the schema; validation errors: 1:flags.is-enabled: Must validate one and only one schema (oneOf) 2:flags.is-enabled.variants.off: Invalid type. Expected: number, given: boolean 3:flags.is-enabled: Must validate all the schemas (allOf)

This leads to inconsistent behavior during evaluations (https://github.com/open-feature/flagd/issues/1481)

Expected Behavior

There are a couple of behaviors that could make sense:

  • when flag config validation fails, return an error and don't accept the updated config, essentially keep working with whatever was already in the flag store (preferred)
  • possibly, accept only parts of the config that conform to the schema and return a warning / error (probably not a good idea)

Steps to reproduce

Use the following flag config. The variants have mismatched types (number and boolean).

{
    "$schema": "https://flagd.dev/schema/v0/flags.json",
    "flags": {
      "is-enabled": {
        "defaultVariant": "off",
        "state": "ENABLED",
        "targeting": {
          "if": [
            {
              "<": [
                {
                  "%": [
                    {
                      "var": "request_id"
                    },
                    1000
                  ]
                },
                100
              ]
            },
            "on",
            "off"
          ]
        },
        "variants": {
          "on": 1,
          "off": false
        }
      }
    }
  }

cupofcat avatar Dec 19 '24 11:12 cupofcat

Hey @cupofcat, I agree that the first option sounds like the right approach. We should start by defining the expected behavior in the provider lifecycle spec.

I believe we'll need to define the expected behavior during:

  • Initialization: what happens when flagd (or an in-process provider) starts with an invalid config. The current behavior is to put flagd in a fatal state.
  • Invalid config updates: should the whole flag set update be rejected or only the invalid flags? I believe that rejecting the whole change would be the best approach.

FYI, @toddbaert @lukas-reining @aepfli

beeme1mr avatar Dec 19 '24 17:12 beeme1mr

Hey @cupofcat, I agree that the first option sounds like the right approach. We should start by defining the expected behavior in the provider lifecycle spec.

I believe we'll need to define the expected behavior during:

  • Initialization: what happens when flagd (or an in-process provider) starts with an invalid config. The current behavior is to put flagd in a fatal state.
  • Invalid config updates: should the whole flag set update be rejected or only the invalid flags? I believe that rejecting the whole change would be the best approach.

FYI, @toddbaert @lukas-reining @aepfli

I agree on both counts. I do think this is more of an enhancement though; the current behavior here is technically expected, though I agree it could be improved on.

One thing to note is that logical errors in the rules are always possible even if they are structurally valid, so having runtime errors in them is inevitable. This is part of the reason we have so far not failed on this sort of validation. Another is that this is how in-process providers behave as well when they get invalid rules (though in some, we have a similar option, for example JS).

Again, I still think this is a good idea, and worth implementing. We should also make sure all in-process providers have this as well, and maybe mention it in the flagd spec.

toddbaert avatar Dec 19 '24 17:12 toddbaert

I know this topic was not active for a while, i started now an adr to specify the behaviour - please have a look at it. any input is highly appreciated.

aepfli avatar Jul 18 '25 08:07 aepfli