Re-organize network endpoints

[letmaik]

Fixes #1954.

[ccf-bot]

letmaik/network-rpcs@17506 aka 20210113.43 vs master ewma over 50 builds from 16916 to 17498

[jumaffre]

Why not use NodeInfoNetwork here directly? Some fields (e.g. public port) are missing for example.

[jumaffre]

Ah, checked the implementation of the endpoints that populate this struct and we do return the public port. In fact, we'd report the public and private RPC interfaces. Is this expected? If my understand is correct, one seems interesting for operators (currently rpc_host and rpc_port) while the public interface is mostly for users/members?

[letmaik]

I used a new type to provide a boundary between internal and public interface, since we don't want to break the latter even if the internal one changes.
And yes, returning the public and private addresses is expected, this was an ask from @lynshi and I don't see any harm in it either way.
We should discuss naming, rpc_* doesn't seem like a great choice.

[jumaffre]

These could be exposed as private_host/port perhaps?

[achamayou]

local would preferable to private, these are the local interfaces the nodes were told to bind to on startup. Whether they differ from public depends on whether there is some routing/NATing going on, but they aren't really private, at least not in way that compares to other things being private in CCF.

[letmaik]

What about nodehost/nodeport? Does it make sense to expose that as well?

[jumaffre]

@letmaik These are only used for node-to-node communications and aren't exposed to clients directly so my hunch is that they shouldn't be exposed directly here.

[letmaik]

OK, I renamed it to local_*

[letmaik]

github pages build fails with:

2021-01-12T14:38:38.6194169Z reading sources... [ 71%] operations/operator_rpc_api
2021-01-12T14:38:38.7029753Z /tmp/tmpql0jqllv/7524c1c8e1d80930a16c1dbc0719b6c0d196ab04/doc/governance/hsm_keys.rst:2: WARNING: Duplicate explicit target name: "here".
2021-01-12T14:38:38.7030487Z /tmp/tmpql0jqllv/7524c1c8e1d80930a16c1dbc0719b6c0d196ab04/doc/governance/hsm_keys.rst:2: WARNING: Duplicate explicit target name: "here".
2021-01-12T14:38:38.9414601Z 
2021-01-12T14:38:38.9415027Z Exception occurred:
2021-01-12T14:38:38.9416063Z   File "/__w/1/s/env/lib/python3.8/site-packages/sphinxcontrib/openapi/openapi30.py", line 281, in _httpresource
2021-01-12T14:38:38.9416591Z     type=param['schema']['type'],
2021-01-12T14:38:38.9416959Z KeyError: 'type'

@eddyashton Any idea? The schema seems valid according to https://apitools.dev/swagger-parser/online/

[eddyashton]

@letmaik I agree this looks like valid OpenAPI. We run the generated API documents through a validator in the e2e tests, and that seems happy, and SwaggerHub is also happy with node_openapi.json, so I'm not sure why this sphinx plugin isn't... Do you know which element it breaks on? I'm guessing its the status parameter in GET /network/nodes, which now has just an enum field but no type field?

[letmaik]

Yes, you're right. Looks like a bug in the library unfortunately. It's the only enum query parameter we have so far. And type is definitely not needed there.

[letmaik]

Although it would be fine to add "type": "string" for enums, since all our enums are string-based. Should we just do that?

https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values

[eddyashton]

Yes, you're right. Looks like a bug in the library unfortunately. It's the only enum query parameter we have so far. And type is definitely not needed there.

Can you raise an issue on the library? As a workaround for now, I think we can just add a "type": "string" field to all our enum schemas (this isn't enforced to be true but is true in practice, I believe). At the bottom of json.h in fill_enum_schema is where we construct these objects, so it should be as simple as adding j["type"] = "string"; after j["enum"] = .... If you want to be more precise, it could guarded by a check that every element in enums is a string (these are nlohmann::json objects, so .is_string()), but this is probably unnecessary. If we can get a fix in the sphinx lib that would be much better - this is a less precise constraint than provided by the enum field.

[letmaik]

sphinx-contrib/openapi#106

I'll add your proposed fix to json.h.