search

ionos-cloud / terraform-provider-ionoscloud

The IonosCloud Terraform provider gives the ability to deploy and configure resources using the IonosCloud APIs.
Mozilla Public License 2.0
34 stars 23 forks source link

Setting firewall rules using icmp_type=0 or icmp_code=0 is broken / Statefiles are not usable with newest version #144

Closed Z3po closed 2 years ago

Z3po commented 2 years ago

Description

When firewall rules using icmp_type, and icmp_code attributes are being managed through the ionoscloud terraform provider it stopped working in version 5.0.4 because of changes introduced in https://github.com/ionos-cloud/terraform-provider-ionoscloud/commit/9b3c28607adea9851c675e65a83ceb4f38f46c71 as those attributes were partly changed in their type. Now the transformation to TypeInt has been continued in https://github.com/ionos-cloud/terraform-provider-ionoscloud/commit/2672a7031bff254d35928bd25e6e66698ed77b70 but unfortunately no state migration mechanism has been introduced. How are statefiles supposed to be migrated to work with new versions?

Additionally when introducing the changes, the properties are being queried using d.GetOk instead of d.GetOkExists (see https://github.com/ionos-cloud/terraform-provider-ionoscloud/blob/master/ionoscloud/resource_firewall.go#L352-L361) which is not handling 0 type values. This causes resources being created with icmp_type or icmp_code set to 0 being rendered broken with no values being set.

icmp_type_broken_5_2_18

Expected behavior

Old statefiles created with TypeString are being migrated to TypeInt and icmp firewall rules are successfully applied via the API.

This is what it looks like when resources are being applied with version 5.0.4. icmp_type_working_5_0_4

Environment

Terraform version: 

❯ terraform version
Terraform v0.15.3
on linux_amd64
+ provider registry.terraform.io/ionos-cloud/ionoscloud v5.2.18

Your version of Terraform is out of date! The latest version
is 1.0.10. You can update by downloading from https://www.terraform.io/downloads.html

Provider version: 

❯ terraform providers

Providers required by configuration:
.
└── provider[registry.terraform.io/ionos-cloud/ionoscloud] 5.2.18

Providers required by state:

    provider[registry.terraform.io/ionos-cloud/ionoscloud]

OS:

❯ cat /etc/issue
Debian GNU/Linux 11 \n \l

Configuration Files

terraform {
  required_providers {
    ionoscloud = {
      source = "ionos-cloud/ionoscloud"
      #version = "5.0.4"
      version = "5.2.18"
    }
  }
}

variable "username" {}
variable "password" {
  sensitive = true
}

provider "ionoscloud" {
  username = var.username
  password = var.password
}

resource "ionoscloud_datacenter" "main" {
  name     = "test icmp"
  location = "us/las"
}

resource "ionoscloud_lan" "main" {
  datacenter_id = ionoscloud_datacenter.main.id
  public        = true
}

resource "ionoscloud_server" "main" {
  name           = "test-icmp"
  datacenter_id  = ionoscloud_datacenter.main.id
  cores          = 1
  ram            = 1024
  image_password = "password123test"
  # ssh_key_path      = var.private_key_path
  image_name        = "debian:10"
  availability_zone = "AUTO"

  volume {
    name      = "test-icmp"
    size      = 5
    disk_type = "SSD"
  }

  nic {
    lan             = ionoscloud_lan.main.id
    dhcp            = true
    firewall_active = true
  }
}

resource "ionoscloud_firewall" "audio_icmp" {
  datacenter_id = ionoscloud_datacenter.main.id
  server_id     = ionoscloud_server.main.id
  nic_id        = ionoscloud_server.main.primary_nic
  protocol      = "ICMP"
  name          = "ICMP"
  icmp_type     = 0
}

How to Reproduce

Applying the test resource from above.

Statefile issue

default-type issue with GetOk

Error and Debug Output

Already attached above

Additional Notes

References

oana-ungureanu-ionos commented 2 years ago

Hi @Z3po thank you for raising this issue. We are currently investigating these changes and we'll get back to you shortly.

Z3po commented 2 years ago

Hi @omungureanu is there anything we can help with? Does it help if we're preparing a Pull Request to this for doing the state migration?

cristiGuranIonos commented 2 years ago

Hi @Z3po very nice bug description btw, we actually have a fix for this, but we want to go the route of actually reverting the Int fields back to string. This is because Terraform has a problem with unsetting integer variables that can have valid '0' values. Even GetOkExists will just set '0' when removing a field, instead of setting it to empty(null). There are workarounds, like using '-1' to specify unsetting the value, but we think they're an ugly workaround. One question, would you need a state migration function for this(int -> string) or is it enough to just revert the fields to String?

Z3po commented 2 years ago

Hi @cristiGuranIonos,

thanks for your fast reply! We weren't able to use the int type module because of our existing statefiles, therefore we don't need an int to string migration. Reverting the fields solves it for us.

cristiGuranIonos commented 2 years ago

Hi, please try with release v5.2.19 and let us know if the problem is fixed.

Thanks!

Z3po commented 2 years ago

I've tried it with version 5.2.19 and I still get the following Error when running a plan:

Error: a number is required

I didn't take a deeper look yet of what's causing this...

UPDATE: Funny thing is it works in my test terraform files, but not with those we use in production...

cristiGuranIonos commented 2 years ago

Will look to see what might be the problem. Thanks!

Z3po commented 2 years ago

Ah I think I found it...it's not at all with the icmp_code setting. Actually it's a different firewall rule.\ The problem seems to be the missing state migration for port_range_start and port_range_end values. These have been initialized in 5.0.4 as "string" and have since been changed to "int" therefore they are "not a number" when reading the statefile.

cristiGuranIonos commented 2 years ago

Port_range_start and port_range_end did not change recently. Only the documentation for the resource was wrong, they had been ints all along. Is it defined inside a server resource, or separately? Can you attach the firewall rule that fails?

Thanks!

Z3po commented 2 years ago

Oh I'm Sorry...I confused these actually...you're right the type hasn't been changed.

If I'm creating resources using my example definition from above and change the server definition to contain a tcp firewall rule:

resource "ionoscloud_server" "main" {
  name           = "test-icmp"
  datacenter_id  = ionoscloud_datacenter.main.id
  cores          = 1
  ram            = 1024
  image_password = "password123test"
  # ssh_key_path      = var.private_key_path
  image_name        = "debian:10"
  availability_zone = "AUTO"

  volume {
    name      = "test-icmp"
    size      = 5
    disk_type = "SSD"
  }

  nic {
    lan             = ionoscloud_lan.main.id
    dhcp            = true
    firewall_active = true

    firewall {
      protocol         = "TCP"
      name             = "SSH"
      port_range_start = 22
      port_range_end   = 22
    }
  }
}

Render it with the 5.0.4 version and then try to plan it with the 5.2.19 version I get this error.

UPDATE: I tried with a seperate firewall resource where this is working as expected. So this is specific to inline firewall resources.

Z3po commented 2 years ago

Ok further digging this is still related to icmp_type and icmp_code not changed back to String everywhere (the inline resources have been forgotten). The default non-set value for icmp_code and icmp_type gets written as "" in the statefile and therefore it's not readable for the newest provider version.\ When changing these to e.g. "null" in the statefile it works like expected.

I guess you found that as well already as you specifically asked for the inline :)

cristiGuranIonos commented 2 years ago

Yes, exactly overlooked changing back in inline resources. We will have a fix for this hopefully today or tomorrow at the latest.

Z3po commented 2 years ago

So this will be included int he next release, right? Do you have a timeframe so we can test if the issue has been really solved then?

cristiGuranIonos commented 2 years ago

Hi, yes the new release should be published on the terraform registry(5.20.21) right now. The pull request automatically closed the issue. I will re-open and you can close it if it is fixed.

Z3po commented 2 years ago

@cristiGuranIonos we just tested your latest release and it was able to read our "old" statefile successfully. plan and apply are working as expected.

Thanks for your help!