New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

`r\iothub`: Extract `file_upload` and support `identityBased` auth #15466

Closed

myc2h6o wants to merge 1 commit into hashicorp:main from myc2h6o:iothub_file_upload

Contributor

myc2h6o commented Feb 17, 2022

Similar to #14705. Add support for Identity-Based authentication for File Upload.

Added a extracted resource azurerm_iothub_file_upload so that File Upload can use SystemAssigned identity of its IotHub at creation time.
The Identity can also be specified on azurerm_iothub directly, however SystemAssigned can only be used at Update because it requires a Role Assignment and it can only be done after the IotHub is created.

github-actions bot added documentation service/iot-hub size/XL labels

Contributor Author

myc2h6o commented Feb 18, 2022

Teste result (Failed test is due to transient latency of Role Assignment)

myc2h6o force-pushed the iothub_file_upload branch from 6c77b99 to 3fa76d8 Compare

February 23, 2022 03:32


          r\iothub: Extract file_upload and support identityBased auth

fb0be9e

myc2h6o force-pushed the iothub_file_upload branch from 3fa76d8 to fb0be9e Compare

March 7, 2022 04:34

stephybun requested changes

View reviewed changes

Member

stephybun left a comment

Thanks for this PR @myc2h6o. This is off to a good start, i've left some questions and comments in the review. Once those are addressed we can run the tests and take another look!

internal/services/iothub/iothub_file_upload_resource.go

+              				Sensitive:        true,
+              			},
+              			"container_name": {

Member

stephybun Mar 15, 2022

Could we add some validation here?

internal/services/iothub/iothub_file_upload_resource.go

+              	}
+              	if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {
+              		return fmt.Errorf("waiting for the completion of the creating/updating of %s: %+v", id, err)

Member

stephybun Mar 15, 2022

Suggested change

      
            		return fmt.Errorf("waiting for the completion of the creating/updating of %s: %+v", id, err)
          
            		return fmt.Errorf("waiting for creation/update of %s: %+v", id, err)

internal/services/iothub/iothub_file_upload_resource.go

Comment on lines +22 to +24

+              		Create: resourceIotHubFileUploadCreateUpdate,
+              		Read:   resourceIotHubFileUploadRead,
+              		Update: resourceIotHubFileUploadCreateUpdate,

Member

stephybun Mar 15, 2022

We should split create and update into separate methods.

internal/services/iothub/iothub_file_upload_resource.go

+              	future, err := client.CreateOrUpdate(ctx, id.ResourceGroup, id.IotHubName, iothub, "")
+              	if err != nil {
+              		return fmt.Errorf("updating %s: %+v", id, err)

Member

stephybun Mar 15, 2022

Suggested change

      
            		return fmt.Errorf("updating %s: %+v", id, err)
          
            		return fmt.Errorf("updating %s: %+v", *id, err)

internal/services/iothub/iothub_file_upload_resource.go

+              	}
+              	if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {
+              		return fmt.Errorf("waiting for %s to finish updating: %+v", id, err)

Member

stephybun Mar 15, 2022

Suggested change

      
            		return fmt.Errorf("waiting for %s to finish updating: %+v", id, err)
          
            		return fmt.Errorf("waiting for update of %s: %+v", *id, err)

website/docs/r/iothub_file_upload.html.markdown


		* `lock_duration` - (Optional) The lock duration for the file upload notifications queue, specified as an [ISO 8601 timespan duration](https://en.wikipedia.org/wiki/ISO_8601#Durations). This value must be between 5 and 300 seconds, and evaluates to `PT1M` by default.

		* `max_delivery_count` - (Optional) The number of times the IoT hub attempts to deliver a file upload notification message. It evaluates to `10` by default.

Member

stephybun Mar 15, 2022

Suggested change

      
            * `max_delivery_count` - (Optional) The number of times the IoT hub attempts to deliver a file upload notification message. It evaluates to `10` by default.
          
            * `max_delivery_count` - (Optional) The number of times the IoT hub attempts to deliver a file upload notification message. Defaults to `10`.

website/docs/r/iothub_file_upload.html.markdown


		* `max_delivery_count` - (Optional) The number of times the IoT hub attempts to deliver a file upload notification message. It evaluates to `10` by default.

		* `notifications` - (Optional) Used to specify whether file notifications are sent to IoT Hub on upload. It evaluates to false by default.

Member

stephybun Mar 15, 2022

Suggested change

      
            * `notifications` - (Optional) Used to specify whether file notifications are sent to IoT Hub on upload. It evaluates to false by default.
          
            * `notifications` - (Optional) Specifies whether file notifications are sent to IoT Hub on upload. Defaults to `false`.

website/docs/r/iothub_file_upload.html.markdown


		* `notifications` - (Optional) Used to specify whether file notifications are sent to IoT Hub on upload. It evaluates to false by default.

		* `sas_ttl` - (Optional) The period of time for which the SAS URI generated by IoT Hub for file upload is valid, specified as an [ISO 8601 timespan duration](https://en.wikipedia.org/wiki/ISO_8601#Durations). This value must be between 1 minute and 24 hours, and evaluates to `PT1H` by default.

Member

stephybun Mar 15, 2022

Suggested change

      
            * `sas_ttl` - (Optional) The period of time for which the SAS URI generated by IoT Hub for file upload is valid, specified as an [ISO 8601 timespan duration](https://en.wikipedia.org/wiki/ISO_8601#Durations). This value must be between 1 minute and 24 hours, and evaluates to `PT1H` by default.
          
            * `sas_ttl` - (Optional) The period of time for which the SAS URI generated by IoT Hub for file upload is valid, specified as an [ISO 8601 timespan duration](https://en.wikipedia.org/wiki/ISO_8601#Durations). This value must be between 1 minute and 24 hours. Defaults to `PT1H`.

website/docs/r/iothub_file_upload.html.markdown


		In addition to the Arguments listed above - the following Attributes are exported:

		* `id` - The ID of the IotHub File Upload.

Member

stephybun Mar 15, 2022

I've seen multiple spellings of IotHub in this doc (IoT Hub, Iot Hub, IotHub) could we be consistent and update the doc to use just one?

website/docs/r/iothub_file_upload.html.markdown

+              ```shell
+              terraform import azurerm_iothub_file_upload.file_upload1 /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/mygroup1/providers/Microsoft.Devices/IotHubs/hub1/FileUpload/default
+              ```
+              ~> **NOTE:** As there may only be a single fallback route per IoTHub, the id always ends with `/FileUpload/default`.

Member

stephybun Mar 15, 2022

Is this comment relevant?

Member

stephybun commented Mar 17, 2022

After looking into this further it appears that even though the service does allow this functionality once the relevant permissions have been assigned to the System Assigned Identity, the portal recommends against it. Since the IoTHub resource already supports file upload with all the available properties there is no need to split this out into a separate resource.

Whilst I'd like to thank you for your contribution, as this functionality can already be achieved in the recommended manner I am going to close this PR for the moment.

Thanks!

stephybun closed this

This was referenced Mar 18, 2022

r\iothub: Add support for identity-based file_upload #15874

Merged

Fix iothub docs #15876

Merged

myc2h6o deleted the iothub_file_upload branch

March 18, 2022 07:06

Contributor Author

myc2h6o commented Mar 18, 2022 •

edited

Loading

Hi @stephybun thanks for reviewing this big change, there are many useful suggestions! I did some research after looking at your comment and Azure does recommend User-Assigned identity in this case. This is the doc I found.

This pr also includes two new properties under file_upload I added to the iothub_resource itself, to align to the authentication_type and identity_id which we already support for the endpoint property. I've created a new pr #15874 to include only these changes and resolving the comments in this pr as well.
Also, the suggestions you gave also apply to some existing documents, so I opened #15876 to fix those existing ones. Please take a look at these when you got some time.

github-actions bot commented Apr 18, 2022

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation service/iot-hub size/XL