Skip to content

Commit

Permalink
Omit capacity when creating basic plans
Browse files Browse the repository at this point in the history
The new [Basic SKU][0] does not support any scaling, and it is an error to
provide `Capacity` or `AutoScaleSettings`. The resource is implemented
in such a way that it was not possible to call
`DeploymentsCreateOrUpdateThenPoll` without one of these in the
`ScalingProperties`, which is illegal for the Basic SKUs.

Currently the backend has a hack to silently drop `ScalingProperties`
for Basic SKUs, but this is a confusing user experience:

- initial `terraform apply` submits a `PUT` with `"scalingProperties":
{"capacity": 20}` - this validation error is currently suppressed and
the server reports back `"scalingProperties": null`

- next `terraform plan` detects a `capacity 0 -> 20`

- applying that plan requests a `PATCH` with `"scalingProperties":
{"capacity": 20}`, that fails with a `UnsupportedOnBasicPlan` error

It gets more misleading if users specify a capacity explicity;
`capacity = 500` or `auto_scale_profile` will apply successfully, but
get silently ignored server-side.

This patch checks for basic plans and omits the `scalingProperties`
when the user hasn't changed from the default value. We already
instruct users to ignore `capacity`, but the check is needed to
prevent the implicit default value of 20 from getting into the
`DeploymentsCreateOrUpdateThenPoll` request.

[0]: https://docs.nginx.com/nginxaas/azure/billing/overview/#basic-plan
  • Loading branch information
ryepup committed Aug 12, 2024
1 parent d6d89f8 commit 3fe08db
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
12 changes: 10 additions & 2 deletions internal/services/nginx/nginx_deployment_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package nginx
import (
"context"
"fmt"
"strings"
"time"

"github.com/hashicorp/go-azure-helpers/lang/pointer"
Expand All @@ -20,6 +21,8 @@ import (
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation"
)

const defaultCapacity = 20

type FrontendPrivate struct {
IpAddress string `tfschema:"ip_address"`
AllocationMethod string `tfschema:"allocation_method"`
Expand Down Expand Up @@ -123,7 +126,7 @@ func (m DeploymentResource) Arguments() map[string]*pluginsdk.Schema {
Type: pluginsdk.TypeInt,
Optional: true,
ConflictsWith: []string{"auto_scale_profile"},
Default: 20,
Default: defaultCapacity,
ValidateFunc: validation.IntPositive,
},

Expand Down Expand Up @@ -435,7 +438,12 @@ func (m DeploymentResource) Create() sdk.ResourceFunc {
prop.NetworkProfile.NetworkInterfaceConfiguration.SubnetId = pointer.FromString(model.NetworkInterface[0].SubnetId)
}

if model.Capacity > 0 {
isBasicSKU := strings.HasPrefix(model.Sku, "basic")
if isBasicSKU && (model.Capacity != defaultCapacity || len(model.AutoScaleProfile) > 0) {
return fmt.Errorf("basic SKUs are incompatible with `capacity` or `auto_scale_profiles`")
}

if model.Capacity > 0 && !isBasicSKU {
prop.ScalingProperties = &nginxdeployment.NginxDeploymentScalingProperties{
Capacity: pointer.FromInt64(model.Capacity),
}
Expand Down
2 changes: 1 addition & 1 deletion website/docs/r/nginx_deployment.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ The following arguments are supported:

* `sku` - (Required) Specifies the NGINX Deployment SKU. Possible values are `standard_Monthly` and `basic_Monthly`. Changing this forces a new resource to be created.

-> **NOTE:** If you are setting the `sku` to `basic_Monthly`, you should use [Terraform's `ignore_changes` functionality](https://www.terraform.io/language/meta-arguments/lifecycle#ignore_changes) to ignore changes to the `capacity` field.
-> **NOTE:** If you are setting the `sku` to `basic_Monthly`, setting a non-default `capacity` or `auto_scale_profile` will fail. You should use [Terraform's `ignore_changes` functionality](https://www.terraform.io/language/meta-arguments/lifecycle#ignore_changes) to ignore changes to the `capacity` field.

* `managed_resource_group` - (Optional) Specify the managed resource group to deploy VNet injection related network resources. Changing this forces a new NGINX Deployment to be created.

Expand Down

0 comments on commit 3fe08db

Please sign in to comment.