New Resource: Network Watcher Flow Log #2262
Conversation
* Update tests and move to network watcher tests. * Add docs.
ghost
added
the
There was a problem hiding this comment.
Hi @liemnotliam,
Thank you for this contribute. In addition to the comments I have left inline I think this makes more sense to be part of the network watcher resource rather than separate.
It's not creating or deleting an actual resource, just configuring an existing one, plus there is only one per network watcher. What do you think?
| "workspace_id": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| }, |
There was a problem hiding this comment.
Can we validate the ID here?
| } | ||
|
|
||
| err = future.WaitForCompletionRef(ctx, client.Client) | ||
| if err != nil { |
There was a problem hiding this comment.
Very minor but these lines could be joined:
if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {| } | ||
|
|
||
| err = future.WaitForCompletionRef(ctx, client.Client) | ||
| if err != nil { |
There was a problem hiding this comment.
Very minor but these lines could be joined:
if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {| resource.TestCheckResourceAttrSet(resourceName, "network_security_group_id"), | ||
| resource.TestCheckResourceAttrSet(resourceName, "storage_account_id"), | ||
| resource.TestCheckResourceAttr(resourceName, "retention_policy.#", "1"), | ||
| resource.TestCheckResourceAttrSet(resourceName, "retention_policy.0.enabled"), |
There was a problem hiding this comment.
We should be able to actually check this value for true/false
| resource.TestCheckResourceAttrSet(resourceName, "storage_account_id"), | ||
| resource.TestCheckResourceAttr(resourceName, "retention_policy.#", "1"), | ||
| resource.TestCheckResourceAttrSet(resourceName, "retention_policy.0.enabled"), | ||
| resource.TestCheckResourceAttrSet(resourceName, "retention_policy.0.days"), |
| } | ||
|
|
||
| err = future.WaitForCompletionRef(ctx, client.Client) | ||
| if err != nil { |
There was a problem hiding this comment.
Very minor but these lines could be joined too
| } | ||
|
|
||
| _, err = future.Result(client) | ||
| if err != nil { |
There was a problem hiding this comment.
Very minor but these lines could be joined too
|
|
||
| func testAccAzureRMNetworkWatcherFlowLog_retentionPolicyConfig(rInt int, rString string, location string) string { | ||
| return fmt.Sprintf(` | ||
| resource "azurerm_resource_group" "test" { |
There was a problem hiding this comment.
Not a blocker, but we could use a template for the resources we depend on so shorten these tests
|
Thanks for the comments @katbyte I’ll push out the changes later. There isn’t a 1-1 relationship between a network watcher and the flow log. A single network watcher in a region can configure multiple flow logs for NSGs in that region. There’s a 1-1 mapping between an NSG and a flow log, but I didn’t want to embed it in the NSG resource because it uses different clients. I thought it would be more flexible to have the flow log as a separate resource so it’s not too tied down to either network watcher or NSG. I didn’t go down the route of having multiple flow log blocks within the network watcher resource to match how subnet-vnet, subnet-NSG, subnet-routetable associations are mapped as separate resources. There is a network watcher flow resource ID in Azure if you look deep enough, but it’s not supported in the SDKs as of now. What are your thoughts? |
|
If there can be multiple per network watcher then maybe a list of flow logs would make sense? It seems you always need to pass in a network watcher to the client, and the flow log is associated to that network watcher. I think having as a separate resource kinda complicated the logic because it is a setting/configured as part of a network watcher, and it can't be deleted. While if it was part of that resource it would just be deleted naturally. |
|
Yeah it's possible to have multiple flow logs in a single network watcher resource, but it presents a different set of problems and complexity. It is a separate API call to configure each flow log; there is no function in the SDK to update a network watcher that accepts a list of flow logs. This means that there needs to be extra logic in the resource provider to keep track of what the changes were vs what's deployed in Azure and then create(enable)/delete(disable)/update network watcher flow logs accordingly. A couple of tricky scenarios include:
Another reason for having it as a separate resource was to be able to dynamically create multiple flow logs like resource "azurerm_network_watcher_flow" "test" {
count = "${length(var.nsgs)}"
network_security_group_id = "${var.nsgs[count.index]}"
# ...rest of config
}It's a minor point as I realise it's possible with the new version of Terraform and HCL to do loops and dynamic blocks within a resource, but that hasn't been released yet.. What are your thoughts @katbyte ? I'm alright with either approach |
|
@liemnotliam, I think it should be part of the resource and not separate. Any thoughts on this @tombuildsstuff? You can see how another resource handles the logic in #2055 |
|
Just wondering if you are still working on this 🙂 |
|
@katbyte yes, I'll have time to check this again this/next week, but feel free to add/make changes if you have any 🙂 |
|
|
||
| if err := d.Set("traffic_analytics", flattenAzureRmNetworkWatcherFlowLogTrafficAnalytics(fli.FlowAnalyticsConfiguration)); err != nil { | ||
| return fmt.Errorf("Error setting `traffic_analytics`: %+v", err) | ||
| } |
There was a problem hiding this comment.
since the properties block may not necessarily be returned (due to an invalid API response) - could we wrap this in an if statement, e.g.:
if props := fli.FlowKLogProperties; props != nil {
d.Set("network_security_group_id", props.TargetResourceID)
d.Set("enabled", props.Enabled)
if props.StorageID != nil && *props.StorageID != "" {
d.Set("storage_account_id", props.StorageID)
}
if err := d.Set("retention_policy", flattenAzureRmNetworkWatcherFlowLogRetentionPolicy(props.RetentionPolicy)); err != nil {
return fmt.Errorf("Error setting `retention_policy`: %+v", err)
}
if err := d.Set("traffic_analytics", flattenAzureRmNetworkWatcherFlowLogTrafficAnalytics(props.FlowAnalyticsConfiguration)); err != nil {
return fmt.Errorf("Error setting `traffic_analytics`: %+v", err)
}
}
| func flattenAzureRmNetworkWatcherFlowLogTrafficAnalytics(input *network.TrafficAnalyticsProperties) []interface{} { | ||
| if input == nil { | ||
| return []interface{}{} | ||
| } else if isDefaultDisabledFlowLogTrafficAnalytics(input) { |
There was a problem hiding this comment.
minor we could probably combine these e.g.
| } else if isDefaultDisabledFlowLogTrafficAnalytics(input) { | |
| if input == nil || isDefaultDisabledFlowLogTrafficAnalytics(input) { | |
| return []interface{}{} |
| } | ||
|
|
||
| result := make(map[string]interface{}) | ||
| if input != nil { |
There was a problem hiding this comment.
with the changes above, since this is now always covered above, I believe we should be able to remove this if statement?
|
I have merged master & fixed the above error. Now the tests fail with: I have opened an issue on the azure sdk as all the parameters look correct. |
katbyte
added
the
upstream/microsoft
|
Just out of curiosity - where are we with this? Currently, I'm having to run a script to configure the FlowLogs post TF Deploy and would love to wrap that into the actual deployment. |
It's been only a year and a half! Be patient my friend! |
|
@BenMitchell1979, @peimanja, see my last comment. The resource simply does not work as is and while i have tried to get it into a usable state there's not much i can do with an "internal sever error" message. Without a reasonable error message from azure that makes sense it is not easy to figure out what we need to do. If you would like to see this resource in terraform please +1 the issue on the SDK as we are currently waiting on microsoft. Also for what it's worth this PR is only 6 months old, not 18. |
Sorry, my bad. Do not get me wrong I am ranting about Microsoft support and poorly API and lots of bugs they introduce with each release. I appreciate people contributing and I see most of the contributors are frustrated. many PRs are hanging around for months. This is just bad ... |
@katbyte If you'd be willing to include a link to the respective issue here, our team will add our +1's to it (as many as have access). Thanks! |
|
👋 Since this PR is blocked on the upstream issue, rather than leaving this PR open blocked until that's resolved I'm going to temporarily close this PR for the moment (and assign this to the Thanks! |
|
@tombuildsstuff There has been confirmation that this has been fixed on the Azure Go SDK issue. |
|
@adjohns thanks, we've been trying to re-run the tests to confirm this is fixed - will update when they've finished :) |
Hi @tombuildsstuff, any news about this? It would be a relief, if it's solved. Big thanks anyway 👍 |
|
@tombuildsstuff Have the tests finished yet? If so will this be looked at again |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR adds support for a new resource - network watcher flow log.
Addresses issue #1776
Test results:
(fixes #1776)