-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: De-centralize the network to support multiple clusters #82
Conversation
main.tf
Outdated
@@ -60,7 +61,8 @@ resource "equinix_metal_device" "bastion" { | |||
user_data = templatefile("${path.module}/templates/bastion-userdata.tmpl", { | |||
metal_vlan_id = local.vxlan, | |||
address = cidrhost(var.cluster_subnet, 2), | |||
netmask = cidrnetmask(var.cluster_subnet), | |||
netmask = cidrnetmask(cidrsubnet(var.cluster_subnet, -1, -1)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Netmask should be optional variable to the module.
When netmask is given, it will be used. Your example will pass the VRF /21 netmask in to this module.
When netmask is omitted (default, empty), a local
will calculate it using the cidrnetmask
function.
Since netmask is only used in one place in the userdata script, this seems sufficient.
https://github.com/equinix-labs/terraform-equinix-metal-nutanix-cluster/blob/main/templates/bastion-userdata.tmpl#L44
main.tf
Outdated
@@ -134,7 +136,8 @@ resource "equinix_metal_device" "nutanix" { | |||
wait_for_reservation_deprovision = length(var.nutanix_reservation_ids) > 0 | |||
|
|||
ip_address { | |||
type = "private_ipv4" | |||
type = "private_ipv4" | |||
cidr = 21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The private_addr
CIDR assigned on equinix_metal_device create can not be a /21
.
ip_address
represents the Layer3, per-server, IP assignments.
Modifying the VRF range to represent a /21 would be done in your example (#68), not in the main module.
The range passed to this module would be a /22
, from which the gateway would be derived or its IP also passed in as a variable.
@@ -51,12 +51,15 @@ write_files: | |||
dhcp-range=${host_dhcp_start},${host_dhcp_end},${lease_time} | |||
dhcp-mac=set:${set},${nutanix_mac} | |||
dhcp-range=tag:${set},${vm_dhcp_start},${vm_dhcp_end},${lease_time} | |||
dhcp-option=option:netmask,${netmask} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious how this was working without the netmask previously defined. Perhaps the default behavior was to use the netmask and gateway from the host's interface where the DHCP range fits.
I think these explicit definitions do make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it's blocking the connectivity to outside. Unable to download the image.
templates/bastion-userdata.tmpl
Outdated
packages: | ||
- iptables-persistent | ||
- expect | ||
- sshpass | ||
- dnsmasq | ||
runcmd: | ||
- sysctl -p /etc/sysctl.d/10-ip-forwarding.conf | ||
- sysctl -w net.ipv4.ip_forward=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is redundant to the line above it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in latest commits
How to verify/confirm metal gateways are reachable OR not? |
Two clusters(like Bastion, Nutanix AHV, CVM controller) are to reachable to their gateways IP's
Cluster -A is not reachable for Cluster -B Gateway, Do you think, do we have to add any firewall rules ? |
I can successfully ping VIPs between clusters. |
variables.tf
Outdated
variable "cluster_gateway" { | ||
description = "The cluster gateway IP address" | ||
type = string | ||
default = "192.168.96.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this the default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in latest commits, Can you have a quick review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the default value, made it as User Optional Input, In case user didn't provided it, module it's self generate from mandatory varialbe cluster_subnet.
PR ready for review |
main.tf
Outdated
@@ -20,6 +21,10 @@ resource "terraform_data" "input_validation" { | |||
condition = (var.metal_project_name != "" && var.metal_project_id == "") || (var.metal_project_name == "" && var.metal_project_id != "") | |||
error_message = "One (and only one) of `metal_project_name` or `metal_project_id` is required" | |||
} | |||
precondition { | |||
condition = (var.cluster_subnet != "" && var.cluster_subnet == "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is impossible. I don't believe we need a precondition for this at all.
Did you mean to check if var.cluster_subnet is not empty but local.cluster_gateway is empty, in which case cluster_gateway would be required?
@ctreatma / @displague ,
Is there any known issues in Infra side. oberserved the issue in main branch too. |
@codinja1188 I think someone else ran into a similar problem recently. Could you run with One thing to note is that the parse error is happening on an error response from the API, which likely means there's something wrong with the attributes being passed in to terraform rather than a problem inside the terraform provider. When we've seen this parse error before, it seemed to be triggered by sending invalid IP addresses to the API, so you should double-check that you're not including unnecessary slashes or cidr notation in the |
@displague / @ctreatma , I verified, it's working. plz check and approve it. |
Description:
This pull request introduces changes to decentralize the network setup in the Terraform Equinix Metal Nutanix cluster module to support multiple clusters. Key modifications include:
Additional issues it fixes.
https://github.com/equinix-labs/terraform-equinix-metal-nutanix-cluster/issues/74