Skip to content

[For review] - Proposed update to implementation for RESTCONF platforms #145

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

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

Conversation

palmersample
Copy link

While working on a PR to support host / IPv6 addressing on some implementations, I noticed some differences in how implementations handled the same tasks. There were some inconsistencies across supported testbed attributes and some code duplication.

Requesting review of changes to add a new RestconfImplementation class at the top-level - this would permit existing implementations to be migrated as they are updated / tested without impacting current deployments.

  • Add support for SOCKS 4/4a/5/5h proxy types as well as current HTTP proxy.
  • Refactor 'utils' module into a package so additional features such as response hooks can be consolidated.
  • Add default response hook to catch response exceptions and re-raise.
  • Add OPTIONS and HEAD to base implementation (required by RFC8040)
  • Update base implementation as well as iosxe to simplify code and enable 'host' key in testbed.
  • Add unittests for IPv6 and FQDN instances and default response hook.
  • Add utils module to unittests with helper function to generate a mocker URL for a given connection type
  • Add checking for testbed attribute "verify" to enable/disable TLS chain validation with default of False for backward compatibility (I see this was completed for the February 2025 release - should be no difference in code)

After a super().init() call, the calling implementation will have a requests.Session() object and can then perform any request by calling the self.session._request(*args, **kwargs), similar to the familiar requests interface.

In the end - this solves my initial requirements of supporting a 'host:' key for the IOS XE implementation as well as the IPv6 URL support... with some added code.

If this looks usable / feasible, I will update the documentation for the iosxe implementation and will have APIC support following close behind.

Tagging @omehrabi as requested

…support SOCKS 4/4a/5/5h proxy types as well as HTTP proxy.

- Refactor 'utils' module into a package so additional features such as response hooks can be consolidated.
- Add default response hook to catch response exceptions and re-raise.
- Add OPTIONS and HEAD to base implementation (required by RFC8040)
- Update base implementation as well as iosxe to simplify code and enable 'host' key in testbed.
- Add unittests for IPv6 and FQDN instances and default response hook.
- Add utils module to unittests with helper function to generate a mocker URL for a given connection type
- Add checking for testbed attribute "verify_tls" to enable/disable TLS chain validation with default of False for backward compatibility
@palmersample palmersample requested a review from a team as a code owner March 17, 2025 17:41
@palmersample
Copy link
Author

Note: I just now realized that some of the request prep tasks won't apply to APIC, so will move some of the RestconfImplementation._request() code out to make it more generic. Easy enough to fix if the determination is made to continue work on this PR for eventual inclusion.

@palmersample
Copy link
Author

Adding this comment per Webex discussion, requested to include the following overview:

a near-complete refactoring of the implementation, but based on the fact that

  1. Most implementations use almost identical code - but there is no consistency. That's just how it came to be over the years, I'm guessing
  2. The refactoring is designed around RFC8040 which is the standard for RESTCONF. New methods added (HEAD and OPTIONS).
  3. Moved some code to different modules so we can extend it easily - such as custom Exception classes, which would probably be desired.
  4. Some nuances weren't included - such as performing a Response.raise_for_status() after a request to check for errors instead of just trying to access the data. And, for the implementations that have a Retry / retry_wait that enters a while loop - I'll refactor those to use the proper method which is attaching an adapter to the Requests.session() object to handle the retries properly. I've done this a few times with my own projects, it works well, and I have code to include for the unit testing when we get to that point
  5. probably missing some things - but my goal was to improve readability, maintainability, and extensibility by reducing duplicated / inconsistent code that eventually performed the same tasks.
  6. Ensure full backward-compatibility by making a new class that can be inherited as implementations get upgraded. No impact to current make test output, but the IOSXE implementation has been migrated and passes all tests. Added a couple new ones also and refactored the unittest code for it

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.

1 participant