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

Add support for TFTP single port mode #14

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Dec 19, 2024

Add support for TFTP single port mode, which addresses the issue described in OpenCHAMI/deployment-recipes#86 (comment). With that configured, I can successfully retrieve resources over a LoadBalancer using the draft chart at OpenCHAMI/deployment-recipes#87 (comment)

Updates the pin/tftp dependency to v3. The new major was housekeeping for Go module stuff(see https://github.com/pin/tftp/releases/tag/v3.0.0), and is where all ongoing development will land upstream AFAICT. v3 is necessary for single port mode support.

Refactor the plugin wrapper server construction to let us add new settings more cleanly and clean up several constants.

This results in a breaking change because of coredhcp plugin configuration requiring explicit values for all fields.

Use %s instead of %w for non-fmt.Errorf() calls.
Upgrade the pin/tftp package to the v3 release. This provides single
port mode.

Refactor the TFTP server plugin wrapper to use a struct with settings
for port, address, etc.

Convert several constants embedded in code to actual constants.

Add a single port mode argument to TFTP server.
@rainest
Copy link
Contributor Author

rainest commented Dec 19, 2024

Originally in OP:

Can we shove arbitary objects under the plugin keys? Something like:

plugins:                                                                                                                                                                                                                                                                                 
    # Base CoreDHCP config                                                                                                                                                                                                                                                                 
   - server_id: 192.168.0.254                                                                                                                                                                                                                                                             
    - dns: 1.1.1.1 1.0.0.1
    - coresmd:
        bssUrl: http://example.com/bss
        smdUrl: http://example.com/smd

would be a lot more flexible regarding defaults, and would be easier to parse into a matching struct. Sticking with the positionals for now for a somewhat less breaking change.


No, sadly we cannot: the setup4() and setup6() signatures for the plugin interface can only pass []string, so we can't do arbitrary objects.

We could consider KV pairs, like bssURL=http://example.com/bss smdUrl=http://example.com/smd ... with the minor limitation that we'd be unable to use values with = easily, which is probably fine.

@synackd
Copy link
Collaborator

synackd commented Jan 7, 2025

I like these changes and I think you're onto something with the key-value layout for arguments. I think coresmd would be a lot easier to understand and configure that way. I would be interested in hearing what others think on that.

Could you update the example config file to document these changes?

@rainest
Copy link
Contributor Author

rainest commented Jan 7, 2025

@synackd example config updated.

Copy link
Member

@alexlovelltroy alexlovelltroy left a comment

Choose a reason for hiding this comment

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

LGTM

@alexlovelltroy alexlovelltroy merged commit b0c2802 into OpenCHAMI:main Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants