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

reverseproxy: Support multiple 'to' upstreams in reverse-proxy cmd #4693

Merged

Conversation

parrotmac
Copy link
Contributor

From #4692

This is a proposal to add the ability to specify multiple upstream/to targets to the reverse-proxy command.

Example usage:

caddy reverse-proxy --from :8080 --to host1.example.com,host2.example.com

What do we think of the implementation? Any suggestions for a different interface?

@CLAassistant
Copy link

CLAassistant commented Apr 10, 2022

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie added feature ⚙️ New feature or request under review 🧐 Review is pending before merging labels Apr 10, 2022
@francislavoie francislavoie added this to the 2.x milestone Apr 10, 2022
@mholt
Copy link
Member

mholt commented Apr 11, 2022

Thanks for the contribution!

I think it is more typical to repeat a flag than to have custom parsing within a flag's value, i.e. --to host1 --to host2 instead of --to host1,host2. This would probably be my preference, anyway.

That said, I'm not entirely sure how nicely our flags API handles that at the moment. What do you think?

@francislavoie
Copy link
Member

francislavoie commented Apr 11, 2022

@mholt is there even a way to repeat flags with Go's built-in stuff? We probably can with cobra #4565 but I don't think we can as-is? I don't see a fl.Array() or fl.Slice() etc, cause it wouldn't be type safe 🤔

@mholt
Copy link
Member

mholt commented Apr 11, 2022

@francislavoie Yeah, I had to look this up but I knew I did it once before, years ago. We would need to define our own type: https://stackoverflow.com/questions/45487377/golang-flag-ignore-missing-flag-and-parse-multiple-duplicate-flags

Which, I think maybe that's suitable for this. It's just a few lines and very simple. I do think the CLI looks cleaner this way, personally; but maybe that's just my preference. 🤷‍♂️

@francislavoie
Copy link
Member

Ah ok, that's not so bad. Yeah I suppose we should do it that way then 👍

@parrotmac
Copy link
Contributor Author

parrotmac commented Apr 11, 2022

I think it is more typical to repeat a flag than to have custom parsing within a flag's value, i.e. --to host1 --to host2 instead of --to host1,host2. This would probably be my preference, anyway.

That said, I'm not entirely sure how nicely our flags API handles that at the moment. What do you think?

Yeah, I actually quite prefer the multiple flags form but wasn't sure if it was worth going that route. Here's an example of how that could be implemented. If that's preferred I'm happy to switch to something like that. Edit ha! Looks like y'all beat me to exactly this same thing! 😂

@mholt
Copy link
Member

mholt commented Jul 6, 2022

@parrotmac So, did we decide to use multiple --to or to stick with comma separated values? Honestly I'm probably OK with either, I just think the multiple flag form will be clearer.

@mholt mholt modified the milestones: 2.x, v2.6.0 Sep 15, 2022
Rather than comma-separating in a single flag
@mholt mholt removed the under review 🧐 Review is pending before merging label Sep 15, 2022
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.

I refactored it to use multiple --to flags rather than comma-separating them. I think it is slightly more natural. Thank you for the contribution! Sorry it took me a while.

@mholt mholt merged commit e338648 into caddyserver:master Sep 15, 2022
@mholt mholt changed the title feat: Multiple 'to' upstreams in reverse-proxy cmd reverseproxy: Support multiple 'to' upstreams in reverse-proxy cmd Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants