-
Notifications
You must be signed in to change notification settings - Fork 491
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
cmd/bosun: Shorten-only http proxy #1590
cmd/bosun: Shorten-only http proxy #1590
Conversation
Silly question: what problem does this solve? |
Our outbound proxies are on a different network than where OpentTSDB is. In our case we simply cannot use the same for both sets of calls. I also think going through a proxy to fetch data from OpenTSDB is generally unneeded in most deployments, unlike reaching out to googleapis. |
yeah... Internet calls vs Intranet calls makes sense. I'd change the name to InternetProxy so it can be reused if we ever add any other APIs, but otherwise sounds fine to me. |
if *flagQuiet { | ||
c.Quiet = true | ||
} | ||
go func() { slog.Fatal(web.Listen(c.HTTPListen, *flagDev, c.TSDBHost)) }() | ||
go func() { slog.Fatal(web.Listen(c.HTTPListen, *flagDev, c.TSDBHost, shortenProxy)) }() |
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.
I don't love the idea of yet another argument to Listen. How about a package variable in web that you set when reading the config above here?
3098748
to
9768b90
Compare
9768b90
to
5e9cdec
Compare
How about this? |
Thanks for those changes. I like it. |
LGTM |
cmd/bosun: Shorten-only http proxy
This adds shortenProxy as option in bosun.toml to add a proxy only to requests out to googleapis' shortener api. The alternative is making use of the default http client's http.ProxyFromEnvironment but it results in all http calls to use it. That includes calls to OpenTSDB which isn't a something we want.