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

tls_enabled param missing from ruler's alertmanager_client config #3220

Closed
mac133k opened this issue Oct 14, 2022 · 3 comments · Fixed by #3432
Closed

tls_enabled param missing from ruler's alertmanager_client config #3220

mac133k opened this issue Oct 14, 2022 · 3 comments · Fixed by #3432
Labels
component/ruler enhancement New feature or request help wanted Extra attention is needed

Comments

@mac133k
Copy link

mac133k commented Oct 14, 2022

Describe the bug

Ruler's alertmanager_client config section does not have tls_enabled param. I am not sure if that is a bug, but other config sections with TLS settings do have such param.

To Reproduce

Steps to reproduce the behavior:

In Mimir config add this bit:

ruler:
  alertmanager_client:
    tls_ca_path: /etc/mimir/ca.crt
    tls_cert_path: /etc/mimir/client.crt
    tls_enabled: true
    tls_key_path: /etc/mimir/client.key

Start Mimir:

# mimir -config.file /etc/mimir/mimir-ruler.yml
error loading config from /etc/mimir/mimir-ruler.yml: Error parsing config file: yaml: unmarshal errors:
  line 141: field tls_enabled not found in type ruler.NotifierConfig

Expected behavior

Alertmanager client config for ruler was expected to have tls_enabled toggle.

Environment

# mimir --version
Mimir, version 2.3.1 (branch: release-2.3, revision: 64a71a566)
  go version:       go1.18.4
  platform:         linux/amd64

# uname -r
4.18.0-372.13.1.el8_6.x86_64

Additional Context

Configuration reference doc does not mention tls_enabled in ruler's alertmanager client config either.

@pracucci
Copy link
Collaborator

You're right. The tls_enabled config option is not available for the ruler's alertmanager client, but TLS is still supported. Basically in the ruler's alertmanager client, TLS is implicitly enabled as soon as you set any TLS-related configuration option.

That being said, I understand this configuration discrepancy can be misleading and we could work to fix it. Willing to give it a try? I could offer some guidance.

@pracucci pracucci added enhancement New feature or request help wanted Extra attention is needed component/ruler labels Oct 17, 2022
@mac133k
Copy link
Author

mac133k commented Oct 18, 2022

Is it about a missing block entry in pkg/mimirtool/config/descriptors/mimir-v2.3.0.json?

@dimitarvdimitrov
Copy link
Contributor

It would be changing the configuration structure for the ruler - which is here

Notifier NotifierConfig `yaml:"alertmanager_client"`

here is an example of the TLS config for the store-gateway client (querier.store_gateway_client in the config)

type ClientConfig struct {
TLSEnabled bool `yaml:"tls_enabled" category:"advanced"`
TLS tls.ClientConfig `yaml:",inline"`
}
func (cfg *ClientConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
f.BoolVar(&cfg.TLSEnabled, prefix+".tls-enabled", cfg.TLSEnabled, "Enable TLS for gRPC client connecting to store-gateway.")
cfg.TLS.RegisterFlagsWithPrefix(prefix, f)
}

which is using the same underlying configuration as the ruler, but also has the tls_enabled flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ruler enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants