HCCP91 hvn routes data source

[nombiezinja]

🛠️ Description

Jira: https://hashicorp.atlassian.net/browse/HCCP-90

Adds the hcp_hvn_route data source.; another PR will follow to add the resource.

This data source is the first in the provider to use self_link (change introduced in #111) as references in lieu of ids (e.g. hvn_id)

Example output from read:

[xargs-P]

@nombiezinja do you have example output for the data source with more than one route? Is it just a list?
Also I do not think we support route priority at the moment, but if so, how would priority be reflected in the list?

[smaant]

@xargs-P this datasource is intended to return only single route (there could be only one route for each CIDR)

[bcmdarroch]

Nice work on this @nombiezinja! Definitely a complex challenge for your first provider contribution.

I had some comments, and a proposal. This data source could be made more flexible if we turned it into something like AWS's subnet IDs data source, which always returns a list.

I would like to know more about how users might use this data source. Will they usually need the list, or just one route, or both? I'm leaning towards having both a singular and plural version of this data source for the greatest amount of flexibility.

Happy to make some time to chat about this over zoom!

[xargs-P]

@xargs-P this datasource is intended to return only single route (there could be only one route for each CIDR)

Got it @smaant . Related to priority, are we tracking? If so, is that expressed in the data source?

[smaant]

@xargs-P actually just discussed with Ti, we would take Brenna's suggestion and make it be able to return multiple routes. For routes priority, it is always from more specific cidr to less specific, this is a standard behavior for route tables:

• https://docs.aws.amazon.com/vpc/latest/userguide/VPC_Route_Tables.html#route-table-routes
• https://docs.microsoft.com/en-us/azure/virtual-network/virtual-networks-udr-overview#how-azure-selects-a-route

[nombiezinja]

@xargs-P actually just discussed with Ti, we would take Brenna's suggestion and make it be able to return multiple routes. For routes priority, it is always from more specific cidr to less specific, this is a standard behavior for route tables:

• https://docs.aws.amazon.com/vpc/latest/userguide/VPC_Route_Tables.html#route-table-routes
• https://docs.microsoft.com/en-us/azure/virtual-network/virtual-networks-udr-overview#how-azure-selects-a-route

Just looked into the topic of data source returning multiple values a bit more; it looks like the existing pattern for writing providers is to have one data source for singular entity, and one for plurals. examples:

https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/data_source_aws_outposts_outpost.go
https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/data_source_aws_outposts_outposts.go

https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/data_source_aws_security_group.go
https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/data_source_aws_security_groups.go

seems like it might make sense to keep the current data source the way it is, and keep destination_cidr a required param. when the need arises to query all routes for an HVN, we can create another data source for plural hvn routes, following existing patterns like the examples above

[xargs-P]

Got it 👍 @nombiezinja

[xargs-P]

Thanks for the examples 👌

@bcmdarroch good for you?

[roaks3]

@xargs-P Brenna is out today, but she did ping me before she signed off yesterday to say that the singular version of this data source is good for her if it's good for you 😄 . So sounds like we're happy to move forward with the data source the way it is in this PR

[nombiezinja]

Added validation for destination_cidr. Originally I was planning to use the IsCIDR helper as @bcmdarroch had suggested, and use validation.All following the example here(https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/resource_aws_eks_cluster.go#L133). However, I went with a custom validator for 3 reasons:

1- terraform-provider-hcp only uses ValidateDiagFunc, and validation.All(https://github.com/hashicorp/terraform-plugin-sdk/blob/v1.17.2/helper/validation/meta.go#L30) would not work
2- ValidateFunc is deprecated (https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L308) but most importantly
3- HCP create HVN route endpoint validation (https://github.com/hashicorp/cloud-network/blob/master/service/network/private/service_create_hvn_route.go#L218) uses the validation rule here: https://github.com/hashicorp/cloud-sdk/blob/master/validation/rule_cidrblock.go#L0-L1; this validation is reused in several HCP APIs taking a CIDR parameter.

validateIsStartOfPrivateCIDRRange validates based on the same rules as the cloud-sdk CIDR validation:

a) CIDR is valid
b) CIDR is private CIDR block
c) Address is at beginning of CIDR range

We could reuse this validator elsewhere in the provider too

[smaant]

I would suggest to remove validation here (and probably in other places too), the least duplicated logic we have in Tf provider the easier it will be to maintain. Basic request validation will be done by the backend very quickly and won't impact UX of the TF provider.

[roaks3]

Thanks for adding client-side validation for this! Looks great, and agreed that this validator should be used for our other CIDRs

[roaks3]

Oops, just missed @smaant 's response on this. IMO it is reasonable to have this be validated client-side (I'm assuming it is generic and unlikely to change over time), and another slight benefit is that the user will get feedback on the plan step instead of seeing it later during apply. But otherwise, I do agree that this could be deferred to the backend and the experience would be similar.

[nombiezinja]

Replaced the custom validator with IsCIDR as we are looking to gain some traction on this work, and merged this PR

Also spoke to provider team (@bcmdarroch & @roaks3 ) as well as some Terraform practitioners; my recommendation is to ensure the custom validator is added back at some point. The aforementioned 3 rules are rudimentary and are unlikely to be removed from the cloud-sdk validation package, hence drift is less of a concern. If the validation rules in cloud-sdk were to change, we are unlikely to remove rules, and only likely to add additional rules, in which case the custom validator covering 3 existing rules will be a good idea in terms of improving the practitioner experience for this provider.

[roaks3]

LGTM