jenkinsci / docker-plugin

Jenkins cloud plugin that uses Docker
https://plugins.jenkins.io/docker-plugin/
MIT License
486 stars 322 forks source link

fix: Port binding is not respected for SSH port #1043

Closed ericlapier closed 1 month ago

ericlapier commented 5 months ago

1- Dashboard > Manage Jenkins > Configure Clouds 2- Add a new Cloud 3- Connect method SSH 4- Container settings 5- Port Bindings 0.0.0.0:20000-20100:22 Expected Results All docker-proxy process should use a port between 20000 and 20100

Tested for a year now with this fix and docker-proxy always gets a port in the specified range.

Testing done

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [ ] Link to relevant issues in GitHub or Jira
- [ ] Link to relevant pull requests, esp. upstream and downstream changes
- [ ] Ensure you have provided tests - that demonstrates feature works or fixes the issue
krisstern commented 4 months ago

The code change proposed looks sensible enough. Thanks @ericlapier for the contribution!

krisstern commented 4 months ago

I have just checked the official doc at https://docs.docker.com/network/proxy/. It does not seem to have a prescribed port range. Also, some of the CI/CD tests are not passing. May I ask if this port range is used by your company for a particular network configuration? If we accept this PR many users will be impacted I am afraid.

ericlapier commented 4 months ago

no particular network configuration. We have other software running on the slaves, and if we don't specify a range, allocated ssh port start to collide with running daemons.

On Fri, Mar 8, 2024 at 11:26 AM Kris Stern @.***> wrote:

I have just checked the official doc at https://docs.docker.com/network/proxy/. It does not seem to have a prescribed port range. Also, some of the CI/CD tests are not passing. May I ask if this port range is used by your company for a particular network configuration? If we accept this PR many users will be impacted I am afraid.

— Reply to this email directly, view it on GitHub https://github.com/jenkinsci/docker-plugin/pull/1043#issuecomment-1986000641, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFR3ROVNEJ5UQNTVTMKVY7DYXHRDFAVCNFSM6AAAAABCHYCFQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBWGAYDANRUGE . You are receiving this because you were mentioned.Message ID: @.***>

-- .

krisstern commented 4 months ago

We cannot approve the PR until the CI/CD checks are passing. @ericlapier Could you please get this done first?

ericlapier commented 1 month ago

@krisstern I added a new testcase and fix the code such that all testcases are now successful.

krisstern commented 1 month ago

Thanks @ericlapier! The PR looks good to me. I will have it merged over the next 24 hours if I do not see any major objections from anyone.