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

fix: route snapshot missing entries fix #146

Merged
merged 11 commits into from
Feb 16, 2024
Merged

fix: route snapshot missing entries fix #146

merged 11 commits into from
Feb 16, 2024

Conversation

acelebanski
Copy link
Contributor

@acelebanski acelebanski commented Jan 22, 2024

Description

The route table snapshot was missing entries when ECMP routes (differed only by next-hop) were present. This was because dict key generation was based only on VR name, destination CIDR and interface (if applicable). This PR adds next-hop IP address or name to dict key, so the keys are unique every time.

Motivation and Context

#56

How Has This Been Tested?

It has been tested with example python scripts.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

Copy link

github-actions bot commented Jan 24, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1056 1036 98% 95% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
panos_upgrade_assurance/firewall_proxy.py 99% 🟢
panos_upgrade_assurance/snapshot_compare.py 99% 🟢
TOTAL 99% 🟢

updated for commit: aff9f50 by action🐍

Copy link

A Preview PR in PanDev repo has been created. You can view it here.

@acelebanski acelebanski marked this pull request as ready for review January 24, 2024 11:47
Copy link
Collaborator

@alperenkose alperenkose left a comment

Choose a reason for hiding this comment

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

Lets check if we have a similar issue with get fib routes, if different next hop is possible for same destination/interface. We can merge this afterwards.

@acelebanski
Copy link
Contributor Author

Lets check if we have a similar issue with get fib routes, if different next hop is possible for same destination/interface. We can merge this afterwards.

We had identical issue with FIB entries when ECMP was used, nice catch @alperenkose! Updated the PR accordingly.

Copy link
Collaborator

@adambaumeister adambaumeister left a comment

Choose a reason for hiding this comment

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

Makes sense, LGTM

@alperenkose alperenkose merged commit d946462 into main Feb 16, 2024
7 checks passed
@alperenkose alperenkose deleted the 56-route-key branch February 16, 2024 11:09
github-actions bot pushed a commit that referenced this pull request Mar 7, 2024
## [0.3.4](v0.3.3...v0.3.4) (2024-03-07)

### Features

* bgp peers snapshot comparison ([#154](#154)) ([4fec622](4fec622))

### Bug Fixes

* calculate_diff_on_dicts to Support Integer Values for ARP Table ttl ([#153](#153)) ([47ebea5](47ebea5))
* **firewall_proxy/is_panorama_connected:** Added fix for panorama check for FWs in version PAN-OS 11 or later ([#159](#159)) ([e617dbc](e617dbc))
* route snapshot missing entries fix ([#146](#146)) ([d946462](d946462))
@horiagunica
Copy link
Collaborator

🎉 This PR is included in version 0.3.4 🎉

The release is available on PyPI and GitHub release

Posted by semantic-release bot

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.

Routes snapshot missing route entries for different next-hop addresses
4 participants