Skip to content

Add peer interface annotation for physical interfaces #191

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

Open
felix-kaestner wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add peer interface annotation for physical interfaces #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions api/core/v1alpha1/groupversion_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ const VRFLabel = "networking.metal.ironcore.dev/vrf-name"
// to trigger certain disruptive operations, such as reboots or firmware upgrades.
const DeviceMaintenanceAnnotation = "networking.metal.ironcore.dev/maintenance"

// InterfacePeerAnnotation references the peer Interface resource on the other end of a physical link.
// The value must be a reference to another Interface resource in the format "namespace/name"
// or just "name" (for same-namespace references).
// This annotation is only valid for interfaces of type Physical.
const InterfacePeerAnnotation = "networking.metal.ironcore.dev/peer-interface"
Comment on lines +66 to +70
Copy link
Contributor

@nikatza nikatza Feb 19, 2026

Choose a reason for hiding this comment

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

I think aggregated interfaces should be allowed too: "An An LLDP agent can be configured on an IEEE 802.1AX™ Aggregated Port and/or on any number of the physical ports belonging to the Aggregation" [IEEE8021AB-2016]. May be we could leave a note here or extend the commit message if we want to enforce this condition for now.

Copy link
Contributor Author

@felix-kaestner felix-kaestner Feb 19, 2026

Choose a reason for hiding this comment

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

I would expect that configuring LLDP on an Aggregate Interface, would lead to configuring it's member Interfaces. As the Aggregate Interface is a logical/virtual construct, for me it's not directly related to the physical cabling, so in this case I would expect that annotation to be on the members instead and not on the aggregate. But I'm happy to discuss.

Copy link
Contributor

@nikatza nikatza Feb 19, 2026

Choose a reason for hiding this comment

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

Fair point. We also discussed it in our alignment and we won't consider link aggregation. In this context, what do you think about renaming the Annotation to PhysicalInterfaceNeighborAnnotation = "networking.metal.ironcore.dev/interface-neighbor". This would also be more consistent with the LLDP terminology.

Comment on lines +66 to +70
Copy link
Contributor

@nikatza nikatza Feb 19, 2026

Choose a reason for hiding this comment

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

I like using the reference to another resource here. However, I am wondering how we will deal with those interfaces that have peers outside our control and for which we do not have an Interface resource. We won't have a way to verify the cabling.

Copy link
Contributor

@nikatza nikatza Feb 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

We also discussed this and our use case includes also verifying the cabling of interfaces connected to other devices and interfaces that we do not control. Usually the neighbor is a combination of the chassisID or sysName (typically the first since that one is mandatory), and the portId, which is the MAC, the ifAliasor ifName (typically the last one).

So in principle we would like to support both a reference to a physical interface and some sort of string. While this is not immediately pressing, its probably considering this already. What do you think?


// Device maintenance actions that can be requested via the DeviceMaintenanceAnnotation.
const (
// DeviceMaintenanceReboot requests a device reboot.
Expand Down
25 changes: 23 additions & 2 deletions internal/webhook/core/v1alpha1/interface_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,32 @@ func (v *InterfaceCustomValidator) ValidateDelete(ctx context.Context, obj runti

// validateInterfaceSpec performs validation on the Interface spec.
func validateInterfaceSpec(intf *v1alpha1.Interface) error {
if intf.Spec.IPv4 == nil {
var errAgg []error

if err := validateInterfacePeerAnnotation(intf); err != nil {
errAgg = append(errAgg, err)
}

if intf.Spec.IPv4 != nil {
if err := validateInterfaceIPv4(intf.Spec.IPv4); err != nil {
errAgg = append(errAgg, err)
}
}

return errors.Join(errAgg...)
}

// validateInterfacePeerAnnotation validates that the InterfacePeerAnnotation is only used on Physical interfaces.
func validateInterfacePeerAnnotation(intf *v1alpha1.Interface) error {
if _, ok := intf.Annotations[v1alpha1.InterfacePeerAnnotation]; !ok {
return nil
}

return validateInterfaceIPv4(intf.Spec.IPv4)
if intf.Spec.Type != v1alpha1.InterfaceTypePhysical {
return fmt.Errorf("annotation %q is only valid for interfaces of type %s, but interface has type %s", v1alpha1.InterfacePeerAnnotation, v1alpha1.InterfaceTypePhysical, intf.Spec.Type)
}

return nil
}

// validateInterfaceIPv4 performs validation on the InterfaceIPv4 spec.
Expand Down
36 changes: 36 additions & 0 deletions internal/webhook/core/v1alpha1/interface_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,41 @@ var _ = Describe("Interface Webhook", func() {
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("overlaps with"))
})

It("Should allow peer-interface annotation on Physical interfaces", func() {
obj.Spec.Type = v1alpha1.InterfaceTypePhysical
obj.Annotations = map[string]string{
v1alpha1.InterfacePeerAnnotation: "default/peer-interface",
}
_, err := validator.ValidateCreate(context.Background(), obj)
Expect(err).NotTo(HaveOccurred())
})

It("Should reject peer-interface annotation on Loopback interfaces", func() {
obj.Spec.Type = v1alpha1.InterfaceTypeLoopback
obj.Annotations = map[string]string{
v1alpha1.InterfacePeerAnnotation: "default/peer-interface",
}
_, err := validator.ValidateCreate(context.Background(), obj)
Expect(err).To(HaveOccurred())
})

It("Should reject peer-interface annotation on Aggregate interfaces", func() {
obj.Spec.Type = v1alpha1.InterfaceTypeAggregate
obj.Annotations = map[string]string{
v1alpha1.InterfacePeerAnnotation: "default/peer-interface",
}
_, err := validator.ValidateCreate(context.Background(), obj)
Expect(err).To(HaveOccurred())
})

It("Should reject peer-interface annotation on RoutedVLAN interfaces", func() {
obj.Spec.Type = v1alpha1.InterfaceTypeRoutedVLAN
obj.Annotations = map[string]string{
v1alpha1.InterfacePeerAnnotation: "default/peer-interface",
}
_, err := validator.ValidateCreate(context.Background(), obj)
Expect(err).To(HaveOccurred())
})
})
})