-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: enable custom transports in configs #213
Conversation
Any thoughts? |
This is super cool! so basically to define a new wrapper (for example
and implement I think there are other places in the SDK where I can also update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, please update the PR title to "refactor: make configs dynamically configurable".
For your example in the PR description, I'd prefer to include the config file name in the URL:
go run myproxy -transport 'smart:///usr/home/config.json|ss://$SECRET@$HOST:$PORT'
} | ||
|
||
// NewDefaultConfigParser creates a [ConfigParser] with a set of default wrappers already registered. | ||
func NewDefaultConfigParser() *ConfigParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can have another NewEmptyConfigParser()
? This can be used by OutlineCLI (NewEmptyConfigParser().RegisterShadowsocksDialerWrappers()
), because it only expects the ss://
config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyyi1 I also think having NewEmptyConfigParser()
is useful in general but in the case of Outline CLI why do we want to limit the config to ss://
only? I am working on a sock5-over-tls
server that I believe would be useful to folks who are using Outline CLI.
https://github.com/amircybersec/socks5-over-tls
This way CLI users can also use fragmentation and packet split capability of the Outline SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me, we don't have to limit the config type of Outline CLI, we can enable other VPN protocols (such as socks5
or maybe vmess
) if we support them. Now we only have ss
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyyi1 BTW, since ConfigParser
type is exported, one could just do p := new(ConfigParser)
to create an empty config parser instead of NewEmptyConfigParser()
but the explicit method can help too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the zero struct is valid and can be used.
@fortuna I added a section at the end of the doc file on creating a custom config. Everything else looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me; I added a section to doc.go with explanation on how to define and register a new config.
Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>
@jyyi1 I added your example, but this is a feature in the sense of new functionality, not a refactor. |
This PR makes the config parser configurable. This way people can extend it with new protocols. For example, one may add an "Intra" wrapper, or a "V2Ray" wrapper, or a "Psiphon" wrapper that is configured with a separate file. Here is a hypothetical example:
Or:
This will help @amircybersec, who wanted to add v2ray to his app and still reuse our config.