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

Add TLS and basic auth #4211

Merged
merged 4 commits into from
May 17, 2021
Merged

Add TLS and basic auth #4211

merged 4 commits into from
May 17, 2021

Conversation

saswatamcode
Copy link
Member

@saswatamcode saswatamcode commented May 10, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This PR adds in TLS and basic auth to Thanos HTTP APIs using exporter-toolkit. Configuration follows a similar pattern to Prometheus, where a file containing TLS configuration is provided using http-tls-config flag.
Addresses issue #4168

Verification

Tested locally.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Some thoughts. Also we might want some docs 🙈

@@ -54,6 +55,10 @@ func (hc *httpConfig) registerFlag(cmd extkingpin.FlagClause) *httpConfig {
cmd.Flag("http-grace-period",
"Time to wait after an interrupt received for HTTP Server.").
Default("2m").SetValue(&hc.gracePeriod)
cmd.Flag(
"http-tls-config",
"[EXPERIMENTAL] Path to configuration file that can enable TLS or authentication.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"[EXPERIMENTAL] Path to configuration file that can enable TLS or authentication.",
"[EXPERIMENTAL] Path to the configuration file that can enable TLS or authentication for all HTTP endpoints.",

@@ -54,6 +55,10 @@ func (hc *httpConfig) registerFlag(cmd extkingpin.FlagClause) *httpConfig {
cmd.Flag("http-grace-period",
"Time to wait after an interrupt received for HTTP Server.").
Default("2m").SetValue(&hc.gracePeriod)
cmd.Flag(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about doing pathorcontent flag? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Will add that in!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, exporter-toolkit doesn't take in content(byte[]) of the TLS config file as an argument for its server, it only takes in the path(string) as highlighted here. So using pathorcontent isn't currently possible I think, since this flag fetches bytes. @bwplotka

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

httpBindAddr = cmd.Flag("http-address", "Listen host:port for HTTP endpoints.").Default("0.0.0.0:10902").String()
httpGracePeriod = ModelDuration(cmd.Flag("http-grace-period", "Time to wait after an interrupt received for HTTP Server.").Default("2m")) // by default it's the same as query.timeout.

return httpBindAddr, httpGracePeriod
httpTLSConfig = cmd.Flag(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Comment on lines 64 to 67
err := toolkit_web.Validate(s.opts.tlsConfigPath)
if err != nil {
level.Error(s.logger).Log("msg", "server could not be started", "err", err)
return errors.Wrap(err, "server could not be started")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
err := toolkit_web.Validate(s.opts.tlsConfigPath)
if err != nil {
level.Error(s.logger).Log("msg", "server could not be started", "err", err)
return errors.Wrap(err, "server could not be started")
}
if err := toolkit_web.Validate(s.opts.tlsConfigPath); err != nil {
return errors.Wrap(err, "server could not be started")
}

Let's make sure to avoid handling the same error twice.

bwplotka
bwplotka previously approved these changes May 11, 2021
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! LGTM, just one nit.


return httpBindAddr, httpGracePeriod
httpTLSConfig = cmd.Flag(
"http-tls-config",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, I heard from @roidelapluie that Prometheus flag is called web.config.file and not tls-config.file for a good reason: It's not only TLS. I think it's good idea to change it to http.config maybe? Also we can expand (contribute) to exporter toolking to support passing parsed configuration too if we want pathorcontent flag (up to us)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http.config sounds nice, will change it to that. I'll try to add the passing configuration functionality to exporter-toolkit, but once merged they would probably need to release that. Once it's released with configuration, we can switch over to pathorcontent flag(will make it a separate PR). WDYT? @bwplotka

Copy link

@roidelapluie roidelapluie May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand you do not need changes in the downstream exporter toolkit to achieve what you want.

You could probably do all of this in thanos. --http-config={file} or --http-config=/path:{file} could be supported without upstream changes.

Copy link
Member Author

@saswatamcode saswatamcode May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably do all of this in thanos. --http-config={file} or --http-config=/path:{file} could be supported without upstream changes.

Currently we are doing this, i.e, taking a file path string. But if we try to use the pathorcontent flag that would return the content of the file in byte[] which we can't pass to any of the toolkit methods. Wouldn't that require some changes? @roidelapluie

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I do not understand the change. Are you trying to use basic auth on certain paths only? Or am I misunderstanding?

Copy link
Member Author

@saswatamcode saswatamcode May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to add TLS and basic auth in all HTTP endpoints. This PR currently adds a flag(http-tls-config which I'll change tohttp.config), which like Prometheus takes in the path of the file containing TLS/auth config. If we use the pathorcontent method for this flag, user can pass path of the config file or directly pass the content of the file. Either way pathorcontent will return the content of the config file. But then we can't pass it to exporter-toolkit since the methods only take in path. So, I think the change needs to be made in toolkit...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a reason why we take a path. The file is read on every request to make sure that the TLS certificate and the users are up to date.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, okay. Then I guess using pathorcontent isn't suitable and the normal Flag method would be better. Also, no change is needed in exporter or Thanos then. Thanks a lot for explaining! 🙂

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Epic work! LGTM 💪🏽

cc @kakkoyun and anyone else

I will merge in few hours, if no objections

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bwplotka bwplotka merged commit f2564a7 into thanos-io:main May 17, 2021
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.

4 participants