Skip to content

[NXOS] fix: do attempt to delete icmp redirection config on swithport interfaces #229

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

Closed
nikatza wants to merge 1 commit into from
Closed

[NXOS] fix: do attempt to delete icmp redirection config on swithport interfaces #229

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
11 changes: 7 additions & 4 deletions internal/provider/cisco/nxos/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,9 +947,10 @@ func (p *Provider) EnsureInterface(ctx context.Context, req *provider.EnsureInte
conf = append(conf, addr)
}

bfd := new(BFD)
bfd.ID = name
if req.Interface.Spec.BFD != nil {
bfd := new(BFD)
bfd.ID = name

f := new(Feature)
f.Name = "bfd"
f.AdminSt = AdminStEnabled
Expand Down Expand Up @@ -985,8 +986,10 @@ func (p *Provider) EnsureInterface(ctx context.Context, req *provider.EnsureInte
icmp.ID = name
switch req.Interface.Spec.Type {
case v1alpha1.InterfaceTypePhysical:
if err := p.client.Delete(ctx, icmp); err != nil {
return err
if req.Interface.Spec.Switchport == nil {
Copy link
Copy Markdown
Contributor

@felix-kaestner felix-kaestner Mar 13, 2026

Choose a reason for hiding this comment

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

Is this creating an error? Because the delete call seems to be working just fine for switchport interfaces.

λ gnmic -a 127.0.0.1 -u admin -p admin --port 9339 --skip-verify get --path 'System/intf-items/phys-items/PhysIf-list[id=eth1/12]/layer' --values-only --type config
[
  "Layer2"
]

λ gnmic -a 127.0.0.1 -u admin -p admin --port 9339 --skip-verify set --delete 'System/icmpv4-items/inst-items/dom-items/Dom-list[name=default]/if-items/If-list[id=eth1/12]'
{
  "source": "127.0.0.1",
  "timestamp": 1773406679784925525,
  "time": "2026-03-13T13:57:59.784925525+01:00",
  "results": [
    {
      "operation": "DELETE",
      "path": "System/icmpv4-items/inst-items/dom-items/Dom-list[name=default]/if-items/If-list[id=eth1/12]"
    }
  ]
}

Copy link
Copy Markdown
Contributor Author

@nikatza nikatza Mar 13, 2026

Choose a reason for hiding this comment

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

Ignore this commit for now, i am still investigating an issue that may have some relation to this. One thing that caught our attention is that we configure icmp settings on interfaces that have no addressing configured. I will probably change this and push into this PR.

Copy link
Copy Markdown
Contributor

@felix-kaestner felix-kaestner Mar 23, 2026

Choose a reason for hiding this comment

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

we configure icmp settings on interfaces that have no addressing configured

Is that really the case? The logic should be to disable icmp redirects on bfd-enabled interfaces, and making sure that there is no icmp redirects config for physical interfaces which have bfd disabled (hence the Delete). The delete should be there in any case, to clean up any potential leftovers, despite the current spec of the resource.

However, I've noticed that we also disabled icmp redirects, when a user explicitly disabled bfd in the spec of the resource, via .spec.bfd.enabled=false as we only checked for .spec.bfd=nil before updating the icmp redirection config. I've opened to #253 fix this.

if err := p.client.Delete(ctx, icmp); err != nil {
return err
}
}
case v1alpha1.InterfaceTypeLoopback:
icmp.Ctrl = "port-unreachable,redirect"
Expand Down
Loading