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

Allow reading secrets from Vault to configure TLS #4446

Merged
merged 19 commits into from
Mar 15, 2023

Conversation

fayzal-g
Copy link
Contributor

@fayzal-g fayzal-g commented Mar 9, 2023

What this PR does

If the VaultEnabled flag is set, creates a Vault client and reads secrets from the Vault to configure TLS for the mimir components listed here. Currently the only supported auth method is via an auth token.

dskit was updated to accept a SecretReader interface when configuring TLS. These are all the flags where TLS paths can currently be configured (*.tls-cert-path, *.tls-key-path, *.tls-ca-path):

  • -distributor.ha-tracker.etcd.tls-cert-path
  • -distributor.ring.etcd.tls-cert-path
  • -distributor.forwarding.grpc-client.tls-cert-path
  • -querier.store-gateway-client.tls-cert-path
  • -ingester.client.tls-cert-path
  • -ingester.ring.etcd.tls-cert-path
  • -querier.frontend-client.tls-cert-path
  • -query-frontend.grpc-client-config.tls-cert-path
  • -query-frontend.results-cache.redis.tls-cert-path
  • -blocks-storage.bucket-store.index-cache.redis.tls-cert-path
  • -blocks-storage.bucket-store.chunks-cache.redis.tls-cert-path
  • -blocks-storage.bucket-store.metadata-cache.redis.tls-cert-path
  • -compactor.ring.etcd.tls-cert-path
  • -store-gateway.sharding-ring.etcd.tls-cert-path
  • -ruler.client.tls-cert-path
  • -ruler.alertmanager-client.tls-cert-path
  • -ruler.ring.etcd.tls-cert-path
  • -ruler.query-frontend.grpc-client-config.tls-cert-path
  • -alertmanager.sharding-ring.etcd.tls-cert-path
  • -alertmanager.alertmanager-client.tls-cert-path
  • -memberlist.tls-cert-path
  • -query-scheduler.grpc-client-config.tls-cert-path
  • -query-scheduler.ring.etcd.tls-cert-path
  • -overrides-exporter.ring.etcd.tls-cert-path

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@fayzal-g fayzal-g marked this pull request as ready for review March 10, 2023 14:56
@fayzal-g fayzal-g requested review from a team as code owners March 10, 2023 14:56
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I just reviewed the config, so don't take this as a review :)

@@ -98,6 +99,7 @@ type Config struct {
Target flagext.StringSliceCSV `yaml:"target"`
MultitenancyEnabled bool `yaml:"multitenancy_enabled"`
NoAuthTenant string `yaml:"no_auth_tenant" category:"advanced"`
VaultEnabled bool `yaml:"vault_enabled" category:"experimental"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this inside the vault config data structure? We typically have a enabled config option inside the data struct config you want to enable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@fayzal-g fayzal-g force-pushed the vault-client-mimir branch 2 times, most recently from db91c3c to 2157f23 Compare March 13, 2023 11:21
pkg/vault/vault.go Outdated Show resolved Hide resolved
pkg/vault/vault.go Show resolved Hide resolved
pkg/vault/vault.go Outdated Show resolved Hide resolved
pkg/vault/vault_test.go Outdated Show resolved Hide resolved
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback. I've left some more minor comments, but this looks good.

pkg/vault/vault.go Outdated Show resolved Hide resolved
pkg/vault/vault_test.go Show resolved Hide resolved
pkg/vault/vault_test.go Show resolved Hide resolved
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you!

@pstibrany pstibrany merged commit 52fb469 into grafana:main Mar 15, 2023
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.

3 participants