From 85aa53a5b8635d2a7b08a7fa8b3fee56e65b4929 Mon Sep 17 00:00:00 2001 From: Jeremy Muriel Date: Mon, 15 Jan 2024 09:09:26 +0100 Subject: [PATCH] r/interface_physical: skip conflict errors with unknown value avoid trigger the conflict errors when Terraform call resource config validate and value for potential conflict is unknown (can be null afterwards) Fix #611 --- .changes/issue-611.md | 4 + .../resource_interface_physical.go | 127 +++++++++------ internal/tfdata/check_asblock.go | 39 +++++ internal/tfdata/check_asblock_test.go | 152 ++++++++++++++++++ 4 files changed, 272 insertions(+), 50 deletions(-) create mode 100644 .changes/issue-611.md diff --git a/.changes/issue-611.md b/.changes/issue-611.md new file mode 100644 index 00000000..cc14f73d --- /dev/null +++ b/.changes/issue-611.md @@ -0,0 +1,4 @@ + +BUG FIXES: + +* **resource/junos_interface_physical**: avoid trigger the conflict errors when Terraform call resource config validate and value for potential conflict is unknown (can be null afterwards) (Fix [#611](https://github.com/jeremmfr/terraform-provider-junos/issues/611)) diff --git a/internal/providerfwk/resource_interface_physical.go b/internal/providerfwk/resource_interface_physical.go index 68a4430c..a14c8f31 100644 --- a/internal/providerfwk/resource_interface_physical.go +++ b/internal/providerfwk/resource_interface_physical.go @@ -810,6 +810,10 @@ func (block *interfacePhysicalBlockEtherOpts) isEmpty() bool { return tfdata.CheckBlockIsEmpty(block) } +func (block *interfacePhysicalBlockEtherOpts) hasKnownValue() bool { + return tfdata.CheckBlockHasKnownValue(block) +} + type interfacePhysicalBlockParentEtherOpts struct { FlowControl types.Bool `tfsdk:"flow_control"` NoFlowControl types.Bool `tfsdk:"no_flow_control"` @@ -846,6 +850,10 @@ func (block *interfacePhysicalBlockParentEtherOptsConfig) isEmpty() bool { return tfdata.CheckBlockIsEmpty(block) } +func (block *interfacePhysicalBlockParentEtherOptsConfig) hasKnownValue() bool { + return tfdata.CheckBlockHasKnownValue(block) +} + type interfacePhysicalBlockParentEtherOptsBlockBFDLivenessDetection struct { AuthenticationLooseCheck types.Bool `tfsdk:"authentication_loose_check"` NoAdaptation types.Bool `tfsdk:"no_adaptation"` @@ -886,11 +894,16 @@ type interfacePhysicalBlockParentEtherOptsBlockMCAE struct { EventsIccpPeerDown *interfacePhysicalBlockParentEtherOptsBlockMCAEBlockEventsIccpPeerDown `tfsdk:"events_iccp_peer_down"` } +func (block *interfacePhysicalBlockParentEtherOptsBlockMCAE) hasKnownValue() bool { + return tfdata.CheckBlockHasKnownValue(block) +} + type interfacePhysicalBlockParentEtherOptsBlockMCAEBlockEventsIccpPeerDown struct { ForceIclDown types.Bool `tfsdk:"force_icl_down"` PreferStatusControlActive types.Bool `tfsdk:"prefer_status_control_active"` } +//nolint:gocyclo func (rsc *interfacePhysical) ValidateConfig( ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse, ) { @@ -908,7 +921,8 @@ func (rsc *interfacePhysical) ValidateConfig( "mode must be specified in esi block", ) } - if !config.ESI.AutoDeriveLACP.IsNull() && !config.ESI.Identifier.IsNull() { + if !config.ESI.AutoDeriveLACP.IsNull() && !config.ESI.AutoDeriveLACP.IsUnknown() && + !config.ESI.Identifier.IsNull() && !config.ESI.Identifier.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("esi").AtName("auto_derive_lacp"), tfdiag.ConflictConfigErrSummary, @@ -917,128 +931,148 @@ func (rsc *interfacePhysical) ValidateConfig( } } if config.EtherOpts != nil { - if config.GigetherOpts != nil || config.ParentEtherOpts != nil { + if config.EtherOpts.isEmpty() { + resp.Diagnostics.AddAttributeError( + path.Root("ether_opts").AtName("*"), + tfdiag.MissingConfigErrSummary, + "ether_opts block is empty", + ) + } else if config.EtherOpts.hasKnownValue() && + ((config.GigetherOpts != nil && config.GigetherOpts.hasKnownValue()) || + (config.ParentEtherOpts != nil && config.ParentEtherOpts.hasKnownValue())) { resp.Diagnostics.AddAttributeError( path.Root("ether_opts").AtName("*"), tfdiag.ConflictConfigErrSummary, "only one of ether_opts, gigether_opts or parent_ether_opts block can be specified", ) } - if !config.EtherOpts.Ae8023ad.IsNull() && !config.EtherOpts.RedundantParent.IsNull() { + if !config.EtherOpts.Ae8023ad.IsNull() && !config.EtherOpts.Ae8023ad.IsUnknown() && + !config.EtherOpts.RedundantParent.IsNull() && !config.EtherOpts.RedundantParent.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("ether_opts").AtName("ae_8023ad"), tfdiag.ConflictConfigErrSummary, "ae_8023ad and redundant_parent cannot be configured together in ether_opts block", ) } - if !config.EtherOpts.AutoNegotiation.IsNull() && !config.EtherOpts.NoAutoNegotiation.IsNull() { + if !config.EtherOpts.AutoNegotiation.IsNull() && !config.EtherOpts.AutoNegotiation.IsUnknown() && + !config.EtherOpts.NoAutoNegotiation.IsNull() && !config.EtherOpts.NoAutoNegotiation.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("ether_opts").AtName("auto_negotiation"), tfdiag.ConflictConfigErrSummary, "auto_negotiation and no_auto_negotiation cannot be configured together in ether_opts block", ) } - if !config.EtherOpts.FlowControl.IsNull() && !config.EtherOpts.NoFlowControl.IsNull() { + if !config.EtherOpts.FlowControl.IsNull() && !config.EtherOpts.FlowControl.IsUnknown() && + !config.EtherOpts.NoFlowControl.IsNull() && !config.EtherOpts.NoFlowControl.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("ether_opts").AtName("flow_control"), tfdiag.ConflictConfigErrSummary, "flow_control and no_flow_control cannot be configured together in ether_opts block", ) } - if !config.EtherOpts.Loopback.IsNull() && !config.EtherOpts.NoLoopback.IsNull() { + if !config.EtherOpts.Loopback.IsNull() && !config.EtherOpts.Loopback.IsUnknown() && + !config.EtherOpts.NoLoopback.IsNull() && !config.EtherOpts.NoLoopback.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("ether_opts").AtName("loopback"), tfdiag.ConflictConfigErrSummary, "loopback and no_loopback cannot be configured together in ether_opts block", ) } - - if config.EtherOpts.isEmpty() { + } + if config.GigetherOpts != nil { + if config.GigetherOpts.isEmpty() { resp.Diagnostics.AddAttributeError( - path.Root("ether_opts").AtName("*"), + path.Root("gigether_opts").AtName("*"), tfdiag.MissingConfigErrSummary, - "ether_opts block is empty", + "gigether_opts block is empty", ) - } - } - if config.GigetherOpts != nil { - if config.EtherOpts != nil || config.ParentEtherOpts != nil { + } else if config.GigetherOpts.hasKnownValue() && + ((config.EtherOpts != nil && config.EtherOpts.hasKnownValue()) || + (config.ParentEtherOpts != nil && config.ParentEtherOpts.hasKnownValue())) { resp.Diagnostics.AddAttributeError( path.Root("gigether_opts").AtName("*"), tfdiag.ConflictConfigErrSummary, "only one of ether_opts, gigether_opts or parent_ether_opts block can be specified", ) } - if !config.GigetherOpts.Ae8023ad.IsNull() && !config.GigetherOpts.RedundantParent.IsNull() { + if !config.GigetherOpts.Ae8023ad.IsNull() && !config.GigetherOpts.Ae8023ad.IsUnknown() && + !config.GigetherOpts.RedundantParent.IsNull() && !config.GigetherOpts.RedundantParent.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("gigether_opts").AtName("ae_8023ad"), tfdiag.ConflictConfigErrSummary, "ae_8023ad and redundant_parent cannot be configured together in gigether_opts block", ) } - if !config.GigetherOpts.AutoNegotiation.IsNull() && !config.GigetherOpts.NoAutoNegotiation.IsNull() { + if !config.GigetherOpts.AutoNegotiation.IsNull() && !config.GigetherOpts.AutoNegotiation.IsUnknown() && + !config.GigetherOpts.NoAutoNegotiation.IsNull() && !config.GigetherOpts.NoAutoNegotiation.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("gigether_opts").AtName("auto_negotiation"), tfdiag.ConflictConfigErrSummary, "auto_negotiation and no_auto_negotiation cannot be configured together in gigether_opts block", ) } - if !config.GigetherOpts.FlowControl.IsNull() && !config.GigetherOpts.NoFlowControl.IsNull() { + if !config.GigetherOpts.FlowControl.IsNull() && !config.GigetherOpts.FlowControl.IsUnknown() && + !config.GigetherOpts.NoFlowControl.IsNull() && !config.GigetherOpts.NoFlowControl.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("gigether_opts").AtName("flow_control"), tfdiag.ConflictConfigErrSummary, "flow_control and no_flow_control cannot be configured together in gigether_opts block", ) } - if !config.GigetherOpts.Loopback.IsNull() && !config.GigetherOpts.NoLoopback.IsNull() { + if !config.GigetherOpts.Loopback.IsNull() && !config.GigetherOpts.Loopback.IsUnknown() && + !config.GigetherOpts.NoLoopback.IsNull() && !config.GigetherOpts.NoLoopback.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("gigether_opts").AtName("loopback"), tfdiag.ConflictConfigErrSummary, "loopback and no_loopback cannot be configured together in gigether_opts block", ) } - - if config.GigetherOpts.isEmpty() { + } + if config.ParentEtherOpts != nil { + if config.ParentEtherOpts.isEmpty() { resp.Diagnostics.AddAttributeError( - path.Root("gigether_opts").AtName("*"), + path.Root("parent_ether_opts").AtName("*"), tfdiag.MissingConfigErrSummary, - "gigether_opts block is empty", + "parent_ether_opts block is empty", ) - } - } - if config.ParentEtherOpts != nil { - if !config.Name.IsUnknown() { - if v := config.Name.ValueString(); !strings.HasPrefix(v, "ae") && !strings.HasPrefix(v, "reth") { + } else if config.ParentEtherOpts.hasKnownValue() { + if !config.Name.IsUnknown() { + if v := config.Name.ValueString(); !strings.HasPrefix(v, "ae") && !strings.HasPrefix(v, "reth") { + resp.Diagnostics.AddAttributeError( + path.Root("parent_ether_opts").AtName("*"), + tfdiag.ConflictConfigErrSummary, + fmt.Sprintf("parent_ether_opts not compatible with this interface %q "+ + "(need to be ae* or reth* interface)", v), + ) + } + } + if (config.EtherOpts != nil && config.EtherOpts.hasKnownValue()) || + (config.GigetherOpts != nil && config.GigetherOpts.hasKnownValue()) { resp.Diagnostics.AddAttributeError( path.Root("parent_ether_opts").AtName("*"), tfdiag.ConflictConfigErrSummary, - fmt.Sprintf("parent_ether_opts not compatible with this interface %q "+ - "(need to be ae* or reth* interface)", v), + "only one of ether_opts, gigether_opts or parent_ether_opts block can be specified", ) } } - if config.EtherOpts != nil || config.GigetherOpts != nil { - resp.Diagnostics.AddAttributeError( - path.Root("parent_ether_opts").AtName("*"), - tfdiag.ConflictConfigErrSummary, - "only one of ether_opts, gigether_opts or parent_ether_opts block can be specified", - ) - } - if !config.ParentEtherOpts.FlowControl.IsNull() && !config.ParentEtherOpts.NoFlowControl.IsNull() { + if !config.ParentEtherOpts.FlowControl.IsNull() && !config.ParentEtherOpts.FlowControl.IsUnknown() && + !config.ParentEtherOpts.NoFlowControl.IsNull() && !config.ParentEtherOpts.NoFlowControl.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("parent_ether_opts").AtName("flow_control"), tfdiag.ConflictConfigErrSummary, "flow_control and no_flow_control cannot be configured together in parent_ether_opts block", ) } - if !config.ParentEtherOpts.Loopback.IsNull() && !config.ParentEtherOpts.NoLoopback.IsNull() { + if !config.ParentEtherOpts.Loopback.IsNull() && !config.ParentEtherOpts.Loopback.IsUnknown() && + !config.ParentEtherOpts.NoLoopback.IsNull() && !config.ParentEtherOpts.NoLoopback.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("parent_ether_opts").AtName("loopback"), tfdiag.ConflictConfigErrSummary, "loopback and no_loopback cannot be configured together in parent_ether_opts block", ) } - if !config.ParentEtherOpts.MinimumBandwidth.IsNull() && !config.ParentEtherOpts.MinimumLinks.IsNull() { + if !config.ParentEtherOpts.MinimumBandwidth.IsNull() && !config.ParentEtherOpts.MinimumBandwidth.IsUnknown() && + !config.ParentEtherOpts.MinimumLinks.IsNull() && !config.ParentEtherOpts.MinimumLinks.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("parent_ether_opts").AtName("minimum_bandwidth"), tfdiag.ConflictConfigErrSummary, @@ -1064,7 +1098,7 @@ func (rsc *interfacePhysical) ValidateConfig( } } if config.ParentEtherOpts.MCAE != nil { - if !config.Name.IsUnknown() { + if config.ParentEtherOpts.MCAE.hasKnownValue() && !config.Name.IsUnknown() { if v := config.Name.ValueString(); !strings.HasPrefix(v, "ae") { resp.Diagnostics.AddAttributeError( path.Root("parent_ether_opts").AtName("mc_ae").AtName("*"), @@ -1103,7 +1137,7 @@ func (rsc *interfacePhysical) ValidateConfig( ) } } - if !config.ParentEtherOpts.RedundancyGroup.IsNull() { + if !config.ParentEtherOpts.RedundancyGroup.IsNull() && !config.ParentEtherOpts.RedundancyGroup.IsUnknown() { if !config.Name.IsUnknown() { if v := config.Name.ValueString(); !strings.HasPrefix(v, "reth") { resp.Diagnostics.AddAttributeError( @@ -1115,14 +1149,6 @@ func (rsc *interfacePhysical) ValidateConfig( } } } - - if config.ParentEtherOpts.isEmpty() { - resp.Diagnostics.AddAttributeError( - path.Root("parent_ether_opts").AtName("*"), - tfdiag.MissingConfigErrSummary, - "parent_ether_opts block is empty", - ) - } } if config.Disable.ValueBool() && config.Description.ValueString() == "NC" { @@ -1134,7 +1160,8 @@ func (rsc *interfacePhysical) ValidateConfig( ) } - if !config.GratuitousArpReply.IsNull() && !config.NoGratuitousArpReply.IsNull() { + if !config.GratuitousArpReply.IsNull() && !config.GratuitousArpReply.IsUnknown() && + !config.NoGratuitousArpReply.IsNull() && !config.NoGratuitousArpReply.IsUnknown() { resp.Diagnostics.AddAttributeError( path.Root("gratuitous_arp_reply"), tfdiag.ConflictConfigErrSummary, diff --git a/internal/tfdata/check_asblock.go b/internal/tfdata/check_asblock.go index 882d488a..ff0495df 100644 --- a/internal/tfdata/check_asblock.go +++ b/internal/tfdata/check_asblock.go @@ -55,3 +55,42 @@ func CheckBlockIsEmpty[B any](block B, excludeFields ...string) bool { return true } + +// check if struct has either : +// - an framework attribute with known value (not null and not unknown) +// - an pointer to an other struct with a known framework attribute value. +func CheckBlockHasKnownValue[B any](block B) bool { + v := reflect.Indirect(reflect.ValueOf(block).Elem()) + + for i := 0; i < v.NumField(); i++ { + fieldValue := v.Field(i) + if !fieldValue.IsValid() { + continue + } + + if attr, ok := fieldValue.Interface().(attr.Value); ok { + if !attr.IsNull() && !attr.IsUnknown() { + return true + } + + continue + } + + if fieldValue.Type().Kind() == reflect.Pointer { + if !fieldValue.IsNil() { + if CheckBlockHasKnownValue(v.Field(i).Interface()) { + return true + } + } + + continue + } + + panic(fmt.Sprintf( + "don't know how to determine if field %q (type: %s) is known", + v.Type().Field(i).Name, fieldValue.Type().Name(), + )) + } + + return false +} diff --git a/internal/tfdata/check_asblock_test.go b/internal/tfdata/check_asblock_test.go index 46f3ddc7..7f4348cb 100644 --- a/internal/tfdata/check_asblock_test.go +++ b/internal/tfdata/check_asblock_test.go @@ -170,3 +170,155 @@ func TestCheckBlockIsEmpty(t *testing.T) { }) } } + +func TestCheckBlockHasKnownValue(t *testing.T) { + t.Parallel() + + type block2 struct { + SubStringAttr types.String `tfsdk:"sub_string_attr"` + SubInt64Attr types.Int64 `tfsdk:"sub_int64_attr"` + } + + type block struct { + StringAttr types.String `tfsdk:"string_attr"` + Int64Attr types.Int64 `tfsdk:"int64_attr"` + BlockAttr *block2 `tfsdk:"block_attr"` + StructAttr *struct { + SubBoolAttr types.Bool `tfsdk:"sub_bool_attr"` + } `tfsdk:"struct_attr"` + } + + type testCase struct { + val *block + expectResponse bool + } + + tests := map[string]testCase{ + "empty": { + val: new(block), + expectResponse: false, + }, + "all_null": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Null(), + BlockAttr: nil, + StructAttr: nil, + }, + expectResponse: false, + }, + "StringAttr_known": { + val: &block{ + StringAttr: types.StringValue("value_string"), + Int64Attr: types.Int64Null(), + BlockAttr: nil, + StructAttr: nil, + }, + expectResponse: true, + }, + "Int64Attr_unknown": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: nil, + StructAttr: nil, + }, + expectResponse: false, + }, + "BlockAttr_set_empty": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: new(block2), + StructAttr: nil, + }, + expectResponse: false, + }, + "BlockAttr_set_null": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: &block2{ + SubStringAttr: types.StringNull(), + SubInt64Attr: types.Int64Null(), + }, + StructAttr: nil, + }, + expectResponse: false, + }, + "BlockAttr_set_unknown": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: &block2{ + SubStringAttr: types.StringUnknown(), + SubInt64Attr: types.Int64Null(), + }, + StructAttr: nil, + }, + expectResponse: false, + }, + "BlockAttr_set_known": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: &block2{ + SubStringAttr: types.StringValue("sub_value_string"), + SubInt64Attr: types.Int64Unknown(), + }, + StructAttr: nil, + }, + expectResponse: true, + }, + "StrucAttr_set_null": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: nil, + StructAttr: &struct { + SubBoolAttr types.Bool `tfsdk:"sub_bool_attr"` + }{ + SubBoolAttr: types.BoolNull(), + }, + }, + expectResponse: false, + }, + "StrucAttr_set_unknown": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: nil, + StructAttr: &struct { + SubBoolAttr types.Bool `tfsdk:"sub_bool_attr"` + }{ + SubBoolAttr: types.BoolUnknown(), + }, + }, + expectResponse: false, + }, + "StrucAttr_set_known": { + val: &block{ + StringAttr: types.StringNull(), + Int64Attr: types.Int64Unknown(), + BlockAttr: nil, + StructAttr: &struct { + SubBoolAttr types.Bool `tfsdk:"sub_bool_attr"` + }{ + SubBoolAttr: types.BoolValue(true), + }, + }, + expectResponse: true, + }, + } + + for name, test := range tests { + name, test := name, test + t.Run(name, func(t *testing.T) { + t.Parallel() + + if resp := tfdata.CheckBlockHasKnownValue(&test.val); resp != test.expectResponse { + t.Errorf("the expected response %v, got %v", test.expectResponse, resp) + } + }) + } +}