Fixes to IoTHub diff algorithm

[maxbog]

Specific fixes:

• properties batch_frequency_in_seconds, max_chunk_size_in_bytes, encoding, container_name, file_name_format are only applicable to endpoints of type AzureIotHub.StorageContainer
• connection_string diff algorithm takes into account the masked Access Keys when reading the connection string from Azure
• connection_string diff algorithm takes into account the default port for EventHub endpoints

[maxbog]

@tombuildsstuff Is there anyone that can review the changes? One of the things this fixes is secret leakage to the console. Right now, connection strings with secret access keys are shown in plain text while printing the execution plan.

[katbyte]

Hi @maxbog,

Thank you for the PR,

There is alot going on in here so ideally this would have been a bit better split up into a couple PRs so we could have merged the obvious ones such as marking the connection string as sensitive.

I am struggling to understand the need for the suppressIfTypeIsNot calls, could you elaborate upon them? Aside from those the rest of the PR LGTM 🙂

[maxbog]

@katbyte Thanks for the review, I extracted the obvious parts to a different PR: #3007
I will remove those changes from this PR.

As for the suppressIfTypeIsNot checks:
The problem is that the endpoint element supports different types of endpoints (Storage container, Service Bus, Event Hub). However, the properties that have suppressed diffs are only applicable to AzureIotHub.StorageContainer type. This, combined with them being optional and having a default value, caused the diff to show changes in their value when creating an endpoint of a different type.

For example when I defined the following simple endpoint:

endpoint {
    type = "AzureIotHub.EventHub"

    connection_string = "${azurerm_eventhub_authorization_rule.device_messages_iothubroutes.primary_connection_string}"
    name = "device-messages"
}

The second apply, which should not show any changes, actually shows this:

endpoint.0.batch_frequency_in_seconds: "0" => "300"
endpoint.0.max_chunk_size_in_bytes:    "0" => "314572800"
endpoint.0.type:                       "" => "AzureIotHub.EventHub"

Of course, this does not actually result in any changes being made, so the diff output is incorrect.

The approach adopted here with the SuppressDiffFunc is used in AWS provider, that's where I got the inspiration:
https://github.com/terraform-providers/terraform-provider-aws/blob/042ed092aaeafb5e4beef583adb0d6bb8695c9c4/aws/resource_aws_lb.go#L176

However, please do not merge this PR for now, as I think there may be an issue with DiffSuppressFunc of the connection_string property.

[maxbog]

@katbyte I believe the PR can be merged now, I changed the diff supression logic a bit for connection_string property

[katbyte]

Thanks for splitting it out @maxbog, we'll get the other one merged shortly, however this one is currently failing on the tests with:

------- Stdout: -------
=== RUN   TestAccAzureRMIotHub_customRoutes
=== PAUSE TestAccAzureRMIotHub_customRoutes
=== CONT  TestAccAzureRMIotHub_customRoutes
--- FAIL: TestAccAzureRMIotHub_customRoutes (97.26s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
        	* azurerm_iothub.test: 1 error occurred:
        	* azurerm_iothub.test: Error creating/updating IotHub "acctestIoTHub-190306190727160725" (Resource Group "acctestRG-190306190727160725"): devices.IotHubResourceClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="Failed" Message="The async operation failed." InnerError={"unmarshalError":"json: cannot unmarshal number into Go struct field serviceError2.code of type string"} AdditionalInfo=[{"code":400116,"httpStatusCode":"BadRequest","message":"endpointName:export, exceptionMessage:The argument must not be empty string.\r\nParameter name: containerName. If you contact a support representative please include this correlation identifier: 4b1e7873-fa43-4ef9-9d1c-6e09d146e04f, timestamp: 2019-03-06 19:08:04Z, errorcode: IH400116."}]

[maxbog]

@katbyte I fixed the code, the culprit was: c750319

I also extended the acceptance test to account for EventHub endpoint type.

[maxbog]

@katbyte: Any comments?

[katbyte]

Apologies for the delay @maxbog, my attention was elsewhere. Tests now pass and so this LGTM 🚀

[ghost]

This has been released in version 1.24.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 = "~> 1.24.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!