Add support for custom internal server #7069
Conversation
periklis
force-pushed
the
internal-server
branch
3 times, most recently
from
f3da4b8
to
706e94a
Compare
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
- loki -0.5% |
pkg/server/internal_server.go
Outdated
|
|
||
| // RegisterFlags add internal server flags to flagset | ||
| func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | ||
| f.StringVar(&cfg.Config.HTTPListenAddress, "internal-server.http-listen-address", "", "HTTP internal server listen address.") |
There was a problem hiding this comment.
Wonder if it should default to 'localhost'?
There was a problem hiding this comment.
I took the defaults from weaveworks verbatim, but for an internal server that is used for readiness and metrics it makes sense to default listen on localhost.
| @@ -131,6 +133,7 @@ func (c *Config) registerServerFlagsWithChangedDefaultValues(fs *flag.FlagSet) { | |||
| // Register to throwaway flags first. Default values are remembered during registration and cannot be changed, | |||
| // but we can take values from throwaway flag set and reregister into supplied flags with new default values. | |||
| c.Server.RegisterFlags(throwaway) | |||
| c.InternalServer.RegisterFlags(throwaway) | |||
There was a problem hiding this comment.
Don't think that is necessary
There was a problem hiding this comment.
Actually it is needed to register the flags to main flagset of Loki or else we can't configure the internal server.
|
|
||
| // Config extends weaveworks server config | ||
| type Config struct { | ||
| serverww.Config `yaml:",inline"` |
There was a problem hiding this comment.
That also includes a lot of struct fields which are unused, I would prefer to shrink those down a bit more.
I don't know if it even needs to be supporting TLS, but I guess the list of the registered flags would be a good start.
There was a problem hiding this comment.
In the scope of exposing only the ready handler it makes sense to shrink this more. However, as you mention above it makes sense to expose the metrics from this handler too and thus I would like to keep the inline struct as such for start but limit the inputs via the shrinked registered flags. As per exposing metrics allowing TLS from start is imho a good thing to do. For example on OpenShift we allow only scraping over HTTPS.
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
- loki -0.5% |
periklis
force-pushed
the
internal-server
branch
from
2ef2377
to
ab183d4
Compare
Co-authored-by: Christian Simon <simon@swine.de>
Co-authored-by: Christian Simon <simon@swine.de>
periklis
force-pushed
the
internal-server
branch
from
ab183d4
to
a7a48a1
Compare
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
- loki -0.5% |
Co-authored-by: Christian Simon <simon@swine.de>
What this PR does / why we need it:
The following PR adds support for a custom internal server listener dedicated for the start to serve the ready handler on a separate port and optionally TLS configuration. The key purpose for this one is to serve the Loki HTTP API via mTLS while serving the readiness endpoints without (i.e. kubernetes readiness/liveness probes do not support authentication)
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CHANGELOG.md.docs/sources/upgrading/_index.md