fix: set ip_restrictions type to set on containerregistry_ip_restrict…

[mxm-tr]

…ions

Description

Changes the type of ip_restrictions in containerregistry_ip_restrictions resources and data from schema.TypeList to schema.TypeSet.

Fixes #636

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tests have been added to verify that the order of the ip_restrictions does not influence the configuration and the state.

• Test A: make testacc TESTARGS="-run ^.*ContainerRegistry.*$"

Test Configuration:

• Terraform version: terraform version: Terraform v1.5.6
• Existing HCL configuration you used:

terraform {
  required_providers {
    ovh = {
      source = "ovh/ovh"
      source = "terraform.local/local/ovh"
    }
  }
}

provider "ovh" {
  endpoint           = "ovh-eu"
  application_key    = "xxx"
  application_secret = "yyy"
  consumer_key       = "zzz"
}

data "ovh_cloud_project_capabilities_containerregistry_filter" "regcap" {
  service_name = "xxx"
  plan_name    = "SMALL"
  region       = "GRA"
}

resource "ovh_cloud_project_containerregistry" "registry" {
  service_name = data.ovh_cloud_project_capabilities_containerregistry_filter.regcap.service_name
  plan_id      = data.ovh_cloud_project_capabilities_containerregistry_filter.regcap.id
  region       = data.ovh_cloud_project_capabilities_containerregistry_filter.regcap.region
  name         = "mydockerregistry"
}

resource "ovh_cloud_project_containerregistry_ip_restrictions_management" "my-mgt-iprestrictions" {
  service_name = ovh_cloud_project_containerregistry.registry.service_name
  registry_id  = ovh_cloud_project_containerregistry.registry.id

  ip_restrictions = [
    {
      ip_block    = "192.168.0.1/32"
      description = "xxxxxxx"
    },
    {
      ip_block    = "192.168.0.3/32"
      description = "xxxxxxx"
    }
  ]
}

resource "ovh_cloud_project_containerregistry_ip_restrictions_registry" "my-registry-iprestrictions" {
  service_name = ovh_cloud_project_containerregistry.registry.service_name
  registry_id  = ovh_cloud_project_containerregistry.registry.id

  ip_restrictions = [
    {
      ip_block    = "192.168.0.1/32"
      description = "xxxxxxx"
    },
    {
      ip_block    = "192.168.0.4/32"
      description = "xxxxxxx"
    }
  ]
}

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings or issues
• I have added acceptance tests that prove my fix is effective or that my feature works
• New and existing acceptance tests pass locally with my changes
• I ran successfully go mod vendor if I added or modify go.mod file

[amstuta]

from what I see in the resources' schemas this can't happen since we verify that ip_block is set, so I guess we can remove this if.
Side node, I'm not sure to understand why the elements in ip_restrictions are maps and not nested objects with mandatory fields, this could be updated.

[mxm-tr]

The reason why I added this test is because I realized that the iprestrictionSet ended up containing an extra empty map[] element:

Here is an example of the values taken by the set during execution:

i = &{F:0x9e3b20 m:map[2884812092:map[description:my new first ip restriction ip_block:121.121.121.121/32] 3923998128:map[description:my second ip description ip_block:122.122.122.122/32] 565629566:map[]] once:{done:1 m:{state:0 sema:0}}}

iprestrictionsSet = &{F:0x9e3b20 m:map[2884812092:map[description:my new first ip restriction ip_block:121.121.121.121/32] 3923998128:map[description:my second ip description ip_block:122.122.122.122/32] 565629566:map[]] once:{done:1 m:{state:0 sema:0}}}

iprestrictionsSet.List() = [map[description:my new first ip restriction ip_block:121.121.121.121/32] map[description:my second ip description ip_block:122.122.122.122/32] map[]]

Maybe I need to figure out what creates this empty map[] instead of doing this check...

Then, I agree that the ip_restrictions attribute would be cleaner that way, do you mean replacing the current setup by the following?

@@ -40,23 +39,26 @@ func resourceCloudProjectContainerRegistryIPRestrictionsManagement() *schema.Res
                                Type:        schema.TypeSet,
                                Description: "List your IP restrictions applied on artifact manager component",
                                Required:    true,
-                               Elem: &schema.Schema{
-                                       Type: schema.TypeMap,
-                                       Set:  schema.HashString,
-                                       ValidateFunc: func(ipRestrictionInterface interface{}, path string) (warning []string, errorList []error) {
-                                               ipRestriction := ipRestrictionInterface.(map[string]interface{})
-
-                                               if ipRestriction["ip_block"] == nil {
-                                                       return nil, []error{errors.New(fmt.Sprintf("ipBlock attribute is mandatory for ip_restrictions: %s", path))}
-                                               }
-
-                                               ipBlockString := ipRestriction["ip_block"].(string)
-
-                                               if _, _, err := net.ParseCIDR(ipBlockString); err != nil {
-                                                       return nil, []error{fmt.Errorf("ip_block: %s is not CIDR format", ipBlockString)}
-                                               }
-
-                                               return nil, nil
+                               Elem: &schema.Resource{
+                                       Schema: map[string]*schema.Schema{
+                                               "ip_block": {
+                                                       Type:        schema.TypeString,
+                                                       Description: "IP Block in CIDR notation",
+                                                       Required:    true,
+                                                       ValidateFunc: func(ipBlock interface{}, path string) (warning []string, errorList []error) {
+                                                               ipBlockString := ipBlock.(string)
+
+                                                               if _, _, err := net.ParseCIDR(ipBlockString); err != nil {
+                                                                       return nil, []error{fmt.Errorf("ip_block: %s is not CIDR format", ipBlockString)}
+                                                               }
+                                                               return nil, nil
+                                                       },
+                                               },
+                                               "description": {
+                                                       Type:        schema.TypeString,
+                                                       Description: "Description of the IP block",
+                                                       Optional:    true,
+                                               },

I tried this method and ended up needing the resources to be specified as blocks instead of attributes, which would create a breaking change:

ip_restriction = {...}

Would become:

ip_restriction {...}

Thank you for your review btw!

[amstuta]

🆙

[mxm-tr]

Ok the empty map part seems to be a known terraform issue, so I guess we could keep the if statement as a workaround: hashicorp/terraform-plugin-sdk#652

[mxm-tr]

Hi @amstuta what do you think about this ip_restriction field? Should I do it knowing that it could be a breaking change? Thanks!

[amstuta]

OK I was not aware of this issue, so keeping the if seems like a good idea indeed !
I want to avoid a breaking change so let's keep the resource as-is, and on new resources we won't have this problem as we use terraform-plugin-framework.

[amstuta]

@mxm-tr any news ?

[mxm-tr]

@mxm-tr any news ?

Yes, I had some questions about the comments on the review, cf the comments above