fix(APITokens): change permission_groups type from list to set of strings #1089

UrosSimovic · 2021-06-02T09:37:04Z

Problem
A diff can be detected when creating API Tokens with multiple permission_groups in a single policy.

resource "cloudflare_api_token" "test" {
  name = "test"

  policy {
    effect = "allow"
    permission_groups = [
      "3030687196b94b638145a3953da2b699",
      "da6d2d6f2ec8442eaadda60d13f42bca",
      "e6d2666161e84845a636613608cee8d5",
      "ed07f6c337da4195b4e72a1fb2c6bcae",
      "4755a26eedb94da69e1066d98aa820be",
      "1af1fa2adc104452b74a9a3364202f20",
      "43137f8d07884d3198dc0ee77ca6e79b",
    ]
    resources = { "com.cloudflare.api.account.*" = "*" }
  }
}

$ terraform apply && terraform plan
...
Terraform will perform the following actions:

  # cloudflare_api_token.test will be updated in-place
  ~ resource "cloudflare_api_token" "test" {
      ...
      - policy {
          - effect            = "allow" -> null
          - permission_groups = [
              - "3030687196b94b638145a3953da2b699",
              - "da6d2d6f2ec8442eaadda60d13f42bca",
              - "e6d2666161e84845a636613608cee8d5",
              - "ed07f6c337da4195b4e72a1fb2c6bcae",
              - "43137f8d07884d3198dc0ee77ca6e79b",
              - "4755a26eedb94da69e1066d98aa820be",
              - "1af1fa2adc104452b74a9a3364202f20",
            ] -> null
          - resources         = {
              - "com.cloudflare.api.account.*" = "*"
            } -> null
        }
      + policy {
          + effect            = "allow"
          + permission_groups = [
              + "3030687196b94b638145a3953da2b699",
              + "da6d2d6f2ec8442eaadda60d13f42bca",
              + "e6d2666161e84845a636613608cee8d5",
              + "ed07f6c337da4195b4e72a1fb2c6bcae",
              + "4755a26eedb94da69e1066d98aa820be",
              + "1af1fa2adc104452b74a9a3364202f20",
              + "43137f8d07884d3198dc0ee77ca6e79b",
            ]
          + resources         = {
              + "com.cloudflare.api.account.*" = "*"
            }
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.
...

Why?
CloudFlare API can return permission_groups in a different order then the posted/updated one.

Solution
Make permission_groups set of strings where order doesn't matter.

…rder - should fail

jacobbednarz · 2021-06-15T05:01:40Z

cloudflare/resource_cloudflare_api_token.go

@@ -22,9 +22,10 @@ func resourceCloudflareApiToken() *schema.Resource {
 				Elem:     &schema.Schema{Type: schema.TypeString},
 			},
 			"permission_groups": {
-				Type:     schema.TypeList,


as we are going from a list => set, we will need to

bump the schema version

provide a state migration (see https://www.terraform.io/docs/extend/resources/state-migration.html) as we will be changing the internal representation of the data

Is this really necessary?

I tested this:

create resource with current provider version

upgrade to fixed provider

run terraform apply

No changes detected.

Because state representation of a list or a set is the same - json array.

TypeList and TypeSet definitely aren't the same state representation as sets are stored indexed by a hash calculated based on the attributes in the set whereas list items are stored as a zero indexed items. I'm unsure why you're not seeing a difference here but we may need to dig into why as I've definitely been bit by this in the past.

Sorry for late response. But I said that because I checked the state before and after the fix. Even at links you posted are examples, both of them with slices. I think terraform is doing hash keys on the fly and not storing them in the state file.

Just do the test please.

For the given resource

resource "cloudflare_api_token" "test" { name = "test" policy { effect = "allow" permission_groups = [ "3030687196b94b638145a3953da2b699", "da6d2d6f2ec8442eaadda60d13f42bca", "e6d2666161e84845a636613608cee8d5", "ed07f6c337da4195b4e72a1fb2c6bcae", "1af1fa2adc104452b74a9a3364202f20", "4755a26eedb94da69e1066d98aa820be", ] resources = { "com.cloudflare.api.account.*" = "*" } } }

Current provider state

{ "version": 4, "terraform_version": "0.13.7", "serial": 1, "lineage": "f3059c1d-de72-cc99-752d-da6446426758", "outputs": {}, "resources": [ { "mode": "managed", "type": "cloudflare_api_token", "name": "test", "provider": "provider[\"registry.terraform.io/terraform-providers/cloudflare\"]", "instances": [ { "schema_version": 0, "attributes": { "condition": [], "id": "e8712ce735c7353116fe7586dcf5bacb", "issued_on": "2021-07-19T12:26:40Z", "modified_on": "2021-07-19T14:26:42.035223871+02:00", "name": "uros-test", "policy": [ { "effect": "allow", "permission_groups": [ "3030687196b94b638145a3953da2b699", "da6d2d6f2ec8442eaadda60d13f42bca", "e6d2666161e84845a636613608cee8d5", "ed07f6c337da4195b4e72a1fb2c6bcae", "1af1fa2adc104452b74a9a3364202f20", "4755a26eedb94da69e1066d98aa820be" ], "resources": { "com.cloudflare.api.account.*": "*" } } ], "status": "active", "value": "REDACTED" }, "private": "bnVsbA==" } ] } ] }

Fixed provider state

{ "version": 4, "terraform_version": "0.13.7", "serial": 1, "lineage": "fb23297b-3d7b-2ce0-21fc-f1e9a7a75219", "outputs": {}, "resources": [ { "mode": "managed", "type": "cloudflare_api_token", "name": "test", "provider": "provider[\"registry.terraform.io/terraform-providers/cloudflare\"]", "instances": [ { "schema_version": 0, "attributes": { "condition": [], "id": "8a712ce0983ed8b47adc2710e0094ef6", "issued_on": "2021-07-19T12:28:08Z", "modified_on": "2021-07-19T14:28:10.23180693+02:00", "name": "uros-test", "policy": [ { "effect": "allow", "permission_groups": [ "1af1fa2adc104452b74a9a3364202f20", "3030687196b94b638145a3953da2b699", "4755a26eedb94da69e1066d98aa820be", "da6d2d6f2ec8442eaadda60d13f42bca", "e6d2666161e84845a636613608cee8d5", "ed07f6c337da4195b4e72a1fb2c6bcae" ], "resources": { "com.cloudflare.api.account.*": "*" } } ], "status": "active", "value": "REDACTED" }, "private": "bnVsbA==" } ] } ] }

You can see that state representation of both is json array. And as said already said, I tested creating resource and then upgrading provider. It works like a charm.

I will do it proper way if you convince me that current approach is not working, but sorry, I will no do something that is unnecessary IMO.

jacobbednarz · 2021-06-15T05:07:12Z

cloudflare/resource_cloudflare_api_token_test.go

+	resource.Test(t, resource.TestCase{
+		PreCheck:  func() { testAccPreCheck(t) },
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{


as we're now needing a schema migration we'll need to extend this a little.

the first is that we'll need to test the data migration from a list => set. https://www.terraform.io/docs/extend/resources/state-migration.html covers some examples of asserting that.

the second is that we'll want to update this test to be 2 steps. the first will create the resource with the permissions and the second will be the same permissions but in a different order and we'll want to assert there is no difference which will ensure we're ignoring the ordering.

I added a test which fails (1st commit) and then a fix for that (2nd commit).

But yeah, I get your point. I can add another step with different order, but don't know how to assert there is no difference. Is there some example of something similar?

jacobbednarz

great start! see comments inline for this changes needed for this one.

jacobbednarz · 2021-06-21T00:32:46Z

cloudflare/resource_cloudflare_api_token.go

 				Required: true,
 				Elem:     &schema.Schema{Type: schema.TypeString},
+				Set:      schema.HashString,


i don't think we need this as it's the default

LanceSandino · 2021-08-06T05:14:58Z

bump.

tete17 · 2022-02-02T11:54:58Z

Hey @UrosSimovic this is an issue it is currently affecting my team and I see not much activity since quite a few months.

Do you want to continue with this MR or should I take over?

UrosSimovic · 2022-02-02T12:11:25Z

As said, MR was prepared, but not accepted for unproven reason. I don't want doing unnecessary things. Feel free to take it from here, @tete17.

tete17 · 2022-02-03T18:25:50Z

Hey @jacobbednarz I opened another MR with the commits of @UrosSimovic with some of my own addressing your comments. #1425

I am still a bit confused with this whole migration thing given we seem to have been through a very similar scenario like this one and no migration was performed 7704f43 .

This is the first time I deal with the go sdk so I hope you excuse me if it is 100% unrelated.

jacobbednarz · 2022-02-03T22:52:01Z

moving to #1425

UrosSimovic changed the title ~~teat(APITokens): add test with multiple permission groups in random o…~~ WIP: chore(APITokens): fix permission group order Jun 2, 2021

UrosSimovic force-pushed the uros/fix_api_tokens branch from d79fcea to f2f9399 Compare June 2, 2021 09:55

feat(APITokens): add test with multiple permission groups in random o…

e355a82

…rder - should fail

UrosSimovic force-pushed the uros/fix_api_tokens branch from f2f9399 to e355a82 Compare June 2, 2021 09:56

UrosSimovic marked this pull request as draft June 2, 2021 09:56

UrosSimovic force-pushed the uros/fix_api_tokens branch 2 times, most recently from 183f60c to cb2c654 Compare June 2, 2021 11:01

UrosSimovic changed the title ~~WIP: chore(APITokens): fix permission group order~~ chore(APITokens): change permission_groups type from list to set of strings Jun 2, 2021

UrosSimovic force-pushed the uros/fix_api_tokens branch from cb2c654 to a2e7fb0 Compare June 2, 2021 11:21

UrosSimovic changed the title ~~chore(APITokens): change permission_groups type from list to set of strings~~ fix(APITokens): change permission_groups type from list to set of strings Jun 2, 2021

fix(APIToken): change 'permission_groups' from list to set of strings

4d84648

UrosSimovic force-pushed the uros/fix_api_tokens branch from a2e7fb0 to 4d84648 Compare June 2, 2021 11:22

UrosSimovic marked this pull request as ready for review June 2, 2021 11:25

jacobbednarz reviewed Jun 15, 2021

View reviewed changes

jacobbednarz requested changes Jun 15, 2021

View reviewed changes

jacobbednarz mentioned this pull request Jun 17, 2021

cloudflare_api_token permission_groups #1105

Closed

2 tasks

jacobbednarz reviewed Jun 21, 2021

View reviewed changes

tete17 mentioned this pull request Feb 3, 2022

Fix api tokens issue with premission_groups #1425

Merged

jacobbednarz added service/iam Categorizes issue or PR as related to the IAM service. triage/unresolved Indicates an issue that can not or will not be resolved. labels Feb 3, 2022

jacobbednarz closed this Feb 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(APITokens): change permission_groups type from list to set of strings #1089

fix(APITokens): change permission_groups type from list to set of strings #1089

UrosSimovic commented Jun 2, 2021 •

edited

Loading

jacobbednarz Jun 15, 2021

UrosSimovic Jun 15, 2021

UrosSimovic Jun 15, 2021 •

edited

Loading

jacobbednarz Jun 21, 2021

UrosSimovic Jul 19, 2021

jacobbednarz Jun 15, 2021

UrosSimovic Jun 15, 2021

jacobbednarz left a comment

jacobbednarz Jun 21, 2021

LanceSandino commented Aug 6, 2021

tete17 commented Feb 2, 2022

UrosSimovic commented Feb 2, 2022

tete17 commented Feb 3, 2022

jacobbednarz commented Feb 3, 2022

fix(APITokens): change permission_groups type from list to set of strings #1089

fix(APITokens): change permission_groups type from list to set of strings #1089

Conversation

UrosSimovic commented Jun 2, 2021 • edited Loading

jacobbednarz Jun 15, 2021

Choose a reason for hiding this comment

UrosSimovic Jun 15, 2021

Choose a reason for hiding this comment

UrosSimovic Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jacobbednarz Jun 21, 2021

Choose a reason for hiding this comment

UrosSimovic Jul 19, 2021

Choose a reason for hiding this comment

jacobbednarz Jun 15, 2021

Choose a reason for hiding this comment

UrosSimovic Jun 15, 2021

Choose a reason for hiding this comment

jacobbednarz left a comment

Choose a reason for hiding this comment

jacobbednarz Jun 21, 2021

Choose a reason for hiding this comment

LanceSandino commented Aug 6, 2021

tete17 commented Feb 2, 2022

UrosSimovic commented Feb 2, 2022

tete17 commented Feb 3, 2022

jacobbednarz commented Feb 3, 2022

UrosSimovic commented Jun 2, 2021 •

edited

Loading

UrosSimovic Jun 15, 2021 •

edited

Loading