[CC-4083] IP Allowlist Feature for Consul Cluster #455

Achooo · 2023-02-10T16:45:08Z

🛠️ Description

Adding a new field ip_allowlist to the hcp_consul_cluster resource to support IP Allowlisting - a Block List (list of objects).
Validation Rules:
- No duplicate CIDR allowed
- Maximum of 3 IP Allowlist entries allowed
- Max length of 255 characters for the description field

🏗️ Acceptance tests

Are there any feature flags that are required to use this functionality?
Have you added an acceptance test for the functionality being added?
Have you run the acceptance tests on this branch?

Output from acceptance testing:

~/Github/HashiCorp/terraform-provider-hcp CC-4083/ip-allowlist* ⇡ 16m 2s
❯ make testacc TESTARGS='-run=TestAccConsulCluster'
==> Checking that code complies with gofmt requirements...
golangci-lint run --config ./golangci-config.yml 
TF_ACC=1 go test ./internal/... -v -run=TestAccConsulCluster -timeout 210m
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/clients    (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/consul     (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/input      (cached) [no tests to run]
=== RUN   TestAccConsulCluster
--- PASS: TestAccConsulCluster (1421.46s)
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider   1421.972s
...

End to End Tests (int)

Resource CREATE

Generate the provider locally with make dev in root of repo
Make a test folder test/
In test folder, create a main.tf with following :

locals {
  hvn_region = "us-west-2"
  cluster_id = "consul-tf-cluster"
  hvn_id     = "consul-tf-hvn"
}

terraform {
  required_providers {
    hcp = {
      source  = "localhost/providers/hcp"
      version = "0.0.1"
    }
  }
}

resource "hcp_hvn" "main" {
  hvn_id         = local.hvn_id
  cloud_provider = "aws"
  region         = local.hvn_region
  cidr_block     = "172.25.32.0/20"
}

resource "hcp_consul_cluster" "main" {
  cluster_id      = local.cluster_id
  hvn_id          = hcp_hvn.main.hvn_id
  public_endpoint = true
  tier            = "development"

  ip_allowlist {
    address     = "192.163.1.0/24"
    description = "this is an IPV4 address"
  }
}

Run terraform init
Run terraform apply
Results: tf apply logs

Datasource (READ ONLY)

Modify main.tf with the following:

data hcp_consul_cluster "test" {
    cluster_id = local.cluster_id
}

output "verify_allowlist_output" {
    description = "output of the ALLOWLIST"
    value = data.hcp_consul_cluster.test.ip_allowlist
}

run terraform apply
Results:

Resource UPDATE

Modify the hcp_consul_cluster resource:

resource "hcp_consul_cluster" "main" {
  cluster_id      = local.cluster_id
  hvn_id          = hcp_hvn.main.hvn_id
  public_endpoint = true
  tier            = "development"

  ip_allowlist {
    address     = "160.20.10.0/24"
    description = " modified!"
  }
}

Run terraform apply
Results: tf update logs

The data source output we added in the step before now shows the updated `ip_allowlist`

Validation

No more than 3 CIDRs

Description Length

Invalid CIDR

internal/provider/resource_consul_cluster.go

Achooo · 2023-02-10T16:53:34Z

internal/provider/validators_test.go

@@ -296,8 +296,8 @@ func Test_validateConsulClusterTier(t *testing.T) {
 			expected: diag.Diagnostics{
 				diag.Diagnostic{
 					Severity:      diag.Error,
-					Summary:       "expected dev to be one of [DEVELOPMENT STANDARD PLUS]",
-					Detail:        "expected dev to be one of [DEVELOPMENT STANDARD PLUS] (value is case-insensitive).",
+					Summary:       "expected dev to be one of [DEVELOPMENT STANDARD PLUS PREMIUM]",


Updating the go SDK (hashicorp/hcp-sdk-go#164) added this field. Had to fix this test

internal/provider/validators.go

Achooo · 2023-02-10T17:00:46Z

internal/provider/resource_consul_cluster_test.go

+	}
+`)
+
+func consulClusterConfig(clusterID string, opt string) string {


Created this helper function that creates the hcp_consul_cluster for the tests cases.

Achooo · 2023-02-10T17:03:10Z

internal/provider/resource_consul_cluster_test.go

 				),
 			},
+			// Tests update failure for IP Allowlist with too duplicate CIDRs


I'm happy to move all these tests in their own TestAccConsulCluster_IPAllowlist function, but this is likely going to increase CI time as resources need to be created for the tests to run. If we prefer clarity, that makes sense. I was reading this contributing guide and it seemed to prefer minimal resource creation.

Achooo · 2023-02-10T17:11:24Z

internal/provider/validators.go

@@ -178,6 +178,40 @@ func validateConsulClusterSize(v interface{}, path cty.Path) diag.Diagnostics {
 	return diagnostics
 }

+func validateConsulClusterCIDR(v interface{}, path cty.Path) diag.Diagnostics {


These could be added into the buildAllowlist validations to remove the code in this file.

However, these validations are pretty neat for terraform plan

In contrast, the duplicates will only be checked when the actual apply runs:

I think the more ideal user experience would be to check for duplicates at the plan stage as well. Unless that's something only the backend can validate?

See this comment for discussion on this. I can't add a validateFunc for the schema type (schema.TypeList not supported) to check for duplicates on all items

Let me know if you prefer another existing pattern to verify on plan, as I'm new to this repo!

Achooo · 2023-02-10T17:15:36Z

internal/provider/resource_consul_cluster.go

@@ -629,9 +673,10 @@ func resourceConsulClusterUpdate(ctx context.Context, d *schema.ResourceData, me
 	// Confirm update fields have been changed
 	sizeChanged := d.HasChange("size")
 	versionChanged := d.HasChange("min_consul_version")
+	ipAllowlistChanged := d.HasChange("ip_allowlist")


For any updates:

Detect changes in ip_allowlist

if changes, buildIPAllowlist() to validate there are no duplicates. This will return an empty list if there are no ip_allowlist entries, and otherwise a list with consul model objects that the API

Send update request

Achooo · 2023-02-10T17:16:33Z

internal/provider/resource_consul_cluster.go

@@ -329,6 +352,13 @@ func resourceConsulClusterCreate(ctx context.Context, d *schema.ResourceData, me
 	// The peering happens within the secondary cluster create operation.
 	autoHvnToHvnPeering := d.Get("auto_hvn_to_hvn_peering").(bool)

+	// Convert ip_allowlist to consul model.


For creates:

Get ip_allowlistvalue
-buildIPAllowlist() to validate there are no duplicates in the entries. This will also return an empty list if there are no ip_allowlist entries, and otherwise a list with consul model objects that the API expects.

Send create request

Achooo · 2023-02-10T17:32:01Z

Updates seem to be failing with the following errors from the consul service backend.

 Error: error updating Consul cluster (test-202302101209): [PATCH /consul/2021-02-04/organizations/{cluster.location.organization_id}/projects/{cluster.location.project_id}/clusters/{cluster.id}][400] Update default  &{Code:3 Details:[] Error:only the following fields can be updated at this time: [consul_version config.capacity_config.size] Message:only the following fields can be updated at this time: [consul_version config.capacity_config.size]}

I think the ip_allowlist field needs to be allowed for updates.

Edit: backend fix in the works, this client code remains 👍🏽

Ready to review, but I will update the test output and screenshots once fixed on the backend side.

Achooo · 2023-02-10T18:04:59Z

internal/provider/resource_consul_cluster.go

+		address := cidrMap["address"].(string)
+		description := cidrMap["description"].(string)
+
+		ip, _, _ := net.ParseCIDR(address)


Validation (validators.go) occurs first , so this should be valid (no error). I omitted handling it, but happy to do it if this feels like bad practice.

Achooo · 2023-02-13T16:40:20Z

internal/provider/resource_consul_cluster.go

+	}
+
+	// Seen holds validated CIDRs.
+	seen := make(map[string]struct{}, len(cidrs))


Use a hashmap to validate duplicates. See the following comment on why I went with this approach:

https://github.com/hashicorp/terraform-provider-hcp/pull/455/files#r1102997970

Alternatives discussed in that comment:

use schema.TypeSet for the ip_allowlist field

don't do validation ( backend does it)

internal/provider/resource_consul_cluster.go

internal/provider/validators.go

loshz · 2023-02-13T18:27:43Z

internal/provider/resource_consul_cluster.go

@@ -767,3 +832,37 @@ func resourceConsulClusterImport(ctx context.Context, d *schema.ResourceData, me

 	return []*schema.ResourceData{d}, nil
 }
+
+// buildIPAllowlist validates there are no duplicate CIDRs and returns a consul model.
+func buildIPAllowlist(cidrs []interface{}) ([]*consulmodels.HashicorpCloudConsul20210204CidrRange, error) {


this function is only used to check for duplicates, right?

Removed duplicate logic now, but it also converts the ip_allowlist from a terraform list (type []interface{}) to a model that can be used by the consul client.

Used across a few operations so it's nice to have it

Achooo · 2023-02-13T19:36:23Z

internal/provider/resource_consul_cluster.go

@@ -534,6 +565,19 @@ func setConsulClusterResourceData(d *schema.ResourceData, cluster *consulmodels.
 		}
 	}

+	var ipAllowlist []interface{}


This part in func setConsulClusterResourceData(...) is used for the read operation. See this line - nothing else is needed for read.

It's also used in to read the created/updated cluster once the API request is completed.

CC @loshz

JolisaBrownHashiCorp · 2023-02-13T19:50:36Z

internal/provider/resource_consul_cluster_test.go

+    	address = "172.25.16.0/24"
+    	description = "this is an IPV4 address"
+  	}
+`)


Could it be worthwhile to include an IPV6 address in the test case objects? Or do we currently only support IPV4 addresses?

Sure I can do that, I believe they are allowed. I checked the original RFC for this feature, but it's not explicit. Maybe @loshz can confirm?

Hmm looks like creation fails with IPV6...

@JolisaBrownHashiCorp We've confirmed that IPV6 is disallowed. I've added a validation to inform the user if they try to use one in this commit: 75ddd6a

Awesome. @Achooo Thanks for following up! May I ask what your slack handle is? I'm getting a deletion error when attempting to run the test suite and would like to inbox you the logs to discuss.

Sure thing, it's @ashvitha, I've sent you a DM :)

Achooo · 2023-02-13T21:00:16Z

internal/provider/resource_consul_cluster_test.go

@@ -193,7 +243,11 @@ func TestAccConsulCluster(t *testing.T) {
 				Config: testConfig(setTestAccConsulClusterConfig(updatedConsulCluster)),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckConsulClusterExists(resourceName),
-					resource.TestCheckResourceAttr(resourceName, "size", "SMALL"),
+					resource.TestCheckResourceAttr(resourceName, "size", "MEDIUM"),


I had to change to a STANDARD. This update test was failing with the following error:

Error: error updating Consul cluster (test-202302131304): [PATCH /consul/2021-02-04/organizations/{cluster.location.organization_id}/projects/{cluster.location.project_id}/clusters/{cluster.id}][400] Update default &{Code:3 Details:[] Error:Development tier clusters only have one size and cannot be changed. Message:Development tier clusters only have one size and cannot be changed.}

Development clusters cannot change sizes

add docs

loshz

Looking great! Have left a few minor comments/nits.

Planning to test this locally asap and report back.

loshz · 2023-02-16T18:23:19Z

internal/provider/resource_consul_cluster.go

@@ -534,6 +564,19 @@ func setConsulClusterResourceData(d *schema.ResourceData, cluster *consulmodels.
 		}
 	}

+	ipAllowlist := make([]map[string]interface{}, len(cluster.Config.NetworkConfig.IPAllowlist))


Do we need the nil NetworkConfig check here too?

loshz · 2023-02-16T18:24:12Z

internal/provider/resource_consul_cluster.go

+
+// buildIPAllowlist returns a consul model for the IP allowlist.
+func buildIPAllowlist(cidrs []interface{}) ([]*consulmodels.HashicorpCloudConsul20210204CidrRange, error) {
+	IPAllowList := make([]*consulmodels.HashicorpCloudConsul20210204CidrRange, len(cidrs))


Very minor nit:

Suggested change

IPAllowList := make([]*consulmodels.HashicorpCloudConsul20210204CidrRange, len(cidrs))

ipAllowList := make([]*consulmodels.HashicorpCloudConsul20210204CidrRange, len(cidrs))

internal/provider/resource_consul_cluster_test.go

internal/provider/validators.go

loshz · 2023-02-16T19:46:28Z

All of the E2E tests work for me on remote-dev. I was able to successfully create and update a cluster with an IP Allowlist, as well as load an existing cluster as a data source.

Commit formatting suggestions Co-authored-by: Dan Bond <[email protected]>

loshz

Solid work, Ash! 👍

Achooo commented Feb 10, 2023

View reviewed changes

internal/provider/resource_consul_cluster.go Show resolved Hide resolved

Achooo commented Feb 10, 2023

View reviewed changes

internal/provider/validators.go Show resolved Hide resolved

Achooo force-pushed the CC-4083/ip-allowlist branch from dff3ddd to 8bb7a16 Compare February 10, 2023 16:58

Achooo commented Feb 10, 2023

View reviewed changes

Achooo marked this pull request as ready for review February 10, 2023 17:32

Achooo requested a review from a team as a code owner February 10, 2023 17:32

Achooo requested a review from a team February 10, 2023 17:32

Achooo commented Feb 10, 2023

View reviewed changes

Achooo changed the title ~~[CC4083] IP Allowlist Feature~~ [CC-4083] IP Allowlist Feature Feb 10, 2023

Achooo changed the title ~~[CC-4083] IP Allowlist Feature~~ [CC-4083] IP Allowlist Feature for Consul Cluster Feb 10, 2023

Achooo commented Feb 13, 2023

View reviewed changes

loshz reviewed Feb 13, 2023

View reviewed changes

Achooo commented Feb 13, 2023

View reviewed changes

JolisaBrownHashiCorp reviewed Feb 13, 2023

View reviewed changes

Achooo commented Feb 13, 2023

View reviewed changes

Achooo requested review from JolisaBrownHashiCorp and loshz February 13, 2023 21:00

JolisaBrownHashiCorp approved these changes Feb 14, 2023

View reviewed changes

Achooo requested a review from bcmdarroch February 14, 2023 19:02

Achooo added 5 commits February 15, 2023 16:22

update hcp-sdk-go

b032d00

Got a working create

0d57f74

fixed go mod

a256e8a

Add changelog

103caff

PR Comment Improvement: use netip.ParsePrefix

9ca71b5

Achooo added 3 commits February 15, 2023 16:22

PR Comment improvement: Removed the duplicate check and fixed tests

084b1e7

IPV6 is disallowed

5665189

update docs

eb8750a

Achooo force-pushed the CC-4083/ip-allowlist branch from 7688839 to 468c6f5 Compare February 15, 2023 21:22

Achooo added 2 commits February 15, 2023 16:23

Added datasource read logic

5bac124

add docs

Fixed use make

9282b3a

Achooo force-pushed the CC-4083/ip-allowlist branch 3 times, most recently from 569011a to 8f90e92 Compare February 15, 2023 22:45

Fix nil check

fde3e7b

Achooo force-pushed the CC-4083/ip-allowlist branch from 8f90e92 to fde3e7b Compare February 15, 2023 22:46

loshz reviewed Feb 16, 2023

View reviewed changes

Achooo and others added 2 commits February 16, 2023 16:53

Apply suggestions from code review

82264ad

Commit formatting suggestions Co-authored-by: Dan Bond <[email protected]>

Fix PR comments

ddde020

loshz approved these changes Feb 16, 2023

View reviewed changes

Achooo merged commit 5b62635 into main Feb 17, 2023

Achooo deleted the CC-4083/ip-allowlist branch February 17, 2023 17:31

kalenarndt mentioned this pull request Mar 8, 2023

Add support for IP Allow List features on Consul Clusters kalenarndt/terraform-hcp-hcp#7

Open

aslamovamir mentioned this pull request Jan 18, 2024

HVT-4407 implement ip allowlist in terraform provider #625

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CC-4083] IP Allowlist Feature for Consul Cluster #455

[CC-4083] IP Allowlist Feature for Consul Cluster #455

Achooo commented Feb 10, 2023 •

edited

Loading

Achooo Feb 10, 2023

Achooo Feb 10, 2023

Achooo Feb 10, 2023

Achooo Feb 10, 2023 •

edited

Loading

bcmdarroch Feb 14, 2023

Achooo Feb 14, 2023 •

edited

Loading

Achooo Feb 14, 2023 •

edited

Loading

Achooo Feb 10, 2023 •

edited

Loading

Achooo Feb 10, 2023 •

edited

Loading

Achooo commented Feb 10, 2023 •

edited

Loading

Achooo Feb 10, 2023

Achooo Feb 13, 2023

loshz Feb 13, 2023

Achooo Feb 13, 2023

Achooo Feb 13, 2023 •

edited

Loading

JolisaBrownHashiCorp Feb 13, 2023

Achooo Feb 13, 2023

Achooo Feb 13, 2023

Achooo Feb 14, 2023 •

edited

Loading

JolisaBrownHashiCorp Feb 14, 2023

Achooo Feb 14, 2023

Achooo Feb 13, 2023

loshz left a comment

loshz Feb 16, 2023

loshz Feb 16, 2023

loshz commented Feb 16, 2023

loshz left a comment

	IPAllowList := make([]*consulmodels.HashicorpCloudConsul20210204CidrRange, len(cidrs))
	ipAllowList := make([]*consulmodels.HashicorpCloudConsul20210204CidrRange, len(cidrs))

[CC-4083] IP Allowlist Feature for Consul Cluster #455

[CC-4083] IP Allowlist Feature for Consul Cluster #455

Conversation

Achooo commented Feb 10, 2023 • edited Loading

🛠️ Description

🏗️ Acceptance tests

End to End Tests (int)

Resource CREATE

Datasource (READ ONLY)

Resource UPDATE

Validation

No more than 3 CIDRs

Description Length

Invalid CIDR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Achooo Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Achooo Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Achooo Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Achooo Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Achooo Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Achooo commented Feb 10, 2023 • edited Loading

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

Achooo Feb 13, 2023 • edited Loading

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

Achooo Feb 14, 2023 • edited Loading

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

loshz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loshz commented Feb 16, 2023

loshz left a comment

Choose a reason for hiding this comment

Achooo commented Feb 10, 2023 •

edited

Loading

Achooo Feb 10, 2023 •

edited

Loading

Achooo Feb 14, 2023 •

edited

Loading

Achooo Feb 14, 2023 •

edited

Loading

Achooo Feb 10, 2023 •

edited

Loading

Achooo Feb 10, 2023 •

edited

Loading

Achooo commented Feb 10, 2023 •

edited

Loading

Achooo Feb 13, 2023 •

edited

Loading

Achooo Feb 14, 2023 •

edited

Loading