Thanos: Feature Request: Detect credentials changes without application restarts

[Capitrium]

Currently, Thanos reads the bucket configuration file only at startup. This works fine for most cases, but becomes problematic when the credentials are generated and rotated by an external service. In this case, the credentials that Thanos started with will become invalid and interactions with the configured object storage will fail until Thanos is restarted, which is not ideal.

Some potential solutions to this problem include implementing a hot reloading feature, or adding a sidecar container that triggers a restart of Thanos when the config file is updated.

[bwplotka]

Thanks!

TBH I would rather go for the latter option. Something that might be helpful is this: https://github.com/jetstack/cert-manager/issues/1168 as it is exactly the same problem.

Particularly https://github.com/pusher/wave

Essentially on k8s you would have such cases all the time, so this might be the way to go for now. Not sure if it's worth to add complexity to the code as restart might be just enough. We might want to enumerate all components and see if restart is safe. E.g all components, especially Prometheus + sidecar required HA for safe rolling restart etc.

[Capitrium]

While Wave seems interesting, to be honest I'm not too enthusiastic about depending on yet another service just to handle the rotation of the config file. It means I need to worry about the integrations with this extra service (does it support StatefulSets and DaemonSets? Or just Deployments, and the others are left out in the cold?), whether the service is highly available, any physical resources required to run this additional service, etc.

It also makes it more difficult to integrate with other services. As an example, the prometheus-operator completely manages the StatefulSets it creates. If it didn't provide a way to set custom annotations on those StatefulSets, how would I opt those pods into using Wave? (Thankfully the prometheus CRD has a podMetadata field for this purpose, but hopefully you get my point. :smile:)

K8s already provides updates to ConfigMap and Secret volumes on the kubelet's resync interval, so IMO any applications that expect to run on K8s should also expect any config file mounted from a ConfigMap or Secret to be updated during execution and should handle that accordingly. However, your concerns about maintaining availability during restarts triggered by a config rotation are definitely valid; applications that need to coordinate restarts between highly-available components would need to do so through a ConfigMap or something similar, which definitely adds complexity on the Thanos side.

As another option, it should be pretty simple to propagate the credentials errors that are thrown when the credentials are invalid (err="check exists: stat s3 object: Access Denied."), so that Thanos exits with an error and K8s can simply restart the container. This is the easiest/laziest option, but since Thanos is semi-broken when it's given invalid credentials I don't see it as a particularly bad one for now. 😆

[bwplotka]

I agree with your points. Let's see what others think, cc @GiedriusS @domgreen

Current options.

1. Add hot-reload. I find the implementation quite tricky as you need to swap the credentials in the ongoing requests to obj store so it must be syncronized. But doable and maybe worth it.
2. err="check exists: stat s3 object: Access Denied." option is not great as essentially you get random k8s reset no matter what. And in case of some components it is kind of painful (store gw, prometheus + sidecar), also is this working for non k8s setup as well? And how to even get access denied error in every provider? (:

Maybe we should look on other projects like envoy, prometheus (creds to kubernetes API eg), etc how they do it? I think because Prometheus configuration is reloadable, then it might be doable.

[Capitrium]

Add hot-reload. I find the implementation quite tricky as you need to swap the credentials in the ongoing requests to obj store so it must be syncronized.

I'm not sure I follow you here. My understanding is that the signature of S3 upload requests is calculated up-front using the provided access key/secret, the region, the date, and a few other parameters. Just changing the credentials on-disk wouldn't invalidate requests in-flight, as their signatures would have been calculated beforehand using the previous set of credentials. Can you clarify why swapping the credentials for ongoing requests is an issue?

[bwplotka]

Sure, I meant it's just you need to swap "HTTP clients" while not affecting application. To do so you need to close all pending connections and not start new ones and swap and unblock again. In terms of current implementation it's not trivial.

[Capitrium]

Sorry, I'm still not understanding which part is non-trivial. I'm also confused as to why we would need to close pending requests and block while swapping clients, as both clients should be able to process requests during the cutover; it seems like you might be confusing _rotation_, which deploys a new set of credentials, and _revocation_, which deletes the old set of credentials (sorry again if this is not the case).

Here's the flow I have in mind:

1) Thanos starts with credentials set A
2) Later, the credentials are _rotated_, resulting in 2 sets of credentials, A and B
3) Thanos receives credentials set B on the Secret volume mount on the kubelet's resync interval, replacing set A
4) Thanos swaps the http clients / bucket config / whatever, so that _new_ requests use set B; in-flight requests are unaffected and are still using set A
5) Later, the old credentials are _revoked_. At this point, Thanos is only using set B, so there's nothing else to do application-wise

The old client's credentials are still valid after _rotation_, it's only after _revocation_ that they become invalid. Shouldn't this be as simple as checking / updating the credentials at the start of every Sync call? Or maybe I'm just missing something 😅

Any synchronization around a hard switch of credentials affecting in-flight requests I'd think would definitely be out-of-scope for this feature.

[Capitrium]

@bwplotka After digging into the code a bit more, it appears that (at least for S3/Minio clients) this is only an issue when passing the credentials in either as environment variables, or (as we're currently doing) setting the access_key/secret_key values in the objstore config file. The client itself tries to get fresh credentials for every request, but in both of those cases the credentials provided are static, and the credentials provider will always read the same set of credentials until Thanos is restarted.

The solution for those who want dynamically-provided credentials from an external service is to format them as a standard config file and mount it where the client expects it (i.e. ~/.aws/credentials). The client will then read those credentials for every request, picking up any changes that might be propagated through the file mount.

Closing since there's no code updates required, but expanding the documentation around providing credentials to explain which ones are static and which ones can be refreshed dynamically might be a good idea. :wink:

Thanks again for taking the time to look into this.

[bwplotka]

@Capitrium asked:

hey @bwplotka, before I reopen that feature request, can you help me understand why the implementation is non-trivial? I’m having a lot of trouble understanding why we can’t just check if the credentials have changed at the start of every Sync call and re-instantiate the bucket client if they have. I need hot-reloading pretty urgently so I can work on it myself if I need to, but I just don’t understand why it’s non-trivial to update the credentials.

1. We create bucket client on very start and it shared by everyone
2. For compactor it's easy I agree -> the start of every Sync as there is just single loop (it might change soon). BUT what about other components? sidecar, store, ruler? they will have to rotate as well? And there there is no single loop.

[bwplotka]

The solution for those who want dynamically-provided credentials from an external service is to format them as a standard config file and mount it where the client expects it (i.e. ~/.aws/credentials). The client will then read those credentials for every request, picking up any changes that might be propagated through the file mount.

So ... it's doable? have you tested it?

[Capitrium]

So ... it's doable? have you tested it?

I dug into this a little more on Friday and over the weekend; the short answer is no.

It _would_ be doable for S3 if Thanos was using the aws sdk, but the minio-go client doesn't expose the function in the aws-go-sdk to re-read credentials from disk, so it will keep using the same set of cached/expired credentials. 😞

For compactor it's easy I agree -> the start of every Sync as there is just single loop (it might change soon). BUT what about other components? sidecar, store, ruler? they will have to rotate as well? And there there is no single loop.

Doesn't the sidecar run a single sync loop here? This is probably the most critical one since without it, metrics stop getting uploaded to the objstore after old credentials are revoked.

The store may be trickier, but there aren't any HA concerns for the store or the compactor that I'm aware of, so those could probably just be restarted by a sidecar.

I don't actually run the ruler component so I have no idea as far as that one goes 😆

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[george-angel]

unstale please )

[stale[bot]]

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

[george-angel]

unstale please )

[bwplotka]

Thanks for being patient @george-angel.

It adds some complexity, but I think this could be added to Thanos. We are already working on reloading TLS certs for various HTTP and gRPC connections, so we can do this for buckets as well.

Help wanted to design and implement :+1:

[george-angel]

We are a little busy right now, but as soon as we have some time, will make sure to drop a line here to say we are working on this.

[prg3]

I'm running into this issue as well, we use Hashicorp Vault for secrets management and Vault supplies short lived AWS credentials, so we rotate the S3 keys regularly. Looks like the only option at present is to restart Thanos when the keys change?

[bwplotka]

Right now, yes.

On Wed, 12 Feb 2020, 21:15 Paul Greidanus, [e-mail] wrote:

I'm running into this issue as well, we use Hashicorp Vault for secrets
management and Vault supplies short lived AWS credentials, so we rotate the
S3 keys regularly. Looks like the only option at present is to restart
Thanos when the keys change?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/thanos-io/thanos/issues/985?email_source=notifications&email_token=ABVA3OZWWEQKU4N65ZR2OP3RCRRHRA5CNFSM4HB22AG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELSNDCI#issuecomment-585421193,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABVA3O6OQUNBPXHGXCINCD3RCRRHRANCNFSM4HB22AGQ
.

[kush-patel-hs]

Hello all. I just dealt with this for a different Go project https://github.com/rwynn/monstache/issues/333. The end solution seems to work really well. I have a design/rough draft for Thanos which does something similar https://github.com/thanos-io/thanos/pull/2135. If this looks like something we want to do then we can polish up the code and get it production ready. Thoughts?

I hope this helps move this forward!

[bwplotka]

Thanks!

Question is do we have agreement on the implementation details or do we
need to go through design proposal process:
https://thanos.io/contributing/#adding-new-features-components

I looked briefly through your PR and I think we need something on top of
object storage interface / client, not the particular implementation like
S3.

On Thu, 13 Feb 2020, 19:11 Kush Patel, [e-mail] wrote:

Hello all. I just dealt with this for a different Go project
rwynn/monstache#333 https://github.com/rwynn/monstache/issues/333. The
end solution seems to work really well. I have a design/rough draft for
Thanos which does something similar #2135
https://github.com/thanos-io/thanos/pull/2135. If this looks like
something we want to do then we can polish up the code and get it
production ready. Thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/thanos-io/thanos/issues/985?email_source=notifications&email_token=ABVA3O6WSGW4D4MG7OPIC4TRCWLOVA5CNFSM4HB22AG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELWH42Q#issuecomment-585924202,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABVA3O7UTKOSK3SBMQO4ABDRCWLOVANCNFSM4HB22AGQ
.

[kush-patel-hs]

It's a non-breaking change and relatively small, so as long as there are no objections we probably don't need to create a design doc for it.

In order for it to be on top of object interface/client we would need to have an interface for Credentials across the different clients and implement that interface for each. Then we can call a general Expire function on those Credentials. Right now we don't store credentials at that layer so I kept it close to where we create the AWS Credentials. If we want to do a larger re-factor later to generalize credentials then this would not impede that because of its non-breaking nature. Thoughts? I think it would be hard to put it at the interface/client level unless you have an easy idea and I missed something.

[bwplotka]

It's a non-breaking change and relatively small.

Maybe it is not now, but if it will become problematic or we will need to switch something else - it will be :warning:

In order for it to be on top of object interface/client, we would need to have an interface for Credentials across the different clients and implement that interface for each.

Not at all. We could just make our objstore.config-file reloadable (e.g watching/reload endpoint) or at part of it. Then we can write an implementation of objstore.Bucket that will recreate wrapped objstore.Bucket if the content of the config changes in relevant part. DONE.

To sum up: Thanks for your work @kush-patel-hs, but let's not rush too much. Design doc is always nice to avoid introducing snowflakes, especially if the proper solution is not that hard.

cc @brancz @GiedriusS @metalmatze @squat opinions?

[kush-patel-hs]

The one thing that may not work out with making the objstore.config-file reloadable is we don't merge AWS Credentials into objstore.config-file if we want to read them from ~/.aws/credentials. So that's the entire else if config.AccessKey isn't set.

[bwplotka]

Sorry, I don't get this part. Why there cannot be something (e.g sidecar) that will send SIGHUP or POST /-/reload that will trigger reload of config? I think what you propose is definitely super specific to S3 and we are interested in Thanos to have a consistent experience across object storages.

[kush-patel-hs]

Sorry we're having two conversations, we can keep it to here. So we have a sidecar for Vault that all (ones that need secrets) of our services run which would get credentials. It will write these credentials to ~/.aws/credentials. So we would need to have another sidecar for thanos to watch the directory and somehow merge the credentials into the config of thanos (Since the AWS Credentials are not stored in the main config).

Currently Thanos would check if there is a AWS Key in config, but otherwise it would use credentials from Environmental variable or from a file which is not config. In this case calling SIGHUP or POST /-/reload would not be sufficient to reload the credentials unless it does NewBucketWithConfig again. I'm unsure how many other object storages in Thanos also read from an external file rather than the config.

[bwplotka]

unless it does NewBucketWithConfig

Yes, and that's what we probably need to do. It's should be fine.

[kush-patel-hs]

In that case it could be ok. We would have to run an extra sidecar or something to watch for our AWS Credentials change then hit the Thanos POST /-reload endpoint as opposed to Thanos just knowing and doing it, but I guess that is fine. Is there many places where NewBucket is created and stored? Might be hard to swap them all if so 🤔

[bwplotka]

We already have this: https://github.com/coreos/prometheus-operator/tree/master/cmd/prometheus-config-reloader sidecar which already does a similar things. So it might be just copy&paste for your needs.

Is there many places where NewBucket is created and stored? Might be hard to swap them all if so

Just one in every command. The implementation might be bit challenging to do it right though (: But happy to guide anyone who wants to help.

Also, I would love to hear opinions of others e.g @brancz Maybe we are going to far with allowing this on Thanos. However, I would say it's fine for all objstores as we already do that for AMs and certificates etc...

[kush-patel-hs]

Yeah I'm unsure of where to start for the implementation. Seems like Buckets are not stored in structs either, they are created then a go routine is started and it is passed to a Shipper 🤔

Would love other thoughts. If we could avoid going all out it would be nice to get the feature as we're a bit blocked for using short lived creds.

[bwplotka]

The struct thing is fine. in my free time I might propose what I mean.

On Fri, 14 Feb 2020, 19:49 Kush Patel, [e-mail] wrote:

Yeah I'm unsure of where to start for the implementation. Seems like
Buckets are not stored in structs either, they are created then a go
routine is started and it is passed to a Shipper 🤔

Would love other thoughts. If we could avoid going all out it would be
nice to get the feature. Another thought I had is if the minio client's
FileCredentials did the watch for reload.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/thanos-io/thanos/issues/985?email_source=notifications&email_token=ABVA3OZ6LRBUFVJXRLNXPPLRC3YUBA5CNFSM4HB22AG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL2HQ5I#issuecomment-586446965,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABVA3O64C5XZYEQYYTRFVDDRC3YUBANCNFSM4HB22AGQ
.

[kush-patel-hs]

thanks!!

[brancz]

I think this would be pretty cool, but quite involved, and judging by experience in Prometheus, _very_ hard to get right. But generally I think it would be fantastic to have reloadable configuration, with the same semantics as Prometheus, as in I don't think we should be watching the file, but rather have a mechanism to trigger a reload.

[kush-patel-hs]

bump

[brancz]

@kush-patel-hs I don't think the thanos team is actively working on this, but as signaled before, we're not against it. If someone wants to work on this, go ahead! :)

[stale[bot]]

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

[george-angel]

remove stale

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[george-angel]

remove stale

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[george-angel]

remove stale

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[george-angel]

remove stale

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[george-angel]

remove stale

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[george-angel]

remove stale