BUG1413418: Enable pybsn to connect to port 443 with the /a/api prefix#446
BUG1413418: Enable pybsn to connect to port 443 with the /a/api prefix#446Kinjal-Arista wants to merge 5 commits into
Conversation
Enables pybsn to automatically discover and connect to BigDB services that have been migrated to port 443 with the /a/api prefix Fixes: BUG1413418
seamusarista
left a comment
seamusarista
left a comment
There was a problem hiding this comment.
For some reason argue wasn't able to the comments to github.
Argus Code Review Report
Review ID: 893fe51a
Platform: github
Identifier: #446
Team: bigswitch/pybsn
Mode: deep
Summary
Overview: This PR is a follow-up to a reverted change (CL 48851522 / CL 48954124) that adds port 443 with /a prefix support to pybsn. The key improvement over the reverted version is deriving the session cookie path from the base URL's path prefix, fixing the root cause of BUG-1889369 (401 auth failures due to cookie path /api not matching /a/api/... request paths). The implementation is well-structured: a new ApiEndpointConfig dataclass replaces the raw tuples, guess_url() gains URL parsing for schema-less inputs with explicit port or path prefix, and _attempt_login() dynamically computes the cookie path. Test coverage is comprehensive.
Key findings:
- The core cookie-path fix is correct and directly addresses the root cause of the previous revert (BUG-1889369)
- There is a usability gap:
pybsn.connect("192.0.2.1:443", ...)(explicit port, no/asuffix) will fail to discover the API because the explicit-port branch doesn't auto-apply the/aprefix, while barepybsn.connect("192.0.2.1", ...)works correctly - Naming issue: the
ApiEndpointConfig.schemafield should beschemeper RFC 3986;schemaconflicts with the BigDB schema concept used throughout this codebase - Minor redundancy:
urlparse(url)is called twice in_attempt_login()where one call would suffice
Context consulted:
- Winnow: pybsn cookie handling and /a/api prefix
- Bug: BUG-1413418 (RESOLVED/FIXED) — Move REST API on port 443 behind /a prefix
- Bug: BUG-1889369 (RESOLVED/FIXED) — ZtnTest pybsn auth failure (401 on /a/api paths due to cookie path)
- Bug: BUG-1418083 (NEW) — pybsn client support for port 443 with /a/ prefix
Comments (4 shown, severity ≥ low)
File: pybsn/__init__.py
[LOW] Line 561
Category: clarity
The field name schema should be scheme. In URL/HTTP terminology, the correct term is "scheme" (RFC 3986). Using schema here is confusing because schema already has a specific meaning in this codebase — BigDB schema (SCHEMA_PREFIX, client.schema(), etc.).
Suggested fix:
Rename the field to `scheme`:
```python
@dataclass(frozen=True)
class ApiEndpointConfig:
scheme: str
port_no: int
prefix: str
And update the corresponding references in BIGDB_PROTO_PORTS and guess_url().
#### [MEDIUM] Line 589
**Category:** bug
When a user passes `"192.0.2.1:443"` (explicit port, no `/a` suffix), `parsed.path` is `""`, so the candidate URL becomes `https://192.0.2.1:443` without the `/a` prefix. The health check goes to `https://192.0.2.1:443/api/v1/auth/healthy` instead of `https://192.0.2.1:443/a/api/v1/auth/healthy`, causing discovery to fail with an exception. This is inconsistent with bare hostname behavior where `"192.0.2.1"` correctly probes port 443 with `/a`.
A user who knows their controller runs on port 443 and writes `pybsn.connect("192.0.2.1:443", ...)` will get a confusing failure.
**Suggested fix:**
When parsed.port is specified but parsed.path is empty, look up the default prefix from BIGDB_PROTO_PORTS for known ports:
if parsed.port is not None:
schema = "https" if parsed.port in (443, 8443) else "http"
prefix = parsed.path
if not prefix:
matching = [e for e in BIGDB_PROTO_PORTS if e.port_no == parsed.port]
prefix = matching[0].prefix if matching else ""
candidates = [(schema, parsed.hostname, parsed.port, prefix)]
#### [LOW] Lines 634-639
**Category:** style
`urlparse(url)` is called twice — once on line 634 to extract the path, and again on line 639 to extract the hostname. Store the result in a local variable to avoid the redundant parse.
**Suggested fix:**
parsed_url = urlparse(url)
url_path_prefix = parsed_url.path.rstrip("/")
session_cookie_path = f"{url_path_prefix}/api" if url_path_prefix else "/api"
session_cookie = requests.cookies.create_cookie(
name="session_cookie",
value=json_["session-cookie"],
domain=parsed_url.hostname,
path=session_cookie_path,
)
### File: `test/test_port_443_prefix.py`
#### [LOW] Line 230
**Category:** suggestion
This test is named `test_explicit_port_443_uses_https` but actually tests `"192.0.2.1:443/a"` (port + path prefix), not just `"192.0.2.1:443"`. A test for bare `"192.0.2.1:443"` (no `/a` suffix) is missing — this would document the expected behavior for that input and catch the discovery gap mentioned above.
**Suggested fix:**
Add a test for "192.0.2.1:443" without the /a suffix. Depending on the design decision, it should either auto-apply the /a prefix or clearly fail.
Addressed all three suggestions |
2 participants
Enables pybsn to automatically discover and connect to BigDB services that have been migrated to port 443 with the /a/api prefix
This PR is a follow-up after the previous PR ran into a bug. pybsn was updated to discover the new REST base URL on
port 443 as https://:443/a, but login still created the session cookie with path /api. That meant login could
succeed, while the next authenticated request to /a/api/... failed with 401 because the cookie was not sent. In CVML
this showed up as repeated pybsn client NOT connected polling until timeout.
This change derives the session cookie path from the base URL path prefix, so https://:443/a uses /a/api while
legacy URLs like https://:443 and https://:8443 continue to use /api.
Tested with unit tests and against a CVML controller with a custom script written using codex to utilise the new pybsn
to make a GET request to the controller:
https://10.226.78.185:443/a, set cookie path /api, and failed the first authenticatedGET with 401
https://10.226.78.185:443/a, set cookie path /a/api, and the same authenticated GETsucceeded
Fixes: BUG1413418