From ccce9a774cd77c387d41f9e36a9393cdefa0dcf8 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 7 Jan 2025 12:06:50 +0100 Subject: [PATCH] feat(update-app-base): add support to update `base` for application charms This PR adds support to update the base in application charms by requiring a replace in case of a machine charm, and perform the upgrade in case of a k8s charm. We add the model_type computed field on the application schema, and we use it to make a decision in the planmodifier RequiresReplaceIf. re #635 --- docs/resources/application.md | 3 +- internal/juju/applications.go | 46 +++++++++---- .../provider/modifer_applicationbaseupdate.go | 32 +++++++++ internal/provider/resource_application.go | 33 ++++++++-- .../provider/resource_application_test.go | 66 +++++++++++++++++++ 5 files changed, 160 insertions(+), 20 deletions(-) create mode 100644 internal/provider/modifer_applicationbaseupdate.go diff --git a/docs/resources/application.md b/docs/resources/application.md index 07c19244..001e6a4b 100644 --- a/docs/resources/application.md +++ b/docs/resources/application.md @@ -74,6 +74,7 @@ Notes: ### Read-Only - `id` (String) The ID of this resource. +- `model_type` (String) The type of the model where the application is to be deployed. It is a computed field and is needed to determine if the application should be replaced or updated in case of base updates. - `principal` (Boolean, Deprecated) Whether this is a Principal application @@ -85,7 +86,7 @@ Required: Optional: -- `base` (String) The operating system on which to deploy. E.g. ubuntu@22.04. +- `base` (String) The operating system on which to deploy. E.g. ubuntu@22.04. Changing this value for machine charms will trigger a replace by terraform. - `channel` (String) The channel to use when deploying a charm. Specified as \/\/\. - `revision` (Number) The revision of the charm to deploy. During the update phase, the charm revision should be update before config update, to avoid issues with config parameters parsing. - `series` (String, Deprecated) The series on which to deploy. diff --git a/internal/juju/applications.go b/internal/juju/applications.go index 6e5045e7..3af3c96e 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -271,7 +271,8 @@ type transformedCreateApplicationInput struct { } type CreateApplicationResponse struct { - AppName string + AppName string + ModelType string } type ReadApplicationInput struct { @@ -284,6 +285,7 @@ type ReadApplicationResponse struct { Channel string Revision int Base string + ModelType string Series string Units int Trust bool @@ -307,9 +309,9 @@ type UpdateApplicationInput struct { Trust *bool Expose map[string]interface{} // Unexpose indicates what endpoints to unexpose - Unexpose []string - Config map[string]string - //Series string // Unsupported today + Unexpose []string + Config map[string]string + Base string Placement map[string]interface{} Constraints *constraints.Value EndpointBindings map[string]string @@ -368,9 +370,16 @@ func (c applicationsClient) CreateApplication(ctx context.Context, input *Create // If we have managed to deploy something, now we have // to check if we have to expose something err = c.processExpose(applicationAPIClient, transformedInput.applicationName, transformedInput.expose) - + if err != nil { + return nil, err + } + modelType, err := c.ModelType(input.ModelName) + if err != nil { + return nil, err + } return &CreateApplicationResponse{ - AppName: transformedInput.applicationName, + AppName: transformedInput.applicationName, + ModelType: modelType.String(), }, err } @@ -1114,6 +1123,7 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA Channel: appInfo.Channel, Revision: charmURL.Revision, Base: fmt.Sprintf("%s@%s", appInfo.Base.Name, baseChannel.Track), + ModelType: modelType.String(), Series: seriesString, Units: unitCount, Trust: trustValue, @@ -1194,7 +1204,7 @@ func (c applicationsClient) UpdateApplication(input *UpdateApplicationInput) err // before the operations with config. Because the config params // can be changed from one revision to another. So "Revision-Config" // ordering will help to prevent issues with the configuration parsing. - if input.Revision != nil || input.Channel != "" || len(input.Resources) != 0 { + if input.Revision != nil || input.Channel != "" || len(input.Resources) != 0 || input.Base != "" { setCharmConfig, err := c.computeSetCharmConfig(input, applicationAPIClient, charmsAPIClient, resourcesAPIClient) if err != nil { return err @@ -1379,22 +1389,24 @@ func (c applicationsClient) computeSetCharmConfig( newOrigin.Branch = strPtr(parsedChannel.Branch) } } + if input.Base != "" { + base, err := corebase.ParseBaseFromString(input.Base) + if err != nil { + return nil, err + } + newOrigin.Base = base + } resolvedURL, resolvedOrigin, supportedBases, err := resolveCharm(charmsAPIClient, newURL, newOrigin) if err != nil { return nil, err } - // Ensure that the new charm supports the architecture and - // operating system currently used by the deployed application. + // Ensure that the new charm supports the architecture used by the deployed application. if oldOrigin.Architecture != resolvedOrigin.Architecture { msg := fmt.Sprintf("the new charm does not support the current architecture %q", oldOrigin.Architecture) return nil, errors.New(msg) } - if !basesContain(oldOrigin.Base, supportedBases) { - msg := fmt.Sprintf("the new charm does not support the current operating system %q", oldOrigin.Base.String()) - return nil, errors.New(msg) - } // Ensure the new revision or channel is contained // in the origin to be saved by juju when AddCharm @@ -1406,6 +1418,14 @@ func (c applicationsClient) computeSetCharmConfig( oldOrigin.Risk = newOrigin.Risk oldOrigin.Branch = newOrigin.Branch } + if input.Base != "" { + oldOrigin.Base = newOrigin.Base + } + + if !basesContain(oldOrigin.Base, supportedBases) { + msg := fmt.Sprintf("the new charm does not support the current operating system %q", oldOrigin.Base.String()) + return nil, errors.New(msg) + } resultOrigin, err := charmsAPIClient.AddCharm(resolvedURL, oldOrigin, false) if err != nil { diff --git a/internal/provider/modifer_applicationbaseupdate.go b/internal/provider/modifer_applicationbaseupdate.go new file mode 100644 index 00000000..7dbd136e --- /dev/null +++ b/internal/provider/modifer_applicationbaseupdate.go @@ -0,0 +1,32 @@ +// Copyright 2025 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package provider + +import ( + "context" + + "github.com/hashicorp/terraform-plugin-framework/path" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/juju/juju/core/model" +) + +// baseApplicationRequiresReplaceIf is a plan modifier that sets the RequiresReplace field if the +// model type is IAAS. The reason is that with CAAS the application can be updated in place. +// With IAAS the application needs to be replaced. To make this decision the model type is needed. +// Since you can't access the juju client in the plan modifiers we've added a computed field `model_type`. +// This is set in the state by means of the `stringplanmodifier.UseStateForUnknown()`, so when we update the base +// is always guaranteed to be set. +func baseApplicationRequiresReplaceIf(ctx context.Context, req planmodifier.StringRequest, resp *stringplanmodifier.RequiresReplaceIfFuncResponse) { + if req.State.Raw.IsKnown() { + var modelType types.String + diags := req.State.GetAttribute(ctx, path.Root("model_type"), &modelType) + if diags.HasError() { + resp.Diagnostics.Append(diags...) + return + } + resp.RequiresReplace = modelType.ValueString() == model.IAAS.String() + } +} diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index 72db6a5c..8f286a8d 100644 --- a/internal/provider/resource_application.go +++ b/internal/provider/resource_application.go @@ -82,6 +82,7 @@ type applicationResourceModel struct { Constraints types.String `tfsdk:"constraints"` Expose types.List `tfsdk:"expose"` ModelName types.String `tfsdk:"model"` + ModelType types.String `tfsdk:"model_type"` Placement types.String `tfsdk:"placement"` EndpointBindings types.Set `tfsdk:"endpoint_bindings"` Resources types.Map `tfsdk:"resources"` @@ -147,6 +148,14 @@ func (r *applicationResource) Schema(_ context.Context, _ resource.SchemaRequest stringplanmodifier.RequiresReplaceIfConfigured(), }, }, + "model_type": schema.StringAttribute{ + Description: "The type of the model where the application is deployed. It is a computed field and " + + "is needed to determine if the application should be replaced or updated in case of base updates.", + Computed: true, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.UseStateForUnknown(), + }, + }, "units": schema.Int64Attribute{ Description: "The number of application units to deploy for the charm.", Optional: true, @@ -316,11 +325,12 @@ func (r *applicationResource) Schema(_ context.Context, _ resource.SchemaRequest DeprecationMessage: "Configure base instead. This attribute will be removed in the next major version of the provider.", }, BaseKey: schema.StringAttribute{ - Description: "The operating system on which to deploy. E.g. ubuntu@22.04.", + Description: "The operating system on which to deploy. E.g. ubuntu@22.04. Changing this value for machine charms will trigger a replace by terraform.", Optional: true, Computed: true, PlanModifiers: []planmodifier.String{ stringplanmodifier.UseStateForUnknown(), + stringplanmodifier.RequiresReplaceIf(baseApplicationRequiresReplaceIf, "", ""), }, Validators: []validator.String{ stringvalidator.ConflictsWith(path.Expressions{ @@ -581,7 +591,6 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq return } 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 @@ -590,6 +599,7 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq plan.Placement = types.StringValue(readResp.Placement) plan.Principal = types.BoolNull() plan.ApplicationName = types.StringValue(createResp.AppName) + plan.ModelType = types.StringValue(readResp.ModelType) planCharm.Revision = types.Int64Value(int64(readResp.Revision)) planCharm.Base = types.StringValue(readResp.Base) planCharm.Series = types.StringValue(readResp.Series) @@ -692,6 +702,12 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest } r.trace("read application", map[string]interface{}{"resource": appName, "response": response}) + modelType, err := r.client.Applications.ModelType(modelName) + if err != nil { + resp.Diagnostics.Append(handleApplicationNotFoundError(ctx, err, &resp.State)...) + return + } + state.ApplicationName = types.StringValue(appName) state.ModelName = types.StringValue(modelName) @@ -700,6 +716,7 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest state.Placement = types.StringValue(response.Placement) state.Principal = types.BoolNull() state.UnitCount = types.Int64Value(int64(response.Units)) + state.ModelType = types.StringValue(modelType.String()) state.Trust = types.BoolValue(response.Trust) // state requiring transformation @@ -919,8 +936,10 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq } else if !planCharm.Revision.Equal(stateCharm.Revision) { updateApplicationInput.Revision = intPtr(planCharm.Revision) } - - if !planCharm.Series.Equal(stateCharm.Series) || !planCharm.Base.Equal(stateCharm.Base) { + if !planCharm.Base.Equal(stateCharm.Base) { + updateApplicationInput.Base = planCharm.Base.ValueString() + } + if !planCharm.Series.Equal(stateCharm.Series) { // This violates Terraform's declarative model. We could implement // `juju set-application-base`, usually used after `upgrade-machine`, // which would change the operating system used for future units of @@ -928,7 +947,7 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq // the current. This provider does not implement an equivalent to // `upgrade-machine`. There is also a question of how to handle a // change to series, revision and channel at the same time. - resp.Diagnostics.AddWarning("Not Supported", "Changing an application's operating system after deploy.") + resp.Diagnostics.AddWarning("Not Supported", "Changing operating system's series after deploy.") } } @@ -1051,7 +1070,8 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq if updateApplicationInput.Channel != "" || updateApplicationInput.Revision != nil || updateApplicationInput.Placement != nil || - updateApplicationInput.Units != nil { + updateApplicationInput.Units != nil || + updateApplicationInput.Base != "" { readResp, err := r.client.Applications.ReadApplicationWithRetryOnNotFound(ctx, &juju.ReadApplicationInput{ ModelName: updateApplicationInput.ModelName, AppName: updateApplicationInput.AppName, @@ -1090,6 +1110,7 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq } } + plan.ModelType = state.ModelType plan.ID = types.StringValue(newAppID(plan.ModelName.ValueString(), plan.ApplicationName.ValueString())) plan.Principal = types.BoolNull() r.trace("Updated", applicationResourceModelForLogging(ctx, &plan)) diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index 2479a903..5ab37727 100644 --- a/internal/provider/resource_application_test.go +++ b/internal/provider/resource_application_test.go @@ -276,6 +276,37 @@ func TestAcc_CharmUpdates(t *testing.T) { }) } +func TestAcc_CharmUpdateBase(t *testing.T) { + modelName := acctest.RandomWithPrefix("tf-test-charmbaseupdates") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: frameworkProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccApplicationUpdateBaseCharm(modelName, "ubuntu@22.04"), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("juju_application.this", "charm.0.base", "ubuntu@22.04"), + ), + }, + { + // move to base ubuntu 20.04 + Config: testAccApplicationUpdateBaseCharm(modelName, "ubuntu@20.04"), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("juju_application.this", "charm.0.base", "ubuntu@20.04"), + ), + }, + { + // move back to ubuntu 22.04 + Config: testAccApplicationUpdateBaseCharm(modelName, "ubuntu@22.04"), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("juju_application.this", "charm.0.base", "ubuntu@22.04"), + ), + }, + }, + }) +} + func TestAcc_ResourceRevisionUpdatesLXD(t *testing.T) { if testingCloud != LXDCloudTesting { t.Skip(t.Name() + " only runs with LXD") @@ -1021,6 +1052,41 @@ func testAccResourceApplicationUpdatesCharm(modelName string, channel string) st } } +func testAccApplicationUpdateBaseCharm(modelName string, base string) string { + if testingCloud == LXDCloudTesting { + return fmt.Sprintf(` + resource "juju_model" "this" { + name = %q + } + + resource "juju_application" "this" { + model = juju_model.this.name + name = "test-app" + charm { + name = "ubuntu" + base = %q + } + } + `, modelName, base) + } else { + return fmt.Sprintf(` + resource "juju_model" "this" { + name = %q + } + + resource "juju_application" "this" { + model = juju_model.this.name + name = "test-app" + charm { + name = "coredns" + channel = "1.25/stable" + base = %q + } + } + `, modelName, base) + } +} + // testAccResourceApplicationConstraints will return two set for constraint // applications. The version to be used in K8s sets the juju-external-hostname // because we set the expose parameter.