BUG1413418: Enable pybsn to connect to port 443 with the /sys/api prefix#411
BUG1413418: Enable pybsn to connect to port 443 with the /sys/api prefix#411Kinjal-Arista wants to merge 9 commits intobigswitch:mainfrom
Conversation
Enables pybsn to automatically discover and connect to BigDB services that have been migrated to port 443 with the /sys/api prefix
Kinjal-Arista
requested review from
andi-bigswitch,
pazur-arista and
seamusarista
pybsn/__init__.py
Outdated
| ("https", 8443, ""), | ||
| ("http", 8080, ""), |
There was a problem hiding this comment.
do we want to also try with the /sys prefix on these ports?
There was a problem hiding this comment.
I thought if we are running pybsn and the connection via port 443 and the /sys prefix fails, that would indicate we are running it in a older floodlight version where it is not supported. So, any of the other ports with the sys prefix also wouldn't work. So, I didn't add it to the other ports
test/test_port_443_prefix.py
Outdated
| @responses.activate | ||
| def test_port_443_with_sys_prefix_tried_first(self): | ||
| """Port 443 with /sys prefix should be tried first.""" | ||
| responses.add(responses.GET, "https://10.0.0.1:443/sys/api/v1/auth/healthy", status=200, body="true") |
There was a problem hiding this comment.
nit: there are some special reserved IP ranges for testing/documentation use. Using them should be safer (at least in theory). https://www.rfc-editor.org/rfc/rfc5737:
The blocks 192.0.2.0/24 (TEST-NET-1), 198.51.100.0/24 (TEST-NET-2),
and 203.0.113.0/24 (TEST-NET-3) are provided for use in
documentation.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class PortAndProtocol: |
There was a problem hiding this comment.
nit: I don't like the name PortAndProtocol (especially that's actually PortAndProtocolAndPrefix 🙃 ). Maybe sth like ApiEndpointConfig?
| return response | ||
|
|
||
|
|
||
| ATLAS_PREFIX = "/a" |
There was a problem hiding this comment.
nit: I'm not sure if we want to name it "ATLAS", this is a public package. I also don't see much point in having this as an extra constant, that is just embedded in other constant and is not used stand-alone (not counting unit tests)
There was a problem hiding this comment.
I don't have a strong opinion either way, so ok to delete it. But given it's repeated lots of times (11) in the code I'd prefer to have something. Naming is difficult...
| for schema, port in BIGDB_PROTO_PORTS: | ||
| url = "%s://%s:%d" % (schema, host, port) | ||
| for entry in BIGDB_PROTO_PORTS: | ||
| url = f"{entry.schema}://{host}:{entry.port_no}{entry.prefix}" |
There was a problem hiding this comment.
this wasn't handled before as well, but we can consider what if the host isn't a host but a schema-less url. E.g. example.com:8443 or example.com/my-custom-prefix.
3 participants
Enables pybsn to automatically discover and connect to BigDB services
that have been migrated to port 443 with the /sys/api prefix
Fixes: BUG1413418