From dd12926f720226936f906f3feba30a91c8975fc6 Mon Sep 17 00:00:00 2001 From: Talina Shrotriya <5362897+Talina06@users.noreply.github.com> Date: Thu, 28 May 2026 20:57:47 -0700 Subject: [PATCH] =?UTF-8?q?Revert=20"[AV-129893]=20Enhance=20network=20pee?= =?UTF-8?q?r=20error=20handling=20and=20state=20management=20=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 8570973e54ffffb934bc6be01ce8012a057fb426. --- internal/resources/network_peer.go | 131 +++++++-------------------- internal/schema/network_peer.go | 37 ++------ internal/schema/network_peer_test.go | 80 ---------------- 3 files changed, 43 insertions(+), 205 deletions(-) diff --git a/internal/resources/network_peer.go b/internal/resources/network_peer.go index 8182577b1..b3c52ac5a 100644 --- a/internal/resources/network_peer.go +++ b/internal/resources/network_peer.go @@ -7,15 +7,15 @@ import ( "net/http" "strings" - "github.com/hashicorp/terraform-plugin-framework/path" - "github.com/hashicorp/terraform-plugin-framework/resource" - "github.com/hashicorp/terraform-plugin-framework/types" - "github.com/hashicorp/terraform-plugin-log/tflog" - "github.com/couchbasecloud/terraform-provider-couchbase-capella/internal/api" network_peer_api "github.com/couchbasecloud/terraform-provider-couchbase-capella/internal/api/network_peer" "github.com/couchbasecloud/terraform-provider-couchbase-capella/internal/errors" providerschema "github.com/couchbasecloud/terraform-provider-couchbase-capella/internal/schema" + + "github.com/hashicorp/terraform-plugin-framework/path" + "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-log/tflog" ) // Ensure the implementation satisfies the expected interfaces. @@ -129,38 +129,10 @@ func (n *NetworkPeer) Create(ctx context.Context, req resource.CreateRequest, re nil, ) if err != nil { - // The API may persist a failed peering record even when returning a non-success status. - // Attempt to find the resource so we can save it to state for lifecycle management. - peerID := n.findNetworkPeerByName(ctx, organizationId, projectId, clusterId, plan.Name.ValueString()) - if peerID == "" { - resp.Diagnostics.AddError( - "Error creating network peer", - errorMessageWhileNetworkPeerCreation+api.ParseError(err), - ) - return - } - - tflog.Info(ctx, "network peer was persisted despite creation error, saving to state", map[string]interface{}{ - "peer_id": peerID, - }) - - resp.Diagnostics.AddWarning( - "Network peer created with errors", - fmt.Sprintf( - "The API returned an error during creation (%s) but the network peer %q was persisted. "+ - "The resource has been saved to state for lifecycle management. "+ - "Review the peer status in Capella and, if necessary, run terraform destroy to clean up.", - api.ParseError(err), peerID, - ), + resp.Diagnostics.AddError( + "Error creating network peer", + errorMessageWhileNetworkPeerCreation+api.ParseError(err), ) - - diags = resp.State.Set(ctx, initializeNetworkPeerPlanId(plan, peerID)) - resp.Diagnostics.Append(diags...) - if resp.Diagnostics.HasError() { - return - } - - n.setRefreshedStateOrWarn(ctx, resp, organizationId, projectId, clusterId, peerID, plan.ProviderType.ValueString(), err) return } @@ -169,7 +141,7 @@ func (n *NetworkPeer) Create(ctx context.Context, req resource.CreateRequest, re if err != nil { resp.Diagnostics.AddError( "Error creating network peer", - errorMessageWhileNetworkPeerCreation+"error during unmarshalling: "+err.Error(), + errorMessageWhileNetworkPeerCreation+"error during unmarshalling:"+err.Error(), ) return } @@ -180,28 +152,21 @@ func (n *NetworkPeer) Create(ctx context.Context, req resource.CreateRequest, re return } - n.setRefreshedStateOrWarn(ctx, resp, organizationId, projectId, clusterId, networkPeerResponse.Id.String(), plan.ProviderType.ValueString(), nil) -} - -// setRefreshedStateOrWarn attempts to read the full peer state and set it on the response. -// If the read fails (e.g. peer is in failed state with incomplete providerConfig), a warning -// is emitted instead of an error so that the partial state (containing the ID) is preserved. -func (n *NetworkPeer) setRefreshedStateOrWarn(ctx context.Context, resp *resource.CreateResponse, organizationId, projectId, clusterId, peerID, providerType string, originalErr error) { - refreshedState, err := n.retrieveNetworkPeer(ctx, organizationId, projectId, clusterId, peerID, providerType) + refreshedState, err := n.retrieveNetworkPeer(ctx, organizationId, projectId, clusterId, networkPeerResponse.Id.String(), plan.ProviderType.ValueString()) if err != nil { - detail := "The network peer was created but its current status could not be fully read: " + err.Error() - if originalErr != nil { - detail += ". Original creation error: " + api.ParseError(originalErr) - } - resp.Diagnostics.AddWarning( - "Network peer created with incomplete state", - detail, + resp.Diagnostics.AddError( + "Error reading network peering service status", + "Error reading network peering service status, unexpected error: "+err.Error(), ) + return } - diags := resp.State.Set(ctx, refreshedState) + diags = resp.State.Set(ctx, refreshedState) resp.Diagnostics.Append(diags...) + if resp.Diagnostics.HasError() { + return + } } // createAWSProviderConfig is the function to handle AWS configuration. @@ -458,15 +423,7 @@ func (n *NetworkPeer) retrieveNetworkPeer( ) (*providerschema.NetworkPeer, error) { networkPeerResp, err := n.getNetworkPeer(ctx, organizationId, projectId, clusterId, peerId) if err != nil { - return nil, fmt.Errorf("%w: %w", errors.ErrNotFound, err) - } - - // Use the known provider type when the response doesn't include it - // (common for failed peers with empty providerConfig). - if networkPeerResp.ProviderType == "" { - networkPeerResp.ProviderType = providerType - } else if validateProviderTypeIsSameInPlanAndState(providerType, networkPeerResp.ProviderType) { - networkPeerResp.ProviderType = providerType + return nil, fmt.Errorf("%s: %w", errors.ErrNotFound, err) } audit := providerschema.NewCouchbaseAuditData(networkPeerResp.Audit) @@ -476,6 +433,10 @@ func (n *NetworkPeer) retrieveNetworkPeer( return nil, fmt.Errorf("%s: %w", errors.ErrUnableToConvertAuditData, err) } + if validateProviderTypeIsSameInPlanAndState(providerType, networkPeerResp.ProviderType) { + networkPeerResp.ProviderType = providerType + } + refreshedState, err := providerschema.NewNetworkPeer(ctx, networkPeerResp, organizationId, projectId, clusterId, auditObj) if err != nil { return nil, fmt.Errorf("%s: %w", errors.ErrRefreshingState, err) @@ -486,8 +447,8 @@ func (n *NetworkPeer) retrieveNetworkPeer( return refreshedState, nil } -// getNetworkPeer retrieves network peer information from the specified organization and project -// using the provided cluster ID and peer ID by open-api call. +// getNetworkPeer retrieves cluster information from the specified organization and project +// using the provided cluster ID by open-api call. func (n *NetworkPeer) getNetworkPeer( ctx context.Context, organizationId, projectId, clusterId, peerId string, ) (*network_peer_api.GetNetworkPeeringRecordResponse, error) { @@ -510,21 +471,16 @@ func (n *NetworkPeer) getNetworkPeer( return nil, fmt.Errorf("%s: %w", errors.ErrUnmarshallingResponse, err) } - // Best-effort provider type detection from response config fields. - // For failed peers, providerConfig may be empty so this detection is not fatal. - _ = defineProviderForResponse(&networkResp) + if err := defineProviderForResponse(networkResp); err != nil { + return nil, err + } return &networkResp, nil } -// defineProviderForResponse sets the provider type on the response based on populated -// provider config fields. Returns an error if provider type cannot be determined (e.g. -// when a failed peer has an empty providerConfig), but callers may choose to ignore it. -func defineProviderForResponse(networkResp *network_peer_api.GetNetworkPeeringRecordResponse) error { - if len(networkResp.ProviderConfig) == 0 || string(networkResp.ProviderConfig) == "null" { - return fmt.Errorf("%w: providerConfig is empty", errors.ErrReadingProviderConfig) - } - +// defineProviderForResponse sets the provider type in the retrieved network peer as per the fields populated in the provider config. +// If the provider type is not set through terraform separately in this manner, it will throw error as v4 get doesn't return it, but it's a field in resources. +func defineProviderForResponse(networkResp network_peer_api.GetNetworkPeeringRecordResponse) error { azure, err := networkResp.AsAZURE() if err != nil { return fmt.Errorf("%s: %w", errors.ErrReadingAzureConfig, err) @@ -540,6 +496,7 @@ func defineProviderForResponse(networkResp *network_peer_api.GetNetworkPeeringRe return fmt.Errorf("%s: %w", errors.ErrReadingAWSConfig, err) } + // if there is no error, set the provider type for the provider config as per the populated fields in the get response. switch { case azure.AzureConfigData.AzureTenantId != "": networkResp.ProviderType = "azure" @@ -548,34 +505,12 @@ func defineProviderForResponse(networkResp *network_peer_api.GetNetworkPeeringRe case aws.AWSConfigData.VpcId != "": networkResp.ProviderType = "aws" default: - return fmt.Errorf("%w: unable to determine provider type from config fields", errors.ErrReadingProviderConfig) + return fmt.Errorf("%s: %w", errors.ErrReadingProviderConfig, err) } return nil } -// findNetworkPeerByName lists all network peers for a cluster and returns the ID -// of the peer whose name matches. Returns empty string if not found. -func (n *NetworkPeer) findNetworkPeerByName(ctx context.Context, organizationId, projectId, clusterId, name string) string { - url := fmt.Sprintf("%s/v4/organizations/%s/projects/%s/clusters/%s/networkPeers", n.HostURL, organizationId, projectId, clusterId) - cfg := api.EndpointCfg{Url: url, Method: http.MethodGet, SuccessStatus: http.StatusOK} - - peers, err := api.GetPaginated[[]network_peer_api.GetNetworkPeeringRecordResponse](ctx, n.ClientV1, n.Token, cfg, api.SortById) - if err != nil { - tflog.Warn(ctx, "failed to list network peers while searching for persisted record", map[string]interface{}{ - "error": err.Error(), - }) - return "" - } - - for i := range peers { - if peers[i].Name == name { - return peers[i].Id.String() - } - } - return "" -} - func validateProviderTypeIsSameInPlanAndState(planProviderType, stateProviderType string) bool { return strings.EqualFold(planProviderType, stateProviderType) } diff --git a/internal/schema/network_peer.go b/internal/schema/network_peer.go index e895e7f00..b155bc8df 100644 --- a/internal/schema/network_peer.go +++ b/internal/schema/network_peer.go @@ -210,16 +210,13 @@ func NewNetworkPeer(ctx context.Context, networkPeer *network_peer_api.GetNetwor newProviderConfig, err := morphToProviderConfig(networkPeer) if err != nil { - return nil, fmt.Errorf("%w: %w", errors.ErrConvertingProviderConfig, err) + return nil, fmt.Errorf("%s: %w", errors.ErrConvertingProviderConfig, err) } newNetworkPeer.ProviderConfig = &newProviderConfig if networkPeer.Status.State != nil { state := *networkPeer.Status.State - var reasoning string - if networkPeer.Status.Reasoning != nil { - reasoning = *networkPeer.Status.Reasoning - } + reasoning := *networkPeer.Status.Reasoning status := PeeringStatus{ State: types.StringValue(state), Reasoning: types.StringValue(reasoning), @@ -233,7 +230,7 @@ func NewNetworkPeer(ctx context.Context, networkPeer *network_peer_api.GetNetwor newCommands, err := MorphCommands(networkPeer.Commands) if err != nil { - return nil, fmt.Errorf("%w: %w", errors.ErrConvertingCidr, err) + return nil, fmt.Errorf("%s: %w", errors.ErrConvertingCidr, err) } newNetworkPeer.Commands = newCommands @@ -242,28 +239,22 @@ func NewNetworkPeer(ctx context.Context, networkPeer *network_peer_api.GetNetwor } // morphToProviderConfig is used to convert ProviderConfig from json.RawMessage format to ProviderConfig type. -// Returns an empty ProviderConfig with no error when the providerConfig field is null or empty -// (which occurs for network peers in a failed state). func morphToProviderConfig(networkPeer *network_peer_api.GetNetworkPeeringRecordResponse) (ProviderConfig, error) { var newProviderConfig ProviderConfig - if len(networkPeer.ProviderConfig) == 0 || string(networkPeer.ProviderConfig) == "null" { - return newProviderConfig, nil - } - aws, err := networkPeer.AsAWS() if err != nil { - return ProviderConfig{}, fmt.Errorf("%w: %w", errors.ErrReadingAWSConfig, err) + return ProviderConfig{}, fmt.Errorf("%s: %w", errors.ErrReadingAWSConfig, err) } gcp, err := networkPeer.AsGCP() if err != nil { - return ProviderConfig{}, fmt.Errorf("%w: %w", errors.ErrReadingGCPConfig, err) + return ProviderConfig{}, fmt.Errorf("%s: %w", errors.ErrReadingGCPConfig, err) } azure, err := networkPeer.AsAZURE() if err != nil { - return ProviderConfig{}, fmt.Errorf("%w: %w", errors.ErrReadingAzureConfig, err) + return ProviderConfig{}, fmt.Errorf("%s: %w", errors.ErrReadingAzureConfig, err) } switch { @@ -296,7 +287,7 @@ func morphToProviderConfig(networkPeer *network_peer_api.GetNetworkPeeringRecord } return newProviderConfig, nil default: - return newProviderConfig, nil + return ProviderConfig{}, fmt.Errorf("%s: %w", errors.ErrReadingProviderConfig, err) } } @@ -328,27 +319,19 @@ func MorphCommands(commands []string) (basetypes.SetValue, error) { // NewNetworkPeerData create new network peer data object. func NewNetworkPeerData(networkPeer *network_peer_api.GetNetworkPeeringRecordResponse, organizationId, projectId, clusterId string, auditObject basetypes.ObjectValue) (*NetworkPeerData, error) { - var state, reasoning string - if networkPeer.Status.State != nil { - state = *networkPeer.Status.State - } - if networkPeer.Status.Reasoning != nil { - reasoning = *networkPeer.Status.Reasoning - } - newNetworkPeerData := NetworkPeerData{ Id: types.StringValue(networkPeer.Id.String()), Name: types.StringValue(networkPeer.Name), Audit: auditObject, Status: PeeringStatus{ - State: types.StringValue(state), - Reasoning: types.StringValue(reasoning), + State: types.StringValue(*networkPeer.Status.State), + Reasoning: types.StringValue(*networkPeer.Status.Reasoning), }, } newProviderConfig, err := morphToProviderConfig(networkPeer) if err != nil { - return nil, fmt.Errorf("%w: %w", errors.ErrConvertingProviderConfig, err) + return nil, fmt.Errorf("%s: %w", errors.ErrConvertingProviderConfig, err) } newNetworkPeerData.ProviderConfig = newProviderConfig diff --git a/internal/schema/network_peer_test.go b/internal/schema/network_peer_test.go index d348fc916..32887f5d7 100644 --- a/internal/schema/network_peer_test.go +++ b/internal/schema/network_peer_test.go @@ -1,19 +1,12 @@ package schema import ( - "context" - "encoding/json" "testing" - network_peer_api "github.com/couchbasecloud/terraform-provider-couchbase-capella/internal/api/network_peer" "github.com/couchbasecloud/terraform-provider-couchbase-capella/internal/errors" - "github.com/google/uuid" - "github.com/hashicorp/terraform-plugin-framework/attr" - "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestNetworkPeerSchemaValidate(t *testing.T) { @@ -75,76 +68,3 @@ func TestNetworkPeerSchemaValidate(t *testing.T) { }) } } - -func TestMorphToProviderConfig_NilProviderConfig(t *testing.T) { - resp := &network_peer_api.GetNetworkPeeringRecordResponse{ - ProviderConfig: nil, - } - cfg, err := morphToProviderConfig(resp) - require.NoError(t, err) - assert.Nil(t, cfg.AWSConfig) - assert.Nil(t, cfg.GCPConfig) - assert.Nil(t, cfg.AzureConfig) -} - -func TestMorphToProviderConfig_NullProviderConfig(t *testing.T) { - resp := &network_peer_api.GetNetworkPeeringRecordResponse{ - ProviderConfig: json.RawMessage("null"), - } - cfg, err := morphToProviderConfig(resp) - require.NoError(t, err) - assert.Nil(t, cfg.AWSConfig) - assert.Nil(t, cfg.GCPConfig) - assert.Nil(t, cfg.AzureConfig) -} - -func TestNewNetworkPeer_NilReasoning(t *testing.T) { - state := "failed" - resp := &network_peer_api.GetNetworkPeeringRecordResponse{ - Id: uuid.New(), - Name: "test-peer", - ProviderType: "aws", - ProviderConfig: nil, - Status: network_peer_api.PeeringStatus{ - State: &state, - Reasoning: nil, - }, - } - - auditObj, _ := types.ObjectValueFrom(context.Background(), map[string]attr.Type{}, nil) - - peer, err := NewNetworkPeer(context.Background(), resp, "org-1", "proj-1", "clust-1", auditObj) - require.NoError(t, err) - assert.Equal(t, "test-peer", peer.Name.ValueString()) - - // Status should be set with empty reasoning - assert.False(t, peer.Status.IsNull()) - var status PeeringStatus - diags := peer.Status.As(context.Background(), &status, basetypes.ObjectAsOptions{}) - require.False(t, diags.HasError()) - assert.Equal(t, "failed", status.State.ValueString()) - assert.Equal(t, "", status.Reasoning.ValueString()) -} - -func TestNewNetworkPeer_NilProviderConfigAndNilReasoning(t *testing.T) { - state := "failed" - resp := &network_peer_api.GetNetworkPeeringRecordResponse{ - Id: uuid.New(), - Name: "failed-peer", - ProviderType: "aws", - ProviderConfig: json.RawMessage("null"), - Status: network_peer_api.PeeringStatus{ - State: &state, - Reasoning: nil, - }, - } - - auditObj, _ := types.ObjectValueFrom(context.Background(), map[string]attr.Type{}, nil) - - peer, err := NewNetworkPeer(context.Background(), resp, "org-1", "proj-1", "clust-1", auditObj) - require.NoError(t, err) - assert.NotNil(t, peer.ProviderConfig) - assert.Nil(t, peer.ProviderConfig.AWSConfig) - assert.Nil(t, peer.ProviderConfig.GCPConfig) - assert.Nil(t, peer.ProviderConfig.AzureConfig) -}