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

feat: Unsupported Transceivers #111

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

adambaumeister
Copy link
Collaborator

Description

In certain software versions, following an upgrade, it's observed that non-OEM Transceivers (SFPs) can become non-functional after a software upgrade. The unsupported_transceiver test fails when any such transceiver is found in output of "show system state".

Motivation and Context

If a user upgrades and is using unsupported transceivers that become unrecognized by the system, this is a potential major issue that could a severe network outage.

There are two major points up for debate before we can finish this test:

  • The majority of our customers may use non-OEM SFPs, which would mean this test fails all the time. As far as I can find, there is no further way to narrow down this test to a specific software version. Is this ok?
  • After looking at some real examples, there are optics that are listed in support documentation as "supported" but are not shown as "OEM" in the output of system state, so the test as implemented here would incorrectly mark them as failed. We could implement this list in code, but then we would be responsible for keeping it in sync which might not be good.

How Has This Been Tested?

  • Unit tests using real - though truncated - show system state details

The tests have been run against vm-series firewalls but of course they don't have SFPs so we can't fully validate this without a live hardware lab.

Types of changes

  • New feature (non-breaking change which adds functionality)

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.

I think it's good to have this, even though the test might fail for actually supported non-OEM SFPs, we can still use it to highlight this to the user and be aware of it during the upgrade. I think we can have the logic in the wrapper tools (Ansible, XSOAR) to fail or just to warn about it. Having said that I don't think we can maintain a manually updated list of SFPs but we can test only the OEM SFPs.

@adambaumeister
Copy link
Collaborator Author

I agree @alperenkose. Maybe we should rename the test to "non-OEM transceivers" instead?

@FoSix thoughts?

@horiagunica
Copy link
Collaborator

Just my personal thought - but the rest of the checks are Green light -> proceed with upgrade . If we reverse the logic - this one would look odd in the same context. Since the checks are a list and we can cherry pick which ones you want - we can maybe leave it as the initial proposal (fail check in case of non-OEM transceivers) ?

@FoSix
Copy link
Contributor

FoSix commented Sep 12, 2023

maybe we could do both?

  • with default settings check for non-OEM transceivers, fail if any are found
  • add an optional parameter that would be a list of ignored transceivers (question is in what format one should specify this list). This way we could add a possibility to ignore the ones that are supported w/o maintaining the list ourselves. One would have to of course verify the current list with the running HW. This alone is already fulfilling the check so maybe this option is pointless.

Is the non-oem, but supported list publicly available? Maybe we could download that list in the code and do some regex search on it?

FoSix and others added 5 commits September 12, 2023 15:16
## [0.2.0](v0.1.4...v0.2.0) (2023-09-13)

### ⚠ BREAKING CHANGES

* check for scheduled update jobs (#106)

### Features

* check for scheduled update jobs ([#106](#106)) ([4b303f9](4b303f9))
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@adambaumeister
Copy link
Collaborator Author

@FoSix the full list of "supported" transceivers is ONLY availalbe on the Live and requires a support login. So it's available to customers but is NOT publicly available.

I like your solution of an option to pass a list of additionally supported optics, I think that's a very clean and nice solution. I will work on it.

@adambaumeister adambaumeister marked this pull request as ready for review September 19, 2023 01:20
@adambaumeister adambaumeister changed the title Unsupported Transceivers feat: Unsupported Transceivers Sep 19, 2023
@adambaumeister
Copy link
Collaborator Author

As discussed, added an option to provide a list of regex that can be used as a comparison while testing for OEM optics. This allows the user to specify vendor part numbers like PAN-.* so they can specify which are supported in a scalable way.

FoSix and others added 5 commits September 20, 2023 15:35
## [0.3.0](v0.2.0...v0.3.0) (2023-09-20)

### ⚠ BREAKING CHANGES

* **FirewallProxy:** add a constructor (#119)

### Features

* **FirewallProxy:** add a constructor ([#119](#119)) ([e790bda](e790bda))
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Copy link
Contributor

@FoSix FoSix left a comment

Choose a reason for hiding this comment

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

@adambaumeister I've noticed that for whatever reason the CI PR did not trigger. When I run all checks locally I get some errors: some about formatting, but the biggest one is this one:

image

You have to rebase with main as well, some breaking changes were merged recently.

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
953 938 98% 95% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
panos_upgrade_assurance/init.py 100% 🟢
panos_upgrade_assurance/check_firewall.py 98% 🟢
panos_upgrade_assurance/exceptions.py 100% 🟢
panos_upgrade_assurance/firewall_proxy.py 100% 🟢
panos_upgrade_assurance/utils.py 98% 🟢
TOTAL 99% 🟢

updated for commit: b186eba by action🐍

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

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

@adambaumeister
Copy link
Collaborator Author

@FoSix I fixed that issue (was 3.11 prob), updated API docs and fixed the formatting issues. Anything outstanding here you think?

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.

LGTM

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.

None yet

5 participants