Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

ericlapier
Copy link
Contributor

@ericlapier ericlapier commented Jan 23, 2024

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

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.
@ericlapier ericlapier requested a review from a team as a code owner January 23, 2024 22:16
@krisstern krisstern added the bug An issue reporting a bug or a PR fixing one. label Mar 8, 2024
@krisstern krisstern self-assigned this Mar 8, 2024
@krisstern
Copy link
Member

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

@krisstern
Copy link
Member

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
Copy link
Contributor Author

ericlapier commented Mar 8, 2024 via email

@krisstern
Copy link
Member

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

@ericlapier
Copy link
Contributor Author

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

@krisstern krisstern changed the title Port Bindings is not respected for SSH port fix: Port binding is not respected for SSH port Jun 4, 2024
@krisstern
Copy link
Member

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.

Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

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

LGTM

@krisstern krisstern merged commit 16e30a6 into jenkinsci:master Jun 5, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a bug or a PR fixing one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants