Skip to content

Fix httpapi plugin for ND4.1 #452

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 9 commits into
base: develop
Choose a base branch
from
Open

Conversation

mikewiebe
Copy link
Collaborator

@mikewiebe mikewiebe commented Jul 1, 2025

Short Summary

We discovered a problem in ND4.1 where the httpapi plugin gets stuck in a login loop. What I discovered is that the login method gets called and then immediately the logout method is called and we get stuck in a login/logout loop.

Updates in this PR:

  • Fix 4.1 login issue described in the summary
  • Run black formatter
  • Refactor to simplify and consolidate functions
  • Add unit pytests

NOTE: In addition to the new pytests this code has been testing using our VXLAN as Code collection nightly sanities

DCNM HttpAPI Plugin Refactoring Detailed Summary

Overview

This document summarizes the major changes made to plugins/httpapi/dcnm.py compared to the develop branch. The refactoring focused on code consolidation, improved maintainability, and enhanced error handling.


🔧 Code Refactoring

1. Login Method Consolidation

Before:

  • Separate methods: _login_old(), _login_latestv1(), _login_latestv2()
  • Each method handled different DCNM/NDFC versions independently
  • Duplicated error handling and authentication logic

After:

  • Unified _attempt_login() method
  • Configuration-driven approach using login_configs array
  • Single authentication flow handling all versions

Benefits:

  • ✅ Eliminated code duplication
  • ✅ Easier to add new authentication methods
  • ✅ Consistent error handling across all login types

2. Logout Method Simplification

Before:

  • _logout_old() for DCNM v11
  • _logout_latest() for NDFC v12+
  • Separate error handling for each version

After:

  • Unified _attempt_logout() method
  • Version-based configuration approach
  • Streamlined logout flow

Benefits:

  • ✅ Consistent logout behavior
  • ✅ Reduced code complexity
  • ✅ Better error messages

3. Request Handling Consolidation

Before:

  • send_request() and send_txt_request() had duplicate validation and error handling
  • Repeated path validation and connection checking

After:

  • New _send_request_internal() method handles common logic
  • Both public methods now delegate to the internal method
  • Single source of truth for request processing

Benefits:

  • ✅ DRY principle applied
  • ✅ Easier maintenance and debugging
  • ✅ Consistent error handling

📝 Code Quality Improvements

4. Constants Definition

Added module-level constants:

DCNM_VERSION = 11
NDFC_VERSION = 12
HTTP_SUCCESS_MIN = 200
HTTP_SUCCESS_MAX = 600
DEFAULT_LOGIN_DOMAIN = "local"
DEFAULT_RETRY_COUNT = 5

@mikewiebe mikewiebe added the wip Work In Progress - DO NOT MERGE YET label Jul 1, 2025
@mikewiebe mikewiebe added ready for review PR is ready to be reviewed and removed wip Work In Progress - DO NOT MERGE YET labels Jul 29, 2025
@@ -1,4 +1,4 @@
# Copyright (c) 2020-2023 Cisco and/or its affiliates.
# Copyright (c) 2020-2025 Cisco and/or its affiliates.
Copy link
Collaborator Author

@mikewiebe mikewiebe Jul 29, 2025

Choose a reason for hiding this comment

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

NOTE: I would not suggest reviewing the diffs but rather compare the code here to what we have in the develop branch. Much simpler code structure while maintaining the same functionality.

Copy link
Collaborator

@allenrobel allenrobel left a comment

Choose a reason for hiding this comment

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

Approving with just one question about the order in which we attempt login (v11 first, v12 second).

One other observation in the main plugins/httpapi/dcnm.py file, related to the following (lines 295-299).

        try:
            return json.loads(response_text) if response_text else {}
        # # JSONDecodeError only available on Python 3.5+
        except ValueError:
            return "Invalid JSON response: {0}".format(response_text)

Since our minimum Python version now supports JSONDecodeError should we use it? Also, maybe remove the extra # in the comment.

Really like the new unit tests and the cleaner code in general!

)
login_domain = self.get_option("login_domain") or "local"

# Define login configurations in order of preference
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering if the order should be to try v12 first since (hopefully) v11 sees less use than v12?

Please ignore though if the delay due to the v11 login attempt is minimal since this would require changes to multiple of the unit tests as well.

Copy link
Collaborator Author

@mikewiebe mikewiebe Jul 31, 2025

Choose a reason for hiding this comment

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

Good suggestion. I will update

@mikewiebe
Copy link
Collaborator Author

One other observation in the main plugins/httpapi/dcnm.py file, related to the following (lines 295-299).

Yeah, that is a good call. I will update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants