From f4031ef6681b79bf56bf7a5b426b1e57a0f52c6a Mon Sep 17 00:00:00 2001 From: Matt White <16320656+matt-FFFFFF@users.noreply.github.com> Date: Tue, 25 Feb 2025 04:53:13 +0000 Subject: [PATCH] feat: add skip external request (#786) * feat: add skip external request * refactor: move skip to internal * test: fix test --- README.md | 14 +- .../services/azapi_data_plane_resource.go | 30 ++- internal/services/azapi_resource.go | 31 ++- .../azapi_resource_action_resource.go | 26 ++- internal/services/azapi_update_resource.go | 24 +- internal/skip/skip.go | 57 +++++ internal/skip/skip_test.go | 205 ++++++++++++++++++ 7 files changed, 358 insertions(+), 29 deletions(-) create mode 100644 internal/skip/skip.go create mode 100644 internal/skip/skip_test.go diff --git a/README.md b/README.md index 84376233a..f4d68104c 100644 --- a/README.md +++ b/README.md @@ -163,13 +163,13 @@ Please ensure that the `MarkdownDescription` field is set in the schema for each To generate the documentation run either: ```sh -$ make docs +make docs ``` or... ```sh -$ go generate ./... +go generate ./... ``` ### Templates @@ -210,6 +210,16 @@ provider_installation { } ``` +## Developer: Using the `skip_on` struct field tag + +The `skip_on` struct field tag is used to skip the external API call when only attributes that affect the internal state are modified, e.g. retry configuration. The `skip_on` struct field tag is used to skip the external API call when only attributes that affect the internal state are modified, e.g. retry configuration. The `skip_on` struct field tag is a comma-separated list of operations that must be met in order to skip the field. + +The provider will compare the state with the plan, and check for changes. If the only fields to me modified are those with the `skip_on` struct field tag set to the supplied operation, e.g. `update`, the provider will skip the external API call. + +The following operations are supported: + +* `update` - Skip the external API call when the operation is an update. + ## Credits We wish to thank HashiCorp for the use of some MPLv2-licensed code from their open source project [terraform-provider-azurerm](https://github.com/hashicorp/terraform-provider-azurerm). diff --git a/internal/services/azapi_data_plane_resource.go b/internal/services/azapi_data_plane_resource.go index cfe004024..65235060a 100644 --- a/internal/services/azapi_data_plane_resource.go +++ b/internal/services/azapi_data_plane_resource.go @@ -19,6 +19,7 @@ import ( "github.com/Azure/terraform-provider-azapi/internal/services/myplanmodifier/planmodifierdynamic" "github.com/Azure/terraform-provider-azapi/internal/services/myvalidator" "github.com/Azure/terraform-provider-azapi/internal/services/parse" + "github.com/Azure/terraform-provider-azapi/internal/skip" "github.com/Azure/terraform-provider-azapi/internal/tf" "github.com/Azure/terraform-provider-azapi/utils" "github.com/cenkalti/backoff/v4" @@ -49,18 +50,18 @@ type DataPlaneResourceModel struct { ReplaceTriggersExternalValues types.Dynamic `tfsdk:"replace_triggers_external_values"` ReplaceTriggersRefs types.List `tfsdk:"replace_triggers_refs"` ResponseExportValues types.Dynamic `tfsdk:"response_export_values"` - Retry retry.RetryValue `tfsdk:"retry"` + Retry retry.RetryValue `tfsdk:"retry" skip_on:"update"` Locks types.List `tfsdk:"locks"` Output types.Dynamic `tfsdk:"output"` - Timeouts timeouts.Value `tfsdk:"timeouts"` - CreateHeaders types.Map `tfsdk:"create_headers"` - CreateQueryParameters types.Map `tfsdk:"create_query_parameters"` + Timeouts timeouts.Value `tfsdk:"timeouts" skip_on:"update"` + CreateHeaders types.Map `tfsdk:"create_headers" skip_on:"update"` + CreateQueryParameters types.Map `tfsdk:"create_query_parameters" skip_on:"update"` UpdateHeaders types.Map `tfsdk:"update_headers"` UpdateQueryParameters types.Map `tfsdk:"update_query_parameters"` - DeleteHeaders types.Map `tfsdk:"delete_headers"` - DeleteQueryParameters types.Map `tfsdk:"delete_query_parameters"` - ReadHeaders types.Map `tfsdk:"read_headers"` - ReadQueryParameters types.Map `tfsdk:"read_query_parameters"` + DeleteHeaders types.Map `tfsdk:"delete_headers" skip_on:"update"` + DeleteQueryParameters types.Map `tfsdk:"delete_query_parameters" skip_on:"update"` + ReadHeaders types.Map `tfsdk:"read_headers" skip_on:"update"` + ReadQueryParameters types.Map `tfsdk:"read_query_parameters" skip_on:"update"` } type DataPlaneResource struct { @@ -357,6 +358,19 @@ func (r *DataPlaneResource) Create(ctx context.Context, request resource.CreateR } func (r *DataPlaneResource) Update(ctx context.Context, request resource.UpdateRequest, response *resource.UpdateResponse) { + // See if we can skip the external API call (changes are to state only) + var state, plan DataPlaneResourceModel + if response.Diagnostics.Append(request.Plan.Get(ctx, &plan)...); response.Diagnostics.HasError() { + return + } + if response.Diagnostics.Append(request.State.Get(ctx, &state)...); response.Diagnostics.HasError() { + return + } + if skip.CanSkipExternalRequest(state, plan, "update") { + tflog.Debug(ctx, "azapi_resource.CreateUpdate skipping external request as no unskippable changes were detected") + response.Diagnostics.Append(response.State.Set(ctx, plan)...) + } + tflog.Debug(ctx, "azapi_resource.CreateUpdate proceeding with external request as no skippable changes were detected") r.CreateUpdate(ctx, request.Plan, &response.State, &response.Diagnostics) } diff --git a/internal/services/azapi_resource.go b/internal/services/azapi_resource.go index 51c83bbfe..d6bae3f39 100644 --- a/internal/services/azapi_resource.go +++ b/internal/services/azapi_resource.go @@ -27,6 +27,7 @@ import ( "github.com/Azure/terraform-provider-azapi/internal/services/myvalidator" "github.com/Azure/terraform-provider-azapi/internal/services/parse" "github.com/Azure/terraform-provider-azapi/internal/services/preflight" + "github.com/Azure/terraform-provider-azapi/internal/skip" "github.com/Azure/terraform-provider-azapi/internal/tf" "github.com/Azure/terraform-provider-azapi/utils" "github.com/cenkalti/backoff/v4" @@ -63,19 +64,19 @@ type AzapiResourceModel struct { ReplaceTriggersExternalValues types.Dynamic `tfsdk:"replace_triggers_external_values"` ReplaceTriggersRefs types.List `tfsdk:"replace_triggers_refs"` ResponseExportValues types.Dynamic `tfsdk:"response_export_values"` - Retry retry.RetryValue `tfsdk:"retry"` + Retry retry.RetryValue `tfsdk:"retry" skip_on:"update"` SchemaValidationEnabled types.Bool `tfsdk:"schema_validation_enabled"` Tags types.Map `tfsdk:"tags"` - Timeouts timeouts.Value `tfsdk:"timeouts"` + Timeouts timeouts.Value `tfsdk:"timeouts" skip_on:"update"` Type types.String `tfsdk:"type"` - CreateHeaders types.Map `tfsdk:"create_headers"` - CreateQueryParameters types.Map `tfsdk:"create_query_parameters"` + CreateHeaders types.Map `tfsdk:"create_headers" skip_on:"update"` + CreateQueryParameters types.Map `tfsdk:"create_query_parameters" skip_on:"update"` UpdateHeaders types.Map `tfsdk:"update_headers"` UpdateQueryParameters types.Map `tfsdk:"update_query_parameters"` - DeleteHeaders types.Map `tfsdk:"delete_headers"` - DeleteQueryParameters types.Map `tfsdk:"delete_query_parameters"` - ReadHeaders types.Map `tfsdk:"read_headers"` - ReadQueryParameters types.Map `tfsdk:"read_query_parameters"` + DeleteHeaders types.Map `tfsdk:"delete_headers" skip_on:"update"` + DeleteQueryParameters types.Map `tfsdk:"delete_query_parameters" skip_on:"update"` + ReadHeaders types.Map `tfsdk:"read_headers" skip_on:"update"` + ReadQueryParameters types.Map `tfsdk:"read_query_parameters" skip_on:"update"` } var _ resource.Resource = &AzapiResource{} @@ -588,6 +589,20 @@ func (r *AzapiResource) Create(ctx context.Context, request resource.CreateReque } func (r *AzapiResource) Update(ctx context.Context, request resource.UpdateRequest, response *resource.UpdateResponse) { + // See if we can skip the external API call (changes are to state only) + var plan, state AzapiResourceModel + if response.Diagnostics.Append(request.Plan.Get(ctx, &plan)...); response.Diagnostics.HasError() { + return + } + if response.Diagnostics.Append(request.State.Get(ctx, &state)...); response.Diagnostics.HasError() { + return + } + if skip.CanSkipExternalRequest(plan, state, "update") { + response.Diagnostics.Append(response.State.Set(ctx, plan)...) + tflog.Debug(ctx, "azapi_resource.CreateUpdate skipping external request as no unskippable changes were detected") + return + } + tflog.Debug(ctx, "azapi_resource.CreateUpdate proceeding with external request as no skippable changes were detected") r.CreateUpdate(ctx, request.Plan, &response.State, &response.Diagnostics) } diff --git a/internal/services/azapi_resource_action_resource.go b/internal/services/azapi_resource_action_resource.go index be1dd1924..a0a05d53b 100644 --- a/internal/services/azapi_resource_action_resource.go +++ b/internal/services/azapi_resource_action_resource.go @@ -16,6 +16,7 @@ import ( "github.com/Azure/terraform-provider-azapi/internal/services/myplanmodifier" "github.com/Azure/terraform-provider-azapi/internal/services/myvalidator" "github.com/Azure/terraform-provider-azapi/internal/services/parse" + "github.com/Azure/terraform-provider-azapi/internal/skip" "github.com/cenkalti/backoff/v4" "github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts" "github.com/hashicorp/terraform-plugin-framework-validators/listvalidator" @@ -45,8 +46,8 @@ type ActionResourceModel struct { SensitiveResponseExportValues types.Dynamic `tfsdk:"sensitive_response_export_values"` Output types.Dynamic `tfsdk:"output"` SensitiveOutput types.Dynamic `tfsdk:"sensitive_output"` - Timeouts timeouts.Value `tfsdk:"timeouts"` - Retry retry.RetryValue `tfsdk:"retry"` + Timeouts timeouts.Value `tfsdk:"timeouts" skip_on:"update"` + Retry retry.RetryValue `tfsdk:"retry" skip_on:"update"` Headers types.Map `tfsdk:"headers"` QueryParameters types.Map `tfsdk:"query_parameters"` } @@ -280,20 +281,31 @@ func (r *ActionResource) Create(ctx context.Context, request resource.CreateRequ } func (r *ActionResource) Update(ctx context.Context, request resource.UpdateRequest, response *resource.UpdateResponse) { - var model ActionResourceModel - if response.Diagnostics.Append(request.Plan.Get(ctx, &model)...); response.Diagnostics.HasError() { + var state, plan ActionResourceModel + if response.Diagnostics.Append(request.Plan.Get(ctx, &plan)...); response.Diagnostics.HasError() { + return + } + if response.Diagnostics.Append(request.State.Get(ctx, &state)...); response.Diagnostics.HasError() { return } - timeout, diags := model.Timeouts.Update(ctx, 30*time.Minute) + timeout, diags := plan.Timeouts.Update(ctx, 30*time.Minute) if response.Diagnostics.Append(diags...); response.Diagnostics.HasError() { return } ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() - if model.When.ValueString() == "apply" { - r.Action(ctx, model, &response.State, &response.Diagnostics) + // See if we can skip the external API call (changes are to state only) + if skip.CanSkipExternalRequest(state, plan, "update") { + tflog.Debug(ctx, "azapi_resource.CreateUpdate skipping external request as no unskippable changes were detected") + response.Diagnostics.Append(response.State.Set(ctx, plan)...) + return + } + tflog.Debug(ctx, "azapi_resource.CreateUpdate proceeding with external request as no skippable changes were detected") + + if plan.When.ValueString() == "apply" { + r.Action(ctx, plan, &response.State, &response.Diagnostics) } } diff --git a/internal/services/azapi_update_resource.go b/internal/services/azapi_update_resource.go index 554a5bb82..71ce7f189 100644 --- a/internal/services/azapi_update_resource.go +++ b/internal/services/azapi_update_resource.go @@ -17,6 +17,7 @@ import ( "github.com/Azure/terraform-provider-azapi/internal/services/myplanmodifier" "github.com/Azure/terraform-provider-azapi/internal/services/myvalidator" "github.com/Azure/terraform-provider-azapi/internal/services/parse" + "github.com/Azure/terraform-provider-azapi/internal/skip" "github.com/Azure/terraform-provider-azapi/utils" "github.com/cenkalti/backoff/v4" "github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts" @@ -45,12 +46,12 @@ type AzapiUpdateResourceModel struct { ResponseExportValues types.Dynamic `tfsdk:"response_export_values"` Locks types.List `tfsdk:"locks"` Output types.Dynamic `tfsdk:"output"` - Timeouts timeouts.Value `tfsdk:"timeouts"` - Retry retry.RetryValue `tfsdk:"retry"` + Timeouts timeouts.Value `tfsdk:"timeouts" skip_on:"update"` + Retry retry.RetryValue `tfsdk:"retry" skip_on:"update"` UpdateHeaders types.Map `tfsdk:"update_headers"` UpdateQueryParameters types.Map `tfsdk:"update_query_parameters"` - ReadHeaders types.Map `tfsdk:"read_headers"` - ReadQueryParameters types.Map `tfsdk:"read_query_parameters"` + ReadHeaders types.Map `tfsdk:"read_headers" skip_on:"update"` + ReadQueryParameters types.Map `tfsdk:"read_query_parameters" skip_on:"update"` } type AzapiUpdateResource struct { @@ -302,6 +303,21 @@ func (r *AzapiUpdateResource) Create(ctx context.Context, request resource.Creat } func (r *AzapiUpdateResource) Update(ctx context.Context, request resource.UpdateRequest, response *resource.UpdateResponse) { + // See if we can skip the external API call (changes are to state only) + var plan, state AzapiUpdateResourceModel + if response.Diagnostics.Append(request.Plan.Get(ctx, &plan)...); response.Diagnostics.HasError() { + return + } + if response.Diagnostics.Append(request.State.Get(ctx, &state)...); response.Diagnostics.HasError() { + return + } + if skip.CanSkipExternalRequest(plan, state, "update") { + tflog.Debug(ctx, "azapi_resource.CreateUpdate skipping external request as no unskippable changes were detected") + response.Diagnostics.Append(request.State.Set(ctx, plan)...) + return + } + tflog.Debug(ctx, "azapi_resource.CreateUpdate proceeding with external request as no skippable changes were detected") + r.CreateUpdate(ctx, request.Plan, &response.State, &response.Diagnostics) } diff --git a/internal/skip/skip.go b/internal/skip/skip.go new file mode 100644 index 000000000..1b62771fb --- /dev/null +++ b/internal/skip/skip.go @@ -0,0 +1,57 @@ +package skip + +import ( + "reflect" + "slices" + "strings" +) + +// CanSkipExternalRequest checks if the external request can be skipped based on the plan and state. +// Two of the same objects are supplied as parameters, together with the operation that is being performed. +// The function uses the `skip_on` struct tag to determine if the field should be skipped. +// The value of the `skip_on` tag is a comma-separated list of operations that mean that changes to this field value do not require an external request and are in state only. +// The function will return true if the external request can be skipped, false otherwise. +func CanSkipExternalRequest[T any](a, b T, operation string) bool { + valA := reflect.ValueOf(a) + valB := reflect.ValueOf(b) + + // Since we are using generics, we know that the types of a and b are the same. + // Therefore we can check the type of a to determine if it is a struct. + if valA.Kind() != reflect.Struct { + return false + } + + typeOfA := valA.Type() + // iterate over all fields of the struct + for i := 0; i < typeOfA.NumField(); i++ { + field := typeOfA.Field(i) + // Check if the field has the skip_on tag + // If it doesn't we need to compare the valued as we cannot determine if the field should be skipped. + // If the field has the skip_on tag, we can check if the operation is in the list of operations that should be skipped. + tag := field.Tag.Get("skip_on") + if tag != "" { + // Split the tag values by comma and check if the operation is in the list. + // If the operation is in the list, then this field represents a change in state only + // and does not require an external request to be made. + // Therefore we can skip tp the next field. + tagValues := strings.Split(tag, ",") + if slices.Contains(tagValues, operation) { + continue + } + } + + // If we get here then we need to compare the field values. + // By now we have determined that the struct fields do not have a valid skip value for this operation. + // Therefore if the field values are not equal, then the external request cannot be skipped. + fieldA := valA.Field(i) + fieldB := valB.Field(i) + + if !fieldA.IsValid() || !fieldB.IsValid() { + return false + } + if !reflect.DeepEqual(fieldA.Interface(), fieldB.Interface()) { + return false + } + } + return true +} diff --git a/internal/skip/skip_test.go b/internal/skip/skip_test.go new file mode 100644 index 000000000..241de402f --- /dev/null +++ b/internal/skip/skip_test.go @@ -0,0 +1,205 @@ +package skip + +import ( + "testing" + + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-framework/types/basetypes" +) + +type SkipTestType1 struct { + SkipOnUpdate types.String `skip_on:"update"` + NoSkip types.String // no skip_on tag + SkipOnCreateRead types.String `tfsdk:"field_3" skip_on:"create,read"` +} + +func TestCanSkipExternalRequest(t *testing.T) { + testCases := []struct { + desc string + a SkipTestType1 + b SkipTestType1 + operation string + result bool + }{ + { + desc: "skip on update", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1_updated"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "update", + result: true, + }, + { + desc: "do not skip on update, changes to untagged fields", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2_updated"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "update", + result: false, + }, + { + desc: "skip on create", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3_updated"), + }, + operation: "create", + result: true, + }, + { + desc: "do not skip on create, changes to non-skippable fields", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2_updated"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "create", + result: false, + }, + { + desc: "skip on read", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3_updated"), + }, + operation: "read", + result: true, + }, + { + desc: "do not skip on read, changes to non-skippable fields", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2_updated"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "read", + result: false, + }, + { + desc: "skip on update but no changes", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "update", + result: true, + }, + { + desc: "skip on update, changes to read skippable field", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3_updated"), + }, + operation: "update", + result: false, + }, + { + desc: "skip on update, unknown value", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringUnknown(), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringUnknown(), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "update", + result: true, + }, + { + desc: "skip on update, null value", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringNull(), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringNull(), + NoSkip: basetypes.NewStringValue("value2"), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "update", + result: true, + }, + { + desc: "no skip, null and unknown value differ", + a: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringUnknown(), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + b: SkipTestType1{ + SkipOnUpdate: basetypes.NewStringValue("value1"), + NoSkip: basetypes.NewStringNull(), + SkipOnCreateRead: basetypes.NewStringValue("value3"), + }, + operation: "update", + result: false, + }, + } + for _, tC := range testCases { + t.Run(tC.desc, func(t *testing.T) { + if CanSkipExternalRequest(tC.a, tC.b, tC.operation) != tC.result { + t.Errorf("Expected %v, got %v", tC.result, !tC.result) + } + }) + } +} + +func TestCanSkipExternalRequestInvalidInputs(t *testing.T) { + // Test with non-struct values + if CanSkipExternalRequest(1, 2, "update") { + t.Errorf("Expected false, got true") + } + +}