azurerm_virtual_network: support for bgp_community and vnet_protection_enabled

[magodo]

Support for bgp_community and vnet_protection_enabled properties for vnet.

[katbyte]

thanks @magodo - just have a question about schema design but otherwise this looks pretty good

[katbyte]

should this be

Suggested change

	"bgp_community": {
	"bgp_community_asn": {

and as the first is always the microsoft one, should we just take a single int and then silently send the microsoft one? if it does change in the future to allow multiple we can always add a new property

[magodo]

I'm not quite sure whether this is a good idea, as the asn format of bgp community (defined by the rfc1997) is to use this 32bit as a whole and split into first two bytes and final two bytes only:

The rest of the community attribute values shall be encoded using an
autonomous system number in the first two octets. The semantics of
the final two octets may be defined by the autonomous system.

In other word, I think if we only export the final two bytes to users, it doesn't make sense to name that property as bgp_community. However, I can't come up with a better name 🤷‍♂️

In addition, as mentioned above bgp community attribute is combined of asn and some custom field, so it also not a fit to rename it to bgp_community_asn.

[katbyte]

I've come back to this multiple times over the last week and really can't think of a good way to express this.. right now you have:

bgp_community = [12076, 20000]

we could also expose what your doing under the hood and the API expects:

 bgp_community = "12076:20000"

and then we can validate it always starts with the correct as-number.. i'm leaning towards the 2nd option even if the code would be a little messier to validate it as it matches documentation i've found online eg: https://www.juniper.net/documentation/en_US/junos/topics/concept/policy-bgp-communities-extended-communities-match-conditions-overview.html

[magodo]

Yeah, that seems better than a two-length list :)

[katbyte]

Thanks @magodo - i see yourj point and have addressed it inline

[katbyte]

I've come back to this multiple times over the last week and really can't think of a good way to express this.. right now you have:

bgp_community = [12076, 20000]

we could also expose what your doing under the hood and the API expects:

 bgp_community = "12076:20000"

and then we can validate it always starts with the correct as-number.. i'm leaning towards the 2nd option even if the code would be a little messier to validate it as it matches documentation i've found online eg: https://www.juniper.net/documentation/en_US/junos/topics/concept/policy-bgp-communities-extended-communities-match-conditions-overview.html

[magodo]

@katbyte Thank you for the comments! I have updated the bgp community format per your suggestion, please take another look!

[katbyte]

Thanks @magodo - LGTM 👍

[ghost]

This has been released in version 2.36.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.36.0"
}
# ... other configuration ...

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!