cloud-ca / terraform-provider-cloudca

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

[Proposal] Schema change #80

Open khos2ow opened 5 years ago

khos2ow commented 5 years ago

Some of the resources in the provider are really tied to each other and currently being managed under their standalone resources. But they can be moved under block of code under corresponding parent of their own.

  1. Move volume inside instance:

    resource "cloudca_instance" "my_instance" {
        environment_id         = "4cad744d-bf1f-423d-887b-bbb34f4d1b5b"
        name                   = "test-instance"
        network_id             = "672016ef-05ee-4e88-b68f-ac9cc462300b"
        template               = "Ubuntu 16.04.03 HVM"
        compute_offering       = "Standard"
        cpu_count              = 4
        memory_in_mb           = 8192
        ssh_key_name           = "my_ssh_key"
        root_volume_size_in_gb = 100
        private_ip             = "10.2.1.124"
        dedicated_group_id      = "78fdce97-3a46-4b50-bca7-c70ef8449da8"
    
        volume {
          name           = "Data Volume"
          disk_offering  = "20GB - 20 IOPS Min."
          size_in_gb     = 50
        }
    }
  2. Move network_acl_rule under network_acl:

    resource "cloudca_network_acl" "my_acl" {
        environment_id = "4cad744d-bf1f-423d-887b-bbb34f4d1b5b"
        name           = "test-acl"
        description    = "This is a test acl"
        vpc_id         = "8b46e2d1-bbc4-4fad-b3bd-1b25fcba4cec"
    
        rule {
            rule_number    = 55
            cidr           = "10.212.208.0/22"
            action         = "Allow"
            protocol       = "TCP"
            start_port     = 80
            end_port       = 80
            traffic_type   = "Ingress"
        }
    }
  3. Move port_forwarding_rule and static_nat and load_balancer_rule under public_ip:

    resource "cloudca_public_ip" "my_publicip" {
        environment_id = "4cad744d-bf1f-423d-887b-bbb34f4d1b5b"
        vpc_id         = "8b46e2d1-bbc4-4fad-b3bd-1b25fcba4cec"
    
        port_forwarding {
            public_port_start  = 80
            private_ip_id      = "30face92-f1cf-4064-aa7f-008ea09ef7f0"
            private_port_start = 8080
            protocol           = "TCP"
        }
    
        static_nat {
            private_ip_id  = "c0d9824b-cb83-45ca-baca-e7e6c63a96a8"
        }
    
        load_balancer {
            name              = "web_lb"
            protocol          = "tcp"
            algorithm         = "leastconn"
            public_port       = 80
            private_port      = 80
            instance_ids      = ["071e2929-672e-45bc-a5b6-703d17c08367"]
            stickiness_method = "AppCookie"
            stickiness_params {
                cookieName = "allo"
            }
        }
    }
khos2ow commented 5 years ago

@pdube @franzpgarcia @simongodard @swill what do you think? any thoughts, concerns, comments?

swill commented 5 years ago

I have not thought through all the implications, but here are my initial instincts.

I like the concept, but there are a few things I think we would need to understand, especially defining multiple sub-items (like rule or port_forwarding for example).

khos2ow commented 5 years ago

Thanks @swill for the feedback.

I didn't quite get what you meant by this, what do you have in mind?

I like the concept, but there are a few things I think we would need to understand, especially defining multiple sub-items (like rule or port_forwarding for example).

swill commented 5 years ago

The sub-items, you have covered with this:

These are essentially just nested blocks by definition and it's up to us to design them to be Zero or One OR Zero or Many. So I'd say most of them (except may be static_nat) must be able to define more than one block at each resource if needed.

We may want to support both options, but we may be able to deprecate the non-nested approach if we are unable to think of use cases where it is important to provision the top level element without the sub-elements. I guess you still could. So if you just didn't associate a rule for example, then the public IP would be provisioned, but the sub-items would not be created. I think Volume is likely the only one that would potentially need to handle both stand alone and a nested structure.

I like this idea though. Much easier to grasp for the users.

khos2ow commented 5 years ago

That's the point for all of them. You can provision top level item without being forced to define the nested items. The nested items are optional, and in some cases mutually exclusive (e.g. static_nat and port_forwarding).

It's not only easier to grasp for the users, but also makes the configs easier to maintain and less LoC thanks to for_each and dynamic block of Terraform 0.12. This will be completely valid snippet:

variable "list_of_port_forwarding" {
    type = list(object({
        public_port_start  = number,
        private_port_start = number,
        protocol = string,
    }))
    default = [{
        public_port_start  = 80
        private_port_start = 80
        protocol = "TCP"
    }, {
        public_port_start  = 443
        private_port_start = 443
        protocol = "TCP"
    }]
}

resource "cloudca_public_ip" "my_publicip" {
    environment_id = "4cad744d-bf1f-423d-887b-bbb34f4d1b5b"
    vpc_id         = "8b46e2d1-bbc4-4fad-b3bd-1b25fcba4cec"

    dynamic "port_forwarding" {
        iterator = pfr
        for_each = var.list_of_port_forwarding

        content {
            public_port_start  = pfr.value.public_port_start
            private_ip_id      = "30face92-f1cf-4064-aa7f-008ea09ef7f0"
            private_port_start = pfr.value.private_port_start
            protocol           = pfr.value.protocol
        }
    }
}
pdube commented 5 years ago

Definitely a nice structure for the ACL and IP resources. It feels more self contained.

For Volumes, we need to be careful because when the owner object is deleted, TF will delete the sub resources as well. We had considered including it initially, but decided not to given the implications of the delete instance.

In the case of ACL rules, PFRs and LBRs, this is completely fine because if you delete the IP (or ACl list), it removes everything anyways.

khos2ow commented 5 years ago

Thanks @pdube for the feedback.

TF will delete the sub resources as well

I think this is not true 100% of the times, I mean we can implement it in the way for the child resource not to get deleted when parent gets deleted. At least that's what I knew, we can validate this though.

pdube commented 5 years ago

It's true that we can implement in a way that the resource isn't deleted, but then there will be a dangling resource that isn't managed by TF. It's possible to import a resource now, so there's a workaround to re-manage that volume, but it's still a bit of a manual process

Just something to keep in mind

khos2ow commented 5 years ago

That's a really good point. So may be we can start converting everything except Volume, then later circle back to it and decide will it be justified to even attempt it or not. Makes sense?