Upgrade network connection monitor from v1 to v2 #8640
Conversation
azurerm/internal/services/network/network_connection_monitor_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/network/network_connection_monitor_resource.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the PR @neil-yechenwei - i've given this a quick review and left comments inline that need to be asddressed
azurerm/internal/services/network/network_connection_monitor_resource.go
Show resolved
Hide resolved
azurerm/internal/services/network/network_connection_monitor_resource.go
Show resolved
Hide resolved
azurerm/internal/services/network/network_connection_monitor_resource.go
Show resolved
Hide resolved
| "address": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| }, |
azurerm/internal/services/network/network_connection_monitor_resource.go
Show resolved
Hide resolved
|
|
||
| A `success_threshold` block supports the following: | ||
|
|
||
| * `checks_failed_percent` - (Optional) The maximum percentage of failed checks permitted for a test to evaluate as successful. |
There was a problem hiding this comment.
grammer
| * `checks_failed_percent` - (Optional) The maximum percentage of failed checks permitted for a test to evaluate as successful. | |
| * `checks_failed_percent` - (Optional) The maximum percentage of failed checks permitted for a test to be successful. |
|
|
||
| * `checks_failed_percent` - (Optional) The maximum percentage of failed checks permitted for a test to evaluate as successful. | ||
|
|
||
| * `round_trip_time_ms` - (Optional) The maximum round-trip time in milliseconds permitted for a test to evaluate as successful. |
There was a problem hiding this comment.
| * `round_trip_time_ms` - (Optional) The maximum round-trip time in milliseconds permitted for a test to evaluate as successful. | |
| * `round_trip_time_ms` - (Optional) The maximum round-trip time in milliseconds permitted for a test to be successful. |
|
|
||
| * `port` - (Required) The port to connect to. | ||
|
|
||
| * `disable_trace_route` - (Optional) Should path evaluation with trace route be disabled? Defaults to `false`. |
There was a problem hiding this comment.
we always end with enabled/disabled
| * `disable_trace_route` - (Optional) Should path evaluation with trace route be disabled? Defaults to `false`. | |
| * `trace_route_disabled` - (Optional) Should path evaluation with trace route be disabled? Defaults to `false`. |
or even change it to trace_route_enabled
|
|
||
| * `name` - (Required) The name of the connection monitor test group. | ||
|
|
||
| * `destinations` - (Required) A list of destination endpoint names. |
There was a problem hiding this comment.
would this be more clear as
| * `destinations` - (Required) A list of destination endpoint names. | |
| * `destination_endpoints` - (Required) A list of destination endpoint names. |
|
|
||
| A `icmp_configuration` block supports the following: | ||
|
|
||
| * `disable_trace_route` - (Optional) Will path evaluation with trace route be disabled? Defaults to `false`. |
There was a problem hiding this comment.
we should always have disabled on the end of the property name
| * `disable_trace_route` - (Optional) Will path evaluation with trace route be disabled? Defaults to `false`. | |
| * `trace_route_disable` - (Optional) Will path evaluation with trace route be disabled? Defaults to `false`. |
There was a problem hiding this comment.
@neil-yechenwei I have scanned through and left some comments inline. Once it is addressed, we can go on reviewing this PR.
|
|
||
| connectionMonitor := NetworkConnectionMonitorId{ | ||
| ResourceGroup: id.ResourceGroup, | ||
| WatcherId: strings.Split(input, "/connectionMonitors/")[0], |
There was a problem hiding this comment.
Can we insist on using the id.Popsegment to do the id parsing?
There was a problem hiding this comment.
Here is the id of network watcher. So I assume we have to do this since id.Popsegment cannot expose it.
|
|
||
| type NetworkConnectionMonitorId struct { | ||
| ResourceGroup string | ||
| WatcherId string |
There was a problem hiding this comment.
Now that we have the WatcherName, we do we need to export this?
There was a problem hiding this comment.
Because we have to use it to set watcher id to state file.
d.Set("network_watcher_id", id.WatcherId)
There was a problem hiding this comment.
In this case, can we opt in ID formatter for the network watcher, and use the NewXXX() function in place where you need to set the network watcher id?
There was a problem hiding this comment.
thanks. updated
| func NetworkConnectionMonitorHttpPath(v interface{}, k string) (warnings []string, errors []error) { | ||
| value := v.(string) | ||
|
|
||
| if !regexp.MustCompile(`^(/.*)?$`).MatchString(value) { |
There was a problem hiding this comment.
Should we put more thoughts on the validation logic here? Otherwise, path like ////// is also a valid input?
| @@ -10,61 +10,144 @@ description: |- | |||
|
|
|||
| Manages a Network Connection Monitor. | |||
|
|
|||
| ~> **NOTE:** Any Network Connection Monitor resource created with API versions 2019-06-01 or earlier (v1) are now incompatible with terraform which now only supports v2. | |||
There was a problem hiding this comment.
| ~> **NOTE:** Any Network Connection Monitor resource created with API versions 2019-06-01 or earlier (v1) are now incompatible with terraform which now only supports v2. | |
| ~> **NOTE:** Any Network Connection Monitor resource created with API versions 2019-06-01 or earlier (v1) are now incompatible with Terraform, which now only supports v2. |
|
|
||
| --- | ||
|
|
||
| * `auto_start` - (Optional) Will the connection monitor start automatically once created? Changing this forces a new Network Connection Monitor to be created. | ||
| * `notes` - (Optional) Any notes about the Network Connection Monitor. |
There was a problem hiding this comment.
Can we modify this statement a bit?
There was a problem hiding this comment.
This sentence is copied from existing resource.
There was a problem hiding this comment.
Then could we fix this statement and apply the updated improved versinoit to where it was copied from?
There was a problem hiding this comment.
Sure. Updated the description for this property. For the others, I will update them in another separate PR.
| A `destination` block supports the following: | ||
| A `endpoint` block supports the following: | ||
|
|
||
| * `name` - (Required) The name of the connection monitor endpoint. |
There was a problem hiding this comment.
Can we capitalize the first letter of the resource type to be consistent (this applies to other parts)?
|
|
||
| * `name` - (Required) The name of the connection monitor endpoint. | ||
|
|
||
| * `address` - (Optional) The address of the connection monitor endpoint (IP or domain name). |
There was a problem hiding this comment.
| * `address` - (Optional) The address of the connection monitor endpoint (IP or domain name). | |
| * `address` - (Optional) The IP address or domain name of the Network Connection Monitor endpoint. |
|
|
||
| * `filter` - (Optional) A `filter` block as defined below. | ||
|
|
||
| * `virtual_machine_id` - (Optional) The ID of the virtual machine which is used as the endpoint by connection monitor. |
There was a problem hiding this comment.
Can we capitalize the first letter of the resource type to be consistent?
| A `source` block supports the following: | ||
| A `test_configuration` block supports the following: | ||
|
|
||
| * `name` - (Required) The name of the connection monitor test configuration. |
There was a problem hiding this comment.
Can we capitalize the first letter of the resource type to be consistent?
|
|
||
| * `protocol` - (Required) The protocol used to evaluate tests. Possible values are `Tcp`, `Http` and `Icmp`. | ||
|
|
||
| * `test_frequency_iin_seconds` - (Optional) The frequency of test evaluation, in seconds. Defaults to `60`. |
There was a problem hiding this comment.
@neil-yechenwei Thank you for the update, I left more comments inline, which shall be addressed.
| func (id NetworkWatcherId) ID(subscriptionId string) string { | ||
| return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkWatchers/%s", | ||
| subscriptionId, id.ResourceGroup, id.Name) | ||
| } | ||
|
|
||
| func NewNetworkWatcherID(resourceGroup, name string) NetworkWatcherId { | ||
| return NetworkWatcherId{ | ||
| ResourceGroup: resourceGroup, | ||
| Name: name, | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we move these impl to network_watcher.go?
| return warnings, errors | ||
| } | ||
|
|
||
| if !regexp.MustCompile(`^((/[^/]+)+[/]?|/)$`).MatchString(value) { |
There was a problem hiding this comment.
I'm afraid this regexp is still error prone, is there any possiblity that we don't use regexp to validate a http path, but leverage some other parsing functions if any?
| return warnings, errors | ||
| } | ||
|
|
||
| if !regexp.MustCompile(`^(([a-zA-Z]{1})|([a-zA-Z]{1}[a-zA-Z]{1})|([a-zA-Z]{1}[0-9]{1})|([0-9]{1}[a-zA-Z]{1})|([a-zA-Z0-9][a-zA-Z0-9-_]{1,61}[a-zA-Z0-9]))\.([a-zA-Z]{2,6}|[a-zA-Z0-9-]{2,30}\.[a-zA-Z |
There was a problem hiding this comment.
Can you explain this complex regexp here? (some parts can be simplified by removing the {1})
| return warnings, errors | ||
| } | ||
|
|
||
| if len(value) == 7 { |
There was a problem hiding this comment.
Can you add some comments to tell what the expected code range is?
|
|
||
| * `interval_in_seconds` - (Optional) Monitoring interval in seconds. | ||
| * `output_workspace_resource_ids` - (Optional) A list of the Log Analytics Workspace id that should be used for producing output into a Log Analytics for the Network Connection Monitor. |
There was a problem hiding this comment.
| * `output_workspace_resource_ids` - (Optional) A list of the Log Analytics Workspace id that should be used for producing output into a Log Analytics for the Network Connection Monitor. | |
| * `output_workspace_resource_ids` - (Optional) A list of IDs of the Log Analytics Workspace which will accept the output from the Network Connection Monitor. |
|
|
||
| * `method` - (Optional) The HTTP method for the HTTP request. Possible values are `Get` and `Post`. Defaults to `Get`. | ||
|
|
||
| * `port` - (Optional) The port for the Http connection. |
There was a problem hiding this comment.
Can we insist on using HTTP (all capitalized) all over the document to be consistent?
|
|
||
| * `port` - (Optional) The port for the Http connection. | ||
|
|
||
| * `path` - (Optional) The path component of the URI. For instance, `/dir1/dir2`. |
There was a problem hiding this comment.
I guess there is no need to provide the example, as the path component of a URI is a well-known fact.
| * `path` - (Optional) The path component of the URI. For instance, `/dir1/dir2`. | |
| * `path` - (Optional) The path component of the URI. |
|
|
||
| A `icmp_configuration` block supports the following: | ||
|
|
||
| * `trace_route_disabled` - (Optional) Should path evaluation with trace route be disabled? Defaults to `false`. |
There was a problem hiding this comment.
Can we negate this to be trace_route_enabled, and change the logic to reflect this? Since it is more intuitive for users.
|
|
||
| * `port` - (Required) The port for the Tcp connection. | ||
|
|
||
| * `trace_route_disabled` - (Optional) Should path evaluation with trace route be disabled? Defaults to `false`. |
There was a problem hiding this comment.
Can we negate this to be trace_route_enabled, and change the logic to reflect this? Since it is more intuitive for users.
|
|
||
| * `virtual_machine_id` - (Required) The ID of the virtual machine used as the source by connection monitor. | ||
| * `test_configurations` - (Required) A list of test configuration names. |
There was a problem hiding this comment.
| * `test_configurations` - (Required) A list of test configuration names. | |
| * `test_configuration_names` - (Required) A list of test configuration names. |
|
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 ... |
|
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! |
ghost
locked as resolved and limited conversation to collaborators
This PR is to fix below issues:
azurerm_network_connection_monitordoesn't work anymore for id segment issue while deleting resource, so made this PR to update segment of resource id for network connection monitor.auto_start,interval_in_seconds,sourceanddestination. These properties have been retired after version 2019-06-01.endpoint,notes,test_configuration,test_groupandoutputfor new api version.Note: Please merge PR 7349 first for connection monitor v1, and release a new azurerm provider version, then merge this PR for connection monitor v2.