Use new private endpoint connections API #183

carloruiz · 2024-03-10T03:46:29Z

Previously, the private_endpoint_connection resource only supported AWS
private link connections. This commit updates the provider code to use
the new CC API Private Endpoint Connections methods. These cloud-neutral
methods will work for all cloud providers and cluster types, except
Serverless clusters on Azure as that configuration is not yet
supported.

Commit checklist

fantapop

These changes look good to me. I'm slightly confused about the state of the example here:
https://github.com/cockroachdb/terraform-provider-cockroach/blob/main/examples/resources/cockroach_aws_private_endpoint_connection/cockroach_aws_private_endpoint_connection.tf

I actually have an outstanding PR to rename it anyways but perhaps the contents should be update as part of this to reflect the correct usage.

Along similar lines, I'm wondering if there is any necessary migration as part of this or period where we need to keep a deprecated resource name around?

carloruiz · 2024-03-11T17:54:26Z

perhaps the contents should be update as part of this to reflect the correct usage.

ack. will update

Along similar lines, I'm wondering if there is any necessary migration as part of this or period where we need to keep a deprecated resource name around?

Fortunately, we're not deprecating any TF fields as part of this change.

carloruiz · 2024-03-11T19:47:56Z

Acceptance test: had do manually change some code to make this work. There's no straight-forward way to automatically test Private endpoint connection creation bc the client needs to create the endpoint out of band before calling the API. I created the endpoint manually and hardcoded the endpoint id into the test.

COCKROACH_API_KEY=<KEY> COCKROACH_SERVER=https://management-staging.crdb.io TF_ACC=1 make test
go test ./... -run TestAccServerlessPrivateEndpointConnectionResource -v  -timeout 20m
?   	github.com/cockroachdb/terraform-provider-cockroach	[no test files]
?   	github.com/cockroachdb/terraform-provider-cockroach/mock	[no test files]
=== RUN   TestAccServerlessPrivateEndpointConnectionResource
=== PAUSE TestAccServerlessPrivateEndpointConnectionResource
=== CONT  TestAccServerlessPrivateEndpointConnectionResource
--- PASS: TestAccServerlessPrivateEndpointConnectionResource (68.70s)
PASS
ok  	github.com/cockroachdb/terraform-provider-cockroach/internal/provider	69.241s

carloruiz · 2024-03-11T20:42:33Z

Actually I was able to add a fixture for the serverless test so it runs with the acceptance tests. It will reuse the real, existing endpoint id.

fantapop

Hey, I had a few concerns. Left some comments in there.

internal/provider/private_endpoint_connection_resource.go

fantapop · 2024-03-12T18:55:05Z

internal/provider/private_endpoint_connection_resource_test.go

@@ -38,17 +38,18 @@ func TestAccDedicatedPrivateEndpointConnectionResource(t *testing.T) {
 }

 func TestAccServerlessPrivateEndpointConnectionResource(t *testing.T) {
-	t.Skip("Skipping until we can either integrate the AWS provider " +
-		"or import a permanent test fixture.")
+	// This test relies on a pre-created private endpoint created via the AWS


This feels a little fragile to me. Do we have anything protecting it from being removed? Many teams won't have the perms to fix it if it breaks.

I can't think of a way to make it "unfragile", except actually allocating an AWS account for TF testing, creating an AWS service account, creating an API key for the account, using the AWS SDK to create and delete the private endpoint connection on each test run, running the tests with the AWS API key. We'd also need to figure out a way to store AWS API Key so all engineers have access. Maybe vault, but then the TF tests would have a vault dep. I don't think it's a viable approach atm, too much hassle.

I know it's janky but I think the approach in this PR could get us working acceptance tests for a year since the staging Host clusters will prob not be re-created any time soon. It will break eventually and it be annoying to fix but IMO the future pain is worth having coverage. If you feel differently, I can leave the acceptance test as skipped and punt on this.

Hmm actually I'm going to leave this test as skipped since the endpointId only works for staging. Any acceptance test ran against any other env will fail.

Is there a way to make it conditional on the env? Or maybe we can add instructions for running the test in staging?

fantapop · 2024-03-12T18:56:29Z

internal/provider/private_endpoint_connection_resource_test.go

@@ -238,7 +231,7 @@ resource "cockroach_private_endpoint_services" "services" {

 resource "cockroach_private_endpoint_connection" "connection" {
    cluster_id = cockroach_cluster.dedicated.id
-    endpoint_id = "endpoint-id"
+    endpoint_id = "vpce-0011573c1fa6afa3d"


rather than hard coding this id everywhere, is it possible to use a datasource to fetch the endpoint using something else like name?

is it possible to use a datasource to fetch the endpoint using something else like name?

To fetch from AWS? We'd need AWS creds and we'd need to figure out a way to share the AWS API key across CC eng. Also, the endpoint is only uniquely identified by ID, so we'd still need to store the endpointId somewhere.

Previously, the private_endpoint_connection resource only supported AWS private link connections. This commit updates the provider code to use the new CC API Private Endpoint Connections methods. These cloud-neutral methods will work for all cloud providers and cluster types, except Serverless clusters on Azure as that configuration is not yet supported.

fantapop

LGTM

carloruiz force-pushed the use-new-private-endpoints-api branch from b75ef36 to 08138d7 Compare March 10, 2024 04:02

carloruiz requested a review from rgcase March 11, 2024 16:27

carloruiz assigned fantapop and unassigned fantapop Mar 11, 2024

carloruiz requested a review from fantapop March 11, 2024 16:28

fantapop reviewed Mar 11, 2024

View reviewed changes

carloruiz force-pushed the use-new-private-endpoints-api branch from 08138d7 to 3850859 Compare March 11, 2024 18:39

carloruiz requested a review from fantapop March 11, 2024 18:40

carloruiz force-pushed the use-new-private-endpoints-api branch from 3850859 to f96fd7d Compare March 11, 2024 21:01

fantapop reviewed Mar 12, 2024

View reviewed changes

carloruiz force-pushed the use-new-private-endpoints-api branch from f96fd7d to 60055fc Compare March 13, 2024 03:59

carloruiz requested a review from fantapop March 13, 2024 17:21

carloruiz force-pushed the use-new-private-endpoints-api branch from 60055fc to faccbd4 Compare March 13, 2024 17:35

carloruiz force-pushed the use-new-private-endpoints-api branch from faccbd4 to 88b879c Compare March 13, 2024 17:37

fantapop approved these changes Mar 13, 2024

View reviewed changes

carloruiz merged commit 1a3b84f into main Mar 13, 2024
3 checks passed

carloruiz deleted the use-new-private-endpoints-api branch March 13, 2024 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new private endpoint connections API #183

Use new private endpoint connections API #183

carloruiz commented Mar 10, 2024 •

edited

Loading

fantapop left a comment

carloruiz commented Mar 11, 2024

carloruiz commented Mar 11, 2024

carloruiz commented Mar 11, 2024 •

edited

Loading

fantapop left a comment

fantapop Mar 12, 2024

carloruiz Mar 13, 2024 •

edited

Loading

carloruiz Mar 13, 2024

fantapop Mar 13, 2024

fantapop Mar 12, 2024

carloruiz Mar 13, 2024 •

edited

Loading

fantapop left a comment

Use new private endpoint connections API #183

Use new private endpoint connections API #183

Conversation

carloruiz commented Mar 10, 2024 • edited Loading

fantapop left a comment

Choose a reason for hiding this comment

carloruiz commented Mar 11, 2024

carloruiz commented Mar 11, 2024

carloruiz commented Mar 11, 2024 • edited Loading

fantapop left a comment

Choose a reason for hiding this comment

fantapop Mar 12, 2024

Choose a reason for hiding this comment

carloruiz Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

carloruiz Mar 13, 2024

Choose a reason for hiding this comment

fantapop Mar 13, 2024

Choose a reason for hiding this comment

fantapop Mar 12, 2024

Choose a reason for hiding this comment

carloruiz Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

fantapop left a comment

Choose a reason for hiding this comment

carloruiz commented Mar 10, 2024 •

edited

Loading

carloruiz commented Mar 11, 2024 •

edited

Loading

carloruiz Mar 13, 2024 •

edited

Loading

carloruiz Mar 13, 2024 •

edited

Loading