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

[client] Don't choke on non-existent interface in route updates #2922

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

Conversation

lixmal
Copy link
Contributor

@lixmal lixmal commented Nov 21, 2024

Describe your changes

Improves debugging in cases like #2742

Issue ticket number and link

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@lixmal lixmal changed the title [client] Add interface index in windows route update log [client] Don't choke on non-existent interface in route updates Nov 21, 2024
Copy link

sonarcloud bot commented Nov 21, 2024

@@ -230,10 +230,13 @@ func (rm *RouteMonitor) parseUpdate(row *MIB_IPFORWARD_ROW2, notificationType MI
if idx != 0 {
intf, err := net.InterfaceByIndex(idx)
if err != nil {
return update, fmt.Errorf("get interface name: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the description of the PR, it was to help debug things only. But if you remove the return the logic will be different. Or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic is slightly different, we won't error out here, instead, we will return the interface info we have (index). Index is sufficient for the current checks we have in the network monitor for Windows.

This case where we cannot lookup the interface only happens if we get a route update notification from the OS at the same time as the interface is removed (e.g. the interface is removed and all routes that point to it are removed as well, like in the linked issue).

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.

2 participants