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

TFTP with single port mode #40

Closed
wants to merge 4 commits into from
Closed

Conversation

lholota
Copy link

@lholota lholota commented Jun 27, 2022

linuxserver.io

Please see #39


  • I have read the contributing guideline and understand that I have made the correct modifications

Description:

This PR replaces tftp-hpa with dnsmasq which supports single-port mode and thus enables compatibility with Kubernetes CNI "NATing" and generic firewalls.

  • New environment variable TFTP_SINGLE_PORT added to ensure backward compatibility (the feature is disabled by default)
  • Changed format of the PORT_RANGE environment variable (dnsmasq requires the port to be specified in the "<from>,<to>" format unlike tftp-hpa which uses "<from>:<to>" format.
    • Bash replace expression has been added to ensure backward compatibility.
    • Docs updated to reflect the new format

Benefits of this PR and context:

This implements feature request in #39

How Has This Been Tested?

TFTP tested with curl and tftp client (part of the tftp-hpa package) while the image was running in local Docker instance and K3s cluster with Flannel and MetalLB.

Source / References:

dnsmasq-discuss

@project-bot project-bot bot added this to PRs & in progress issues in Issue & PR Tracker Jun 27, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for opening this pull request! Be sure to follow the pull request template!

@github-actions
Copy link

github-actions bot commented Aug 5, 2022

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. Thank you for your contributions.

@drizuid drizuid added the enhancement New feature or request label Oct 11, 2022
- { env_var: "SUBFOLDER", env_value: "/", desc: "Specify a sobfolder if running this behind a reverse proxy (IE /proxy/)"}
- { env_var: "PORT_RANGE", env_value: "30000,30010", desc: "Specify the port range tftp will use for data transfers [(see Wikipedia)](https://en.wikipedia.org/wiki/Trivial_File_Transfer_Protocol#Details)"}
- { env_var: "TFTP_SINGLE_PORT", env_value: "1", desc: "Specify the port range tftp will use for data transfers [(see Wikipedia)](https://en.wikipedia.org/wiki/Trivial_File_Transfer_Protocol#Details)"}
- { env_var: "SUBFOLDER", env_value: "/", desc: "Specify the TFTP server should run in a single port mode (i.e. ephemeral ports are not used). This enables running the image in Kubernetes behind NAT based proxies. This env. variable is mutually exclusive with `PORT_RANGE`, only one of them can be specified."}
Copy link
Member

Choose a reason for hiding this comment

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

looks like you pasted your text for single_port into subfolder and duplicated the port_range text here. this also applies to README.md

- { env_var: "PORT_RANGE", env_value: "30000:30010", desc: "Specify the port range tftp will use for data transfers [(see Wikipedia)](https://en.wikipedia.org/wiki/Trivial_File_Transfer_Protocol#Details)"}
- { env_var: "SUBFOLDER", env_value: "/", desc: "Specify a sobfolder if running this behind a reverse proxy (IE /proxy/)"}
- { env_var: "PORT_RANGE", env_value: "30000,30010", desc: "Specify the port range tftp will use for data transfers [(see Wikipedia)](https://en.wikipedia.org/wiki/Trivial_File_Transfer_Protocol#Details)"}
- { env_var: "TFTP_SINGLE_PORT", env_value: "1", desc: "Specify the port range tftp will use for data transfers [(see Wikipedia)](https://en.wikipedia.org/wiki/Trivial_File_Transfer_Protocol#Details)"}
Copy link
Member

Choose a reason for hiding this comment

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

no explanation of what the options are, this seems like it should be a true false. I will assume you intend 1 to be true and 0 to be false, but this should be clarified and likely just be "true" or "false" to follow the standard of other options like this. Not adding this var (which should be optional) should imply false (or 0)

@drizuid
Copy link
Member

drizuid commented Oct 11, 2022

there are also some issues with this PR running in docker, it seems you may have tested in kubs but not docker?
https://0bin.net/paste/9jhIu7nG#LY0tMMmUpLJDVxWsGKGjSzue5BmqZbOeT9ECsMDtH5u

I'll note that after talking with the team, I am willing to entertain this PR, but you'll need to adjust the things I pointed out and when i test the pr, it'll need to work on our supported stuff (docker on debian based distros for arm and amd64)

@drizuid drizuid added the bug Something isn't working label Oct 31, 2022
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

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. Thank you for your contributions.

@github-actions github-actions bot closed this Apr 4, 2023
Issue & PR Tracker automation moved this from PRs & in progress issues to Done Apr 4, 2023
@github-actions
Copy link

github-actions bot commented May 4, 2023

This pull request is locked due to inactivity

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working closed-pr-activity enhancement New feature or request no-pr-activity
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants