-
Notifications
You must be signed in to change notification settings - Fork 186
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(caddy): add Caddy support for Outline services #198
base: sbruens/logger
Are you sure you want to change the base?
Conversation
…r proxy protocol.
b7bbfa0
to
7480f3c
Compare
caddy/app.go
Outdated
func (OutlineApp) CaddyModule() caddy.ModuleInfo { | ||
return caddy.ModuleInfo{ | ||
ID: outlineModuleName, | ||
New: func() caddy.Module { return new(OutlineApp) }, |
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.
Something seems wrong here. Is OutlineApp
both an App and a Module?
You create an OutlineApp to then create another OutlineApp? That's a circular dependency and looks like a bug.
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.
This API is so broken... I think we should create something like:
type ModuleRegistration caddy.ModuleInfo
func (m ModuleRegistration) CaddyModule() caddy.ModuleInfo {
return m
}
func init() {
caddy.RegisterModule(ModuleRegistration{
ID: "outline"
New: func() caddy.Module { return new(OutlineApp) }
})
}
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.
Something seems wrong here. Is
OutlineApp
both an App and a Module?
In Caddy apps are modules. So yes, OutlineApp
is both an App
and a Module
.
You create an OutlineApp to then create another OutlineApp? That's a circular dependency and looks like a bug.
Yeah it's a bit of an odd API. I don't think we can get around it though; OutlineApp
must implement caddy.Module
.
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.
Discussed offline. I misunderstood the suggestion. Will split them out.
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.
Ugh, this API is so bad. Typical global registration badness, unfortunately... I had to deal with that a lot at Google.
I would still take the ModuleRegistration
approach, and change OutlineApp
to have a constructor that takes the replayCache as input. This is a more extensible approach and doesn't confuse the OutlineApp lifecycle (otherwise you have two OutlineApp objects!).
In order to do that, I think you will need to add a dummy CaddyModule()
method to OutlineApp
so the compiler doesn't complain, but that can just log error and return.
) | ||
|
||
func (OutlineApp) CaddyModule() caddy.ModuleInfo { | ||
return caddy.ModuleInfo{ID: outlineModuleName} |
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.
Note we still need to provide a ModuleInfo with an accurate ID, so there is some duplication here now, but New()
isn't used from it as far as I can tell. This is also why I'm not logging any errors here, since it is called and used in a few places.
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's unfortunate. At least we don't have the two module objects co-existing anymore. Thanks for the changes.
…fer resizing (#213) * Add regression test that covers the issue. * Fix the test. * Use consistent comments.
) | ||
|
||
func (OutlineApp) CaddyModule() caddy.ModuleInfo { | ||
return caddy.ModuleInfo{ID: outlineModuleName} |
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's unfortunate. At least we don't have the two module objects co-existing anymore. Thanks for the changes.
This proof of concept lays the foundation for integrating Outline service support into Caddy through the implementation of two new Caddy modules:
outline
Host Module: This module serves as the central hub for server-wide configuration settings related to the Outline service. It also handles the configuration of application-level metrics, ensuring we can monitor the service's performance effectively.shadowsocks
Layer4 App Guest Module: This module introduces a specialized handler to manage both stream-based and packet-based Shadowsocks connections. It operates in conjunction with the outline host module, utilizing its shared configuration settings.Note:
While this PR establishes the core functionality, there are outstanding tasks related to metrics that might be addressed in separate PRs for better organization and focus:
shadowsocks_
prefix, particularly for server-wide metrics like{PREFIX}_build_info
, and{PREFIX}_ports
, which aren't specifically tied to Shadowsocks.outline
module.{PREFIX}_keys
and{PREFIX_ports}
metrics in this Caddy implementation.