From fc099485072f841a99607bc7f3f2748d227911ef Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Thu, 22 Aug 2024 14:04:45 +0100 Subject: [PATCH] fix(constraints): ensure constraints are non-nil Fixes: https://github.com/juju/terraform-provider-juju/issues/466 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. 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. Since we know that subordinate applications cannot have constraints, on Create or Read we can easily 'compute' the constraints ourselves as "". --- internal/provider/provider_test.go | 6 +- internal/provider/resource_application.go | 18 +++-- .../provider/resource_application_test.go | 70 +++++++++++++++++++ 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/internal/provider/provider_test.go b/internal/provider/provider_test.go index 6027c911..41269a5d 100644 --- a/internal/provider/provider_test.go +++ b/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 } diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index cf885ad0..dc17f5e4 100644 --- a/internal/provider/resource_application.go +++ b/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) @@ -986,7 +990,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)) diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index 93066573..3b5779ca 100644 --- a/internal/provider/resource_application_test.go +++ b/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) { @@ -801,6 +855,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" {