From a0a643e6a38d971421116756b3cfd5697f03e19c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Graber?= Date: Fri, 14 Jun 2024 01:29:44 -0400 Subject: [PATCH 1/2] internal/*: Support NULL values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Terraform allows for configuration to be set to NULL. This is generally assumed to be equivalent to not having put the configuration line in question and is particularly useful when dealing with conditional behaviors. A couple of examples: ``` resource "lxd_network" "this" { name = "foo" config = { "ipv4.address" = var.deploy == "production" ? "none" : null } } resource "lxd_profile" "this" { name = "foo" config = { "environment.http_proxy" = var.deploy == "production" ? "http://my-proxy:3128" : null "environment.https_proxy" = var.deploy == "production" ? "http://my-proxy:3128" : null } } ``` To handle this properly, a few changes are needed: - Read the Terraform config as a map[string]*string to allow for NULL - When set to NULL and config key doesn't exist, tell Terraform it's NULL - When set to NULL and config key is computed, tell Terraform it's NULL Signed-off-by: Stéphane Graber (cherry picked from commit 5f8effa895a4edb3324402c66276534750b0bb7e) Signed-off-by: Din Music --- internal/common/lxd_config.go | 76 ++++++++++++++++--- internal/instance/resource_instance.go | 11 +-- internal/network/resource_network.go | 7 +- internal/network/resource_network_lb.go | 2 +- internal/network/resource_network_zone.go | 2 +- .../network/resource_network_zone_record.go | 2 +- internal/profile/resource_profile.go | 2 +- internal/project/resource_project.go | 7 +- internal/storage/resource_storage_pool.go | 7 +- internal/storage/resource_storage_volume.go | 7 +- 10 files changed, 83 insertions(+), 40 deletions(-) diff --git a/internal/common/lxd_config.go b/internal/common/lxd_config.go index c12b5360..3511babe 100644 --- a/internal/common/lxd_config.go +++ b/internal/common/lxd_config.go @@ -14,16 +14,61 @@ func ToConfigMap(ctx context.Context, configMap types.Map) (map[string]string, d return make(map[string]string), nil } - config := make(map[string]string, len(configMap.Elements())) - diags := configMap.ElementsAs(ctx, &config, false) - return config, diags + // Convert to an intermediate nullable type. + tfConfig := make(map[string]*string, len(configMap.Elements())) + diags := configMap.ElementsAs(ctx, &tfConfig, false) + if diags != nil { + return nil, diags + } + + // Then convert to our native type. + config := make(map[string]string, len(tfConfig)) + for k, v := range tfConfig { + if v == nil { + continue + } + + config[k] = *v + } + + return config, nil } // ToConfigMapType converts map[string]string into config of type types.Map. -func ToConfigMapType(ctx context.Context, config map[string]string) (types.Map, diag.Diagnostics) { +func ToConfigMapType(ctx context.Context, config map[string]*string, modelConfig types.Map) (types.Map, diag.Diagnostics) { + // Add any missing nil values. + nullConfig := map[string]*string{} + if !modelConfig.IsNull() && !modelConfig.IsUnknown() { + _ = modelConfig.ElementsAs(context.Background(), &nullConfig, false) + } + + for k, v := range nullConfig { + if v != nil { + continue + } + + _, ok := config[k] + if !ok { + config[k] = nil + } + } + return types.MapValueFrom(ctx, types.StringType, config) } +// ToNullableConfig converts map[string]string to map[string]*string. +func ToNullableConfig(config map[string]string) map[string]*string { + nullConfig := make(map[string]*string, len(config)) + + for k := range config { + // Copy the value. + v := string(config[k]) + nullConfig[k] = &v + } + + return nullConfig +} + // MergeConfig merges resource (existing) configuration with user defined // configuration. Non-empty resource config entries that are contained in // the provided computed keys are inserted in the user config. @@ -55,14 +100,20 @@ func MergeConfig(resConfig map[string]string, usrConfig map[string]string, compu // file in order to be able to produce a consistent Terraform plan. If there // is a non-computed-key entry, it will be retained in the configuration and // will trigger an error. -func StripConfig(resConfig map[string]string, usrConfig map[string]string, computedKeys []string) map[string]string { - config := make(map[string]string) +func StripConfig(resConfig map[string]string, modelConfig types.Map, computedKeys []string) map[string]*string { + // Handle nulls in modelConfig. + usrConfig := map[string]*string{} + if !modelConfig.IsNull() && !modelConfig.IsUnknown() { + _ = modelConfig.ElementsAs(context.Background(), &usrConfig, false) + } // Populate empty values from user config, so they do not "disappear" // from the state. + config := make(map[string]*string) + for k, v := range usrConfig { - if v == "" { - config[k] = v + if v == nil { + config[k] = nil } } @@ -76,7 +127,14 @@ func StripConfig(resConfig map[string]string, usrConfig map[string]string, compu _, ok := usrConfig[k] if ok || !isComputedKey(k, computedKeys) { - config[k] = v + if usrConfig[k] == nil && isComputedKey(k, computedKeys) { + // Keep as null. + config[k] = nil + } else { + // Copy the value. + v := string(resConfig[k]) + config[k] = &v + } } } diff --git a/internal/instance/resource_instance.go b/internal/instance/resource_instance.go index c5115368..47781184 100644 --- a/internal/instance/resource_instance.go +++ b/internal/instance/resource_instance.go @@ -1112,26 +1112,23 @@ func (r InstanceResource) SyncState(ctx context.Context, tfState *tfsdk.State, s } // Extract user defined config and merge it with current resource config. - usrConfig, diags := common.ToConfigMap(ctx, m.Config) - respDiags.Append(diags...) - - stateConfig := common.StripConfig(instance.Config, usrConfig, m.ComputedKeys()) + stateConfig := common.StripConfig(instance.Config, m.Config, m.ComputedKeys()) // Extract enteries with "limits." prefix. instanceLimits := make(map[string]string) for k, v := range stateConfig { key, ok := strings.CutPrefix(k, "limits.") if ok { - instanceLimits[key] = v + instanceLimits[key] = *v delete(stateConfig, k) } } // Convert config, limits, profiles, and devices into schema type. - config, diags := common.ToConfigMapType(ctx, stateConfig) + config, diags := common.ToConfigMapType(ctx, stateConfig, m.Config) respDiags.Append(diags...) - limits, diags := common.ToConfigMapType(ctx, instanceLimits) + limits, diags := common.ToConfigMapType(ctx, common.ToNullableConfig(instanceLimits), m.Config) respDiags.Append(diags...) profiles, diags := ToProfileListType(ctx, instance.Profiles) diff --git a/internal/network/resource_network.go b/internal/network/resource_network.go index c12f3fe9..b1ce55ff 100644 --- a/internal/network/resource_network.go +++ b/internal/network/resource_network.go @@ -323,13 +323,10 @@ func (r NetworkResource) SyncState(ctx context.Context, tfState *tfsdk.State, se } // Extract user defined config and merge it with current config state. - usrConfig, diags := common.ToConfigMap(ctx, m.Config) - respDiags.Append(diags...) - - stateConfig := common.StripConfig(network.Config, usrConfig, m.ComputedKeys()) + stateConfig := common.StripConfig(network.Config, m.Config, m.ComputedKeys()) // Convert config state into schema type. - config, diags := common.ToConfigMapType(ctx, stateConfig) + config, diags := common.ToConfigMapType(ctx, stateConfig, m.Config) respDiags.Append(diags...) m.Name = types.StringValue(network.Name) diff --git a/internal/network/resource_network_lb.go b/internal/network/resource_network_lb.go index 04847406..c73e14e1 100644 --- a/internal/network/resource_network_lb.go +++ b/internal/network/resource_network_lb.go @@ -368,7 +368,7 @@ func (r LxdNetworkLBResource) SyncState(ctx context.Context, tfState *tfsdk.Stat ports, diags := ToLBPortSetType(ctx, lb.Ports) respDiags.Append(diags...) - config, diags := common.ToConfigMapType(ctx, lb.Config) + config, diags := common.ToConfigMapType(ctx, common.ToNullableConfig(lb.Config), m.Config) respDiags.Append(diags...) m.Description = types.StringValue(lb.Description) diff --git a/internal/network/resource_network_zone.go b/internal/network/resource_network_zone.go index 83af6de1..7362a41c 100644 --- a/internal/network/resource_network_zone.go +++ b/internal/network/resource_network_zone.go @@ -275,7 +275,7 @@ func (r NetworkZoneResource) SyncState(ctx context.Context, tfState *tfsdk.State } // Convert config state into schema type. - config, diags := common.ToConfigMapType(ctx, zone.Config) + config, diags := common.ToConfigMapType(ctx, common.ToNullableConfig(zone.Config), m.Config) if diags.HasError() { return diags } diff --git a/internal/network/resource_network_zone_record.go b/internal/network/resource_network_zone_record.go index 16b53189..883271f0 100644 --- a/internal/network/resource_network_zone_record.go +++ b/internal/network/resource_network_zone_record.go @@ -341,7 +341,7 @@ func (r NetworkZoneRecordResource) SyncState(ctx context.Context, tfState *tfsdk entries, diags := ToZoneRecordEntrySetType(ctx, record.Entries) respDiags.Append(diags...) - config, diags := common.ToConfigMapType(ctx, record.Config) + config, diags := common.ToConfigMapType(ctx, common.ToNullableConfig(record.Config), m.Config) respDiags.Append(diags...) m.Zone = types.StringValue(zoneName) diff --git a/internal/profile/resource_profile.go b/internal/profile/resource_profile.go index fa7594b5..2675d2c8 100644 --- a/internal/profile/resource_profile.go +++ b/internal/profile/resource_profile.go @@ -326,7 +326,7 @@ func (r ProfileResource) SyncState(ctx context.Context, tfState *tfsdk.State, se } // Convert config state and devices into schema types. - config, diags := common.ToConfigMapType(ctx, profile.Config) + config, diags := common.ToConfigMapType(ctx, common.ToNullableConfig(profile.Config), m.Config) respDiags.Append(diags...) devices, diags := common.ToDeviceSetType(ctx, profile.Devices) diff --git a/internal/project/resource_project.go b/internal/project/resource_project.go index 69f2f448..db3c4183 100644 --- a/internal/project/resource_project.go +++ b/internal/project/resource_project.go @@ -277,13 +277,10 @@ func (r ProjectResource) SyncState(ctx context.Context, tfState *tfsdk.State, se } // Extract user defined config and merge it with current config state. - usrConfig, diags := common.ToConfigMap(ctx, m.Config) - respDiags.Append(diags...) - - stateConfig := common.StripConfig(project.Config, usrConfig, m.ComputedKeys()) + stateConfig := common.StripConfig(project.Config, m.Config, m.ComputedKeys()) // Convert config state into schema type. - config, diags := common.ToConfigMapType(ctx, stateConfig) + config, diags := common.ToConfigMapType(ctx, stateConfig, m.Config) respDiags.Append(diags...) m.Name = types.StringValue(project.Name) diff --git a/internal/storage/resource_storage_pool.go b/internal/storage/resource_storage_pool.go index 02de4de6..f26918da 100644 --- a/internal/storage/resource_storage_pool.go +++ b/internal/storage/resource_storage_pool.go @@ -328,13 +328,10 @@ func (r StoragePoolResource) SyncState(ctx context.Context, tfState *tfsdk.State } // Extract user defined config and merge it with current config state. - userConfig, diags := common.ToConfigMap(ctx, m.Config) - respDiags.Append(diags...) - - stateConfig := common.StripConfig(pool.Config, userConfig, m.ComputedKeys(pool.Driver)) + stateConfig := common.StripConfig(pool.Config, m.Config, m.ComputedKeys(pool.Driver)) // Convert config state into schema type. - config, diags := common.ToConfigMapType(ctx, stateConfig) + config, diags := common.ToConfigMapType(ctx, stateConfig, m.Config) respDiags.Append(diags...) m.Name = types.StringValue(pool.Name) diff --git a/internal/storage/resource_storage_volume.go b/internal/storage/resource_storage_volume.go index 0188d3eb..c2a3a2a3 100644 --- a/internal/storage/resource_storage_volume.go +++ b/internal/storage/resource_storage_volume.go @@ -349,12 +349,9 @@ func (r StorageVolumeResource) SyncState(ctx context.Context, tfState *tfsdk.Sta } // Extract user defined config and merge it with current config state. - userConfig, diags := common.ToConfigMap(ctx, m.Config) - respDiags.Append(diags...) - - stateConfig := common.StripConfig(vol.Config, userConfig, m.ComputedKeys()) + stateConfig := common.StripConfig(vol.Config, m.Config, m.ComputedKeys()) - config, diags := common.ToConfigMapType(ctx, stateConfig) + config, diags := common.ToConfigMapType(ctx, stateConfig, m.Config) respDiags.Append(diags...) m.Name = types.StringValue(vol.Name) From be8800659d22d450712a3cc3b24957004202e007 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Graber?= Date: Fri, 14 Jun 2024 11:53:21 -0400 Subject: [PATCH 2/2] internal/network: Add test for nullable config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stéphane Graber (cherry picked from commit 97258f8d4bc2bd5d58345f148f422f27c1687b5e) Signed-off-by: Din Music --- internal/network/resource_network_test.go | 38 +++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/internal/network/resource_network_test.go b/internal/network/resource_network_test.go index 1ca53cc3..09028423 100644 --- a/internal/network/resource_network_test.go +++ b/internal/network/resource_network_test.go @@ -51,6 +51,27 @@ func TestAccNetwork_description(t *testing.T) { }) } +func TestAccNetwork_nullable(t *testing.T) { + networkName := acctest.GenerateName(2, "-") + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccNetwork_nullable(networkName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("lxd_network.network", "name", networkName), + resource.TestCheckResourceAttr("lxd_network.network", "type", "bridge"), + resource.TestCheckResourceAttr("lxd_network.network", "config.%", "2"), + resource.TestCheckNoResourceAttr("lxd_network.network", "config.ipv4.address"), + resource.TestCheckResourceAttr("lxd_network.network", "config.ipv6.address", "none"), + ), + }, + }, + }) +} + func TestAccNetwork_attach(t *testing.T) { networkName := acctest.GenerateName(2, "-") profileName := acctest.GenerateName(2, "-") @@ -276,6 +297,23 @@ resource "lxd_network" "network" { `, networkName) } +func testAccNetwork_nullable(networkName string) string { + return fmt.Sprintf(` +locals { + foo = "bar" +} + +resource "lxd_network" "network" { + name = "%s" + + config = { + "ipv4.address" = local.foo == "bar" ? null : "10.0.0.1/24" + "ipv6.address" = "none" + } +} +`, networkName) +} + func testAccNetwork_attach(networkName string, profileName string, instanceName string) string { return fmt.Sprintf(` resource "lxd_network" "network" {