From 62ea468d6118a0c9a35e57db2acc0e3c8c9c1766 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Mon, 25 Sep 2023 17:15:57 -0400 Subject: [PATCH 1/4] Do not leak github.com/juju/juju structures and concerns into provider. The Provider does not need to know about details of handling the juju api such as different structures and bulk api calls. Keep a separation of concerns. --- internal/juju/machines.go | 132 +++++++++++++---------- internal/provider/data_source_machine.go | 2 +- internal/provider/resource_machine.go | 22 ++-- 3 files changed, 86 insertions(+), 70 deletions(-) diff --git a/internal/juju/machines.go b/internal/juju/machines.go index 5d24ec57..db5d7169 100644 --- a/internal/juju/machines.go +++ b/internal/juju/machines.go @@ -33,6 +33,7 @@ type CreateMachineInput struct { ModelName string Constraints string Disks string + Base string Series string InstanceId string @@ -48,23 +49,26 @@ type CreateMachineInput struct { } type CreateMachineResponse struct { - Machine params.AddMachinesResult - Series string + ID string + Base string + Series string } type ReadMachineInput struct { ModelName string - MachineId string + ID string } type ReadMachineResponse struct { - MachineId string - MachineStatus params.MachineStatus + ID string + Base string + Constraints string + Series string } type DestroyMachineInput struct { ModelName string - MachineId string + ID string } func newMachinesClient(sc SharedClient) *machinesClient { @@ -80,12 +84,10 @@ func (c machinesClient) CreateMachine(input *CreateMachineInput) (*CreateMachine } machineAPIClient := apimachinemanager.NewClient(conn) - defer machineAPIClient.Close() + defer func() { _ = machineAPIClient.Close() }() modelConfigAPIClient := apimodelconfig.NewClient(conn) - defer modelConfigAPIClient.Close() - - var machineParams params.AddMachineParams + defer func() { _ = modelConfigAPIClient.Close() }() if input.SSHAddress != "" { configAttrs, err := modelConfigAPIClient.ModelGet() @@ -96,21 +98,12 @@ func (c machinesClient) CreateMachine(input *CreateMachineInput) (*CreateMachine if err != nil { return nil, errors.Trace(err) } - machine_series, machineId, err := manualProvision(machineAPIClient, cfg, + return manualProvision(machineAPIClient, cfg, input.SSHAddress, input.PublicKeyFile, input.PrivateKeyFile) - if err != nil { - return nil, errors.Trace(err) - } else { - return &CreateMachineResponse{ - Machine: params.AddMachinesResult{ - Machine: machineId, - Error: nil, - }, - Series: machine_series, - }, nil - } } + var machineParams params.AddMachineParams + if input.Constraints == "" { modelConstraints, err := modelConfigAPIClient.GetModelConstraints() if err != nil { @@ -139,39 +132,63 @@ func (c machinesClient) CreateMachine(input *CreateMachineInput) (*CreateMachine jobs := []model.MachineJob{model.JobHostUnits} machineParams.Jobs = jobs - var paramsBase params.Base - if input.Series != "" { - seriesBase, err := series.GetBaseFromSeries(input.Series) - if err != nil { - return nil, err - } - - paramsBase.Name = seriesBase.Name - paramsBase.Channel = series.Channel.String(seriesBase.Channel) + opSys := input.Base + if opSys == "" { + opSys = input.Series + } + machineParams.Base, err = baseFromOperatingSystem(opSys) + if err != nil { + return nil, err } - machineParams.Base = ¶msBase - addMachineArgs := []params.AddMachineParams{machineParams} machines, err := machineAPIClient.AddMachines(addMachineArgs) + if err != nil { + return nil, err + } + if machines[0].Error != nil { + return nil, machines[0].Error + } return &CreateMachineResponse{ - Machine: machines[0], - Series: input.Series, + ID: machines[0].Machine, + Base: input.Base, + Series: input.Series, }, err } -// manualProvision calls the sshprovisioner.ProvisionMachine on the Juju side to provision an -// existing machine using ssh_address, public_key and private_key in the CreateMachineInput +func baseFromOperatingSystem(opSys string) (*params.Base, error) { + if opSys == "" { + return nil, nil + } + // opSys is a base or a series, check base first. + info, err := series.ParseBaseFromString(opSys) + if err != nil { + info, err = series.GetBaseFromSeries(opSys) + if err != nil { + return nil, errors.NotValidf("Base or Series %q", opSys) + } + } + base := ¶ms.Base{ + Name: info.Name, + Channel: info.Channel.String(), + } + base.Channel = series.FromLegacyCentosChannel(base.Channel) + return base, nil +} + +// manualProvision calls the sshprovisioner.ProvisionMachine on the Juju side +// to provision an existing machine using ssh_address, public_key and +// private_key in the CreateMachineInput. func manualProvision(client manual.ProvisioningClientAPI, config *config.Config, sshAddress string, publicKey string, - privateKey string) (string, string, error) { + privateKey string) (*CreateMachineResponse, error) { // Read the public keys cmdCtx, err := cmd.DefaultContext() if err != nil { - return "", "", errors.Trace(err) + return nil, errors.Trace(err) } authKeys, err := common.ReadAuthorizedKeys(cmdCtx, publicKey) if err != nil { - return "", "", errors.Annotatef(err, "cannot read authorized-keys from : %v", publicKey) + return nil, errors.Annotatef(err, "cannot read authorized-keys from : %v", publicKey) } // Extract the user and host in the SSHAddress @@ -179,7 +196,7 @@ func manualProvision(client manual.ProvisioningClientAPI, if at := strings.Index(sshAddress, "@"); at != -1 { user, host = sshAddress[:at], sshAddress[at+1:] } else { - return "", "", errors.Errorf("invalid ssh_address, expected , "+ + return nil, errors.Errorf("invalid ssh_address, expected , "+ "given %v", sshAddress) } @@ -199,19 +216,22 @@ func manualProvision(client manual.ProvisioningClientAPI, }, } - // Call the ProvisionMachine + // Call ProvisionMachine machineId, err := sshprovisioner.ProvisionMachine(provisionArgs) if err != nil { - return "", "", errors.Trace(err) + return nil, errors.Trace(err) } // Find out about the series of the machine just provisioned // (because ProvisionMachine only returns machineId) - _, series, err := sshprovisioner.DetectSeriesAndHardwareCharacteristics(host) + _, machineSeries, err := sshprovisioner.DetectSeriesAndHardwareCharacteristics(host) if err != nil { - return "", "", errors.Annotatef(err, "error detecting linux hardware characteristics") + return nil, errors.Annotatef(err, "error detecting hardware characteristics") } - return series, machineId, nil + return &CreateMachineResponse{ + ID: machineId, + Series: machineSeries, + }, nil } func (c machinesClient) ReadMachine(input ReadMachineInput) (ReadMachineResponse, error) { @@ -228,15 +248,15 @@ func (c machinesClient) ReadMachine(input ReadMachineInput) (ReadMachineResponse if err != nil { return response, err } - // TODO: hml 14-Aug-2023 - // Do not leak juju api structures into the provider code. - var machineStatus params.MachineStatus - var exists bool - if machineStatus, exists = status.Machines[input.MachineId]; !exists { - return response, fmt.Errorf("no status returned for machine: %s", input.MachineId) + + machineStatus, exists := status.Machines[input.ID] + if !exists { + return response, fmt.Errorf("no status returned for machine: %s", input.ID) } - response.MachineId = machineStatus.Id - response.MachineStatus = machineStatus + response.ID = machineStatus.Id + response.Base = fmt.Sprintf("%s@%s", machineStatus.Base.Name, machineStatus.Base.Channel) + response.Series = machineStatus.Series + response.Constraints = machineStatus.Constraints return response, nil } @@ -248,9 +268,9 @@ func (c machinesClient) DestroyMachine(input *DestroyMachineInput) error { } machineAPIClient := apimachinemanager.NewClient(conn) - defer machineAPIClient.Close() + defer func() { _ = machineAPIClient.Close() }() - _, err = machineAPIClient.DestroyMachinesWithParams(false, false, (*time.Duration)(nil), input.MachineId) + _, err = machineAPIClient.DestroyMachinesWithParams(false, false, (*time.Duration)(nil), input.ID) if err != nil { return err diff --git a/internal/provider/data_source_machine.go b/internal/provider/data_source_machine.go index 3f289277..05d560b0 100644 --- a/internal/provider/data_source_machine.go +++ b/internal/provider/data_source_machine.go @@ -112,7 +112,7 @@ func (d *machineDataSource) Read(ctx context.Context, req datasource.ReadRequest if _, err := d.client.Machines.ReadMachine( juju.ReadMachineInput{ ModelName: data.Model.ValueString(), - MachineId: machine_id, + ID: machine_id, }, ); err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to read machine %q, got error: %s", machine_id, err)) diff --git a/internal/provider/resource_machine.go b/internal/provider/resource_machine.go index cde18180..e99a7d59 100644 --- a/internal/provider/resource_machine.go +++ b/internal/provider/resource_machine.go @@ -260,20 +260,16 @@ func (r *machineResource) Create(ctx context.Context, req resource.CreateRequest resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create machine, got error: %s", err)) return } - if response.Machine.Error != nil { - resp.Diagnostics.AddError("Juju Error", fmt.Sprintf("Failed to create machine, got error: %s", err)) - return - } - r.trace(fmt.Sprintf("create machine resource %q", response.Machine.Machine)) + r.trace(fmt.Sprintf("create machine resource %q", response.ID)) machineName := data.Name.ValueString() if machineName == "" { - machineName = fmt.Sprintf("machine-%s", response.Machine.Machine) + machineName = fmt.Sprintf("machine-%s", response.ID) } - id := newMachineID(data.ModelName.ValueString(), response.Machine.Machine, machineName) + id := newMachineID(data.ModelName.ValueString(), response.ID, machineName) data.ID = types.StringValue(id) - data.MachineID = types.StringValue(response.Machine.Machine) + data.MachineID = types.StringValue(response.ID) data.Series = types.StringValue(response.Series) data.Name = types.StringValue(machineName) resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) @@ -323,7 +319,7 @@ func (r *machineResource) Read(ctx context.Context, req resource.ReadRequest, re response, err := r.client.Machines.ReadMachine(juju.ReadMachineInput{ ModelName: modelName, - MachineId: machineID, + ID: machineID, }) if err != nil { resp.Diagnostics.Append(handleMachineNotFoundError(ctx, err, &resp.State)...) @@ -334,9 +330,9 @@ func (r *machineResource) Read(ctx context.Context, req resource.ReadRequest, re data.Name = types.StringValue(machineName) data.ModelName = types.StringValue(modelName) data.MachineID = types.StringValue(machineID) - data.Series = types.StringValue(response.MachineStatus.Series) - if response.MachineStatus.Constraints != "" { - data.Constraints = types.StringValue(response.MachineStatus.Constraints) + data.Series = types.StringValue(response.Series) + if response.Constraints != "" { + data.Constraints = types.StringValue(response.Constraints) } resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) } @@ -404,7 +400,7 @@ func (r *machineResource) Delete(ctx context.Context, req resource.DeleteRequest if err := r.client.Machines.DestroyMachine(&juju.DestroyMachineInput{ ModelName: modelName, - MachineId: machineID, + ID: machineID, }); err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to delete machine, got error: %s", err)) } From 77cd9e4cbd40b5694ae988a636d6b3288817e0b1 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 27 Sep 2023 11:08:28 -0400 Subject: [PATCH 2/4] Add Base to machine resource schema. Series is now deprecated and will be removed in the 1.0.0 release. Including a new string validator to cover base. This is a feature request and necessary step to move from juju 2.9 to juju 3.x. --- docs/resources/machine.md | 3 +- examples/resources/juju_machine/resource.tf | 2 +- internal/provider/resource_machine.go | 48 ++++++++++++--------- internal/provider/resource_machine_test.go | 12 +++--- internal/provider/validator_base.go | 40 +++++++++++++++++ 5 files changed, 76 insertions(+), 29 deletions(-) create mode 100644 internal/provider/validator_base.go diff --git a/docs/resources/machine.md b/docs/resources/machine.md index aa36a52c..e24760ac 100644 --- a/docs/resources/machine.md +++ b/docs/resources/machine.md @@ -30,12 +30,13 @@ resource "juju_machine" "this_machine" { ### Optional +- `base` (String) The operating system series to install on the new machine(s). - `constraints` (String) Machine constraints that overwrite those available from 'juju get-model-constraints' and provider's defaults. - `disks` (String) Storage constraints for disks to attach to the machine(s). - `name` (String) A name for the machine resource in Terraform. - `private_key_file` (String) The file path to read the private key from. - `public_key_file` (String) The file path to read the public key from. -- `series` (String) The operating system series to install on the new machine(s). +- `series` (String, Deprecated) The operating system series to install on the new machine(s). - `ssh_address` (String) The user@host directive for manual provisioning an existing machine via ssh. Requires public_key_file & private_key_file arguments. ### Read-Only diff --git a/examples/resources/juju_machine/resource.tf b/examples/resources/juju_machine/resource.tf index b5a0faee..731b3eab 100644 --- a/examples/resources/juju_machine/resource.tf +++ b/examples/resources/juju_machine/resource.tf @@ -1,6 +1,6 @@ resource "juju_machine" "this_machine" { model = juju_model.development.name - series = "focal" + base = "ubuntu@22.04" name = "this_machine" constraints = "tags=my-machine-tag" } \ No newline at end of file diff --git a/internal/provider/resource_machine.go b/internal/provider/resource_machine.go index e99a7d59..7de167f9 100644 --- a/internal/provider/resource_machine.go +++ b/internal/provider/resource_machine.go @@ -44,6 +44,7 @@ type machineResourceModel struct { ModelName types.String `tfsdk:"model"` Constraints types.String `tfsdk:"constraints"` Disks types.String `tfsdk:"disks"` + Base types.String `tfsdk:"base"` Series types.String `tfsdk:"series"` MachineID types.String `tfsdk:"machine_id"` SSHAddress types.String `tfsdk:"ssh_address"` @@ -86,6 +87,7 @@ const ( ConstraintsKey = "constraints" DisksKey = "disks" SeriesKey = "series" + BaseKey = "base" MachineIDKey = "machine_id" SSHAddressKey = "ssh_address" PrivateKeyFileKey = "private_key_file" @@ -135,6 +137,22 @@ func (r *machineResource) Schema(_ context.Context, req resource.SchemaRequest, }...), }, }, + BaseKey: schema.StringAttribute{ + Description: "The operating system series to install on the new machine(s).", + Optional: true, + Computed: true, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.RequiresReplaceIfConfigured(), + stringplanmodifier.UseStateForUnknown(), + }, + Validators: []validator.String{ + stringvalidator.ConflictsWith(path.Expressions{ + path.MatchRoot(SSHAddressKey), + path.MatchRoot(SeriesKey), + }...), + stringIsBaseValidator{}, + }, + }, SeriesKey: schema.StringAttribute{ Description: "The operating system series to install on the new machine(s).", Optional: true, @@ -146,8 +164,10 @@ func (r *machineResource) Schema(_ context.Context, req resource.SchemaRequest, Validators: []validator.String{ stringvalidator.ConflictsWith(path.Expressions{ path.MatchRoot(SSHAddressKey), + path.MatchRoot(BaseKey), }...), }, + DeprecationMessage: "Configure base instead. This attribute will be removed in the next major version of the provider.", }, MachineIDKey: schema.StringAttribute{ Description: "The id of the machine Juju creates.", @@ -165,6 +185,7 @@ func (r *machineResource) Schema(_ context.Context, req resource.SchemaRequest, Validators: []validator.String{ stringvalidator.ConflictsWith(path.Expressions{ path.MatchRoot(SeriesKey), + path.MatchRoot(BaseKey), path.MatchRoot(ConstraintsKey), }...), stringvalidator.AlsoRequires(path.Expressions{ @@ -229,30 +250,13 @@ func (r *machineResource) Create(ctx context.Context, req resource.CreateRequest return } - modelInfo, err := r.client.Models.GetModelByName(data.ModelName.ValueString()) - if err != nil { - resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to get model info on %q, got error: %s", data.ModelName.ValueString(), err)) - return - } - - series := data.Series.ValueString() - sshAddress := data.SSHAddress.ValueString() - - if sshAddress == "" { - // If not provisioning manually, check the series argument. - // If not set, get it from the model default. - // TODO (cderici): revisit this part when switched to juju 3.x. - if series == "" { - series = modelInfo.DefaultSeries - } - } - response, err := r.client.Machines.CreateMachine(&juju.CreateMachineInput{ Constraints: data.Constraints.ValueString(), - ModelName: modelInfo.Name, + ModelName: data.ModelName.ValueString(), Disks: data.Disks.ValueString(), - Series: series, - SSHAddress: sshAddress, + Base: data.Base.ValueString(), + Series: data.Series.ValueString(), + SSHAddress: data.SSHAddress.ValueString(), PublicKeyFile: data.PublicKeyFile.ValueString(), PrivateKeyFile: data.PrivateKeyFile.ValueString(), }) @@ -270,6 +274,7 @@ func (r *machineResource) Create(ctx context.Context, req resource.CreateRequest id := newMachineID(data.ModelName.ValueString(), response.ID, machineName) data.ID = types.StringValue(id) data.MachineID = types.StringValue(response.ID) + data.Base = types.StringValue(response.Base) data.Series = types.StringValue(response.Series) data.Name = types.StringValue(machineName) resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) @@ -331,6 +336,7 @@ func (r *machineResource) Read(ctx context.Context, req resource.ReadRequest, re data.ModelName = types.StringValue(modelName) data.MachineID = types.StringValue(machineID) data.Series = types.StringValue(response.Series) + data.Base = types.StringValue(response.Base) if response.Constraints != "" { data.Constraints = types.StringValue(response.Constraints) } diff --git a/internal/provider/resource_machine_test.go b/internal/provider/resource_machine_test.go index d131f0d9..e0ea8c9a 100644 --- a/internal/provider/resource_machine_test.go +++ b/internal/provider/resource_machine_test.go @@ -21,11 +21,11 @@ func TestAcc_ResourceMachine_Edge(t *testing.T) { ProtoV6ProviderFactories: frameworkProviderFactories, Steps: []resource.TestStep{ { - Config: testAccResourceMachine(modelName), + Config: testAccResourceMachine(modelName, "base = \"ubuntu@22.04\""), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("juju_machine.this", "model", modelName), resource.TestCheckResourceAttr("juju_machine.this", "name", "this_machine"), - resource.TestCheckResourceAttr("juju_machine.this", "series", "focal"), + resource.TestCheckResourceAttr("juju_machine.this", "base", "ubuntu@22.04"), ), }, { @@ -90,7 +90,7 @@ func TestAcc_ResourceMachine_Stable(t *testing.T) { }, Steps: []resource.TestStep{ { - Config: testAccResourceMachine(modelName), + Config: testAccResourceMachine(modelName, "series = \"focal\""), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("juju_machine.this", "model", modelName), resource.TestCheckResourceAttr("juju_machine.this", "name", "this_machine"), @@ -106,7 +106,7 @@ func TestAcc_ResourceMachine_Stable(t *testing.T) { }) } -func testAccResourceMachine(modelName string) string { +func testAccResourceMachine(modelName, operatingSystem string) string { return fmt.Sprintf(` resource "juju_model" "this" { name = %q @@ -115,9 +115,9 @@ resource "juju_model" "this" { resource "juju_machine" "this" { name = "this_machine" model = juju_model.this.name - series = "focal" + %s } -`, modelName) +`, modelName, operatingSystem) } func TestAcc_ResourceMachine_AddMachine_Edge(t *testing.T) { diff --git a/internal/provider/validator_base.go b/internal/provider/validator_base.go new file mode 100644 index 00000000..a37a120f --- /dev/null +++ b/internal/provider/validator_base.go @@ -0,0 +1,40 @@ +// Copyright 2023 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package provider + +import ( + "context" + + "github.com/hashicorp/terraform-plugin-framework/schema/validator" + "github.com/juju/juju/core/series" +) + +type stringIsBaseValidator struct{} + +// Description returns a plain text description of the validator's behavior, suitable for a practitioner to understand its impact. +func (v stringIsBaseValidator) Description(context.Context) string { + return "string must conform to name@channel, e.g. ubuntu@22.04" +} + +// MarkdownDescription returns a markdown formatted description of the validator's behavior, suitable for a practitioner to understand its impact. +func (v stringIsBaseValidator) MarkdownDescription(context.Context) string { + return "string must conform to name@channel, e.g. ubuntu@22.04" +} + +// Validate runs the main validation logic of the validator, reading configuration data out of `req` and updating `resp` with diagnostics. +func (v stringIsBaseValidator) ValidateString(_ context.Context, req validator.StringRequest, resp *validator.StringResponse) { + // If the value is unknown or null, there is nothing to validate. + if req.ConfigValue.IsUnknown() || req.ConfigValue.IsNull() { + return + } + + if _, err := series.ParseBaseFromString(req.ConfigValue.ValueString()); err != nil { + resp.Diagnostics.AddAttributeError( + req.Path, + "Invalid Base", + "String must conform to name@channel, e.g. ubuntu@22.04", + ) + return + } +} From 268d0e9ee2b8878523a3e659757bfec8656ad868 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 27 Sep 2023 16:26:14 -0400 Subject: [PATCH 3/4] Doc update based on changed machine resource example --- docs/resources/machine.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/resources/machine.md b/docs/resources/machine.md index e24760ac..5abfd5b6 100644 --- a/docs/resources/machine.md +++ b/docs/resources/machine.md @@ -15,7 +15,7 @@ A resource that represents a Juju machine deployment. Refer to the juju add-mach ```terraform resource "juju_machine" "this_machine" { model = juju_model.development.name - series = "focal" + base = "ubuntu@22.04" name = "this_machine" constraints = "tags=my-machine-tag" } From 2f51cace66819949af827b8e800de890d6babb4f Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 27 Sep 2023 16:42:48 -0400 Subject: [PATCH 4/4] Only write the machine base's channel track. Juju internals has a base a name@track/risk. However no one else uses them that way. Usually it's name@track, e.g ubuntu@22.04. There are no other risks for operating systems at this time. Added a caveat emptor for future people just in case. --- internal/juju/machines.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/juju/machines.go b/internal/juju/machines.go index db5d7169..19869819 100644 --- a/internal/juju/machines.go +++ b/internal/juju/machines.go @@ -254,7 +254,14 @@ func (c machinesClient) ReadMachine(input ReadMachineInput) (ReadMachineResponse return response, fmt.Errorf("no status returned for machine: %s", input.ID) } response.ID = machineStatus.Id - response.Base = fmt.Sprintf("%s@%s", machineStatus.Base.Name, machineStatus.Base.Channel) + channel, err := series.ParseChannel(machineStatus.Base.Channel) + if err != nil { + return response, err + } + // This might cause problems later, but today, no one except for juju internals + // uses the channel risk. Using the risk makes the base appear to have changed + // with terraform. + response.Base = fmt.Sprintf("%s@%s", machineStatus.Base.Name, channel.Track) response.Series = machineStatus.Series response.Constraints = machineStatus.Constraints