puppetlabs / puppetlabs-pecdm

Puppet Bolt driven fusion of puppetlabs/peadm and Terraform.
Apache License 2.0
14 stars 18 forks source link

(feat) Allow for specifying the IP path from terraform #59

Closed jarretlavallee closed 2 years ago

jarretlavallee commented 2 years ago

Prior to this PR, the URI in the terraform resolve_references was hard coded. As different terraform configurations may have different paths to the IP address, the code needs to be dynamic to handle the differences. This commit allows for the terraform output to present an ip_path output to specify the path to the IP address for the resource. If it is not specified, the default will be used.

This can be used with an output like the following.

output "ip_path" {
  value       = var.subnetwork_project == null ? "network_interface.0.access_config.0.nat_ip" : "network_interface.0.network_ip"
  description = "The path to the IP Address for the instance"
}
ody commented 2 years ago

The proposed solution is limited in flexibility. As is it would result in anything attached to a shared network to require SSH via a private IP and I don't think there is any reason to assume that to be true for shared networks outside Puppet's internal usage.

While working on another feature request I incidentallly added something that has a similar effect but leaned explicit over implicit.

https://github.com/puppetlabs/puppetlabs-pecdm/blob/attach_exiting_network/plans/provision.pp#L17

This makes me think more about if there is an automatic solution for discovery the path though. I'd like to opt for leaving the existing linked solution as is, if we can come up with a more flexible solution for path discovery then linked solution just serves as an override.

ody commented 2 years ago

...a list which collects all public IP address values from each provisioned instance then an expression which returns the private IP address path if any element in the list are null?

ody commented 2 years ago

Ah. Looking at the Terraform code, we are already assuming that if a shared subnet is being used then instances don't receive public IPs. I wonder if that is a correct assumption...?

ody commented 2 years ago

@jarretlavallee Here is what I came up with: https://github.com/ody/tf-hacking-space/blob/main/outputs.tf

If any instance lacks a public IP then the path that is returned is network_ip, if they all have public IPs assigned then it'll return the nat_ip path. The usage of the data resource is to ensure the state is queried after the modifications are complete or else stale data gets returned frequently.

jarretlavallee commented 2 years ago

@ody That looks great to me. I think you are right that we are making assumptions in the terraform code, which adds the complexity as we now have to conditionally handle which output will be. I wonder if it is worth having a parameter to determine if the internal or external IP will be used.

I think the https://github.com/ody/tf-hacking-space/blob/main/outputs.tf looks good and fits with the pattern in this PR. Is this PR the best way to go? I put it up thinking that it would allow flexibility from the terraform code in a way that was optional and could be more dynamic on a per terraform code basis.

ody commented 2 years ago

@jarretlavallee I think you may be correct, we should likely include a parameter which determines if a public IP is used or not. The added benefit of doing that is that in situations where we are provisioning a new VPC, we can include a NAT gateway if a public IP is not used so that the instance can still reach the other networks (including the internet).

jarretlavallee commented 2 years ago

@ody I like that it will be much more explicit and allow for more functionality. I'll close out this PR as it is not the right direction. It seems like we can add a parameter to the plan to use the public IP as the default. We can then pass it into terraform and replace the logic to switch on the variable instead of the subnet. Does that sound like a good way to appoach it to you?

jarretlavallee commented 2 years ago

closing this in favor of #65