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 illogical setting defaults re. SSH_PORT and SSH_LISTEN_PORT #8656

Closed

Conversation

leonardw
Copy link

Fix issue whereby setting SSH_PORT, for the purpose of constructing a superficial git-clone URL, would implicitly also set SSH_LISTEN_PORT that the SSH service binds to, if the latter is not explicitly configured.

Manifestation

SSH_PORT=2222
Would cause Gitea to also modify SSH_LISTEN_PORT and start up SSH on port 2222.

Work around would be to simultaneously force service port binding back to 22:
SSH_PORT=2222
SSH_LISTEN_PORT=22

So, the actual service binding SSH_LISTEN_PORT is being defaulted to SSH_PORT that's used for 'external decoration.' It should really be the other way around.

Rationale

Documentation states that:

  • SSH_LISTEN_PORT: Port for the built-in SSH server.
  • SSH_PORT: SSH port displayed in clone URL

Therefore, it stands to reason to expect that:

  • SSH_LISTEN_PORT should default to 22, unless explicitly overriden, and with no other implicit defaults.
  • SSH_PORT should be allowed to be freely set to any value as seen fit, depending on infrastructure/platform (be it Docker or otherwise) without implications on underlying service configuration. Though, it is sensible to default to the SSH_LISTEN_PORT above.

Additional Info

Documentation files are updated accordingly.

This may be considered a corrective fix to prior merged PR issues #8453 and #8477.

… superficial git-clone URL, would implicitly also set SSH_LISTEN_PORT that the SSH service listens on, if the latter is not explicitly configured.

This is a corrective fix to prior issues go-gitea#8453 and go-gitea#8477.
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 24, 2019
@lunny lunny added this to the 1.11.0 milestone Oct 24, 2019
@lunny lunny added the type/enhancement An improvement of existing functionality label Oct 24, 2019
@guillep2k
Copy link
Member

I think this should probably be labeled as kind/breaking.

@zeripath
Copy link
Contributor

I disagree that this is illogical as it currently stands.

@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Oct 24, 2019
@zeripath
Copy link
Contributor

To be clearer:

Rather than break existing instances we should just update the documentation to make it clear which inherits from which. Some may prefer it one way or the other but it's just a choice, one that's already been made. (It may be inconsistent but it's been made already.)

What's the benefit of breaking existing instances here? If this was part of a concerted effort to rationalize config names and make the configuration of http services clearer I'd agree but as a standalone pr just changing the order of inheritance potentially causing everyone who just uses SSH_PORT to have to update their config it seems unnecessary.

@lunny lunny modified the milestones: 1.11.0, 1.12.0 Dec 17, 2019
@stale
Copy link

stale bot commented Feb 15, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Feb 15, 2020
@stale
Copy link

stale bot commented Apr 15, 2020

This pull request has been automatically closed because of inactivity. You can re-open it if needed.

@stale stale bot closed this Apr 15, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/stale lgtm/need 1 This PR needs approval from one additional maintainer to be merged. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants