Merge pull request #556 from jack-w-shaw/JUJU-6545_ensure_constraints

#556

## 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.

```tf
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)`

internal/provider/provider_test.go

@@ -5,6 +5,7 @@ package provider
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"runtime"
 	"testing"
@@ -17,6 +18,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-plugin-go/tfprotov6"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/juju/terraform-provider-juju/internal/juju"
 )
@@ -172,9 +174,9 @@ func testAccPreCheck(t *testing.T) {
 		}
 	}
 	confResp := configureProvider(t, Provider)
-	assert.Equal(t, confResp.Diagnostics.HasError(), false)
+	require.Equal(t, confResp.Diagnostics.HasError(), false, fmt.Sprintf("provider configuration failed: %v", confResp.Diagnostics.Errors()))
 	testClient, ok := confResp.ResourceData.(*juju.Client)
-	assert.Truef(t, ok, "ResourceData, not of type juju client")
+	require.Truef(t, ok, "ResourceData, not of type juju client")
 	TestClient = testClient
 }
 

internal/provider/resource_application.go

@@ -578,6 +578,9 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq
 	r.trace(fmt.Sprintf("read application resource %q", createResp.AppName))
 
 	// Save plan into Terraform state
+
+	// Constraints do not apply to subordinate applications. If the application
+	// is subordinate, the constraints will be set to the empty string.
 	plan.Constraints = types.StringValue(readResp.Constraints.String())
 	plan.Placement = types.StringValue(readResp.Placement)
 	plan.Principal = types.BoolNull()
@@ -688,6 +691,7 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest
 	state.ModelName = types.StringValue(modelName)
 
 	// Use the response to fill in state
+
 	state.Placement = types.StringValue(response.Placement)
 	state.Principal = types.BoolNull()
 	state.UnitCount = types.Int64Value(int64(response.Units))
@@ -708,10 +712,10 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest
 		return
 	}
 
-	// constraints do not apply to subordinate applications.
-	if response.Principal {
-		state.Constraints = types.StringValue(response.Constraints.String())
-	}
+	// Constraints do not apply to subordinate applications. If the application
+	// is subordinate, the constraints will be set to the empty string.
+	state.Constraints = types.StringValue(response.Constraints.String())
+
 	exposeType := req.State.Schema.GetBlocks()[ExposeKey].(schema.ListNestedBlock).NestedObject.Type()
 	if response.Expose != nil {
 		exp := parseNestedExpose(response.Expose)
@@ -998,7 +1002,11 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq
 		}
 	}
 
-	if !plan.Constraints.Equal(state.Constraints) {
+	// Do not use .Equal() here as we should consider null constraints the same
+	// as empty-string constraints. Terraform considers them different, so will
+	// incorrectly attempt to update the constraints, which can cause trouble
+	// for subordinate applications.
+	if plan.Constraints.ValueString() != state.Constraints.ValueString() {
 		appConstraints, err := constraints.Parse(plan.Constraints.ValueString())
 		if err != nil {
 			resp.Diagnostics.AddError("Conversion", fmt.Sprintf("Unable to parse plan constraints, got error: %s", err))

internal/provider/resource_application_test.go

@@ -4,6 +4,7 @@
 package provider
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"regexp"
@@ -20,6 +21,7 @@ import (
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/names/v5"
 
+	"github.com/juju/terraform-provider-juju/internal/juju"
 	internaljuju "github.com/juju/terraform-provider-juju/internal/juju"
 	internaltesting "github.com/juju/terraform-provider-juju/internal/testing"
 )
@@ -156,6 +158,58 @@ func TestAcc_ResourceApplication_Updates(t *testing.T) {
 	})
 }
 
+func TestAcc_ResourceApplication_UpdateImportedSubordinate(t *testing.T) {
+	if testingCloud != LXDCloudTesting {
+		t.Skip(t.Name() + " only runs with LXD")
+	}
+
+	testAccPreCheck(t)
+
+	modelName := acctest.RandomWithPrefix("tf-test-application")
+
+	ctx := context.Background()
+
+	_, err := TestClient.Models.CreateModel(juju.CreateModelInput{
+		Name: modelName,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = TestClient.Applications.CreateApplication(ctx, &juju.CreateApplicationInput{
+		ApplicationName: "telegraf",
+		ModelName:       modelName,
+		CharmName:       "telegraf",
+		CharmChannel:    "latest/stable",
+		CharmRevision:   73,
+		Units:           0,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { testAccPreCheck(t) },
+		ProtoV6ProviderFactories: frameworkProviderFactories,
+		Steps: []resource.TestStep{
+			{
+				Config:             testAccResourceApplicationSubordinate(modelName, 73),
+				ImportState:        true,
+				ImportStateId:      fmt.Sprintf("%s:telegraf", modelName),
+				ImportStatePersist: true,
+				ResourceName:       "juju_application.telegraf",
+			},
+			{
+				Config: testAccResourceApplicationSubordinate(modelName, 75),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("juju_application.telegraf", "charm.0.name", "telegraf"),
+					resource.TestCheckResourceAttr("juju_application.telegraf", "charm.0.revision", "75"),
+				),
+			},
+		},
+	})
+}
+
 // TestAcc_ResourceApplication_UpdatesRevisionConfig will test the revision update that have new config parameters on
 // the charm. The test will check that the config is updated and the revision is updated as well.
 func TestAcc_ResourceApplication_UpdatesRevisionConfig(t *testing.T) {
@@ -1015,6 +1069,22 @@ resource "juju_application" "this" {
 	}
 }
 
+func testAccResourceApplicationSubordinate(modelName string, subordinateRevision int) string {
+	return fmt.Sprintf(`
+resource "juju_application" "telegraf" {
+  model = %q
+  name = "telegraf"
+
+  charm {
+    name = "telegraf"
+    revision = %d
+  }
+
+  units = 0
+}
+`, modelName, subordinateRevision)
+}
+
 func testAccResourceApplicationConstraintsSubordinate(modelName string, constraints string) string {
 	return fmt.Sprintf(`
 resource "juju_model" "this" {