hashicorp / terraform-cdk

Define infrastructure resources using programming constructs and provision them using HashiCorp Terraform
https://www.terraform.io/cdktf
Mozilla Public License 2.0
4.88k stars 456 forks source link

`azurerm` provider: Naming inconsistency with the `private_ip_address_allocation` property of the `NetworkInterfaceIpConfiguration` Class #2669

Open AtomAsking opened 1 year ago

AtomAsking commented 1 year ago

Don't worry, it's a small problem

1. The key name of the dict type cannot be consistent with the property name of the class

My Environment:

anti-human design:

The NetworkInterfaceIpConfiguration source code shows that there is a property named private_ip_address_allocation, which is the same as the name queried in the official Terraform manual, which is fine.

From the source code, we can see that: private_ip_address_allocation type hints: typing.Sequence[typing.Union["NetworkInterfaceIpConfiguration", typing.Dict[builtins.str, typing.Any]]]

ip_configuration=[{
                'name': 'test01-ip-configuration',
                "privateIpAddressAllocation": "Dynamic", 
                'subnet_id': subnet.id,
                'public_ip_address_id': public_ip_address.id
            }]
ip_configuration=[NetworkInterfaceIpConfiguration(
                name='test01-ip-configuration',
                private_ip_address_allocation="Dynamic",
                subnet_id=subnet.id,
                public_ip_address_id=public_ip_address.id
            )],

The above two pieces of Python code both pass the test. You will find that in the code snippet with the structure List(Dict), the property is named privateIpAddressAllocation, while in the code snippet with the structure List(NetworkInterfaceIpConfiguration), the property is named private_ip_address_allocation. By reading the official Terraform manual, we know that this naming rule is the most reasonable and is consistent with the official one.

I think this is an oversight because I tried declaring the security_rule attribute of the NetworkSecurityGroup class again and found that it did not have the above problem and passed the test successfully.

Finally hopefully a fix can be made in a new version as this will bother some developers and I believe it's a good habit to stay consistent with the official Terraform documentation.

Finally, let's take a look at how this is declared in the official Terraform documentation: https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/network_interface#private_ip_address_allocation

2. Report an anti-human design

The ip_configuration property of the NetworkInterface class is a Sequence for all to see, and the declaration of type hints shows that it supports two ways of writing, i.e. using the Dict type and the declaration of the NetworkInterfaceIpConfiguration class.

See some of the core code examples below:

"""
This is the code I tested and it works fine.
"""
virtual_network = VirtualNetwork(
            self,
            'my-virtual-network',
            name='test01-virtual-network',
            location=resource_group.location,
            resource_group_name=resource_group.name,
            address_space=['10.0.0.0/16'],
            subnet=[
                VirtualNetworkSubnet(
                    name='test01-subnet',
                    address_prefix='10.0.0.0/24',
                    security_group=network_security_group.id
                )
            ]
        )

network_interface = NetworkInterface(
            self,
            'my-network-interface',
            name='test01-network-interface',
            ip_configuration=[
                # Note the following line, which uses the `NetworkInterfaceIpConfiguration` class to initialize
                NetworkInterfaceIpConfiguration(
                    name='test01-ip-configuration',
                    private_ip_address_allocation="Dynamic", 
                    subnet_id=virtual_network.subnet.get(0).id,
                    public_ip_address_id=public_ip_address.id
                )
            ],
            location=resource_group.location,
            resource_group_name=resource_group.name
        )
"""
According to the type hints restriction of the source code is possible to use Dict type for initialization, but the code here will report an error, which is also tested by me.
"""
virtual_network = VirtualNetwork(
            self,
            'my-virtual-network',
            name='test01-virtual-network',
            location=resource_group.location,
            resource_group_name=resource_group.name,
            address_space=['10.0.0.0/16'],
            subnet=[
                VirtualNetworkSubnet(
                    name='test01-subnet',
                    address_prefix='10.0.0.0/24',
                    security_group=network_security_group.id
                )
            ]
        )

network_interface = NetworkInterface(
            self,
            'my-network-interface',
            name='test01-network-interface',
            ip_configuration=[
                # Note that the following Dict structure will throw an error.
                {
                    'name': 'test01-ip-configuration',
                    'privateIpAddressAllocation': "Dynamic",
                    'subnet_id': virtual_network.subnet.get(0).id,
                    'public_ip_address_id': public_ip_address.id
                }
            ],
            location=resource_group.location,
            resource_group_name=resource_group.name
        )

The error message is as follows.

my-stack  ╷
          │ Error: expanding `ip_configuration`: A Subnet ID must be specified for an IPv4 Network Interface.
          │ 
          │   with azurerm_network_interface.my-network-interface (my-network-interface),
          │   on cdk.tf.json line 58, in resource.azurerm_network_interface.my-network-interface (my-network-interface):
          │   58:       }

I think both issues are caused by the same unknown problem (i.e., there is an unknown problem with the support for Dict types), so they are put together and hopefully solved.

Jreamz commented 1 year ago

Ran into this issue today as well, friendly bump

cdktf --version
0.17.0