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

NET-6204- Repeating error log in consul-connect-injector #3128

Merged
merged 5 commits into from Oct 24, 2023
Merged

Conversation

sophie-gairo
Copy link
Contributor

@sophie-gairo sophie-gairo commented Oct 24, 2023

Changes proposed in this PR:
-dont error on timeouts in gateway

How I expect reviewers to test this PR:
Setup a consul server and let it sit for >5 minutes, we should not see this error pop up

Checklist:

@sophie-gairo
Copy link
Contributor Author

no error after 5 minutes of consul server running

Copy link
Member

@jm96441n jm96441n left a comment

Choose a reason for hiding this comment

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

LGTM

@nathancoleman nathancoleman changed the title Net 6204- Repeating error log in consul-connect-injector NET-6204- Repeating error log in consul-connect-injector Oct 24, 2023
Comment on lines 128 to 134
// if we timeout we don't care about the error message because it's expected to happen on long polls
// any other error we want to alert on
if !strings.Contains(strings.ToLower(err.Error()), "timeout") &&
!strings.Contains(strings.ToLower(err.Error()), "no such host") &&
!strings.Contains(strings.ToLower(err.Error()), "connection refused") {
r.logger.Error(err, fmt.Sprintf("unable to fetch config entry for gateway: %s/%s", ref.Namespace, ref.Name))
}
Copy link
Member

Choose a reason for hiding this comment

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

I wish we had a better way to do error handling in general, but this matches what we're doing elsewhere

entries, meta, err := client.ConfigEntries().List(kind, opts.WithContext(ctx))
if err != nil {
// if we timeout we don't care about the error message because it's expected to happen on long polls
// any other error we want to alert on
if !strings.Contains(strings.ToLower(err.Error()), "timeout") &&
!strings.Contains(strings.ToLower(err.Error()), "no such host") &&
!strings.Contains(strings.ToLower(err.Error()), "connection refused") {
c.logger.Error(err, fmt.Sprintf("error fetching config entries for kind: %s", kind))
}
continue
}

@nathancoleman nathancoleman enabled auto-merge (squash) October 24, 2023 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants