Skip to content

L3 subinterface configuration for Ciso NX-OS Provider#317

Draft
sven-rosenzweig wants to merge 3 commits intomainfrom
nxos/subinterface
Draft

L3 subinterface configuration for Ciso NX-OS Provider#317
sven-rosenzweig wants to merge 3 commits intomainfrom
nxos/subinterface

Conversation

@sven-rosenzweig
Copy link
Copy Markdown
Contributor

Implement basic CRUD for SubInterface resources:

  • Validate that the parent interface exists and is in a configured state before applying subinterface;
    halt reconciliation otherwise
  • Ensure parent interface is Layer 3 (no switchport configuration)
  • Watch parent interface for create/update events to trigger reconciliation when it becomes available
  • Cascade deletion: removing the parent interface also deletes associated subinterfaces

To support the cascading deletion of interfaces, we set owner reference on subinterface to parent
interface. Omitting device owner reference on subinterface as this
blocks deletion until device gets deleted.

Add a new "subinterface" interface type to support Layer 3 subinterfaces.
Layer 3 subinterfaces allow a single physical router interface to handle
traffic from multiple VLANs by creating separate logical interfaces per VLAN.

Validation rules:
- Encapsulation is allowed for subinterfaces only
- Supported encapsulation types: 802.1Q and 802.1ad
- Validate VLAN ID range
- 802.1ad requires two VLAN IDs (outer and inner VLAN)
- Subinterfaces cannot be configured as aggregate, switchport, or vlanRef
Comment on lines +321 to +332
CreateFunc: func(e event.CreateEvent) bool {
return true
},
UpdateFunc: func(e event.UpdateEvent) bool {
return false
},
DeleteFunc: func(e event.DeleteEvent) bool {
return false
},
GenericFunc: func(e event.GenericEvent) bool {
return false
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
CreateFunc: func(e event.CreateEvent) bool {
return true
},
UpdateFunc: func(e event.UpdateEvent) bool {
return false
},
DeleteFunc: func(e event.DeleteEvent) bool {
return false
},
GenericFunc: func(e event.GenericEvent) bool {
return false
},
UpdateFunc: func(e event.UpdateEvent) bool {
return false
},
GenericFunc: func(e event.GenericEvent) bool {
return false
},

This should trigger on create and delete events. Omitting the XFunc fields yields the default, which is true.

// Watches enqueues member of subinterfaces when their parent interface changes.
Watches(
&v1alpha1.Interface{},
handler.EnqueueRequestsFromMapFunc(r.physicalToSubinterfaces),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
handler.EnqueueRequestsFromMapFunc(r.physicalToSubinterfaces),
handler.EnqueueRequestsFromMapFunc(r.parentToSubinterfaces),

Again, this is not only physical interfaces (but also aggregates).

Reason: v1alpha1.ParentInterfaceNotReadyReason,
Message: fmt.Sprintf("referenced parent interface %q for not found", key),
})
return ctrl.Result{RequeueAfter: 0}, reconcile.TerminalError(fmt.Errorf("failed to get parent interface %q: %w", s.Interface.Spec.ParentInterfaceRef.Name, err))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ctrl.Result{RequeueAfter: 0}, reconcile.TerminalError(fmt.Errorf("failed to get parent interface %q: %w", s.Interface.Spec.ParentInterfaceRef.Name, err))
return ctrl.Result{}, reconcile.TerminalError(fmt.Errorf("failed to get parent interface %q: %w", s.Interface.Spec.ParentInterfaceRef.Name, err))

The RequeueAfter: 0 should be omitted everywhere in this file.

conditions.Set(s.Interface, metav1.Condition{
Type: v1alpha1.ConfiguredCondition,
Status: metav1.ConditionFalse,
Reason: v1alpha1.ParentInterfaceNotReadyReason,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Reason: v1alpha1.ParentInterfaceNotReadyReason,
Reason: v1alpha1.ParentInterfaceNotFoundReason,

I think this should be it's own reason.

}
}

var parentInterfaceRef *v1alpha1.Interface
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
var parentInterfaceRef *v1alpha1.Interface
var parentInterface *v1alpha1.Interface

This is not a ref. So this naming is a bit misleading.

Encap string `json:"encap"`
AdminSt AdminSt2 `json:"adminSt,omitempty"`
Descr string `json:"descr"`
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing fields:

Suggested change
}
Medium Medium `json:"mediumType"`
RtvrfMbrItems *VrfMember `json:"rtvrfMbr-items,omitempty"`

ethernetRe = regexp.MustCompile(`(?i)^(ethernet|eth)(\d+/\d+)$`)
loopbackRe = regexp.MustCompile(`(?i)^(loopback|lo)(\d+)$`)
portchannelRe = regexp.MustCompile(`(?i)^(port-channel|po)(\d+)$`)
subinterfaceRe = regexp.MustCompile(`(?i)^(ethernet|eth)(\d+/\d+)\.(\d+)$`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should also allow port-channel subinterfaces.

Might also rename this to encapRoutedRe in accordance with the struct.

}

v := "vlan-" + strconv.FormatInt(int64(req.Interface.Spec.Encapsulation.Tag), 10)
s.Encap = v
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something like this would be missing for the missing fields:

Suggested change
s.Encap = v
if req.IPv4 != nil {
s.RtvrfMbrItems = NewVrfMember(name, vrf)
}
s.Medium = MediumBroadcast
if isPointToPoint(req.Interface.Spec.IPv4) {
s.Medium = MediumPointToPoint
}

Comment on lines +972 to +973
v := "vlan-" + strconv.FormatInt(int64(req.Interface.Spec.Encapsulation.Tag), 10)
s.Encap = v
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
v := "vlan-" + strconv.FormatInt(int64(req.Interface.Spec.Encapsulation.Tag), 10)
s.Encap = v
s.Encap = "vlan-" + strconv.FormatInt(int64(req.Interface.Spec.Encapsulation.Tag), 10)

No point in creating the variable first. Also, should this be a switch statement on the encap type? And what happens for QinQ subinterfaces?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These test files also have a golden file with the cli command producing this as {$filename}.txt:

interface eth1/1.100
 encapsulation dot1q 100

Implement basic CRUD operations for SubInterfaces:
* Validate that the parent interface exists and is in a configured state before applying subinterface;
  halt reconciliation otherwise
* Ensure parent interface is Layer 3 (no switchport configuration)
* Watch parent interface for create/update events to trigger reconciliation when it becomes available
* Cascade deletion: removing the parent interface also deletes associated subinterfaces

To support the cascading deletion of interfaces, we set owner reference on subinterface to parent
interface. Omitting device owner reference on subinterface as this
blocks deletion until device gets deleted.
@github-actions
Copy link
Copy Markdown

Merging this branch will decrease overall coverage

Impacted Packages Coverage Δ 🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1 0.00% (ø)
github.com/ironcore-dev/network-operator/internal/controller/core 63.75% (-0.04%) 👎
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos 10.15% (-0.02%) 👎

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1/groupversion_info.go 0.00% (ø) 0 0 0
github.com/ironcore-dev/network-operator/api/core/v1alpha1/interface_types.go 0.00% (ø) 0 0 0
github.com/ironcore-dev/network-operator/api/core/v1alpha1/zz_generated.deepcopy.go 0.00% (ø) 0 0 0
github.com/ironcore-dev/network-operator/internal/controller/core/interface_controller.go 72.81% (-1.71%) 467 (+51) 340 (+30) 127 (+21) 👎
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/intf.go 18.88% (+0.44%) 143 (+2) 27 (+1) 116 (+1) 👍
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/name.go 54.17% (-0.38%) 24 (+2) 13 (+1) 11 (+1) 👎
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/provider.go 0.06% (-0.00%) 1660 (+21) 1 1659 (+21) 👎

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/ironcore-dev/network-operator/internal/controller/core/interface_controller_test.go
  • github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/intf_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants