-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
rule: prevent rule crash from no such host error #3672
Conversation
@bwplotka please take a look. |
So no one else is permitted to take a look? :( |
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 think this is the right level of abstraction - it should be up to the end-user (i.e. the caller) to react in a certain way to an error. However, I don't like hardcoding the exact errors into the code at this exact place. What about having a method IsNotFoundError(err error) bool
in the ipLookupResolver
interface that we could use here? Then we would have same pattern as in BucketReader
and I think that works well.
Then, in the case of the miekg
resolver we could also return a special error if lookupWithSearchPath
returns NXDOMAIN & no error
so that the user of that resolver would also know when such an issue occurs.
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.
Just one suggestion but overall LGTM. Thank you!
Signed-off-by: Xiaochao Dong (@damnever) <[email protected]>
Thank you! |
The rule will crash due to alert managers restart, same with #3257 but I use
dnssrv+
/dnssrvnoa+
.Maybe we should make this line fault-tolerant rather than make changes in pkg/discovery/dns.