`azurerm_iothub` - support for new property `endpoint.subscription_id` #27524

neil-yechenwei · 2024-09-27T08:49:17Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Recently we got a MS Internal support ticket created by customer. Customer said that "Event Hub Status" button on the "IoTHub" resource is not highlighted in blue on Azure Portal like below screenshot after the IoTHub resource is created. After I investigated, I found that this issue only happens when IoTHub and the related EventHub are in the different subscriptions. Then I checked TF code. I found currently TF always sets "endpoint.subscriptionId" with the subscription ID in the current TF context. So it's unexpected behavior since service team confirmed that service API allows IoTHub and EventHub are in the different subscriptions. So we need to expose "endpoint.subscription_id" to users.

After I applied this PR fix, now "Event Hub Status" button on the "IoTHub" resource is highlighted in blue on Azure Portal.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Below failed test case is also failed with same error on Teamcity Daily Run. So it's not related with this PR.

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_iothub - support for new property endpoint.subscription_id

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

internal/services/iothub/iothub_resource.go

magodo

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

magodo · 2024-09-29T01:43:56Z

internal/services/iothub/iothub_resource.go

+		if v := d.GetRawConfig().AsValueMap()["endpoint"].AsValueSlice()[k].AsValueMap()["subscription_id"]; v.IsNull() {
+			subscriptionID = subscriptionId
+		}


Instead of setting the schema as O+C, can we make it Optional only by adding a restriction to this attribute (with documenting clearly): If the subscription id is the same as the provider id, simply set it as null or an empty string. This makes sense to me as users need to specify this attribute, only when it is different than the provider's subscription id.

With this, you only need to do a conditional set to state during the flattening.

If we do it this way, it will result in the subscription_id on the service side having a value, but the subscription_id in the state file will be empty. This might confuse the user, so I still recommend using my method to fix it.

The current implementation has no issue in itself, but I'm more concerned about the complexity it brings, from the maintenance perspective. If we decide to go down this route, please remember to add comments to clearly describe the raw config handling is only deant for the case that the user has a eventhub whose endpoint's subscription is not the provider's one. Then the user wants to reset it to the provider's one by unset the subscription_id. The current comment doesn't make sense to me.

Sure. I updated the descriptions.

Do we really need to do this? and how reliable is it given the issues we have with GetOkExists etc not working in every situation?

i do worry about adding this as it will be technical debt for when the transition to framework is done

this is only to enable a user to unset the value and have it default to the current provider subscription?

Yes. Using "d.GetRawConfig()" is to align with the previous TF behavior to enabled a user to unset the value and have it default to the current provider subscription for unblocking the existing users.

internal/services/iothub/iothub_resource.go

katbyte

We have seperate resources for this to in the form of azurerm_iothub_endpoint_* - we should probably fix this for those resources too?

katbyte · 2024-10-24T16:03:01Z

internal/services/iothub/iothub_resource.go

@@ -376,6 +376,14 @@ func resourceIotHub() *pluginsdk.Resource {
 						},

 						"resource_group_name": commonschema.ResourceGroupNameOptional(),
+
+						// `Computed: true` is required since this property would always be set even if it isn't specified in the tf config, otherwise it would cause difference and block the existing users


could we plase make this

Suggested change

// `Computed: true` is required since this property would always be set even if it isn't specified in the tf config, otherwise it would cause difference and block the existing users

// NOTE: O+C : required since this property would always be set even if it isn't specified in the tf config, otherwise it would cause a diff and break existing users

to match other O+C properties

katbyte · 2024-10-24T17:20:57Z

website/docs/r/iothub.html.markdown

@@ -219,6 +219,10 @@ An `endpoint` block supports the following:

 * `resource_group_name` - (Optional) The resource group in which the endpoint will be created.

+* `subscription_id` - (Optional) The subscription ID for the endpoint.
+
+~> **NOTE:** When `subscription_id` isn't specified in the tf config, it would be set with the provider's subscription ID.


Suggested change

~> **NOTE:** When `subscription_id` isn't specified in the tf config, it would be set with the provider's subscription ID.

~> **NOTE:** When `subscription_id` isn't specified it will be set to the subscription ID used in the provider block.

katbyte · 2024-10-24T17:26:32Z

internal/services/iothub/iothub_resource.go

+		if v := d.GetRawConfig().AsValueMap()["endpoint"].AsValueSlice()[k].AsValueMap()["subscription_id"]; v.IsNull() {
+			subscriptionID = subscriptionId
+		}


Do we really need to do this? and how reliable is it given the issues we have with GetOkExists etc not working in every situation?

i do worry about adding this as it will be technical debt for when the transition to framework is done

this is only to enable a user to unset the value and have it default to the current provider subscription?

katbyte · 2024-10-24T17:27:59Z

internal/services/iothub/iothub_resource.go

 		endpoint := endpointRaw.(map[string]interface{})

 		t := endpoint["type"]
 		name := endpoint["name"].(string)
 		resourceGroup := endpoint["resource_group_name"].(string)
 		authenticationType := devices.AuthenticationType(endpoint["authentication_type"].(string))
-		subscriptionID := subscriptionId
+
+		subscriptionID := endpoint["subscription_id"].(string)


something like

Suggested change

subscriptionID := endpoint["subscription_id"].(string)

if v, ok := config["subscription_id"]; ok && v.(string) != "" {

subscriptionID = v.(string)

}

neil-yechenwei · 2024-10-25T02:06:12Z

@katbyte , thanks for the comments.

Yes, we also need to fix "azurerm_iothub_endpoint_*". I think we can fix them in the separate PR.

Actually, I suggested users to deploy IotHub and EventHub in the same subscription and they accept it and the issue is marked as resolved. So can we close this PR?

katbyte · 2024-10-28T19:20:13Z

@neil-yechenwei - no i think we should still support the new property as it is something the API supports and if one person has run into it, someone else likely has/will and might have a reason to do so

neil-yechenwei · 2024-10-29T01:05:32Z

@katbyte , thanks for double confirmation.

neil-yechenwei · 2024-10-31T08:52:14Z

@katbyte , please continue to review this PR. Thanks.

katbyte · 2024-11-05T19:41:15Z

@neil-yechenwei - please also fix the azurerm_iothub_endpoint_* resources too

neil-yechenwei · 2024-11-06T08:40:21Z

@katbyte , I updated PR to fix the azurerm_iothub_endpoint_* resources. Below is the latest test result I just now triggered.

Below failed test cases are also failed with same error on Teamcity Daily Run. So they're not related with this PR.

azurerm_iothub - support for new property endpoint.subscription_id

c01b687

github-actions bot added documentation service/iot-hub size/XS labels Sep 27, 2024

neil-yechenwei added 2 commits September 28, 2024 07:24

update tc

43fb530

update tc

eddcfcc

neil-yechenwei marked this pull request as draft September 28, 2024 00:08

add getrawconfig

ad0fcfe

neil-yechenwei marked this pull request as ready for review September 28, 2024 02:25

update comment

8376781

mybayern1974 reviewed Sep 28, 2024

View reviewed changes

internal/services/iothub/iothub_resource.go Show resolved Hide resolved

github-actions bot added the waiting-response label Sep 28, 2024

neil-yechenwei requested a review from mybayern1974 September 29, 2024 01:10

github-actions bot removed the waiting-response label Sep 29, 2024

magodo requested changes Sep 29, 2024

View reviewed changes

github-actions bot added the waiting-response label Sep 29, 2024

neil-yechenwei requested a review from magodo September 29, 2024 02:16

github-actions bot added waiting-response and removed waiting-response labels Sep 29, 2024

update description per the comments

f9af0f1

github-actions bot added size/S and removed waiting-response size/XS labels Sep 29, 2024

rcskosir added the bug label Oct 14, 2024

katbyte requested changes Oct 24, 2024

View reviewed changes

github-actions bot added the waiting-response label Oct 24, 2024

update pr per comments

c03d487

neil-yechenwei requested a review from a team as a code owner October 25, 2024 01:44

github-actions bot removed the waiting-response label Oct 25, 2024

Merge remote-tracking branch 'upstream/main' into fixiothub02

69ab779

update comment

47b76d7

neil-yechenwei requested a review from katbyte October 25, 2024 02:06

katbyte added the waiting-response label Oct 28, 2024

github-actions bot removed the waiting-response label Oct 29, 2024

katbyte added the waiting-response label Oct 31, 2024

github-actions bot removed the waiting-response label Oct 31, 2024

katbyte added the waiting-response label Nov 5, 2024

Merge remote-tracking branch 'upstream/main' into fixiothub02

e78cab5

github-actions bot removed the waiting-response label Nov 6, 2024

support subscriptionId for other related resources

9f39da7

github-actions bot added size/L and removed size/S labels Nov 7, 2024

katbyte self-assigned this Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_iothub` - support for new property `endpoint.subscription_id` #27524

`azurerm_iothub` - support for new property `endpoint.subscription_id` #27524

neil-yechenwei commented Sep 27, 2024 •

edited

Loading

magodo left a comment

magodo Sep 29, 2024 •

edited

Loading

neil-yechenwei Sep 29, 2024

magodo Sep 29, 2024

neil-yechenwei Sep 29, 2024

katbyte Oct 24, 2024

neil-yechenwei Oct 25, 2024

katbyte left a comment

katbyte Oct 24, 2024

neil-yechenwei Oct 25, 2024

katbyte Oct 24, 2024

neil-yechenwei Oct 25, 2024

katbyte Oct 24, 2024

katbyte Oct 24, 2024

neil-yechenwei Oct 25, 2024

neil-yechenwei commented Oct 25, 2024 •

edited

Loading

katbyte commented Oct 28, 2024

neil-yechenwei commented Oct 29, 2024

neil-yechenwei commented Oct 31, 2024

katbyte commented Nov 5, 2024

neil-yechenwei commented Nov 6, 2024 •

edited

Loading

	// `Computed: true` is required since this property would always be set even if it isn't specified in the tf config, otherwise it would cause difference and block the existing users
	// NOTE: O+C : required since this property would always be set even if it isn't specified in the tf config, otherwise it would cause a diff and break existing users

	~> NOTE: When `subscription_id` isn't specified in the tf config, it would be set with the provider's subscription ID.
	~> NOTE: When `subscription_id` isn't specified it will be set to the subscription ID used in the provider block.

-		subscriptionID := endpoint["subscription_id"].(string)
+if v, ok := config["subscription_id"]; ok && v.(string) != "" {
+				subscriptionID = v.(string)
+			}

azurerm_iothub - support for new property endpoint.subscription_id #27524

Are you sure you want to change the base?

azurerm_iothub - support for new property endpoint.subscription_id #27524

Conversation

neil-yechenwei commented Sep 27, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

magodo left a comment

Choose a reason for hiding this comment

magodo Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Oct 25, 2024 • edited Loading

katbyte commented Oct 28, 2024

neil-yechenwei commented Oct 29, 2024

neil-yechenwei commented Oct 31, 2024

katbyte commented Nov 5, 2024

neil-yechenwei commented Nov 6, 2024 • edited Loading

`azurerm_iothub` - support for new property `endpoint.subscription_id` #27524

`azurerm_iothub` - support for new property `endpoint.subscription_id` #27524

neil-yechenwei commented Sep 27, 2024 •

edited

Loading

magodo Sep 29, 2024 •

edited

Loading

neil-yechenwei commented Oct 25, 2024 •

edited

Loading

neil-yechenwei commented Nov 6, 2024 •

edited

Loading