-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fail with error when autodiscover providers have no defined configs #13078
Conversation
@@ -43,6 +45,10 @@ type MapperSettings []*struct { | |||
|
|||
// NewConfigMapper builds a template Mapper from given settings | |||
func NewConfigMapper(configs MapperSettings) (mapper Mapper, err error) { | |||
if len(configs) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid config, for instance when people is using hints based autodiscover, it's ok to leave templates empty:
filebeat.autodiscover:
providers:
- type: kubernetes
hints.enabled: true
We would need to account for that for an error like this
if len(configs) == 0 { | ||
return nil, fmt.Errorf("no configs defined for autodiscover provider") | ||
} | ||
|
||
for _, c := range configs { | ||
condMap := &ConditionMap{Configs: c.Configs} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another check we could do here is to ensure len(c.Configs) > 0
. I think in most cases this is were bad configs happen? when the user does:
autodiscover:
providers:
type: kubernetes
templates:
- equals:
container.image: ...
config:
type: container
But config is expected to be a list, so a -
is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, is this section used when hints are enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for improving this!
@exekias I just pushed a fix that takes hints into account for providers that support them. |
sorry, I approved as the change looks good to me, but it seems one test is failing, we will probably need to adjust it after this change. A changelog will also be needed |
Kudos on this one!... it would've saved us a lot of time when we had our configuration mis-done. |
Sorry for the delay here, hoping to come back to this one within the next few weeks, I'm sidetracked on other PRs ATM |
If one accidentally misconfigures an autodiscover provider and there are no defined configs no error is raised today. This patch adds an error in this situation and also does a better job adding context to other initialization errors.
You can test this with the following config which looks like it should work in heartbeat:
However, it does not because
templates
should be an array. The correct config is below: