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

feat(caddy): add Caddy support for Outline services #198

Open
wants to merge 198 commits into
base: sbruens/logger
Choose a base branch
from

Conversation

sbruens
Copy link

@sbruens sbruens commented Aug 14, 2024

This proof of concept lays the foundation for integrating Outline service support into Caddy through the implementation of two new Caddy modules:

  1. 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.

  2. 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:

  • Metric Prefixing: Evaluate the necessity of aliasing or modifying the shadowsocks_ prefix, particularly for server-wide metrics like {PREFIX}_build_info, and {PREFIX}_ports, which aren't specifically tied to Shadowsocks.
  • ip2info Support: Implement support for ip2info configuration within the outline module.
  • Key and Port Metrics: Track {PREFIX}_keys and {PREFIX_ports} metrics in this Caddy implementation.

sbruens added 30 commits May 31, 2024 17:03
@sbruens sbruens force-pushed the sbruens/logger branch 3 times, most recently from b7bbfa0 to 7480f3c Compare September 16, 2024 21:36
caddy/app.go Show resolved Hide resolved
caddy/app.go Outdated Show resolved Hide resolved
caddy/app.go Show resolved Hide resolved
caddy/app.go Show resolved Hide resolved
caddy/shadowsocks_handler.go Outdated Show resolved Hide resolved
caddy/shadowsocks_handler.go Outdated Show resolved Hide resolved
caddy/shadowsocks_handler.go Outdated Show resolved Hide resolved
caddy/app.go Outdated
func (OutlineApp) CaddyModule() caddy.ModuleInfo {
return caddy.ModuleInfo{
ID: outlineModuleName,
New: func() caddy.Module { return new(OutlineApp) },
Copy link

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.

Copy link

@fortuna fortuna Sep 17, 2024

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) }
  })
}

Copy link
Author

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.

Copy link
Author

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.

Copy link

@fortuna fortuna Sep 19, 2024

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.

@sbruens sbruens changed the title feat: add Caddy support for Outline services feat(caddy): add Caddy support for Outline services Sep 18, 2024
)

func (OutlineApp) CaddyModule() caddy.ModuleInfo {
return caddy.ModuleInfo{ID: outlineModuleName}
Copy link
Author

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.

Copy link

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.

)

func (OutlineApp) CaddyModule() caddy.ModuleInfo {
return caddy.ModuleInfo{ID: outlineModuleName}
Copy link

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.

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.

2 participants