`net_kernel:allowed/0`, specs and docs

[MarkoMin]

Few changes:

1. net_kernel:allowed/0 is speced and documented. Notice that it is already listed in "documented API exports", but for some reason docs were missing. I think it's a must to have it documented, because if you don't then the user can't figure out if net_kernel:allow/1 was called in the past, therefore blocking nodes with the same cookie to connect (which I believe can be very frustrating and and to debug!). This is just a pure getter function, I see no threat exposing it.
2. net_kernel:allow/1 was missing ignored value it can return. You'll get it if e.g. your node is started without (s)name.
3. 2 small typos in comments

[github-actions[bot]]

CT Test Results

    2 files     70 suites   1h 5m 29s :stopwatch: 1 549 tests 1 304 :white_check_mark: 242 :zzz: 3 :x: 1 765 runs  1 488 :white_check_mark: 274 :zzz: 3 :x:

For more details on these failures, see this check.

Results for commit 12a722ce.

:recycle: This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[kikofernandez]

As per the new documentation, this function has the specs but was not displayed as a public function, which is why it had the -doc false.. By removing the doc false. you are making this a public function.

We will get back to you once we have determined if this function should be public or not. If it becomes public, then it needs to be also tested, which I think is not the case at the moment.

[MarkoMin]

By removing the doc false. you are making this a public function.

That was my intention because I think that user must have an option to retrieve the state of allowed node.

If it becomes public, then it needs to be also tested, which I think is not the case at the moment.

I definitely agree, but FWIW net_kernel:allow/1 is currently public, but not tested (searching net_kernel:a in test dir gives no results). We definitely need to test both functions. IMHO either both functions should be public or none.

[kikofernandez]

I think both functions make sense as public. For the PR to be accepted, we need some test cases. Thanks!

[MarkoMin]

I've added the testcase that goes through 4 different cases:

1. Different cookies, not allowed - no connection
2. Same cookie, not allowed - no connection
3. Same cookie, allowed - connection
4. Different cookie, allowed - connection

[MarkoMin]

Could you add tests to the allow function as well?

Both allow/1 and allowed/0 functions are used in tests. They are literally connected so I don't see a reason to have separate tests for both, I focused on both of them being covered.

I see that you use it via rpc:call but that, unfortunately, we do not have tests for it (AFAIK).

I used it because it is already used in other tests in this suite.

Apart from this, the type of allow seems to be wrong, since it can return ignored.

ignored is added here

[kikofernandez]

I pushed a test for ignored, thanks for catching this. I hope to merge the PR on August 12, 2024.

I also saw that your changes would be mostly the same I would do, and separating them into multiple smaller tests make things more complex, so thanks for this PR and sorry for the delay

[kikofernandez]

We are running tests internally and waiting for some people from OTP to come back. I will merge August 16, if the are no objections

[kikofernandez]

In our internal tests, I see that, non-deterministically, a call to nodes() in your test should return an empty list, but sometimes returns an existing node from other test. I have added some logic to see if the other nodes are really killed.

Since this is a non-deterministic error, I will not merge until I can check for two days or so that the error does not happen anymore.

TL;DR: The merging is delayed until I can pin down and stop existing nodes.

[sverker]

I piggybacked some improvements:

• Documented the return value ignored.
• Documented and tested the fact that disallowing an existing connection does not cause a disconnect.
• Make net_kernel:allowed/0 not return duplicates (by letting allow/1 ignore duplicates).
• Simplified and improved the test case.

[sverker]

I'm happy with this PR. I squashed it into two commits. I'll leave the merge to you @kikofernandez.

[kikofernandez]

I have marked this to run in the internal lab. I am still not sure that we have removed the spurious flaky tests. Checking tomorrow after the results come.