azurerm_container_app - Add validations for the ingress.0.traffic_weight

[magodo]

This PR adds create/update time validation for the azurerm_container_app, in that:

• latest_revision (true) conflicts with revision_suffix (non-empty) in any case, but exactly one of them should be specified
• At most one traffic_weight can have latest_revision set to true. Especially for creation, the latest_revision has to be true, and only one traffic_weight block can be specified

This also removes a confusion line in the document saying that the traffic_weight can't be set if revision_mode is Single, since the traffic_weight is set to Required (I assume this is to prefer explicit, instead of either using the O+C trick, or let user specify it in the ignore_changes).

Fix #20435

[jackofallops]

Hi @magodo - Thanks for this PR. I've left a few minor comments below to take a look at. Can we also add an ExpectError test to validate this new validation functionality? Should be good to go after that.

Thanks again

[jackofallops]

I think we can replace this with:

Suggested change

	if old, _ := metadata.ResourceDiff.GetChange("name"); old == "" {
	if metadata.ResourceDiff.HasChange("name"){

since the only way this can be true is in the case of a new resource?

[magodo]

For force new resource, its first invocation on CustomizeDiff will have the name be marked as to be changed, from "some value" to null?

[jackofallops]

My understanding, based on the docs:

	//   * Existing resource, forced new: One run with state (before ForceNew),
	//     then one run without state (as if new resource)

is that the CustomizeDiff runs with the config values, not the ForceNew changes (i.e. it's run before the resource is marked for ForceNew and evaluated on the changes of config property values based on the state)

Admittedly it's been a while since I've been in this section of code, so if you have evidence of it working differently, I'm happy to have my mind changed?

Also, it's not a critical change here, just simplifies the code a little.

[magodo]

By experimenting, it turns out the first invocation on CustomizeDiff for force new resource will compare the state and the config (instead of state vs null). So you are right, and I've updated the code accordingly.

Whilst one limitation to this resource is that, when users update the container app to have multiple ingress traffic weight, then they can't simply rename the resource to re-provision. The validation will fail and they'll have to manually ensure there is only one traffic weight defined as expected.

[jackofallops]

since there can only be one ingress, this should be:

Suggested change

	return fmt.Errorf("`ingress.%[1]d.traffic_weight.%[1]d.revision_suffix` conflicts with `ingress.%[1]d.traffic_weight.%[1]d.latest_revision`", i)
	return fmt.Errorf("`ingress.0.traffic_weight.%[1]d.revision_suffix` conflicts with `ingress.0.traffic_weight.%[1]d.latest_revision`", i)

or the ingress index will be incorrectly incremented along with the traffic_weight index.

[jackofallops]

as above

Suggested change

	return fmt.Errorf("`ingress.%[1]d.traffic_weight.%[1]d.revision_suffix` is not specified", i)
	return fmt.Errorf("`ingress.0.traffic_weight.%d.revision_suffix` is not specified", i)

[magodo]

@jackofallops Thank you for the comments! I've resolved them, and passed the test:

$ TF_ACC=1 go test -v -timeout=20h ./internal/services/containerapps -run='TestAccContainerAppResource_ingressTrafficValidation'
=== RUN   TestAccContainerAppResource_ingressTrafficValidation
=== PAUSE TestAccContainerAppResource_ingressTrafficValidation
=== CONT  TestAccContainerAppResource_ingressTrafficValidation
--- PASS: TestAccContainerAppResource_ingressTrafficValidation (10.15s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/containerapps 10.155s

[jackofallops]

Hi @magodo - Looks like one of the existing tests contains a violation of this new validation

=== CONT  TestAccContainerAppResource_secretRemoveShouldFail
    testcase.go:113: Step 1/3 error: Error running pre-apply refresh: exit status 1
        Error: at most one `ingress.0.traffic_weight` can be specified during creation
          with azurerm_container_app.test,
          on terraform_plugin_test.tf line 130, in resource "azurerm_container_app" "test":
         130: resource "azurerm_container_app" "test" {
--- FAIL: TestAccContainerAppResource_secretRemoveShouldFail (8.59s)
FAIL

Can you take a look at fixing that up by adding a new config for the first part of the test instead of re-using the completeUpdate config? Thanks!

[magodo]

@jackofallops Thank you for pointing this out! I realized that the failed test case used to pass, and it has two traffic weight configs, but still workt. The reason is that the template has revision_suffix specified, in which case you can specify it in the traffic_weight, even if you have only one traffic_weight.

The config of TestAccContainerAppResource_secretRemoveShouldFail actually has two traffic_weight blocks:

    traffic_weight {
      latest_revision = true
      percentage      = 20
    }
    traffic_weight {
      revision_suffix = "rev1"
      percentage      = 80
    }

This can make the API call successful, but the API will then deduplicate and merge them into one, effectively as:

    traffic_weight {
      latest_revision = true
      percentage      = 100
    }

or

    traffic_weight {
      revision_suffix = "rev1"
      percentage      = 100
    }

(though the API response of the traffic weight is the same as the request)

It is also worth mentioning that the order of the two traffic_weight (s) matters in that if the one with latest_revision specified appear in the latter, it will somehow override the template.0.revision_suffix, and make the next plan to show diff.

Based on this, I think we can still keep the validation as is, and change the configuration for the failing test cases so that they can pass the validation. Whilst this is a "breaking change" any way, so we shall mention that in the release note?

Test

💢  TF_ACC=1 go test -v -timeout=20h ./internal/services/containerapps -run='TestAccContainerAppResource_secretFail|TestAccContainerAppResource_removeDaprAppPort|TestAccContainerAppResource_completeUpdate'
=== RUN   TestAccContainerAppResource_completeUpdate
=== PAUSE TestAccContainerAppResource_completeUpdate
=== RUN   TestAccContainerAppResource_removeDaprAppPort
=== PAUSE TestAccContainerAppResource_removeDaprAppPort
=== RUN   TestAccContainerAppResource_secretFail
=== PAUSE TestAccContainerAppResource_secretFail
=== CONT  TestAccContainerAppResource_completeUpdate
=== CONT  TestAccContainerAppResource_secretFail
=== CONT  TestAccContainerAppResource_removeDaprAppPort
--- PASS: TestAccContainerAppResource_secretFail (805.04s)
--- PASS: TestAccContainerAppResource_completeUpdate (915.24s)
--- PASS: TestAccContainerAppResource_removeDaprAppPort (932.51s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/containerapps 932.519s

[jackofallops]

Thanks for the changes @magodo - This LGTM now 👍

[github-actions]

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.