From 9345719322c7789e16cad90ef1dbc0d3485f7167 Mon Sep 17 00:00:00 2001 From: Carlo Ruiz Date: Wed, 26 Jun 2024 12:00:05 -0700 Subject: [PATCH] private_endpoints_service: wait for all regions Previously, when waiting for private endpoints service creation, the resource could be considered ready if a single resource was ready and others were not yet created. This commit fixes the bug by explicitly waiting for all regional services to be AVAILABLE. --- .../private_endpoint_services_resource.go | 94 +++++++++++-------- ...private_endpoint_services_resource_test.go | 63 +++++++++++++ 2 files changed, 118 insertions(+), 39 deletions(-) diff --git a/internal/provider/private_endpoint_services_resource.go b/internal/provider/private_endpoint_services_resource.go index 1605ad95..1ecc74c2 100644 --- a/internal/provider/private_endpoint_services_resource.go +++ b/internal/provider/private_endpoint_services_resource.go @@ -75,7 +75,8 @@ You must install/upgrade to [version 1.7.6](https://github.com/cockroachdb/terra NestedObject: serviceObject, }, "services": schema.ListNestedAttribute{ - Computed: true, + Computed: true, + MarkdownDescription: "A list of regional private endpoint services for the cluster", PlanModifiers: []planmodifier.List{ listplanmodifier.UseStateForUnknown(), }, @@ -202,9 +203,13 @@ func (r *privateEndpointServicesResource) Create( return } } - var services client.PrivateEndpointServices - err = retry.RetryContext(ctx, endpointServicesCreateTimeout, - waitForEndpointServicesCreatedFunc(ctx, cluster.Id, r.provider.service, &services)) + var services *client.PrivateEndpointServices + var retryErr *retry.RetryError + err = retry.RetryContext(ctx, endpointServicesCreateTimeout, func() *retry.RetryError { + services, retryErr = arePrivateEndpointsServicesCreated(ctx, cluster, r.provider.service) + return retryErr + }) + if err != nil { resp.Diagnostics.AddError( "Error enabling private endpoint services", @@ -215,7 +220,7 @@ func (r *privateEndpointServicesResource) Create( var state PrivateEndpointServices state.ClusterID = plan.ClusterID state.ID = plan.ClusterID - diags = loadEndpointServicesIntoTerraformState(ctx, &services, &state) + diags = loadEndpointServicesIntoTerraformState(ctx, services, &state) resp.Diagnostics.Append(diags...) if resp.Diagnostics.HasError() { return @@ -262,6 +267,7 @@ func loadEndpointServicesIntoTerraformState( ) diag.Diagnostics { serviceList := apiServices.GetServices() services := make([]PrivateEndpointService, len(serviceList)) + for i, service := range serviceList { services[i] = PrivateEndpointService{ RegionName: types.StringValue(service.GetRegionName()), @@ -288,21 +294,20 @@ func loadEndpointServicesIntoTerraformState( } } } - var diags diag.Diagnostics - state.Services, diags = types.ListValueFrom( + + var diags, diags2 diag.Diagnostics + state.Services, diags2 = types.ListValueFrom( ctx, - // Yes, it really does apparently need to be this complicated. - // https://github.com/hashicorp/terraform-plugin-framework/issues/713 endpointServicesSchema.Attributes["services"].(schema.ListNestedAttribute).NestedObject.Type(), services, ) + diags.Append(diags2...) servicesMap := map[string]PrivateEndpointService{} for _, svc := range services { servicesMap[svc.RegionName.ValueString()] = svc } - var diags2 diag.Diagnostics state.ServicesMap, diags2 = types.MapValueFrom( ctx, endpointServicesSchema.Attributes["services_map"].(schema.MapNestedAttribute).NestedObject.Type(), @@ -339,44 +344,55 @@ func (r *privateEndpointServicesResource) ImportState( resource.ImportStatePassthroughID(ctx, path.Root("cluster_id"), req, resp) } -func waitForEndpointServicesCreatedFunc( +func arePrivateEndpointsServicesCreated( ctx context.Context, - clusterID string, + cluster *client.Cluster, cl client.Service, - services *client.PrivateEndpointServices, -) retry.RetryFunc { - return func() *retry.RetryError { - traceAPICall("ListPrivateEndpointServices") - apiServices, httpResp, err := cl.ListPrivateEndpointServices(ctx, clusterID) - if err != nil { - if httpResp != nil && httpResp.StatusCode < http.StatusInternalServerError { - return retry.NonRetryableError(fmt.Errorf("error getting endpoint services: %s", formatAPIErrorMessage(err))) - } else { - return retry.RetryableError(fmt.Errorf("encountered a server error while reading endpoint status - trying again: %v", err)) - } +) (*client.PrivateEndpointServices, *retry.RetryError) { + // Set of all regions. + regions := func() map[string]bool { + m := map[string]bool{} + for _, r := range cluster.Regions { + m[r.Name] = true } + return m + }() - if len(apiServices.Services) == 0 { - return retry.RetryableError(fmt.Errorf("private endpoint services not yet created")) + traceAPICall("ListPrivateEndpointServices") + services, httpResp, err := cl.ListPrivateEndpointServices(ctx, cluster.Id) + if err != nil { + if httpResp != nil && httpResp.StatusCode < http.StatusInternalServerError { + return nil, retry.NonRetryableError(fmt.Errorf("error getting endpoint services: %s", formatAPIErrorMessage(err))) + } else { + return nil, retry.RetryableError(fmt.Errorf("encountered a server error while reading endpoint status - trying again: %v", err)) } + } - *services = *apiServices - var creating bool - // If there's at least one still creating, keep checking. - // If any have failed or have any other non-available status, something went wrong. - // If all the services have been created, we're done. - for _, service := range services.GetServices() { - if service.GetStatus() == client.PRIVATEENDPOINTSERVICESTATUSTYPE_CREATING { - creating = true - } else if service.GetStatus() != client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE { - return retry.NonRetryableError(fmt.Errorf("endpoint service creation failed")) - } + if len(services.Services) == 0 { + return nil, retry.RetryableError(fmt.Errorf("private endpoint services not yet created")) + } + + for _, service := range services.GetServices() { + // We don't care about services in other regions. + if _, ok := regions[service.GetRegionName()]; !ok { + continue } - if creating { - return retry.RetryableError(fmt.Errorf("endpoint services are not ready yet")) + + switch service.GetStatus() { + case client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE: + delete(regions, service.RegionName) + case client.PRIVATEENDPOINTSERVICESTATUSTYPE_CREATING: + return nil, retry.RetryableError(fmt.Errorf("endpoint services are not ready yet")) + default: + return nil, retry.NonRetryableError(fmt.Errorf("endpoint service creation failed")) } - return nil } + + if len(regions) > 0 { + return nil, retry.RetryableError(fmt.Errorf("endpoint services are not ready yet")) + } + + return services, nil } func NewPrivateEndpointServicesResource() resource.Resource { diff --git a/internal/provider/private_endpoint_services_resource_test.go b/internal/provider/private_endpoint_services_resource_test.go index 1e263e46..d1488754 100644 --- a/internal/provider/private_endpoint_services_resource_test.go +++ b/internal/provider/private_endpoint_services_resource_test.go @@ -17,6 +17,7 @@ package provider import ( + "context" "fmt" "net/http" "os" @@ -27,7 +28,9 @@ import ( mock_client "github.com/cockroachdb/terraform-provider-cockroach/mock" "github.com/golang/mock/gomock" "github.com/google/uuid" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/stretchr/testify/require" ) // TestAccDedicatedPrivateEndpointServicesResource attempts to create, check, @@ -268,6 +271,66 @@ func testPrivateEndpointServicesResource( }) } +func TestArePrivateEndpointsServicesCreated(t *testing.T) { + ctx := context.Background() + ctrl := gomock.NewController(t) + s := mock_client.NewMockService(ctrl) + + cluster := &client.Cluster{ + Id: "1111", + Regions: []client.Region{ + {Name: "us-east-1"}, + {Name: "us-west-2"}, + }, + } + + for _, tc := range []struct { + desc string + svcs []client.PrivateEndpointService + retryErr *retry.RetryError + }{ + { + desc: "all available", + svcs: []client.PrivateEndpointService{ + {RegionName: "us-east-1", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE}, + {RegionName: "us-west-2", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE}, + }, + }, + { + desc: "one service creating", + svcs: []client.PrivateEndpointService{ + {RegionName: "us-east-1", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE}, + {RegionName: "us-west-2", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_CREATING}, + }, + retryErr: retry.RetryableError(fmt.Errorf("endpoint services are not ready yet")), + }, + { + desc: "extra regional service", + svcs: []client.PrivateEndpointService{ + {RegionName: "us-east-1", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE}, + {RegionName: "us-west-2", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE}, + {RegionName: "us-central-2", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_DELETING}, + }, + }, + { + desc: "missing service", + svcs: []client.PrivateEndpointService{ + {RegionName: "us-east-1", Status: client.PRIVATEENDPOINTSERVICESTATUSTYPE_AVAILABLE}, + }, + retryErr: retry.RetryableError(fmt.Errorf("endpoint services are not ready yet")), + }, + } { + t.Run(tc.desc, func(t *testing.T) { + s.EXPECT().ListPrivateEndpointServices(gomock.Any(), "1111").Return(&client.PrivateEndpointServices{ + Services: tc.svcs, + }, nil, nil) + + _, retryErr := arePrivateEndpointsServicesCreated(ctx, cluster, s) + require.Equal(t, tc.retryErr, retryErr) + }) + } +} + func getTestPrivateEndpointServicesResourceConfigForDedicatedGCP(clusterName string) string { return fmt.Sprintf(` resource "cockroach_cluster" "dedicated" {