-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_application_load_balancer_subnet_association
#23628
Conversation
ziyeqf
commented
Oct 20, 2023
•
edited
Loading
edited
a6191d2
to
98c786f
Compare
Signed-off-by: ziyeqf <[email protected]>
98c786f
to
31e8553
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍
internal/services/servicenetworking/application_load_balancer_association_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
Id: config.SubnetId, | ||
}, | ||
AssociationType: associationsinterface.AssociationTypeSubnets, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | |
Tags: tags.FromTypedObject(config.Tags), | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cant use it because it returns map[string]*string
, while sdk needs *map[string]string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's two imports for tags
- the legacy one within this provider (./internal/tags
) and the one within go-azure-helpers
(github.com/hashicorp/go-azure-helpers/resourcemanager/tags
) - updating the reference to use go-azure-helpers
should solve that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please follow toms advice here @ziyeqf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm already using "github.com/hashicorp/go-azure-helpers/resourcemanager/tags"
now, is it correct?
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
Signed-off-by: ziyeqf <[email protected]>
1e40f6e
to
5674fea
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
I have left some additional mostly minor comments that once addressed this should be good to merge 👍
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation" | ||
) | ||
|
||
type AssociationResource struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename it something like ApplicationLoadBalancerAssociationResource
or even ApplicationLoadBalancerAssociationSubnetResource
to be more specific?
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
if controller.Model != nil { | ||
association.Location = location.Normalize(controller.Model.Location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the location here required as it typically should be the same as its parent resource? If that is the case, then we can fail early (prior to sending the requset) with telling the user that the required location from the parent model is not retrieved.
internal/services/servicenetworking/application_load_balancer_association_resource_test.go
Outdated
Show resolved
Hide resolved
|
||
* `subnet_id` - (Required) The ID of the subnet which the Application Gateway for Containers associated to. Changing this forces a new resource to be created. | ||
|
||
**Note:** The subnet should be delegated by `Microsoft.ServiceNetworking/trafficControllers` as the example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**Note:** The subnet should be delegated by `Microsoft.ServiceNetworking/trafficControllers` as the example. | |
**Note:** The subnet to be used must be have a delegation for `Microsoft.ServiceNetworking/trafficControllers` as shown in the example above. |
website/docs/r/application_load_balancer_association.html.markdown
Outdated
Show resolved
Hide resolved
Signed-off-by: ziyeqf <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @ziyeqf
Thanks for this PR - I've left some comments inline, but if we can fix those up then we should be able to take another look 👍
Thanks!
"application_load_balancer_id": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: associationsinterface.ValidateTrafficControllerID, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the commonschema
package has a ResourceIDReference
schema property for this purpose:
"application_load_balancer_id": { | |
Type: pluginsdk.TypeString, | |
Required: true, | |
ForceNew: true, | |
ValidateFunc: associationsinterface.ValidateTrafficControllerID, | |
}, | |
"application_load_balancer_id": commonschema.ResourceIDReferenceForceNew(associationsinterface.TrafficControllerId{}), |
"subnet_id": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: commonids.ValidateSubnetID, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here)
"subnet_id": { | |
Type: pluginsdk.TypeString, | |
Required: true, | |
ForceNew: true, | |
ValidateFunc: commonids.ValidateSubnetID, | |
}, | |
"subnet_id": commonschema.ResourceIDReferenceForceNew(commonids.SubnetId{}), |
|
||
type AssociationResource struct{} | ||
|
||
type AssociationModel struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is specific to the Resource, can we make this:
type AssociationModel struct { | |
type AssociationResourceModel struct { |
Id: config.SubnetId, | ||
}, | ||
AssociationType: associationsinterface.AssociationTypeSubnets, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's two imports for tags
- the legacy one within this provider (./internal/tags
) and the one within go-azure-helpers
(github.com/hashicorp/go-azure-helpers/resourcemanager/tags
) - updating the reference to use go-azure-helpers
should solve that?
|
||
controller, err := trafficControllerClient.Get(ctx, *albId) | ||
if err != nil { | ||
return fmt.Errorf("retrieving %s: %+v", *albId, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("retrieving %s: %+v", *albId, err) | |
return fmt.Errorf("retrieving parent %s: %+v", *albId, err) |
if len(config.Tags) > 0 { | ||
association.Tags = &config.Tags | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should always be setting tags
here, else you can't remove them?
return fmt.Errorf("retrieving %s: model was nil", *albId) | ||
} | ||
|
||
association.Location = location.Normalize(controller.Model.Location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be inlined above
|
||
if prop := model.Properties; prop != nil { | ||
if prop.Subnet != nil { | ||
parsedSubnetId, err := commonids.ParseSubnetID(prop.Subnet.Id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When parsing IDs in the Read we need to parse these insensitively:
parsedSubnetId, err := commonids.ParseSubnetID(prop.Subnet.Id) | |
parsedSubnetId, err := commonids.ParseSubnetIDInsensitively(prop.Subnet.Id) |
if response.WasNotFound(resp.HttpResponse) { | ||
return pointer.To(false), nil | ||
} | ||
return nil, fmt.Errorf("while checking existence for %q: %+v", id.String(), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("while checking existence for %q: %+v", id.String(), err) | |
return nil, fmt.Errorf("while checking existence of %s: %+v", *id, err) |
# azurerm_application_load_balancer_association | ||
|
||
Manages an Application Gateway for Containers Association. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is a Subnet association, this resource probably should be _subnet_association
?
Signed-off-by: ziyeqf <[email protected]>
Have updated code except the naming, as the service team is concern about the naming. |
Signed-off-by: ziyeqf <[email protected]>
azurerm_application_load_balancer_association
azurerm_application_load_balancer_subnet_association
resource "azurerm_application_load_balancer_subnet_association" "example" { | ||
name = "example" | ||
application_load_balancer_id = azurerm_application_load_balancer.example.id | ||
subnet_id = azurerm_subnet_example.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subnet_id = azurerm_subnet_example.id | |
subnet_id = azurerm_subnet.example.id |
Signed-off-by: ziyeqf <[email protected]>
kindly ping this pr is ready for another look with the name has been changed |
@@ -0,0 +1,214 @@ | |||
package servicenetworking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This filename needs to be updated to reflect the new name?
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk" | ||
) | ||
|
||
type ApplicationLoadBalancerAssociationResource struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as do all these types?
type ApplicationLoadBalancerAssociationResource struct{} | |
type ApplicationLoadBalancerSubnetAssociationResource struct{} |
} | ||
} | ||
|
||
func (t ApplicationLoadBalancerAssociationResource) Attributes() map[string]*pluginsdk.Schema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
etc
controller, err := trafficControllerClient.Get(ctx, *albId) | ||
if err != nil { | ||
return fmt.Errorf("retrieving %s: %+v", *albId, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ziyeqf could you address this comment?
Id: config.SubnetId, | ||
}, | ||
AssociationType: associationsinterface.AssociationTypeSubnets, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please follow toms advice here @ziyeqf
return fmt.Errorf("parsing id %v", err) | ||
} | ||
|
||
// Thought `AssociationSubnetUpdate` defined in the SDK contains the `subnetId`, while per testing it can not be updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ziyeqf - have you opened an issue yet? could you add it to this comment? and address the question?
return pointer.To(resp.Model != nil), nil | ||
} | ||
|
||
func TestAccApplicationLoadBalancerAssociation_basic(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test names nee to be updated too?
@@ -0,0 +1,14 @@ | |||
package validate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this file name?
@@ -0,0 +1,94 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this file name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @ziyeqf.
Just one more thing to address and I think this will be good to merge.
Additionally, there's a problem in the requiresImport
test for the parent resource, I think there's an extra provider
block in the import
config func that needs to be removed if you can take a look at that also?
Thanks!
Edit: Apologies, apparently I already had another partial review that has already been addressed, just the validate section I think to still look at.
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/servicenetworking/application_load_balancer_association_resource_test.go
Outdated
Show resolved
Hide resolved
website/docs/r/application_load_balancer_association.html.markdown
Outdated
Show resolved
Hide resolved
) | ||
|
||
func ApplicationLoadBalancerSubnetAssociationName() pluginsdk.SchemaValidateFunc { | ||
return validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,62}[a-zA-Z0-9]$`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex is slightly incorrect for the description below, it won't allow a name of 1 character. Can we split this out into separate checks for the limitations to make it easier to maintain and report the correct violation to the users?
kindly ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @ziyeqf, this LGTM now 👍
<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>"hashicorp/azurerm" updated from "3.81.0" to "3.82.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.82.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.82.0
FEATURES:

* New Data Source: `azurerm_monitor_workspace` ([#23928](hashicorp/terraform-provider-azurerm#23928 New Resource: `azurerm_application_load_balancer_subnet_association` ([#23628](https://github.com/hashicorp/terraform-provider-azurerm/issues/23628))

ENHANCEMENTS:

* dependencies: updating to `v0.20231117.1130141` of `github.com/hashicorp/go-azure-sdk` ([#23945](hashicorp/terraform-provider-azurerm#23945 `azurestackhci`: updating to API Version `2023-08-01` ([#23939](hashicorp/terraform-provider-azurerm#23939 `dashboard`: updating to API Version `2023-09-01` ([#23929](hashicorp/terraform-provider-azurerm#23929 `hpccache`: updating to API version `2023-05-01` ([#24005](hashicorp/terraform-provider-azurerm#24005 `mssql`: updating resources using `hashicorp/go-azure-sdk` to API Version `2023-02-01-preview` ([#23721](hashicorp/terraform-provider-azurerm#23721 `templatespecversions`: updating to API Version `2022-02-01` ([#24007](hashicorp/terraform-provider-azurerm#24007 Data Source: `azurerm_template_spec_version` - refactoring to use `hashicorp/go-azure-sdk` ([#24007](hashicorp/terraform-provider-azurerm#24007 `azurerm_cosmosdb_postgresql_cluster` - `coordinator_storage_quota_in_mb` and `coordinator_vcore_count` are no longer required for read replicas ([#23928](hashicorp/terraform-provider-azurerm#23928 `azurerm_dashboard_grafana` - `sku` can now be set to `Essential` ([#23934](hashicorp/terraform-provider-azurerm#23934 `azurerm_gallery_application_version` - add support for the `config_file`, `package_file` and `target_region.exclude_from_latest` properties ([#23816](hashicorp/terraform-provider-azurerm#23816 `azurerm_hdinsight_hadoop_cluster` - `script_actions` is no longer Force New ([#23888](hashicorp/terraform-provider-azurerm#23888 `azurerm_hdinsight_hbase_cluster` - `script_actions` is no longer Force New ([#23888](hashicorp/terraform-provider-azurerm#23888 `azurerm_hdinsight_interactive_query_cluster` - `script_actions` is no longer Force New ([#23888](hashicorp/terraform-provider-azurerm#23888 `azurerm_hdinsight_kafka_cluster` - `script_actions` is no longer Force New ([#23888](hashicorp/terraform-provider-azurerm#23888 `azurerm_hdinsight_spark_cluster` - `script_actions` is no longer Force New ([#23888](hashicorp/terraform-provider-azurerm#23888 `azurerm_kubernetes_cluster` - add support for the `gpu_instance` property ([#23887](hashicorp/terraform-provider-azurerm#23887 `azurerm_kubernetes_cluster_node_pool` - add support for the `gpu_instance` property ([#23887](hashicorp/terraform-provider-azurerm#23887 `azurerm_log_analytics_workspace` - add support for the `identity` property ([#23864](hashicorp/terraform-provider-azurerm#23864 `azurerm_linux_function_app` - add support for dotnet 8 ([#23638](hashicorp/terraform-provider-azurerm#23638 `azurerm_linux_function_app_slot` - add support for dotnet 8 ([#23638](hashicorp/terraform-provider-azurerm#23638 `azurerm_managed_lustre_file_system` - export attribute `mgs_address` ([#23942](hashicorp/terraform-provider-azurerm#23942 `azurerm_mssql_database` - support for Hyperscale SKUs ([#23974](hashicorp/terraform-provider-azurerm#23974 `azurerm_mssql_database` - refactoring to use `hashicorp/go-azure-sdk` ([#23721](hashicorp/terraform-provider-azurerm#23721 `azurerm_mssql_server` - refactoring to use `hashicorp/go-azure-sdk` ([#23721](hashicorp/terraform-provider-azurerm#23721 `azurerm_shared_image` - add support for `trusted_launch_supported` ([#23781](hashicorp/terraform-provider-azurerm#23781 `azurerm_spring_cloud_container_deployment` - add support for the `application_performance_monitoring_ids` property ([#23862](hashicorp/terraform-provider-azurerm#23862 `azurerm_spring_cloud_customized_accelerator` - add support for the `accelerator_type` and `path` properties ([#23797](hashicorp/terraform-provider-azurerm#23797 `azurerm_point_to_site_vpn_gateway` - allow multiple `connection_configurations` blocks ([#23936](hashicorp/terraform-provider-azurerm#23936 `azurerm_private_dns_cname_record` - `ttl` can now be set to 0 ([#23918](hashicorp/terraform-provider-azurerm#23918 `azurerm_windows_function_app` - add support for dotnet 8 ([#23638](hashicorp/terraform-provider-azurerm#23638 `azurerm_windows_function_app_slot` - add support for dotnet 8 ([#23638](https://github.com/hashicorp/terraform-provider-azurerm/issues/23638))

BUG FIXES:
* `azurerm_api_management` - correct a bug with additional location zones within the `additional_location` block ([#23943](hashicorp/terraform-provider-azurerm#23943 `azurerm_dev_test_linux_virtual_machine` - `storage_type` is now ForceNew to match the updated API behaviour ([#23973](hashicorp/terraform-provider-azurerm#23973 `azurerm_dev_test_windows_virtual_machine` - `storage_type` is now ForceNew to match the updated API behaviour ([#23973](hashicorp/terraform-provider-azurerm#23973 `azurerm_disk_encryption_set` - resource will recreate if `identity` changes from `SystemAssigned` to `UserAssigned` ([#23904](hashicorp/terraform-provider-azurerm#23904 `azurerm_eventhub_cluster`: `sku_name` is no longer ForceNew ([#24009](hashicorp/terraform-provider-azurerm#24009 `azurerm_firewall` - recasing the value for `firewall_policy_id` to workaround the API returning the incorrect casing ([#23993](hashicorp/terraform-provider-azurerm#23993 `azurerm_security_center_subscription_pricing` - fix a bug preventing removal of `extensions` and downgrading `tier` to `Free` ([#23821](hashicorp/terraform-provider-azurerm#23821 `azurerm_windows_web_app` - fix an issue of incorrect application stack settings during update ([#23372](https://github.com/hashicorp/terraform-provider-azurerm/issues/23372))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/896/">Jenkins pipeline link</a> </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) <[email protected]>
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. |