cloud-ca / terraform-provider-cloudca

Terraform provider for cloud.ca
https://cloud.ca
MIT License
8 stars 10 forks source link

Fixed a bug which results in errors reading PF Rules, LB Rules and Network ACLs #75

Closed swill closed 5 years ago

swill commented 5 years ago

Without this fix, we run into the following errors which completely breaks the ability for PFRs to function. We are not able to plan, apply or destroy once the following error is hit.

* cloudca_port_forwarding_rule.master_tf_ui_pfr: Error reading Trigger: public_port_start: '' expected type 'int', got unconvertible type 'string'

To ensure that the fix was backwards compatible, I have tested to confirm that it works in both of the following scenarios (with ports defined as either int or string in terraform):

resource "cloudca_port_forwarding_rule" "master_tf_ui_pfr" {
  environment_id = "${cloudca_environment.tf_env.id}"
  public_ip_id = "${cloudca_public_ip.master_public_ip.id}"
  private_ip_id = "${cloudca_instance.master_instance.private_ip_id}"
  public_port_start = "8143"
  private_port_start = "8143"
  protocol = "TCP"
}

// and

resource "cloudca_port_forwarding_rule" "master_tf_ui_pfr" {
  environment_id = "${cloudca_environment.tf_env.id}"
  public_ip_id = "${cloudca_public_ip.master_public_ip.id}"
  private_ip_id = "${cloudca_instance.master_instance.private_ip_id}"
  public_port_start = 8143
  private_port_start = 8143
  protocol = "TCP"
}

NOTE: I have fixed the same issue as described above for both the Load Balancing Rules and Network ACLs and included them in this patch. Thanks @simongodard for pointing out that the same problem existed in these functions as well. I have confirmed that the changes are backwards compatible, so we can pass int values in as we have in the past and they are handled correctly...

simongodard commented 5 years ago

Thanks! I just faced the exact same issue.

simongodard commented 5 years ago

@swill We also have the same problem with Load-balancing rules and Network ACLs.

swill commented 5 years ago

@swill We also have the same problem with Load-balancing rules and Network ACLs.

@simongodard I have updated my PR to include fixes for both the LB Rules and the Network ACLs. I have also confirmed that the changes are backwards compatible by validating that passing in int values as we have documented still functions as expected.

Now the structs defined in the go-cloudca library are represented appropriately in this plugin. Here is the PFR struct for example: https://github.com/cloud-ca/go-cloudca/blob/master/services/cloudca/port_forwarding_rule.go#L15-L31

swill commented 5 years ago

I have also removed the function readIntFromString(valStr string) since this PR removes the last reference to that function. I am assuming it is good practice to remove unused functions if you remove the last reference to them... Please review and confirm this is the desired practice...

swill commented 5 years ago

@simongodard my understanding is that these issues were introduced in v1.3 of the plugin. Previously, it was silently failing because we did not catch the error.

As per @khos2ow 's investigation:

"This was added recently as part of enabling CI and linting

if err := d.Set("public_port_start", pfr.PublicPortStart); err != nil {
    return fmt.Errorf("Error reading Trigger: %s", err)
}

Previously it was simply:

d.Set("private_port_start", pfr.PrivatePortStart)