Skip to content

exporter: limit ser2net port range - #1743

Open
benjamin1313 wants to merge 1 commit into
labgrid-project:masterfrom
benjamin1313:preferredport-networkserial
Open

benjamin1313 wants to merge 1 commit into
labgrid-project:masterfrom
benjamin1313:preferredport-networkserial

Conversation

@benjamin1313

Copy link
Copy Markdown
Contributor

This allows for specifying a start port and a range up from that to be used when looking for a free port to use with ser2net when creating a networkserialport resource. This can be helpful on networks where access to ports on the network is limited.

Currently this feature is planed for use in a labgrid setup where the exporter and duts are placed on a network separate from the main network the developers are on and there are restrictions on the tcp ports. Being able to specify what port(s) labgrid can use makes it easier to manage and keep track of what ports are open between the two networks.

Checklist

  • Documentation for the feature
  • Tests for the feature
  • PR has been tested
  • Man pages have been regenerated

@benjamin1313
benjamin1313 force-pushed the preferredport-networkserial branch from 6229841 to f72b887 Compare September 24, 2025 14:21
@codecov

codecov Bot commented Sep 24, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 21.42857% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.3%. Comparing base (2daa13d) to head (85b2413).
⚠️ Report is 298 commits behind head on master.

Files with missing lines Patch % Lines
labgrid/util/helper.py 23.0% 10 Missing ⚠️
labgrid/remote/exporter.py 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1743     +/-   ##
========================================
- Coverage    45.4%   45.3%   -0.1%     
========================================
  Files         172     172             
  Lines       13503   13514     +11     
========================================
+ Hits         6131    6132      +1     
- Misses       7372    7382     +10     
Flag Coverage Δ
3.10 45.3% <21.4%> (-0.1%) ⬇️
3.11 45.3% <21.4%> (-0.1%) ⬇️
3.12 45.3% <21.4%> (-0.1%) ⬇️
3.13 45.3% <21.4%> (-0.1%) ⬇️
3.9 45.4% <21.4%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

This allows for specifying a start port and a range up from that to be
used when looking for a free port to use with ser2net when creating a
networkserialport resource. This can be helpful on networks where access
to ports on the network is limited.

Signed-off-by: Benjamin B. Frost <benjamin@geanix.com>
@Emantor
Emantor force-pushed the preferredport-networkserial branch from f72b887 to 85b2413 Compare September 25, 2025 05:38

@Emantor Emantor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's usually more common to have a STARTPORT:ENDPORT definition instead of the PORT:RANGE used here, e.g. 4000:5000 instead of 4000:1000. This might also simplify the code, what do you think?

@Bastian-Krause

Copy link
Copy Markdown
Member

I think this should not be limited to ser2net. If the exporter needs to bind anything else in the future, it could use that range as well. So a more generic name would be good.

I'm not a huge fan of adding more environment variables, a CLI argument for labgrid-exporter would be a better fit. Especially since --isolated exists for similar cases where the exporter/DUT network setup is configured in a special way. So maybe --port-range=4000-5000?

@Bastian-Krause Bastian-Krause changed the title set preferred port for networkserialport exporter: limit ser2net port range Sep 25, 2025
@benjamin1313

Copy link
Copy Markdown
Contributor Author

I think it's usually more common to have a STARTPORT:ENDPORT definition instead of the PORT:RANGE used here, e.g. 4000:5000 instead of 4000:1000. This might also simplify the code, what do you think?

Yeah I see what you mean and will look into it.

I think this should not be limited to ser2net. If the exporter needs to bind anything else in the future, it could use that range as well. So a more generic name would be good.

Do you mean a global range for everything else that also needs a port?
Where I'm currently working on a Labgrid setup I'm not in charge of the network and I think giving Labgrid a range of ports to use for whatever is going to be a hard sell to the IT department in charge of the network.
Being able specify which ports are needed for what tool, service etc. is preferred.

@Bastian-Krause

Copy link
Copy Markdown
Member

Letting the exporter bind something is a rather special case. Apart from USBSerialPort/RawSerialPort exports running ser2net, I suppose that would only happen for a new kind of resource (if ever). As long as you don't export such a resource, no new binds will happen.

That being said, maybe it makes more sense (in such a regulated environment) to run ser2net outside labgrid and only export NetworkSerialPorts? If you're worried about misconfigurations, you could even configure your labgrid-exporter.service with SocketBindDeny=any.

@jluebbe

jluebbe commented Oct 2, 2025

Copy link
Copy Markdown
Member

Please keep in mind that without allocating a new port on acquire, you have the risk that an out of date client connects to the port it knows even if it no longer has a lock.

@jluebbe

jluebbe commented Dec 17, 2025

Copy link
Copy Markdown
Member

I don't think we should add a ser2net-specific override. Also, we need to make reasonably sure that a new port is chosen when the the resource is acquired again. So: a global port range of at least 1000 ports, used automatically for all calls of get_free_port().

@benjamin1313 Would you be interested in reworking this PR accordingly? If not, I'd close it for now. @Emantor @Bastian-Krause, what do you think?

@benjamin1313

Copy link
Copy Markdown
Contributor Author

@jluebbe yes i will look into it.

Is is currently understood what needs to be done is.
use --port-range=4000-5000 instead of environment variable.
use start and stop port instead of start and number of ports to be used.
other resources calling get_free_port() should also use these ports.
and ´get_free_port()´ should choose a port at random in the range given in order to avoid the risk an out of date client connects to the port?

@AiyionPrime

Copy link
Copy Markdown
Contributor

@benjamin1313 I'm interested in a solution as well and have a few hours to spare on this topic.

I think I understand the requirements and will open a new PR this weekend if you don't object?

@AiyionPrime

Copy link
Copy Markdown
Contributor

I don't think we should add a ser2net-specific override. [...]

Originally posted by @jluebbe in #1743 (comment)

Looking at it, the strictest approach to not be (ser2net-)specific would be to not implement this feature at all and instead reduce the ephemeral port range to the desired limits in order for the regular bind(('', 0)) to pick it up.

I'll give it a shot tomorrow and find whether this more radical approach limits the systems usability.

@benjamin1313

benjamin1313 commented Mar 2, 2026 •

Copy link
Copy Markdown
Contributor Author

@benjamin1313 I'm interested in a solution as well and have a few hours to spare on this topic.
I think I understand the requirements and will open a new PR this weekend if you don't object?

i don't.

@Bastian-Krause Bastian-Krause added the needs rebase Needs a rebase onto the master branch, maintainter could probably not push to submitter branch. label Jul 7, 2026
@Arthur031221 Arthur031221 mentioned this pull request Oct 4, 2026
5 tasks done
@Arthur031221

Copy link
Copy Markdown

I withdrew #1978 after discussing the SSH proxy guidance in #1832: #1978 (comment). @ozan956 suggested continuing that discussion here. @benjamin1313, does your setup need direct client-to-exporter access, or is there a case where the SSH proxy falls short?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement needs rebase Needs a rebase onto the master branch, maintainter could probably not push to submitter branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants