BUG1413418: Prevent false positives on health checks - #452
Open
Kinjal-Arista wants to merge 10 commits into
Open
BUG1413418: Prevent false positives on health checks#452Kinjal-Arista wants to merge 10 commits into
Kinjal-Arista wants to merge 10 commits into
Conversation
Kinjal-Arista
requested review from
YutaHiguchi-bsn,
pazur-arista and
seamusarista
July 26, 2026 21:42
Contributor
Author
|
Test results: With 8.11 controllers: With 8.10 controllers: |
bigswitch#446) 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://<host>: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://<host>:443/a uses /a/api while legacy URLs like https://<host>:443 and https://<host>: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: - old reverted version resolved `https://10.226.78.185:443/a`, set cookie path /api, and failed the first authenticated GET with 401 - this version resolved `https://10.226.78.185:443/a`, set cookie path /a/api, and the same authenticated GET succeeded Fixes: BUG1413418 (cherry picked from commit 4dc837a)
Kinjal-Arista
force-pushed
the
BUG1413418
branch
2 times, most recently
from
July 28, 2026 10:38
a1a75ae to
9af8745
Compare
Health check on older controllers can return status 200 with port 443 and /a prefix even though they are not suported
pazur-arista
reviewed
Aug 3, 2026
| if parsed.port is not None: | ||
| raise ValueError(f"Schemeless hosts must not include an explicit port: {host!r}") | ||
| hostname = _format_url_authority(hostname) | ||
| path_prefix = _normalize_path_prefix(parsed.path) |
There was a problem hiding this comment.
I'm not sure why do we need this.
If host parameter is:
- just a hostname/ipaddress, e.g.
1.2.3.4-> we try standard API endpoints,https://1.2.3.4:443/a,https://1.2.3.4:8443/,http://1.2.3.4:80/and return the first reachable - full url with schema, e.g.
https://foo.example.com:1234/someprefix/-> we just return the provided host - hostname/ipaddress with path, e.g.
1.2.3.4/foo-> we try standard API ports, but with given prefix, this is,https://1.2.3.4:443/foo,https://1.2.3.4:8443/foo, `http://1.2.3.4:80/foo.
Use-case for 1 is obvious - user wants to connect to controller running on given host. Use-case for 2 is also obvious (but probably less common) - user wants to try only specific endpoint (e.g. knows that 8443/80 won't work) or the controller may be running behind some reverse-proxy that prepends adds its own prefix.
But what's the use-case for 3? Trying out all the standard ports suggests user is trying to connect to the controller directly, without any reverse proxy that might modify path. So, what's the point in allowing user to specify the path?
Contributor
Author
There was a problem hiding this comment.
Removed support for schemeless host/prefix inputs
pazur-arista
approved these changes
Aug 7, 2026
seamusarista
approved these changes
Aug 7, 2026
YutaHiguchi-bsn
approved these changes
Aug 8, 2026
YutaHiguchi-bsn
left a comment
YutaHiguchi-bsn
left a comment
Contributor
There was a problem hiding this comment.
lgtm, there was a typos in PR description: suported
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Health check on older controllers can return status 200 with port 443 and /a prefix even though they
are not suported
Here's a curl request run on an older 8.10.x DMF controller with /a and port 443:
As seen above, it return 200 but the output is unexpected as /a and port 443 is not supported here but it returns a GUI/nginx catch-all HTML page
The right url returns the following :
So, to differentiate between the 2, adding a new check here that checks if content-type is application/json in which case its a valid response, otherwise not
Added the previously reverted pybsn PR in the first commit of this PR as well
PartialFixes: BUG1413418