-
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
azurerm_container_app_environment
: Add support for workload_profile
#23478
Conversation
jiaweitao001
commented
Oct 8, 2023
•
edited
Loading
edited
- All related test passed.
Looking forward for the workload profiles support, thanks! Is this a fix for azurerm_container_app_environment only? If yes, will there also be an accompanying PR to add the support to the azurerm_container_app resource, so we can deploy apps into the workload profile? |
Hi @pietersap , there will be a separate PR for supporting workload profile in azurerm_container_app resource. |
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 @jiaweitao001
Thanks for this PR - I've left a comment inline, but by ignoring the presence of resources within the Resource Group, it appears that there's an issue that's not being surfaced?
Thanks!
resource_group { | ||
prevent_deletion_if_contains_resources = false | ||
} |
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 indicates a bug in the Terraform Resource or Configuration being used, so this should (almost) never be used in test cases - why's this needed?
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.
Hi @tombuildsstuff , when creating this container app environment resource, a NSG will be automatically created along with it. We won't be able to delete the RG without this line since there will be a hanging NSG.
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.
In which case, the API should be clearing that up during the deletion of the Container App Environment, since the API is creating it the API manages it's lifecycle (as other Resource Providers do) - would you mind opening an issue on Azure/azure-rest-api-specs
/ reaching out to the Service Team here so we can get this fixed?
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 NSG was created to comply an Azure policy internally. It should not affect external users. Will remove this line.
* `maximum_count` - (Optional) The maximum number of containers that can be deployed in the Container App Environment. | ||
|
||
* `minimum_count` - (Optional) The minimum number of containers that can be deployed in the Container App Environment. |
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.
would this be more clear/explicit if
* `maximum_count` - (Optional) The maximum number of containers that can be deployed in the Container App Environment. | |
* `minimum_count` - (Optional) The minimum number of containers that can be deployed in the Container App Environment. | |
* `maximum_container_count` - (Optional) The maximum number of containers that can be deployed in the Container App Environment. | |
* `minimum_container_count` - (Optional) The minimum number of containers that can be deployed in the Container App Environment. |
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.
Sure. Will rename.
@katbyte @tombuildsstuff Can this be moved forward? Review-comments seem to have been met. |
Indeed, would be much appreciated! Been eagerly waiting for this to finally migrate back from AzApi resources. |
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.
Hi @jiaweitao001 - Thanks for this PR. I think it needs a little more design consideration and possibly some more information from the Service Team. The workload list appears to always contain a minimum of one workload specification (the Consumption
item, added by default on creation) and can take multiple more of Dedicated
types, specified by their SKUs in the property workloadProfileType
. The docs suggest it's not possible to add additional Consumption
profiles, so I think this needs to be reworked for the default Consumption
profile and additional Dedicated
Profiles? WDYT?
MaximumCount int64 `tfschema:"maximum_count"` | ||
MinimumCount int64 `tfschema:"minimum_count"` |
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 set these to int
, we need to maintain 32bit compatibility.
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.
Changed.
@@ -17,6 +17,8 @@ import ( | |||
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation" | |||
) | |||
|
|||
const consumption = "Consumption" |
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 feels like there are missing Enums in the Spec, could you check with the service team?
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.
According to the service team, this Consumption
is the only possible value for Consumption
block.
@@ -17,6 +17,8 @@ import ( | |||
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation" | |||
) | |||
|
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 have this new code in a new file? container_apps.go
is already of significant siza and relates to the azurerm_container_app
, so perhaps container_app_environment.go
would be an appropriate separation?
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.
Sure. Reorganized this part of code.
"name": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ValidateFunc: validation.StringIsNotEmpty, | ||
}, | ||
|
||
"workload_profile_type": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ValidateFunc: validation.StringIsNotEmpty, | ||
}, |
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 have specific validation here? These I believe need to be specific values and casing based on the values that the API will accept (i.e. the allowed SKU names)
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.
According to the service team, workload_profile_type
does have a limited selection of SKUs, will specify the validation.
Is there anything to add here or is this complete to go? |
GH actions are only part of the process, the Acceptance Tests for this PR are currently queued on our CI server, assuming there's no issues highlighted there then I think this should be good to go. |
@jiaweitao001 Any reason why 'Consumption' is not added as a workload profile option? Can we use this on the azure_container_app resource? |
If you read the discussion above, 'Consumption' is a default option and cannot be removed from the options and the focus is on adding a dedicated skus. However I agree that we should be able to enable the workload profile without having to add dedicated skus. Above setup can be done in the portal. I am thinking along the line of creating a flag that says |
Agreed. Container App Environments run in a Consumption plan or in Workload Profile plan. Documentation of the workload profiles mentioning the Consumption profile type/category Within AZ CLI you simply add the parameter |
By allowing the "Consumption" option, you could instruct TF to just enable workload profiles, but without a dedicated compute. |
I did some testing. When you deploy the following:
You get this: Just delete the dedicated and you're good to go. |
I had expected support for this:
|
Great to see this feature added :) |
<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.82.0" to "3.83.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.83.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.83.0
UPGRADE NOTES

* Key Vaults are now loaded using [the `ListBySubscription` API within the Key Vault Resource Provider](https://learn.microsoft.com/en-us/rest/api/keyvault/keyvault/vaults/list-by-subscription?view=rest-keyvault-keyvault-2022-07-01&tabs=HTTP) rather than [the Resources API](https://learn.microsoft.com/en-us/rest/api/keyvault/keyvault/vaults/list?view=rest-keyvault-keyvault-2022-07-01&tabs=HTTP). This change means that the Provider now caches the list of Key Vaults available within a Subscription, rather than loading these piecemeal to workaround stale data returned from the Resources API ([#24019](https://github.com/hashicorp/terraform-provider-azurerm/issues/24019))

FEATURES:

* New Data Source: `azurerm_stack_hci_cluster` ([#24032](https://github.com/hashicorp/terraform-provider-azurerm/issues/24032))

ENHANCEMENTS:

* dependencies: updating to `v0.20231129.1103252` of `github.com/hashicorp/go-azure-sdk` ([#24063](hashicorp/terraform-provider-azurerm#24063 `automation`: updating to API Version `2023-11-01` ([#24017](hashicorp/terraform-provider-azurerm#24017 `keyvault`: the cache is now populated using the `ListBySubscription` endpoint on the KeyVault Resource Provider rather than via the `Resources` API ([#24019](hashicorp/terraform-provider-azurerm#24019 `keyvault`: updating the cache to populate all Key Vaults available within the Subscription to reduce the number of API calls ([#24019](hashicorp/terraform-provider-azurerm#24019 Data Source `azurerm_private_dns_zone`: refactoring to use the `ListBySubscription` API rather than the Resources API when `resource_group_name` is omitted ([#24024](hashicorp/terraform-provider-azurerm#24024 `azurerm_dashboard_grafana` - support for `grafana_major_version` ([#24014](hashicorp/terraform-provider-azurerm#24014 `azurerm_linux_web_app` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_linux_web_app_slot` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_media_transform` - deprecate `face_detector_preset` and `video_analyzer_preset` ([#24002](hashicorp/terraform-provider-azurerm#24002 `azurerm_postgresql_database` - update the validation of `collation` to include `Norwegian_Norway.1252` ([#24070](hashicorp/terraform-provider-azurerm#24070 `azurerm_postgresql_flexible_server` - updating to API Version `2023-06-01-preview` ([#24016](hashicorp/terraform-provider-azurerm#24016 `azurerm_redis_cache` - support for the `active_directory_authentication_enabled` property ([#23976](hashicorp/terraform-provider-azurerm#23976 `azurerm_windows_web_app` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_windows_web_app_slot` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_storage_account` - add `name` validation in custom diff ([#23799](https://github.com/hashicorp/terraform-provider-azurerm/issues/23799))

BUG FIXES:

* authentication: fix a bug where auxiliary tenants were not correctly authorized ([#24063](hashicorp/terraform-provider-azurerm#24063 `azurerm_app_configuration` - normalize location in `replica` block ([#24074](hashicorp/terraform-provider-azurerm#24074 `azurerm_cosmosdb_account` - cosmosdb version and capabilities can now be updated at the same time ([#24029](hashicorp/terraform-provider-azurerm#24029 `azurerm_data_factory_flowlet_data_flow` - `source` and `sink` properties are now optional ([#23987](hashicorp/terraform-provider-azurerm#23987 `azurerm_datadog_monitor_tag_rule` - correctly handle default rule ([#22806](hashicorp/terraform-provider-azurerm#22806 `azurerm_ip_group`: fixing a crash when `firewall_ids` and `firewall_policy_ids` weren't parsed correctly from the API Response ([#24031](hashicorp/terraform-provider-azurerm#24031 `azurerm_nginx_deployment` - add default value of `20` for `capacity` ([#24033](https://github.com/hashicorp/terraform-provider-azurerm/issues/24033))


</pre> </details> <details> <summary>3.84.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.84.0
FEATURES:

* **New Data Source:** `azurerm_storage_containers` ([#24061](hashicorp/terraform-provider-azurerm#24061 **New Resource:** `azurerm_elastic_san` ([#23619](hashicorp/terraform-provider-azurerm#23619 **New Resource:** `azurerm_key_vault_managed_hardware_security_module_role_assignment` ([#22332](hashicorp/terraform-provider-azurerm#22332 **New Resource:** `azurerm_key_vault_managed_hardware_security_module_role_definition` ([#22332](https://github.com/hashicorp/terraform-provider-azurerm/issues/22332))

ENHANCEMENTS:

* dependencies: updating mssql elasticpools from `v5.0` to `2023-05-01-preview`
* dependencies: updating to `v0.20231207.1122031` of `github.com/hashicorp/go-azure-sdk` ([#24149](hashicorp/terraform-provider-azurerm#24149 Data Source: `azurerm_storage_account` - export the primary and secondary internet and microsoft hostnames for blobs, dfs, files, queues, tables and web ([#23517](hashicorp/terraform-provider-azurerm#23517 Data Source: `azurerm_cosmosdb_account` - export the `connection_strings`, `primary_sql_connection_string`, `secondary_sql_connection_string`, `primary_readonly_sql_connection_string`, `secondary_readonly_sql_connection_string`, `primary_mongodb_connection_string`, `secondary_mongodb_connection_string`, `primary_readonly_mongodb_connection_string`, and `secondary_readonly_mongodb_connection_string` attributes ([#24129](hashicorp/terraform-provider-azurerm#24129 `azurerm_bot_service_azure_bot` - support for the `public_network_access_enabled` property ([#24125](hashicorp/terraform-provider-azurerm#24125 `azurerm_container_app_environment` - support for the `workload_profile` property ([#23478](hashicorp/terraform-provider-azurerm#23478 `azurerm_cosmosdb_cassandra_datacenter` - support for the `seed_node_ip_addresses` property ([#24076](hashicorp/terraform-provider-azurerm#24076 `azurerm_firewall` - support for the `dns_proxy_enabled` property ([#20519](hashicorp/terraform-provider-azurerm#20519 `azurerm_kubernetes_cluster` - support for the `support_plan` property and the `sku_tier` `Premium` ([#23970](hashicorp/terraform-provider-azurerm#23970 `azurerm_mssql_database` - support for `enclave_type` field ([#24054](hashicorp/terraform-provider-azurerm#24054 `azurerm_mssql_elasticpool` - support for `enclave_type` field ([#24054](hashicorp/terraform-provider-azurerm#24054 `azurerm_mssql_managed_instance` - support for more `vcores`: `6`, `10`, `12`, `20`, `48`, `56`, `96`, `128` ([#24085](hashicorp/terraform-provider-azurerm#24085 `azurerm_redis_linked_server` - support for the property `geo_replicated_primary_host_name` ([#23984](hashicorp/terraform-provider-azurerm#23984 `azurerm_storage_account` - expose the primary and secondary internet and microsoft hostnames for blobs, dfs, files, queues, tables and web ([#23517](hashicorp/terraform-provider-azurerm#23517 `azurerm_synapse_role_assignment` - support for the `principal_type` property ([#24089](hashicorp/terraform-provider-azurerm#24089 `azurerm_spring_cloud_build_deployment` - support for the `application_performance_monitoring_ids` property ([#23969](hashicorp/terraform-provider-azurerm#23969 `azurerm_virtual_network_gateway` - support for the `bgp_route_translation_for_nat_enabled`, `dns_forwarding_enabled`, `ip_sec_replay_protection_enabled`, `remote_vnet_traffic_enabled`, `virtual_wan_traffic_enabled`, `radius_server`, `virtual_network_gateway_client_connection`, `policy_group`, and `ipsec_policy` property ([#23220](https://github.com/hashicorp/terraform-provider-azurerm/issues/23220))

BUG FIXES:

* `azurerm_application_insights_api_key` - prevent a bug where multiple keys couldn't be created for an Application Insights instance ([#23463](hashicorp/terraform-provider-azurerm#23463 `azurerm_container_registry` - the `network_rule_set.virtual_network` property has been deprecated ([#24140](hashicorp/terraform-provider-azurerm#24140 `azurerm_hdinsight_hadoop_cluster` - set `roles.edge_node.install_script_action.parameters` into state by retrieving the value provided in the user config since this property isn't returned by the API ([#23971](hashicorp/terraform-provider-azurerm#23971 `azurerm_kubernetes_cluster` - prevent a bug where maintenance window start date was always recalculated and sent to the API ([#23985](hashicorp/terraform-provider-azurerm#23985 `azurerm_mssql_database` - will no longer send all long retention values in payload unless set ([#24124](hashicorp/terraform-provider-azurerm#24124 `azurerm_mssql_managed_database` - will no longer send all long retention values in payload unless set ([#24124](hashicorp/terraform-provider-azurerm#24124 `azurerm_mssql_server_microsoft_support_auditing_policy` - only include storage endpoint in payload if set ([#24122](hashicorp/terraform-provider-azurerm#24122 `azurerm_mobile_network_packet_core_control_plane` - prevent a panic if the HTTP Response is nil ([#24083](hashicorp/terraform-provider-azurerm#24083 `azurerm_storage_account` - revert plan time name validation `(#23799)` ([#24142](hashicorp/terraform-provider-azurerm#24142 `azurerm_web_application_firewall_policy` - split create and update function to fix lifecycle - ignore changes ([#23412](https://github.com/hashicorp/terraform-provider-azurerm/issues/23412))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/931/">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) <60776566+jenkins-infra-bot@users.noreply.github.com> Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
Hi, I get this issu: (expected workload_profile.0.workload_profile_type to be one of ["D4" "D8" "D16" "D32" "E4" "E8" "E16" "E32"], got consumption ) we don't have the default consumption profile ? |
Consumption is enabled by default, you don't need to declare it |
The ARM rest api does accept
|
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. |