iits-consulting / terraform-opentelekomcloud-project-factory

This repository helps to create an OTC-based cloud-native infrastructure landscape with Kubernetes, load balancers, VPCs, etc. With these modules, we provide you a rocket start while you can still deep-dive into detailed configuration later.
GNU General Public License v3.0
83 stars 20 forks source link

Misleading naming of vpc subnet ids / network ids #73

Closed MrRobot420 closed 1 year ago

MrRobot420 commented 1 year ago

CCE-Module For the CCE module, the naming of the attribute "subnet_id" suggests, that indeed the "subnet_id" is needed. Instead it should be named "network_id" because it is the network_id that has to be used here.


Also for the VPC module "subnets" outputs, it might make more sense to use "network_id" and "subnet_id".

Suggestion:

VPC-Module Change

vpc_subnets = {
      id        = "xxxx-xxxx-xxxx-xxxx-xxxx"
      subnet_id = "xxxx-xxxx-xxxx-xxxx-xxxx-yyyy"
 }

to

vpc_subnets = {
      network_id  = "xxxx-xxxx-xxxx-xxxx-xxxx"
      subnet_id   = "xxxx-xxxx-xxxx-xxxx-xxxx-yyyy"
 }
canaykin commented 1 year ago

Hi @MrRobot420

First of all, sorry for the delayed answer as it has been a few busy weeks on my part.


I completely agree with the confusion, which essentially stems from the Openstack and the OTC provider's naming.

The original issue stems from: vpc_subnet_v1 defines:

id - Specifies a resource ID in UUID format. Same as OpenStack network ID (OS_NETWORK_ID).

subnet_id - Specifies the OpenStack subnet ID.

network_id - Specifies the OpenStack network ID.

However when you compare ecs_instance_v1, cce_cluster_v3, and lb_loadbalancer_v2 ecs_instance_v1 describes:

network_id - (Required) The network UUID to attach to the server. Changing this creates a new server.

Where the network_id is correctly meaning id or network_id

cce_cluster_v3 describes:

subnet_id - (Required) The Network ID of the subnet used to create the node. Changing this parameter will create a new cluster resource.

where subnet_id means id or network_id which is incorrect.

and finally, lb_loadbalancer_v2 says:

vip_subnet_id - (Required) The network on which to allocate the loadbalancer's address. A tenant can only create loadalancers on networks authorized by policy (e.g. networks that belong to them or networks that are shared). Changing this creates a new loadbalancer.

where the vip_subnet_id means subnet_id.

This is why we wanted to keep the subnet_id in the variable naming for CCE module and document it the same as it is specified in the provider:

variable "cluster_subnet_id" {
  type        = string
  description = "Subnet network id where the cluster will be created in"
}

Regarding the VPC module outputs, I was initially planning to suggest:

output "subnets" {
  value = { for name, subnet in opentelekomcloud_vpc_subnet_v1.subnets : name => {
    id         = subnet.id
    subnet_id  = subnet.subnet_id
    network_id = subnet.network_id
  } }
}

to keep the backwards compatibility. But I believe this would work and be even more simplified with:

output "subnets" {
  value = opentelekomcloud_vpc_subnet_v1.subnets
}

to simply output all values as they are in the provider, which would have network_id and subnet_id set as you have described in your proposal.

Would this be an acceptable solution for you?

Best, Can.