diff --git a/docs/resources/integration.md b/docs/resources/integration.md index 34595375..4632e927 100644 --- a/docs/resources/integration.md +++ b/docs/resources/integration.md @@ -25,6 +25,25 @@ resource "juju_integration" "this" { name = juju_application.percona-cluster.name endpoint = "server" } + + # Add any RequiresReplace schema attributes of + # an application in this integration to ensure + # it is recreated if one of the applications + # is Destroyed and Recreated by terraform. E.G.: + lifecycle { + replace_triggered_by = [ + juju_application.wordpress.name, + juju_application.wordpress.model, + juju_application.wordpress.constraints, + juju_application.wordpress.placement, + juju_application.wordpress.charm.name, + juju_application.percona-cluster.name, + juju_application.percona-cluster.model, + juju_application.percona-cluster.constraints, + juju_application.percona-cluster.placement, + juju_application.percona-cluster.charm.name, + ] + } } ``` diff --git a/examples/resources/juju_integration/resource.tf b/examples/resources/juju_integration/resource.tf index 2d87b6c9..bb663440 100644 --- a/examples/resources/juju_integration/resource.tf +++ b/examples/resources/juju_integration/resource.tf @@ -11,4 +11,23 @@ resource "juju_integration" "this" { name = juju_application.percona-cluster.name endpoint = "server" } + + # Add any RequiresReplace schema attributes of + # an application in this integration to ensure + # it is recreated if one of the applications + # is Destroyed and Recreated by terraform. E.G.: + lifecycle { + replace_triggered_by = [ + juju_application.wordpress.name, + juju_application.wordpress.model, + juju_application.wordpress.constraints, + juju_application.wordpress.placement, + juju_application.wordpress.charm.name, + juju_application.percona-cluster.name, + juju_application.percona-cluster.model, + juju_application.percona-cluster.constraints, + juju_application.percona-cluster.placement, + juju_application.percona-cluster.charm.name, + ] + } } diff --git a/internal/juju/applications.go b/internal/juju/applications.go index 64461e74..15a0190c 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -177,7 +177,7 @@ func resolveCharmURL(charmName string) (*charm.URL, error) { return charmURL, nil } -func (c applicationsClient) CreateApplication(input *CreateApplicationInput) (*CreateApplicationResponse, error) { +func (c applicationsClient) CreateApplication(ctx context.Context, input *CreateApplicationInput) (*CreateApplicationResponse, error) { appName := input.ApplicationName if appName == "" { appName = input.CharmName @@ -276,25 +276,6 @@ func (c applicationsClient) CreateApplication(input *CreateApplicationInput) (*C userBase, suggestedBase) } - // Add the charm to the model - origin = resolvedOrigin.WithSeries(seriesToUse) - charmURL = resolvedURL.WithSeries(seriesToUse) - - resultOrigin, err := charmsAPIClient.AddCharm(charmURL, origin, false) - if err != nil { - return nil, err - } - - charmID := apiapplication.CharmID{ - URL: charmURL, - Origin: resultOrigin, - } - - resources, err := c.processResources(charmsAPIClient, conn, charmID, appName) - if err != nil { - return nil, err - } - appConfig := input.Config if appConfig == nil { appConfig = make(map[string]string) @@ -318,24 +299,84 @@ func (c applicationsClient) CreateApplication(input *CreateApplicationInput) (*C } } - args := apiapplication.DeployArgs{ - CharmID: charmID, - ApplicationName: appName, - NumUnits: input.Units, - // Still supply series, to be compatible with juju 2.9 controllers. - // 3.x controllers will only use the CharmOrigin and its base. - Series: resultOrigin.Series, - CharmOrigin: resultOrigin, - Config: appConfig, - Cons: input.Constraints, - Resources: resources, - Placement: placements, - } - c.Tracef("Calling Deploy", map[string]interface{}{"args": args}) - err = applicationAPIClient.Deploy(args) + // Add the charm to the model + origin = resolvedOrigin.WithSeries(seriesToUse) + charmURL = resolvedURL.WithSeries(seriesToUse) + // If a plan element, with RequiresReplace in the schema, is + // changed. Terraform calls the Destroy method then the Create + // method for resource. This provider does not wait for Destroy + // to be complete before returning. Therefore, a race may occur + // of tearing down and reading the same charm. + // + // Do the actual work to create an application within Retry. + // Errors seen so far include: + // * cannot add application "replace": charm "ch:amd64/jammy/mysql-196" not found + // * cannot add application "replace": application already exists + // * cannot add application "replace": charm: not found or not alive + err = retry.Call(retry.CallArgs{ + Func: func() error { + resultOrigin, err := charmsAPIClient.AddCharm(charmURL, origin, false) + if err != nil { + err2 := typedError(err) + // If the charm is AlreadyExists, keep going, we + // may still be able to create the application. It's + // also possible we have multiple applications using + // the same charm. + if !jujuerrors.Is(err2, jujuerrors.AlreadyExists) { + return err2 + } + } + + charmID := apiapplication.CharmID{ + URL: charmURL, + Origin: resultOrigin, + } + + resources, err := c.processResources(charmsAPIClient, conn, charmID, appName) + if err != nil && !jujuerrors.Is(err, jujuerrors.AlreadyExists) { + return err + } + + args := apiapplication.DeployArgs{ + CharmID: charmID, + ApplicationName: appName, + NumUnits: input.Units, + // Still supply series, to be compatible with juju 2.9 controllers. + // 3.x controllers will only use the CharmOrigin and its base. + Series: resultOrigin.Series, + CharmOrigin: resultOrigin, + Config: appConfig, + Cons: input.Constraints, + Resources: resources, + Placement: placements, + } + c.Tracef("Calling Deploy", map[string]interface{}{"args": args}) + if err = applicationAPIClient.Deploy(args); err != nil { + return typedError(err) + } + return nil + }, + IsFatalError: func(err error) bool { + // If we hit AlreadyExists, it is from Deploy only under 2 + // scenarios: + // 1. User error, the application has already been created? + // 2. We're replacing the application and tear down hasn't + // finished yet, we should try again. + return !errors.Is(err, jujuerrors.NotFound) && !errors.Is(err, jujuerrors.AlreadyExists) + }, + NotifyFunc: func(err error, attempt int) { + c.Errorf(err, fmt.Sprintf("deploy application %q retry", appName)) + message := fmt.Sprintf("waiting for application %q deploy, attempt %d", appName, attempt) + c.Debugf(message) + }, + BackoffFunc: retry.DoubleDelay, + Attempts: 30, + Delay: time.Second, + Clock: clock.WallClock, + Stop: ctx.Done(), + }) if err != nil { - // unfortunate error during deployment return nil, err } @@ -516,7 +557,7 @@ func splitCommaDelimitedList(list string) []string { func (c applicationsClient) processResources(charmsAPIClient *apicharms.Client, conn api.Connection, charmID apiapplication.CharmID, appName string) (map[string]string, error) { charmInfo, err := charmsAPIClient.CharmInfo(charmID.URL.String()) if err != nil { - return nil, err + return nil, typedError(err) } // check if we have resources to request @@ -615,16 +656,17 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA return nil, fmt.Errorf("no status returned for application: %s", input.AppName) } - allocatedMachines := make([]string, 0) - placementCount := 0 + allocatedMachines := set.NewStrings() for _, v := range appStatus.Units { - allocatedMachines = append(allocatedMachines, v.Machine) - placementCount += 1 + if v.Machine != "" { + allocatedMachines.Add(v.Machine) + } } - // sort the list - sort.Strings(allocatedMachines) - placement := strings.Join(allocatedMachines, ",") + var placement string + if !allocatedMachines.IsEmpty() { + placement = strings.Join(allocatedMachines.SortedValues(), ",") + } unitCount := len(appStatus.Units) // if we have a CAAS we use scale instead of units length @@ -1060,7 +1102,7 @@ func addPendingResources(appName string, resourcesToBeAdded map[string]charmreso toRequest, err := resourcesAPIClient.AddPendingResources(resourcesReq) if err != nil { - return nil, err + return nil, typedError(err) } // now build a map with the resource name and the corresponding UUID diff --git a/internal/juju/errors.go b/internal/juju/errors.go new file mode 100644 index 00000000..8890b280 --- /dev/null +++ b/internal/juju/errors.go @@ -0,0 +1,29 @@ +// Copyright 2023 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package juju + +import ( + "strings" + + jujuerrors "github.com/juju/errors" +) + +func typedError(err error) error { + switch { + case strings.Contains(err.Error(), "not found"): + return jujuerrors.WithType(err, jujuerrors.NotFound) + case strings.Contains(err.Error(), "already exists"): + return jujuerrors.WithType(err, jujuerrors.AlreadyExists) + case strings.Contains(err.Error(), "user not valid"): + return jujuerrors.WithType(err, jujuerrors.UserNotFound) + case strings.Contains(err.Error(), "not valid"): + return jujuerrors.WithType(err, jujuerrors.NotValid) + case strings.Contains(err.Error(), "not implemented"): + return jujuerrors.WithType(err, jujuerrors.NotImplemented) + case strings.Contains(err.Error(), "not yet available"): + return jujuerrors.WithType(err, jujuerrors.NotYetAvailable) + default: + return err + } +} diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index c413423f..b5ed0b76 100644 --- a/internal/provider/resource_application.go +++ b/internal/provider/resource_application.go @@ -385,21 +385,23 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq } modelName := plan.ModelName.ValueString() - createResp, err := r.client.Applications.CreateApplication(&juju.CreateApplicationInput{ - ApplicationName: plan.ApplicationName.ValueString(), - ModelName: modelName, - CharmName: charmName, - CharmChannel: channel, - CharmRevision: revision, - CharmBase: planCharm.Base.ValueString(), - CharmSeries: planCharm.Series.ValueString(), - Units: int(plan.UnitCount.ValueInt64()), - Config: configField, - Constraints: parsedConstraints, - Trust: plan.Trust.ValueBool(), - Expose: expose, - Placement: plan.Placement.ValueString(), - }) + createResp, err := r.client.Applications.CreateApplication(ctx, + &juju.CreateApplicationInput{ + ApplicationName: plan.ApplicationName.ValueString(), + ModelName: modelName, + CharmName: charmName, + CharmChannel: channel, + CharmRevision: revision, + CharmBase: planCharm.Base.ValueString(), + CharmSeries: planCharm.Series.ValueString(), + Units: int(plan.UnitCount.ValueInt64()), + Config: configField, + Constraints: parsedConstraints, + Trust: plan.Trust.ValueBool(), + Expose: expose, + Placement: plan.Placement.ValueString(), + }, + ) if err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create application, got error: %s", err)) return diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index 1f18e897..94fe40f5 100644 --- a/internal/provider/resource_application_test.go +++ b/internal/provider/resource_application_test.go @@ -5,7 +5,6 @@ package provider import ( "fmt" - "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" @@ -15,19 +14,11 @@ import ( func TestAcc_ResourceApplication(t *testing.T) { modelName := acctest.RandomWithPrefix("tf-test-application") appName := "test-app" - appInvalidName := "test_app" resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: frameworkProviderFactories, Steps: []resource.TestStep{ - { - // Mind that ExpectError should be the first step - // "When tests have an ExpectError[...]; this results in any previous state being cleared. " - // https://github.com/hashicorp/terraform-plugin-sdk/issues/118 - Config: testAccResourceApplicationBasic(modelName, appInvalidName), - ExpectError: regexp.MustCompile(fmt.Sprintf("Unable to create application, got error: invalid application name %q,\nunexpected character _", appInvalidName)), - }, { Config: testAccResourceApplicationBasic(modelName, appName), Check: resource.ComposeTestCheckFunc( @@ -42,17 +33,7 @@ func TestAcc_ResourceApplication(t *testing.T) { { // cores constraint is not valid in K8s SkipFunc: func() (bool, error) { - // Failing with terraform 1.4.x and 1.3.x - // Unable to create application, got error: charm - //" ch:amd64/focal/jameinel-ubuntu-lite-10" not found (not found) - // - // Related to: - // https://github.com/juju/terraform-provider-juju/issues/272 - // There is a timing window with destroying an application - // before a new one is created when RequiresReplace is used in the - // resource schema. - return true, nil - //return testingCloud != LXDCloudTesting, nil + return testingCloud != LXDCloudTesting, nil }, Config: testAccResourceApplicationConstraints(modelName, "arch=amd64 cores=1 mem=4096M"), Check: resource.ComposeTestCheckFunc( @@ -67,11 +48,6 @@ func TestAcc_ResourceApplication(t *testing.T) { // Unable to create application, got error: charm // "state changing too quickly; try again soon" // - // Also related to: - // https://github.com/juju/terraform-provider-juju/issues/272 - // There is a timing window with destroying an application - // before a new one is created when RequiresReplace is used in the - // resource schema. return true, nil //return testingCloud != MicroK8sTesting, nil }, @@ -83,17 +59,7 @@ func TestAcc_ResourceApplication(t *testing.T) { }, { SkipFunc: func() (bool, error) { - // Failing with terraform 1.4.x and 1.3.x - // Unable to create application, got error: charm - //" ch:amd64/focal/jameinel-ubuntu-lite-10" not found (not found) - // - // Related to: - // https://github.com/juju/terraform-provider-juju/issues/272 - // There is a timing window with destroying an application - // before a new one is created when RequiresReplace is used in the - // resource schema. - return true, nil - //return testingCloud != LXDCloudTesting, nil + return testingCloud != LXDCloudTesting, nil }, Config: testAccResourceApplicationConstraintsSubordinate(modelName, "arch=amd64 cores=1 mem=4096M"), Check: resource.ComposeTestCheckFunc( @@ -282,7 +248,7 @@ func testAccResourceApplicationBasic_Minimal(modelName, charmName string) string `, modelName, charmName) } -func testAccResourceApplicationBasic(modelName, appInvalidName string) string { +func testAccResourceApplicationBasic(modelName, appName string) string { if testingCloud == LXDCloudTesting { return fmt.Sprintf(` resource "juju_model" "this" { @@ -298,7 +264,7 @@ func testAccResourceApplicationBasic(modelName, appInvalidName string) string { trust = true expose{} } - `, modelName, appInvalidName) + `, modelName, appName) } else { // if we have a K8s deployment we need the machine hostname return fmt.Sprintf(` @@ -318,7 +284,7 @@ func testAccResourceApplicationBasic(modelName, appInvalidName string) string { juju-external-hostname="myhostname" } } - `, modelName, appInvalidName) + `, modelName, appName) } } diff --git a/internal/provider/resource_credential_test.go b/internal/provider/resource_credential_test.go index 3cbd34cc..e21b6729 100644 --- a/internal/provider/resource_credential_test.go +++ b/internal/provider/resource_credential_test.go @@ -5,7 +5,6 @@ package provider import ( "fmt" - "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" @@ -17,9 +16,7 @@ func TestAcc_ResourceCredential(t *testing.T) { t.Skip(t.Name() + " only runs with LXD") } credentialName := acctest.RandomWithPrefix("tf-test-credential") - credentialInvalidName := "tf%test_credential" authType := "certificate" - authTypeInvalid := "invalid" token := "123abc" resourceName := "juju_credential.test-credential" @@ -27,19 +24,6 @@ func TestAcc_ResourceCredential(t *testing.T) { PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: frameworkProviderFactories, Steps: []resource.TestStep{ - { - // Mind that ExpectError should be the first step - // "When tests have an ExpectError[...]; this results in any previous state being cleared. " - // https://github.com/hashicorp/terraform-plugin-sdk/issues/118 - Config: testAccResourceCredential(credentialName, authTypeInvalid), - ExpectError: regexp.MustCompile(fmt.Sprintf("%q not supported", authTypeInvalid)), - PreConfig: func() { testAccPreCheck(t) }, - }, - { - Config: testAccResourceCredential(credentialInvalidName, authType), - ExpectError: regexp.MustCompile(fmt.Sprintf(".*%q is not\na valid credential name.*", - credentialInvalidName)), - }, { Config: testAccResourceCredential(credentialName, authType), Check: resource.ComposeTestCheckFunc( diff --git a/internal/provider/resource_model_test.go b/internal/provider/resource_model_test.go index 1dcaf8ad..7010207f 100644 --- a/internal/provider/resource_model_test.go +++ b/internal/provider/resource_model_test.go @@ -5,7 +5,6 @@ package provider import ( "fmt" - "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" @@ -17,7 +16,6 @@ import ( func TestAcc_ResourceModel(t *testing.T) { modelName := acctest.RandomWithPrefix("tf-test-model") - modelInvalidName := acctest.RandomWithPrefix("tf_test_model") logLevelInfo := "INFO" logLevelDebug := "DEBUG" @@ -26,15 +24,6 @@ func TestAcc_ResourceModel(t *testing.T) { PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: frameworkProviderFactories, Steps: []resource.TestStep{ - { - // Mind that ExpectError should be the first step - // "When tests have an ExpectError[...]; this results in any previous state being cleared. " - // https://github.com/hashicorp/terraform-plugin-sdk/issues/118 - Config: testAccResourceModel(modelInvalidName, testingCloud.CloudName(), logLevelInfo), - ExpectError: regexp.MustCompile(fmt.Sprintf( - "Unable to create model, got error: \"%s\" is not *\na valid name: model names may only contain lowercase letters, digits and *\nhyphens", modelInvalidName)), - }, - { Config: testAccResourceModel(modelName, testingCloud.CloudName(), logLevelInfo), Check: resource.ComposeTestCheckFunc(