Skip to content
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

feat: support attaching interfaces to devices #176

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ednxzu
Copy link

@ednxzu ednxzu commented May 7, 2024

This PR includes the following changes:

  • allow specifying a device id as machine argument for maas_network_interface_physical and maas_network_interface_link resources

note: the current implementation does not permit editing the first created interface of a device, it needs to be imported, and I couldn't find a way around it. I'm also new to go, so please forgive the potentially glaring mistakes I may have made.

resolves: #170

@sempervictus
Copy link

sempervictus commented Jul 2, 2024

Physical nics and block devices cannot be created per se - logical absurdity if you think about it since those are not code products by physical components. Since they exist in machines independent of MaaS or TF, they cannot be created or destroyed in the real world. Treating hardpoint components as data however does permit building atop them:

}
data "maas_network_interface_physical" "metal-1" {
  for_each = maas_machine.metal-1
  machine  = each.value.id
  name     = "enp3s0"
}
resource "maas_network_interface_vlan" "metal-1" {
  for_each = data.maas_network_interface_physical.metal-1
  machine  = each.value.machine
  parent   = each.value.name
  vlan     = maas_vlan.default_50.id
  fabric   = maas_vlan.default_50.fabric
}
resource "maas_network_interface_link" "metal-1" {
  for_each          = data.maas_network_interface_physical.metal-1
  machine           = each.value.machine
  network_interface = each.value.id
  subnet            = maas_subnet.prov_subnet_1.id
  mode              = "STATIC"
  ip_address        = cidrhost(maas_subnet.prov_subnet_1.cidr, 3 + index(keys(data.maas_network_interface_physical.metal-1), each.key))
  default_gateway   = true
}
resource "maas_network_interface_link" "metal50-1" {
  for_each          = maas_network_interface_vlan.metal-1
  machine           = each.value.machine
  network_interface = each.value.id
  subnet            = maas_subnet.prov_50_subnet_1.id
  mode              = "STATIC"
  ip_address        = cidrhost(maas_subnet.prov_50_subnet_1.cidr, 3 + index(keys(data.maas_network_interface_physical.metal-1), each.key))
}

@ednxzu
Copy link
Author

ednxzu commented Jul 3, 2024

In the case of a device, interface are, from the POV of maas, created "out of thin air", since mass has not provisioned the machine, it does not know about its interfaces, so you essentially create it from nothing and can virtually put whatever you like and attach it to the device, hence the "issue" with not being able to edit the first interface.

@skatsaounis
Copy link
Collaborator

Hi @ednxzu and sorry for the delay.

Could you please rebase this PR on latest master so that CI can run?

In addition, please be aware that before we can merge any incoming PR, the author needs to sign the Canonical CLA.

Signing the CLA is mandatory for contributing to Canonical's open source projects. Please see the details here.

Copy link

github-actions bot commented Aug 1, 2024

Everyone contributing to this PR have now signed the CLA. Thanks!

@ednxzu
Copy link
Author

ednxzu commented Aug 1, 2024

Hello, I rebased the PR and signed CLAs, but I had my email private on github so I think the CI check needs a re-run.

@skatsaounis
Copy link
Collaborator

Thank you very much. Everything looks green now. I am wondering whether it would make more sense, instead of considering the machine field as something that can hold either a machine or a device, to add a new field device. That would look like this:

			"machine": {
				Type:        schema.TypeString,
-				Required:    true,
+				Optional:     true,
+				ExactlyOneOf: []string{"machine", "device"},
				ForceNew:    true,
				Description: "The identifier (system ID, hostname, or FQDN) of the machine with the network interface.",
			},
+                        "device": {
+				Type:         schema.TypeString,
+				Optional:     true,
+				ForceNew: true,
+				ExactlyOneOf: []string{"machine", "device"},
+				Description:  "The identifier (system ID, hostname, or FQDN) of the device with the network interface.",
+			},

That would require later an update to examples/resources/maas_network_link/resource.tf and a subsequent run of make generate_docs

Regarding the:

note: the current implementation does not permit editing the first created interface of a device, it needs to be imported, and I couldn't find a way around it. I'm also new to go, so please forgive the potentially glaring mistakes I may have made.

the resource itself lacks an import function at the moment. Maybe we want to add it. Regarding the inability to modify the initial interface of the device, I would love to try it locally and observe the error myself. Otherwise, feel free to share with me here the errors you are receiving.

@ednxzu
Copy link
Author

ednxzu commented Aug 1, 2024

Regarding the inability to modify the initial interface of the device, I would love to try it locally and observe the error myself. Otherwise, feel free to share with me here the errors you are receiving.

This (when I tested it which was a few weeks ago now) was inherent to the way the resource is created, when a new device is added, it cannot hold 0 interface, so eth0 is created, and is not tracked by terraform, so it becomes unmanageable unless you create the device, then import its eth0 interface

I am wondering whether it would make more sense, instead of considering the machine field as something that can hold either a machine or a device, to add a new field device. That would look like this

I would tend to agree, however, I don't think I am proficient enough i go to be able to implement it the correct way. I can try, but feel free to add to this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: maas_network_interface_physical should work for devices
3 participants