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.

Sign up for GitHub

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

allow to add ips to vrouter nics #565

Merged
merged 8 commits into from
Nov 26, 2024
Merged

allow to add ips to vrouter nics #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
efefbd6
allow to add ips to vrouter nics
Thiryn Jul 29, 2024
16684e0
Update the test to have the IP range match the network AR
Thiryn Aug 20, 2024
71630fa
set the ip upon vrouter read and adapt tests
Thiryn Aug 21, 2024
e8ed893
remove the test step that was deleting the second vrouter
Thiryn Aug 21, 2024
482fea9
add back mistakenly removed test step
Thiryn Aug 21, 2024
0f79e87
Add dependency to prevent the VRouter VM to be deleted after the NIC
Thiryn Aug 22, 2024
fbcf514
Make network wait for leases to be free upon delete. Add tests for NI…
Thiryn Aug 26, 2024
eca6a1d
update resource naming for consistency; fix wrong timeout used in vir…
Thiryn Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions opennebula/helpers_vr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ func vrNICAttach(ctx context.Context, timeout time.Duration, controller *goca.Co
updatedNICsLoop:
for i, nic := range updatedNICs {

// For VRouter NICs, floating IPs are set using the "IP" field, but it is represented in the ON API
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless I miss something it seems that this whole code part in helpers_vr.go is useless ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These few lines enable the IP to be set on the NIC resource for vrouter. On VRouter NICs that use floating IP, ON represents the IP in the VROUTER_IP field.
This loop is ran when updating NICs. Without this, when running terraform apply to update the NIC would yield an empty IP (and therefore trigger a recreation when not necessary)

// as the "VROUTER_IP" field.
if vrouterIP, err := nic.GetStr("VROUTER_IP"); err == nil {
if _, err = nic.GetStr("IP"); err != nil {
nic.Add("IP", vrouterIP)
}
}
for _, pair := range nicTpl.Pairs {

value, err := nic.GetStr(pair.Key())
Expand Down
21 changes: 11 additions & 10 deletions opennebula/resource_opennebula_virtual_network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1694,15 +1694,16 @@ func resourceOpennebulaVirtualNetworkDelete(ctx context.Context, d *schema.Resou
}
log.Printf("[INFO] Successfully released reservered IP addresses.")

err = vnc.Delete()
if err != nil {
diags = append(diags, diag.Diagnostic{
Severity: diag.Error,
Summary: "Failed to delete",
Detail: fmt.Sprintf("virtual network (ID: %s): %s", d.Id(), err),
})
return diags
}
err = resource.RetryContext(ctx, d.Timeout(schema.TimeoutDelete), func() *resource.RetryError {
err = vnc.Delete()
if err != nil {
if strings.Contains(err.Error(), "Can not remove a virtual network with leases in use") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}
return nil
})

timeout := d.Timeout(schema.TimeoutDelete)
transient := []string{vn.Init.String(), vn.Ready.String()}
Expand All @@ -1716,7 +1717,7 @@ func resourceOpennebulaVirtualNetworkDelete(ctx context.Context, d *schema.Resou
return diags
}

log.Printf("[INFO] Successfully deleted Vnet\n")
log.Printf("[INFO] Successfully deleted Vnet (ID: %s)\n", d.Id())
return nil
}

Expand Down
18 changes: 18 additions & 0 deletions opennebula/resource_opennebula_virtual_router_nic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ func resourceOpennebulaVirtualRouterNIC() *schema.Resource {
Default: false,
ForceNew: true,
},
"ip": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
},
},
}
}
Expand Down Expand Up @@ -137,6 +143,10 @@ func resourceOpennebulaVirtualRouterNICCreate(ctx context.Context, d *schema.Res
}
}

if v, ok := d.GetOk("ip"); ok {
nicTpl.Add("IP", v.(string))
}

// wait before checking NIC
nicID, err := vrNICAttach(ctx, d.Timeout(schema.TimeoutCreate), controller, vRouterID, nicTpl)
if err != nil {
Expand Down Expand Up @@ -209,6 +219,13 @@ func resourceOpennebulaVirtualRouterNICRead(ctx context.Context, d *schema.Resou

floatingIP, _ := nic.GetStr("FLOATING_IP")
floatingOnly, _ := nic.GetStr("FLOATING_ONLY")
ip, _ := nic.Get("IP")

// For VRouter NICs, floating IPs are set using the "IP" field, but it is represented in the ON API
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny typo: ONE

Copy link
Collaborator

@sk4zuzu sk4zuzu Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I believe this implementation suggestion is incorrect. If you look at implementation of a virtual machine https://registry.terraform.io/providers/OpenNebula/opennebula/latest/docs/resources/virtual_machine#nic there is the computed_ip "attribute" at the same time the ip is a "parameter". The VR NIC is somewhat similar and I believe it should follow the same convention. 🤔

Copy link
Contributor Author

@Thiryn Thiryn Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the need for consistency, but do we really want two fields, an ip to set and a computed_ip to get? It seems counterintuitive, and a bit of an anti-pattern.

// as the "VROUTER_IP" field.
if strings.ToUpper(floatingIP) == "YES" {
ip, _ = nic.Get("VROUTER_IP")
}

d.Set("network_id", networkID)
d.Set("virtual_router_id", vr.ID)
Expand All @@ -219,6 +236,7 @@ func resourceOpennebulaVirtualRouterNICRead(ctx context.Context, d *schema.Resou
d.Set("security_groups", sg)
d.Set("floating_ip", strings.ToUpper(floatingIP) == "YES")
d.Set("floating_only", strings.ToUpper(floatingOnly) == "YES")
d.Set("ip", ip)

return nil
}
Expand Down
Loading
Loading