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

[BUG] flagd accepts invalid flag config with just a warning #1487

Open
cupofcat opened this issue Dec 19, 2024 · 2 comments
Open

[BUG] flagd accepts invalid flag config with just a warning #1487

cupofcat opened this issue Dec 19, 2024 · 2 comments
Labels
bug Something isn't working Needs Triage This issue needs to be investigated by a maintainer

Comments

@cupofcat
Copy link

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 (#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 cupofcat added bug Something isn't working Needs Triage This issue needs to be investigated by a maintainer labels Dec 19, 2024
@beeme1mr
Copy link
Member

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

@toddbaert
Copy link
Member

toddbaert commented Dec 19, 2024

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 added a commit to open-feature/community that referenced this issue Dec 19, 2024
@cupofcat has been working on flagd and flagd providers:

- open-feature/go-sdk-contrib#598
- open-feature/flagd#1481
- open-feature/flagd#1487


Signed-off-by: Todd Baert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Needs Triage This issue needs to be investigated by a maintainer
Projects
None yet
Development

No branches or pull requests

3 participants