fix(constraints): ensure constraints are non-null

[jack-w-shaw]

Description

Fixes: #466

This bug is two-sided, stemming from the fact that Juju does not distinguish between null constraints and empty-string constraints. However, null is not considered by terraform to be a computed value, whereas the empty string is.

1. Since the constraints schema marks constraints as a computed field, this means if we insert null into state it will be considered unknown & will need to be set on the next Update call. Terraform signals this by passing value <unknown> for state.Constraints through to Update, which is unequal to <null>

2. Our provider detects a difference between <null> and <unknown>, so attempts to set the constraints on the application to "". This fails for subordinates

Resolve this by:

1. Since we know that subordinate applications cannot have constraints, on Create or Read we can easily 'compute' the constraints ourselves as "" instead of leaving as null.

2. Changing the condition we set application constraints to not differentiate between <null>, <unknown> and "".

Type of change

• Bug fix (non-breaking change which fixes an issue)

Environment

• Juju controller version: 3.4.5

• Terraform version: v1.9.5

QA steps

Manual QA steps should be done to test this PR.

provider juju {}

resource "juju_application" "telegraf" {
  name  = "telegraf"
  model = "test"

    charm {
    name     = "telegraf"
    revision = 75
  }

  units = 0
}

$ juju bootstrap lxd
$ juju add-model test

$ juju deploy ubuntu
$ juju deploy telegraf --revision=72 --channel=latest/stable
$ juju relate ubuntu telegraf

# Wait for active/idle status on the two charms

$ terraform init
$ terraform import juju_application.telegraf test:telegraf
$ terraform apply
juju_application.telegraf: Refreshing state... [id=m:telegraf]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # juju_application.telegraf will be updated in-place
  ~ resource "juju_application" "telegraf" {
        id        = "m:telegraf"
        name      = "telegraf"
      + principal = (known after apply)
      + storage   = (known after apply)
        # (4 unchanged attributes hidden)

      ~ charm {
            name     = "telegraf"
          ~ revision = 73 -> 75
            # (3 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

juju_application.telegraf: Modifying... [id=m:telegraf]
juju_application.telegraf: Modifications complete after 1s [id=m:telegraf]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

^ notice that the apply runs successfully, and constraints do not appear as (known after compute)

[hmlanigan]

Base on the conventional commits, this should be a fix, not a chore. Please update the commit message and PR title.

This statement from the PR description is incorrect.
Since the constraints schema marks constraints as a computed field, this means if we insert null into state it will be considered uncomputed & on the next apply terraform will attempt to compute the constraints by setting them.

---

The plan listed in the QA steps should really have revision = 72 based on the juju deploy step. Then:

$ terraform plan
juju_application.telegraf: Refreshing state... [id=test:telegraf]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.

[jack-w-shaw]

Base on the conventional commits, this should be a fix, not a chore. Please update the commit message and PR title.

Yes thanks, I forgot that fix is a commit type, so was assuming bug fixes counted a chores. Updated

This statement from the PR description is incorrect. Since the constraints schema marks constraints as a computed field, this means if we insert null into state it will be considered uncomputed & on the next apply terraform will attempt to compute the constraints by setting them.

How so?

The plan listed in the QA steps should really have revision = 72 based on the juju deploy step. Then:

$ terraform plan
juju_application.telegraf: Refreshing state... [id=test:telegraf]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.

I don't think so. This won't verify the bug reported (#466) will be fixed.

The point of these QA steps are to verify that we can run a charm refresh on an imported subordinate. As we see above, if we set the revision to 72 in the plan no upgrade is run

[hmlanigan]

This statement from the PR description is incorrect.

Since the constraints schema marks constraints as a computed field, this means if we insert null into state it will be considered uncomputed & on the next apply terraform will attempt to compute the constraints by setting them.
This leads to bugs since subordinate applications cannot have constraints applied.

How so?

Perhaps misleading is the word instead. In the reported but scenario, even if terraform didn't believe the computed value must be set, the Update method still would have been called and the equality check could have been false - leading to an incorrect attempt to set nil constraints.

I still believe that

terraform-provider-juju/internal/provider/resource_application.go

Line 993 in 253b365

if !plan.Constraints.Equal(state.Constraints) {

should be changed to

if plan.Constraints.ValueString() != state.Constraints.ValueString() {

Thus even if the constraints come up with Unknown vs Null, vs EmptyString, the provider will no attempt to set them and cause havoc. It will also be a good reminder for the future of this type of problem.

[jack-w-shaw]

I still believe that

terraform-provider-juju/internal/provider/resource_application.go

Line 993 in 253b365

if !plan.Constraints.Equal(state.Constraints) {

should be changed to

if plan.Constraints.ValueString() != state.Constraints.ValueString() {

Thus even if the constraints come up with Unknown vs Null, vs EmptyString, the provider will no attempt to set them and cause havoc. It will also be a good reminder for the future of this type of problem.

Happy to apply this change. But it is actually impossible for constraints to be null or unknown here now, since terraform apply starts by refreshing, which will set null constraints in state to "".

So the only value this change will provide is documentary. Still valuable though!

[hmlanigan]

LGTM. Thank you for the updates.

[jack-w-shaw]

/merge

[jack-w-shaw]

/merge