HCS-1716: Add HCP Consul federation support #68

riddhi89 · 2021-02-17T00:12:04Z

Adds federation support for HCP Consul.

mkeeler

If you don't have federation enabled and attempt to use it from the TF provider how does this present itself to the user?

riddhi89 · 2021-02-17T19:45:52Z

@mkeeler : The user would get an error like:

hcp_consul_cluster.secondary: Creating...
Error: unable to create Consul cluster (consul-cluster-secondary): [POST /consul/2020-08-26/organizations/{cluster.location.organization_id}/projects/{cluster.location.project_id}/clusters][400] ConsulService_Create default  &{Code:3 Details:[] Error:federation is unavailable for this cluster Message:federation is unavailable for this cluster}

hanshasselberg

This is exciting. Which changes were generated and which are the ones you did? Mostly asking because that would make it a little easier to review.

hanshasselberg · 2021-02-17T20:29:43Z

internal/provider/resource_consul_cluster.go

+			return diag.Errorf("unable to check for presence of an existing primary Consul cluster (%s): %v", primary.ID, err)
+		}
+		primary.Location.Region = primaryConsulCluster.Location.Region
+	}


I am wondering if this kind of validation should live in cloud-consul-service.

💭 The provider does some basic validation, like input format and verifying the existence of dependent resources, but I'd agree that this seems closer to Consul business logic. That being said, I could see the advantage of having the request fail sooner, giving the user quicker feedback. How much later down the line would the consul service check for the primary?

Let me verify if this is done in the consul-service. If not, definitely sounds like a good place to include it vs here.

Nevermind. The reason I have this check in here is because I need the primary's region config (primary.Location.Region). Else, the below error is raised:

Error: unable to create Consul cluster (consul-cluster-secondary): [POST /consul/2020-08-26/organizations/{cluster.location.organization_id}/projects/{cluster.location.project_id}/clusters][400] ConsulService_Create default &{Code:3 Details:[0xc000151e60] Error:config: (consul_config: (primary: (location: (region: cannot be blank.).).).). Message:config: (consul_config: (primary: (location: (region: cannot be blank.).).).).}```

roaks3

🎉 Code LGTM, just had two comments

internal/clients/response.go

internal/provider/resource_consul_cluster.go

xargs-P · 2021-02-17T22:03:44Z

Hi @riddhi89 , things are looking good. 👌

Can you add a /examples/guides/ markdown for federation? And also update the example in docs/index.md ? Customers will be very interested in this functionality. So it would be sweet to give them as much help as possible.

bcmdarroch · 2021-02-17T22:37:27Z

examples/resources/hcp_consul_cluster/federation.tf

@@ -0,0 +1,15 @@
+data "hcp_hvn" "example" {


In these examples, we've generally been using resource for all the HCP resources, to encourage managing all those resources with the provider. We use data sources only for external resources like an AWS arn

internal/clients/response.go

bcmdarroch

This looks great! There's a few open questions I'm curious about: the necessity of self_link and whether or not we should push the verification of the primary cluster's existence into consul service.

internal/provider/data_source_consul_cluster.go

internal/provider/resource_consul_cluster.go

bcmdarroch · 2021-02-17T23:41:16Z

internal/clients/consul_cluster.go

@@ -12,13 +12,13 @@ import (
 func GetConsulClusterByID(ctx context.Context, client *Client, loc *sharedmodels.HashicorpCloudLocationLocation,
 	consulClusterID string) (*consulmodels.HashicorpCloudConsul20200826Cluster, error) {

-	getParams := consul_service.NewGetParams()
+	getParams := consul_service.NewConsulServiceGetParams()


Thanks for taking care of this update! Awkward timing with my tech debt chore in the sdk 😅

bcmdarroch · 2021-02-18T00:12:48Z

internal/provider/resource_consul_cluster.go

+			return diag.Errorf("unable to check for presence of an existing primary Consul cluster (%s): %v", primary.ID, err)
+		}
+		primary.Location.Region = primaryConsulCluster.Location.Region
+	}


💭 The provider does some basic validation, like input format and verifying the existence of dependent resources, but I'd agree that this seems closer to Consul business logic. That being said, I could see the advantage of having the request fail sooner, giving the user quicker feedback. How much later down the line would the consul service check for the primary?

bcmdarroch · 2021-02-18T00:29:00Z

internal/provider/resource_consul_cluster.go

@@ -417,6 +451,22 @@ func setConsulClusterResourceData(d *schema.ResourceData, cluster *consulmodels.
 		return err
 	}

+	// self_link is equal to the terraform ID of the resource
+	if err := d.Set("self_link", d.Id()); err != nil {


Why is the attribute self_link needed if it's the same as the terraform ID?

The reason it's a separate field is to be explicit to the user that it is a separate property so they aren't confused between, id and cluster_id. It also protects against the Terraform id format being changed in the future (not likely but 🤷 ).

That being said, I think it might be best to "re-build" the link using the newLink and linkUrl functions just so we aren't relying on the id.

might be best to "re-build" the link

Makes sense. Will do.

alexmunda · 2021-02-18T02:32:48Z

internal/clients/consul_cluster.go

@@ -75,9 +75,9 @@ func CreateCustomerRootACLToken(ctx context.Context, client *Client, loc *shared
 // CreateConsulCluster will make a call to the Consul service to initiate the create Consul
 // cluster workflow.
 func CreateConsulCluster(ctx context.Context, client *Client, loc *sharedmodels.HashicorpCloudLocationLocation,
-	clusterID, datacenter, consulVersion string, numServers int32, private, connectEnabled bool, network *sharedmodels.HashicorpCloudLocationLink) (*consulmodels.HashicorpCloudConsul20200826CreateResponse, error) {
+	clusterID, datacenter, consulVersion string, numServers int32, private, connectEnabled bool, network *sharedmodels.HashicorpCloudLocationLink, primary *sharedmodels.HashicorpCloudLocationLink) (*consulmodels.HashicorpCloudConsul20200826CreateResponse, error) {


There are a lot of params to this function and the two links at the end make me think it would be easy to flip the hvn and primary in the future. What do you think about it taking in a single consulmodels.HashicorpCloudConsul20200826Cluster struct?

Sounds like a good improvement to me. Will update.

Adds federation support for HCP Consul.

Co-authored-by: Brenna Hewer-Darroch <[email protected]>

riddhi89 · 2021-02-18T23:57:32Z

@i0rek

Which changes were generated and which are the ones you did?

All the changes within docs/* were generated.

xargs-P · 2021-02-19T01:22:33Z

Thanks @riddhi89 .
Can you also update the example in docs/index.md ?

xargs-P · 2021-02-19T01:24:54Z

docs/guides/consul-federation.md

+
+# Federate a new HCP Consul cluster with an existing one
+
+Once you have a HCP Consul cluster, you can create new Consul cluster to federate with the existing one.


Thank you adding this guide. ❤️
Can we add an a to the sentence here?

Once you have a HCP Consul cluster, you can create a new Consul cluster to federate with the existing one.

hanshasselberg

Looks great!

roaks3 · 2021-02-19T15:04:25Z

internal/provider/resource_consul_cluster.go

+		if err != nil {
+			return diag.Errorf(err.Error())
+		}
+		if primary.Location.OrganizationID == "" {


Nit: With the implementation you have here, you can probably take this block of code out of the primary.Location.OrganizationID == "" conditional. parseLinkURL() parses the URL without attempting to resolve any additional information, so primary.Location.OrganizationID would always be empty.

xargs-P

Thank you!

mkeeler reviewed Feb 17, 2021

View reviewed changes

hanshasselberg reviewed Feb 17, 2021

View reviewed changes

roaks3 reviewed Feb 17, 2021

View reviewed changes

internal/clients/response.go Show resolved Hide resolved

internal/provider/resource_consul_cluster.go Show resolved Hide resolved

bcmdarroch reviewed Feb 17, 2021

View reviewed changes

bcmdarroch reviewed Feb 18, 2021

View reviewed changes

alexmunda reviewed Feb 18, 2021

View reviewed changes

riddhi89 and others added 6 commits February 18, 2021 15:54

HCS-1716: Add HCP Consul federation support

1f583ee

Adds federation support for HCP Consul.

Fix case

0d72276

Co-authored-by: Brenna Hewer-Darroch <[email protected]>

Fix case 2

c3c0fa9

Co-authored-by: Brenna Hewer-Darroch <[email protected]>

Fix case 3

9e2d43c

Co-authored-by: Brenna Hewer-Darroch <[email protected]>

Fix case 4

4a99ab2

Co-authored-by: Brenna Hewer-Darroch <[email protected]>

Update docs

a1f6522

riddhi89 force-pushed the hcs-1716 branch from c81bb69 to a1f6522 Compare February 18, 2021 23:55

riddhi89 added 5 commits February 18, 2021 15:59

Fix client call after rebase

8f81f84

Fix rebase bug

6edd9a0

Reduce create cluster params

5ae8671

Explicitly build the self_link instead of using tf resource id

713419d

HCP Consul Cluster to HCP Consul cluster

77432a4

xargs-P reviewed Feb 19, 2021

View reviewed changes

hanshasselberg approved these changes Feb 19, 2021

View reviewed changes

roaks3 approved these changes Feb 19, 2021

View reviewed changes

riddhi89 added 2 commits February 19, 2021 11:36

Update index.md and fix sentence in docs

862fe68

Remove conditional for checking primary's org id

8ad8ed0

xargs-P approved these changes Feb 19, 2021

View reviewed changes

bcmdarroch approved these changes Feb 20, 2021

View reviewed changes

riddhi89 merged commit 8136e61 into main Feb 22, 2021

riddhi89 deleted the hcs-1716 branch February 22, 2021 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCS-1716: Add HCP Consul federation support #68

HCS-1716: Add HCP Consul federation support #68

riddhi89 commented Feb 17, 2021

mkeeler left a comment

riddhi89 commented Feb 17, 2021

hanshasselberg left a comment

hanshasselberg Feb 17, 2021

bcmdarroch Feb 18, 2021

riddhi89 Feb 18, 2021 •

edited

Loading

riddhi89 Feb 18, 2021

roaks3 left a comment

xargs-P commented Feb 17, 2021

bcmdarroch Feb 17, 2021

riddhi89 Feb 19, 2021

bcmdarroch left a comment

bcmdarroch Feb 17, 2021

bcmdarroch Feb 18, 2021

bcmdarroch Feb 18, 2021

alexmunda Feb 18, 2021

riddhi89 Feb 18, 2021

riddhi89 Feb 19, 2021

alexmunda Feb 18, 2021

riddhi89 Feb 18, 2021

riddhi89 Feb 19, 2021

riddhi89 commented Feb 18, 2021 •

edited

Loading

xargs-P commented Feb 19, 2021

xargs-P Feb 19, 2021

hanshasselberg left a comment

roaks3 Feb 19, 2021

riddhi89 Feb 19, 2021

xargs-P left a comment


		# Federate a new HCP Consul cluster with an existing one

		Once you have a HCP Consul cluster, you can create new Consul cluster to federate with the existing one.

HCS-1716: Add HCP Consul federation support #68

HCS-1716: Add HCP Consul federation support #68

Conversation

riddhi89 commented Feb 17, 2021

mkeeler left a comment

Choose a reason for hiding this comment

riddhi89 commented Feb 17, 2021

hanshasselberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riddhi89 Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roaks3 left a comment

Choose a reason for hiding this comment

xargs-P commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcmdarroch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riddhi89 commented Feb 18, 2021 • edited Loading

xargs-P commented Feb 19, 2021

Choose a reason for hiding this comment

hanshasselberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xargs-P left a comment

Choose a reason for hiding this comment

riddhi89 Feb 18, 2021 •

edited

Loading

riddhi89 commented Feb 18, 2021 •

edited

Loading