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

core: addresses.go funcs renames #6622

Merged
merged 16 commits into from
Oct 21, 2024

Conversation

MayCXC
Copy link
Contributor

@MayCXC MayCXC commented Oct 12, 2024

HEAD removes the unreleased ParseNetworkAddressFromHostPort that I added in #6573. I would also like to rename na.JoinHostPort to na.JoinAt, to make it clear that it does not take the same parameters as net.JoinHostPort. finally, I would like to change the signature

func getListenerFromPlugin(ctx context.Context, network, addr string, config net.ListenConfig) (any, error)

to

func getListenerFromPlugin(ctx context.Context, network, host, portRange string, portOffset uint, config net.ListenConfig) (any, error)

so that plugins are aware of the port range and offset of their network addresses.

Together, these changes are meant to make a clearer interface for plugins to choose either ParseNetworkAddress(JoinNetworkAddress(...)).Listen or net.Listen(ParseNetworkAddress(JoinNetworkAddress(...)).JoinAt(portOffset)) to create their listeners, with preference for the former so they are added to the listener pool. The offset parameter in getListenerFromPlugin also lets https://github.com/MayCXC/caddy-systemd-socket-activation?tab=readme-ov-file use its custom networks to listen to a port range with a systemd named socket unit, which is a very clean integration.

@MayCXC MayCXC marked this pull request as draft October 12, 2024 20:21
@MayCXC MayCXC marked this pull request as ready for review October 12, 2024 23:00
@MayCXC
Copy link
Contributor Author

MayCXC commented Oct 12, 2024

the last commit widens some test cases for SplitNetworkAddress, meaning no incompatibilities with previous versions, but it is new behavior. I think it is a sensible change; caddy already considers the port and the network optional for the argument to SplitNetworkAddress, because they are optional in the config. so now the host is optional, as it is also optional in the config. the result is that network, host, and port can all be returned empty without an err from SplitNetworkAddress, which is what I would expect when parsing the configuration.

so all that is left to put custom network plugins where I want them to be is na.JoinHostPort -> na.JoinAt, and the new getListenerFromPlugin parameters.

@MayCXC
Copy link
Contributor Author

MayCXC commented Oct 12, 2024

it might also be better to return an error if network, host, and port are all "". but I do think any two of the three should be allowed to be blank.

@francislavoie
Copy link
Member

You don't need to keep clicking merge btw, we can do that after reviewed and about to be merged.

This looks like a good simplification to me. But I'll let @mholt take a look first.

@francislavoie francislavoie changed the title addresses.go funcs renames core: addresses.go funcs renames Oct 20, 2024
@MayCXC
Copy link
Contributor Author

MayCXC commented Oct 20, 2024

You don't need to keep clicking merge btw, we can do that after reviewed and about to be merged.

This looks like a good simplification to me. But I'll let @mholt take a look first.

I do that to check if conflicts need to be resolved ahead of time. This PR is also just half the change, to check if changing split is viable.

Copy link
Member

@mholt mholt 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 the simplification / consistency enhancements!

@mholt mholt merged commit 0182fb8 into caddyserver:master Oct 21, 2024
33 checks passed
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