Fix flag patch behavior when CSA or IIS blocks exist

[deregtd]

Patch behavior incorrectly checked for both existence AND changes for both CSA and IIS blocks, and if you didn't BOTH have the block AND change it, fell through to pulling the defaults from the project value. The net effect of this was that any applied patch to a flag caused the mobile SDK flag to be set back to false (our project default) until a SECOND terraform plan noticed that the flag was unset and then a second apply would set it back again.

[sloloris]

Hi @deregtd! Thank you so much for bringing this to our attention.

I've tested out the changes and unfortunately this change does break our existing behavior - that said, a workaround must be possible, so we'd like to investigate further. If possible, could you give us a more precise rundown of what you did to cause this behavior, and on which applies of which configurations it was false? If you have them, we would also appreciate the configurations you ran - feel free to block out any information with XXX or whatever you'd like. Also, please let us know what the project defaults were at the time you ran each of the feature flag applies if you can.

Thank you again so much for starting off this investigation for us!

[deregtd]

Our defaults for CSA are false/false, so right now, if you make ANY changes to something that doesn't include CSA (i.e. add a tag), it sets the environmentid flag back from true->false in the same change. The terraform plan doesn't show the change, because it doesn't know it's going to change it, it's definitely a logic bug. Then you run apply, it adds your tag AND sets the CSA/environment id -> false. Then you do a terraform plan with the same exact terraform, and it says oh look at that, we need to change CSA/environmentid back to true. Then you do an apply, and since the CSAischanged = true, it correctly sets the CSA to the right values from the terraform.

What behavior did this flag break? A couple of us went through the logic I've PR'd here and this seemed like a pretty clear fix for what intended behavior should be. If the CSA in TF isn't touched, it shouldn't be OVERWRITING the CSA in LD with the defaults from the project.

[sloloris]

Hi @deregtd,

This change broke several of our tests, specifically here, here, and here. This is a tricky one because we've tried to keep it backwards compatible with the old include_in_snippet field, meaning we have to be constantly checking and setting both. I agree that this is a logic bug on our part. However, the behavior of these fields is not only dependent on the update function, but also on its interaction with the customizeDiff function we have in place and how they get set to state.

I think, however, that I've managed to reproduce your behavior by running this configuration once:

resource "launchdarkly_feature_flag" "test" {
	project_key = "test-project"
	key = "test-flag"
	name = "Test feature flag"
	variation_type = "boolean"
}

and this one twice:

resource "launchdarkly_feature_flag" "test" {
	project_key = "test-project"
	key = "test-flag"
	name = "Test feature flag"
	variation_type = "boolean"
	tags = ["test"]
}

The first was run against the project with its default default CSA values (using_environment_id = false and using_mobile_key = true). At this point Terraform correctly set the flag defaults to the project defaults. The second configuration was run both times against the same project with both CSA defaults now set to false. On the second apply of the second configuration, we saw this unexpected difference in plan:

~ client_side_availability {
                  ~ using_mobile_key     = true -> false
                    # (1 unchanged attribute hidden)
                }

I will be investigating this to see if there is a workaround that can prevent this without affecting existing behavior and will follow up here. Please let me know if the above example does not align with the issue you are drawing attention to, or if you had client_side_availability or include_in_snippet set on the flag configs that caused the issue.

Otherwise, have a great weekend!

[deregtd]

Yes, we are setting CSA specifically, since we have one project that has both client and service flags in it. So client flags need to set CSA explicitly -- that's the root of this problem: with CSA (or ISS for that matter) specified, it should never ever ever use project defaults.

Example flag:

resource "launchdarkly_feature_flag" "orca_app_allowed_playground_access" {
  project_key = "default"
  key         = "orca-app.allowed-playground-access"
  name        = "Orca App Allowed Playground Access"
  description = "Allows access to Playground. orca-app.allowed-access is also needed."

  client_side_availability {
    using_environment_id = true
    using_mobile_key     = false
  }

  tags = [
    "tag"
  ]

  variation_type = "boolean"

  variations {
    value       = "true"
    name        = "Accessible"
    description = "Thou may entereth"
  }

  variations {
    value       = "false"
    name        = "Not Accessible"
    description = "You shall not pass"
  }
}

When you first create that flag, it does it correctly, with CSA (because CSAisvalid = true and CSAchanged = true, so it goes into the right codepath).

Once it's created, now add a new tag (add "tag2" or whatever to the tags array), and do a plan. The plan will show "adding tag2 and nothing else". You do an apply and verify, just adding tag2, and you say "yes" and it does the apply. You look at your tag in LD now, and you'll find environmentid key set back to false. CSAisvalid = true, but CSAchanged = false, so it skipped through that code block and fell through to the "use defaults" code, incorrectly.

If you do another terraform plan, without changing anything at all, you will now see it saying to set false back to true on the CSA.environmentid, because it looked at LD, saw the real value was false (due to the bug from the previous step), and that you set true. In this case, CSAisvalid = true AND CSAchanged = true (since it "changed"), so it goes back to the correct codepath.

This is why I still think, after reading all of this, that my proposal here is right. The basic logic I'd expect here is:

1. Is CSA specified (CSAisvalid)?

• If yes, CSA is the source of desired truth. Compare specified CSA block to actual truth (CSAischanged) to determine if you need to make a change, stop processing more logic related to CSA.
• If no, continue to next check

2. Is legacy IsSnippet specified (ISSvalid)?

• If yes, snippet is the source of truth. Map it into CSA (by using it for the CSA.environmentid) and compare to actual truth (ISSischanged) to determine if you need to make a change. Stop processing more logic related to CSA.
• If no, continue to next check

3. Neither CSA nor ISS specified in source terraform -- use project defaults. Fetch project defaults and compare to actual truth to determine if you need to make a change.

I'm going to have to check through the tests to see what they're checking to reconcile all of this.

[deregtd]

I've tried running the unit tests via make test and they all pass -- can you help me repro what the failure is so I can see what's going wrong? There was a go fmt failure which I've pushed up, but the tests are all passing.

Also I'm going to need to do a release to the terraform registry of our fork today -- this is blocking a rollout of launchdarkly at our company.

[deregtd]

For now I've forked a release off of our branch, and it's exhibiting the correct behavior there (adding a tag doesn't unset client flags):
https://registry.terraform.io/providers/getoutreach/launchdarkly/latest

[sloloris]

Hi @deregtd,

We've released a fix - please consider upgrading to v2.10.0 to get it. The feature flag resource will now retain its last-set CSA values if removed or not updated, and will only use the project values during create if not otherwise specified.

If you're interested, you can see the release PR here.

[deregtd]

Ah, you have a separate private repo you sync over to here. That change works -- 2.10.0 has no issues with adding/removing a tag, so we'll switch over to that and start rolling out. Thanks!