Skip to content
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.

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

New Resource : azurerm_arc_resource_bridge_appliance #23108

Merged
merged 7 commits into from
Sep 19, 2023

Conversation

sinbai
Copy link
Contributor

@sinbai sinbai commented Aug 29, 2023

Swagger: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/resourceconnector/resource-manager/Microsoft.ResourceConnector/stable/2022-10-27/appliances.json

Test results:

PASS: TestAccArcResourceBridgeAppliance_basic (221.73s)
PASS: TestAccArcResourceBridgeAppliance_update (343.70s)
PASS: TestAccArcResourceBridgeAppliance_complete (243.17s)
PASS: TestAccArcResourceBridgeAppliance_requiresImport (218.86s)

Note: Need to run az provider register --namespace Microsoft.ResourceConnector to register provider before running tests.

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @sinbai

Thanks for this PR - I've taken a look through and left some comments inline, if those can be addressed then we should be able to take another look here.

It's worth noting that when a field has to be set via the Update method, that we can first Create the resource and then use the Update API to update this value - as such we don't need to ask users to conditionally apply their Terraform Configuration in two stages.

It's also worth noting there's conflation between the Appliance and the Resource Connector Appliance in the documentation, presumably the Appliance has to exist within AKS/VMware etc first? If so, then we're also missing test cases showing this working end-to-end - could we clarify the behaviour of this service to make it fully usable here?

Thanks!

Comment on lines 146 to 151
identity, err := identity.ExpandSystemAssignedFromModel(model.Identity)
if err != nil {
return fmt.Errorf("expanding SystemAssigned Identity: %+v", err)
}

parameters.Identity = identity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw we can move this above the parameters definition and then define this inline, we don't gain anything from doing this separately:

Suggested change
identity, err := identity.ExpandSystemAssignedFromModel(model.Identity)
if err != nil {
return fmt.Errorf("expanding SystemAssigned Identity: %+v", err)
}
parameters.Identity = identity
identity, err := identity.ExpandSystemAssignedFromModel(model.Identity)
if err != nil {
return err
}
parameters.Identity = identity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


* `version` - (Optional) The version of the Appliance. Possible values is `latest`.

-> **NOTE:** The `version` cannot be populated apart from upgrade call.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as above) this can be removed / is something we should handle internally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After confirming with service team, the version is not a user settable property, I have removed it.

Appliance can be imported using the `resource id`, e.g.

```shell
terraform import azurerm_resource_connector_appliance.example /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/providers/Microsoft.ResourceConnector/apliances/appliancesExample
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ID isn't correct?

/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ResourceConnector/appliances/{applianceName}

Note apliances vs appliances

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appliances, fixed.

Comment on lines 81 to 84
* `create` - (Defaults to 60 minutes) Used when creating the Appliance.
* `read` - (Defaults to 5 minutes) Used when retrieving the Appliance.
* `update` - (Defaults to 30 minutes) Used when updating the Appliance.
* `delete` - (Defaults to 30 minutes) Used when deleting the Appliance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Appliance, or Resource Connector Appliance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource Connector Appliance, fixed.


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

* `id` - The ID of the Appliance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Appliance, or Resource Connector Appliance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource Connector Appliance, fixed.


* `tags` - (Optional) A mapping of tags which should be assigned to the Appliance.

* `version` - (Optional) The version of the Appliance. Possible values is `latest`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Appliance, or Resource Connector Appliance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource Connector Appliance, fixed.

@sinbai
Copy link
Contributor Author

sinbai commented Sep 12, 2023

hi @sinbai

Thanks for this PR - I've taken a look through and left some comments inline, if those can be addressed then we should be able to take another look here.

It's worth noting that when a field has to be set via the Update method, that we can first Create the resource and then use the Update API to update this value - as such we don't need to ask users to conditionally apply their Terraform Configuration in two stages.

It's also worth noting there's conflation between the Appliance and the Resource Connector Appliance in the documentation, presumably the Appliance has to exist within AKS/VMware etc first? If so, then we're also missing test cases showing this working end-to-end - could we clarify the behaviour of this service to make it fully usable here?

Thanks!

@tombuildsstuff thanks for your detailed feedback. I have fixed the inline feedback. Could you please take another look?

For above test cases showing this working end-to-end, the following is the feedback I got from the service team.

Appliance RP is a hybrid resource provider service. We have a few operations that deploy a cluster on prem for customers like Validate, Prepare, Deploy are run on prem only and can be done via Arc Appliance CLI as this requires specific parameters depending on the on prem stack being used to deploy the cluster on. 

We do create an Azure resource to help link the on prem resource to Azure, and that is done using  RP operations (that we support using various SDKs) like Create, Update, Get, List, etc, for which we are also looking to support Terraform as the RP is GAing.

Link to public doc - Azure Arc resource bridge (preview) overview - Azure Arc | Microsoft Learn

Per above feedback, I assume that we could not add end-to-end testing in TF, what do you think?

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes @sinbai.

There are a few things that still need to be fixed.

Also since this is from the Azure Arc product line this resource should be renamed to azurerm_arc_bridge_appliance/azurerm_arc_bridge_connector_appliance or something of the sort? This is because we should go with names that are familiar to the users, in this case since this is being marketed as Arc Bridge. Also existing arc resources within the provider like the Arc Kubernetes cluster or Arc Virtual Machine are also named as such.

Identity []identity.ModelSystemAssigned `tfschema:"identity"`
Provider appliances.Provider `tfschema:"infrastructure_provider"`
PublicKey string `tfschema:"public_key"`
Tags map[string]string `tfschema:"tags"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Tags map[string]string `tfschema:"tags"`
Tags map[string]interface{} `tfschema:"tags"`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Provider: pointer.To(model.Provider),
},
},
Tags: pointer.To(model.Tags),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Tags: pointer.To(model.Tags),
Tags: tags.Expand(model.Tags),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}

if metadata.ResourceData.HasChange("tags") {
parameters.Tags = pointer.To(model.Tags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parameters.Tags = pointer.To(model.Tags)
parameters.Tags = tags.Expand(model.Tags)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

if model := resp.Model; model != nil {
state.Location = location.Normalize(model.Location)
state.Identity = identity.FlattenSystemAssignedToModel(model.Identity)
state.Tags = pointer.From(resp.Model.Tags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
state.Tags = pointer.From(resp.Model.Tags)
state.Tags = tags.Flatten(model.Tags)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


* `infrastructure_provider`- (Required) The infrastructure provider about the connected Resource Connector Appliance. Possible values are `HCI`,`SCVMM` and `VMWare`. Changing this forces a new resource to be created.

* `public_key`- (Optional) The `public_key` is an RSA public key in PKCS1 format encoded in base64. Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the previous comment on this is expecting the property to be renamed to public_key_base64 like is specified in the naming guidelines of the contributor docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation, fixed:)

@sinbai sinbai changed the title New Resource : azurerm_resource_connector_appliance New Resource : azurerm_arc_resource_bridge_appliance Sep 19, 2023
@sinbai
Copy link
Contributor Author

sinbai commented Sep 19, 2023

Thanks for making those changes @sinbai.

There are a few things that still need to be fixed.

Also since this is from the Azure Arc product line this resource should be renamed to azurerm_arc_bridge_appliance/azurerm_arc_bridge_connector_appliance or something of the sort? This is because we should go with names that are familiar to the users, in this case since this is being marketed as Arc Bridge. Also existing arc resources within the provider like the Arc Kubernetes cluster or Arc Virtual Machine are also named as such.

@stephybun thanks for you detailed feedback. I updated the resource name to azurerm_arc_resource_bridge_appliance based on What is Azure Arc resource bridge (preview), WDYT? Also, I updated the other comments, could you please take another look?

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sinbai LGTM 🍰

@stephybun stephybun merged commit 1063c0f into hashicorp:main Sep 19, 2023
29 checks passed
@github-actions github-actions bot added this to the v3.74.0 milestone Sep 19, 2023
stephybun added a commit that referenced this pull request Sep 20, 2023
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Sep 23, 2023
<Actions>
<action
id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.73.0&#34; to
&#34;3.74.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.74.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.74.0&#xA;NOTES:&#xA;&#xA;*
`azurerm_synapse_sql_pool` - users that have imported
`azurerm_synapse_sql_pool` resources that were created outside of
Terraform using an `LRS` storage account type will need to use
`ignore_changes` to avoid the resource from being destroyed and
recreated.&#xA;&#xA;FEATURES:&#xA;&#xA;* **New Resource**:
`azurerm_arc_resource_bridge_appliance`
([#23108](https://github.com/hashicorp/terraform-provider-azurerm/issues/23108))&#xA;*
**New Resource**: `azurerm_data_factory_dataset_azure_sql_table`
([#23264](https://github.com/hashicorp/terraform-provider-azurerm/issues/23264))&#xA;*
**New Resource**: `azurerm_function_app_connection`
([#23127](https://github.com/hashicorp/terraform-provider-azurerm/issues/23127))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.20230918.1115907` of
`github.com/hashicorp/go-azure-sdk`
([#23337](https://github.com/hashicorp/terraform-provider-azurerm/issues/23337))&#xA;*
dependencies: downgrading to `v1.12.5` of `github.com/rickb777/date`
([#23296](https://github.com/hashicorp/terraform-provider-azurerm/issues/23296))&#xA;*
`mysql`: updating to use API Version `2022-01-01`
([#23320](https://github.com/hashicorp/terraform-provider-azurerm/issues/23320))&#xA;*
`azurerm_app_configuration` - support for the `replica` block
([#22452](https://github.com/hashicorp/terraform-provider-azurerm/issues/22452))&#xA;*
`azurerm_bot_channel_directline` - support for `user_upload_enabled`,
`endpoint_parameters_enabled`, and `storage_enabled`
([#23149](https://github.com/hashicorp/terraform-provider-azurerm/issues/23149))&#xA;*
`azurerm_container_app` - support for scale rules
([#23294](https://github.com/hashicorp/terraform-provider-azurerm/issues/23294))&#xA;*
`azurerm_container_app_environment` - support for zone redundancy
([#23313](https://github.com/hashicorp/terraform-provider-azurerm/issues/23313))&#xA;*
`azurerm_container_group` - support for the `key_vault_user_identity_id`
property for Customer Managed Keys
([#23332](https://github.com/hashicorp/terraform-provider-azurerm/issues/23332))&#xA;*
`azurerm_cosmosdb_account` - support for MongoDB connection strings
([#23331](https://github.com/hashicorp/terraform-provider-azurerm/issues/23331))&#xA;*
`azurerm_data_factory_dataset_delimited_text` - support for the
`dynamic_file_system_enabled`, `dynamic_path_enabled`, and
`dynamic_filename_enabled` properties
([#23261](https://github.com/hashicorp/terraform-provider-azurerm/issues/23261))&#xA;*
`azurerm_data_factory_dataset_parquet` - support for the
`azure_blob_fs_location` block
([#23261](https://github.com/hashicorp/terraform-provider-azurerm/issues/23261))&#xA;*
`azurerm_monitor_diagnostic_setting` - validation to ensure either
`category` or `category_group` are supplied in `enabled_log` and `log`
blocks
([#23308](https://github.com/hashicorp/terraform-provider-azurerm/issues/23308))&#xA;*
`azurerm_network_interface` - support for the `auxiliary_mode` and
`auxiliary_sku` properties
([#22979](https://github.com/hashicorp/terraform-provider-azurerm/issues/22979))&#xA;*
`azurerm_postgresql_flexible_server` - increased the maximum supported
value for `storage_mb`
([#23277](https://github.com/hashicorp/terraform-provider-azurerm/issues/23277))&#xA;*
`azurerm_shared_image_version` - support for the
`replicated_region_deletion_enabled` and
`target_region.exclude_from_latest_enabled` properties
([#23147](https://github.com/hashicorp/terraform-provider-azurerm/issues/23147))&#xA;*
`azurerm_storage_account` - support for setting `domain_name` and
`domain_guid` for `AADKERB`
([#22833](https://github.com/hashicorp/terraform-provider-azurerm/issues/22833))&#xA;*
`azurerm_storage_account_customer_managed_key` - support for
cross-tenant customer-managed keys with the
`federated_identity_client_id`, and `key_vault_uri` properties
([#20356](https://github.com/hashicorp/terraform-provider-azurerm/issues/20356))&#xA;*
`azurerm_web_application_firewall_policy` - support for the
`rate_limit_duration`, `rate_limit_threshold`, `group_rate_limit_by`,
and `request_body_inspect_limit_in_kb` properties
([#23239](https://github.com/hashicorp/terraform-provider-azurerm/issues/23239))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_container_app_environment`: fix
`log_analytics_workspace_name` output to correct value
([#23298](https://github.com/hashicorp/terraform-provider-azurerm/issues/23298))&#xA;*
`azurerm_api_management_api` - set the `service_url` property when
importing the resource
([#23011](https://github.com/hashicorp/terraform-provider-azurerm/issues/23011))&#xA;*
`azurerm_app_configuration` - prevent crash by nil checking the
encryption configuration
([#23302](https://github.com/hashicorp/terraform-provider-azurerm/issues/23302))&#xA;*
`azurerm_app_configuration_feature` - update `percentage_filter_value`
to accept correct type of float
([#23263](https://github.com/hashicorp/terraform-provider-azurerm/issues/23263))&#xA;*
`azurerm_container_app` - fix an issue with `commands` and `args` being
overwritten when using multiple containers
([#23338](https://github.com/hashicorp/terraform-provider-azurerm/issues/23338))&#xA;*
`azurerm_key_vault_certificate` - fix issue where certificates
couldn&#39;t be recovered anymore
([#23204](https://github.com/hashicorp/terraform-provider-azurerm/issues/23204))&#xA;*
`azurerm_key_vault_key` - the ForceNew when `expiration_date` is removed
from the config file
([#23327](https://github.com/hashicorp/terraform-provider-azurerm/issues/23327))&#xA;*
`azurerm_linux_function_app` - fix a bug in setting the storage settings
when using Elastic Premium plans
([#21212](https://github.com/hashicorp/terraform-provider-azurerm/issues/21212))&#xA;*
`azurerm_linux_web_app` - fix docker app stack update
([#23303](https://github.com/hashicorp/terraform-provider-azurerm/issues/23303))&#xA;*
`azurerm_linux_web_app` - fix crash in auto heal expansion
([#21328](https://github.com/hashicorp/terraform-provider-azurerm/issues/21328))&#xA;*
`azurerm_linux_web_app_slot` - fix docker app stack update
([#23303](https://github.com/hashicorp/terraform-provider-azurerm/issues/23303))&#xA;*
`azurerm_linux_web_app_slot` - fix crash in auto heal expansion
([#21328](https://github.com/hashicorp/terraform-provider-azurerm/issues/21328))&#xA;*
`azurerm_log_analytics_solution` - fix bug where the resource wasn&#39;t
handling successful creation on subsequent applies
([#23312](https://github.com/hashicorp/terraform-provider-azurerm/issues/23312))&#xA;*
`azurerm_management_group_subscription_association` - fix bug to
correctly mark resource as gone if not found during read
([#23335](https://github.com/hashicorp/terraform-provider-azurerm/issues/23335))&#xA;*
`azurerm_mssql_elasticpool` - remove check that prevents `license_type`
from being set for certain skus
([#23262](https://github.com/hashicorp/terraform-provider-azurerm/issues/23262))&#xA;*
`azurerm_servicebus_queue` - fixing an issue where `auto_delete_on_idle`
couldn&#39;t be set to `P10675199DT2H48M5.4775807S`
([#23296](https://github.com/hashicorp/terraform-provider-azurerm/issues/23296))&#xA;*
`azurerm_servicebus_topic` - fixing an issue where `auto_delete_on_idle`
couldn&#39;t be set to `P10675199DT2H48M5.4775807S`
([#23296](https://github.com/hashicorp/terraform-provider-azurerm/issues/23296))&#xA;*
`azurerm_storage_account` - prevent sending unsupported blob properties
in payload for `Storage` account kind
([#23288](https://github.com/hashicorp/terraform-provider-azurerm/issues/23288))&#xA;*
`azurerm_synapse_sql_pool` - expose `storage_account_type`
([#23217](https://github.com/hashicorp/terraform-provider-azurerm/issues/23217))&#xA;*
`azurerm_windows_function_app` - fix a bug in setting the storage
settings when using Elastic Premium plans
([#21212](https://github.com/hashicorp/terraform-provider-azurerm/issues/21212))&#xA;*
`azurerm_windows_web_app` - fix docker app stack update
([#23303](https://github.com/hashicorp/terraform-provider-azurerm/issues/23303))&#xA;*
`azurerm_windows_web_app_slot` - fix docker app stack update
([#23303](https://github.com/hashicorp/terraform-provider-azurerm/issues/23303))&#xA;&#xA;DEPRECATIONS:&#xA;&#xA;*
`azurerm_application_gateway` - deprecate `Standard` and `WAF` skus
([#23310](https://github.com/hashicorp/terraform-provider-azurerm/issues/23310))&#xA;*
`azurerm_bot_channel_web_chat` - deprecate `site_names` in favour of
`site` block
([#23161](https://github.com/hashicorp/terraform-provider-azurerm/issues/23161))&#xA;*
`azurerm_monitor_diagnostic_setting` - deprecate `retention_policy` in
favour of `azurerm_storage_management_policy`
([#23260](https://github.com/hashicorp/terraform-provider-azurerm/issues/23260))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
@sinbai sinbai deleted the ResourceConnector/new_resource branch March 28, 2024 02:47
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants