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.

Sign up for GitHub

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

Jump to bottom

Skip retry on bad requests #66

Merged
merged 2 commits into from
Oct 12, 2021
Merged

Skip retry on bad requests #66

merged 2 commits into from
Oct 12, 2021

Conversation

findkim
Copy link
Contributor

@findkim findkim commented Oct 12, 2021

Example response for various endpoints with namespace query param against Consul OSS

$ curl localhost:8500/v1/kv/key?ns=dne
Bad request: Invalid query parameter: "ns" - Namespaces are a Consul Enterprise feature
$ curl localhost:8500/v1/catalog/services?ns=dne
Bad request: Invalid query parameter: "ns" - Namespaces are a Consul Enterprise feature

Client example of requests returning 400s and hcat retrying. This client has 8 retry attempts with total wait of 12mins.

2021/10/12 08:43:58 [ERR] kv.exists.get(key?ns=dne) error querying consul: Unexpected response code: 400
2021-10-12T08:43:58.336-0500 [DEBUG] hcat: couldn't connect with Consul. Waiting to retry: wait_duration=11.907257985s attempt_number=4

Fixes kv.exists.get string method to have delimiters (? between name and params, & between params)

@findkim
Skip retrying fetch on 400
79dea2f 
When querying Consul OSS with the namespace query param, it results
in a 400. And when retry is configured, it attempts to retry a bad
request. Changes now return on first 400 instead of waiting through
all the retries.

on the client side, it is preceived as hanging and blocks waiting
on expected behavior.
@findkim
Fix KVExistsGetQuery.String() format
b75ee1b
@findkim findkim added the enhancement New feature or request label Oct 12, 2021
@findkim
Copy link
Contributor Author

findkim commented Oct 12, 2021

Ideally it would be nice for hashicat to consume new changes that structure the Consul API status errors hashicorp/consul#11054. But it would require waiting on the api module to release a new version and for hashicat to update from depending on the api module v1.4.0 to 1.12+, at first glance it seems like invested work so skipping this altogether and opted for string matching.

@findkim findkim requested a review from a team October 12, 2021 14:32
lornasong
lornasong approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@lornasong lornasong left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

I noticed when discovering this issue that catalog-services didn't do a retry whereas consul-kv did the retry. Is it something on the CTS-side (vs. hcat) that causes the two to have different behavior? If you're answering on the CTS-side PR, feel free to disregard.

@findkim
Copy link
Contributor Author

findkim commented Oct 12, 2021

Yeah! CTS has Consul retry configured. The retry was only attempting for consul-kv and not catalog-services because that cs is custom on the CTS side which was missing the Consul() method to satisify the isConsul interface. This interface is determines which retry method to use, and CTS's catalog-services was missing it so it never retried and returned on the first error.

@findkim findkim merged commit a2ff564 into master Oct 12, 2021
@findkim findkim deleted the consul-query-param branch October 12, 2021 15:22
@lornasong
Copy link
Member

Ah interesting. When an issue with catalog-services (this is me!) causes "correct" behavior elsewhere. Thanks for sharing!

@eikenb
Copy link
Contributor

eikenb commented Oct 12, 2021

Yeah! CTS has Consul retry configured. The retry was only attempting for consul-kv and not catalog-services because that cs is custom on the CTS side which was missing the Consul() method to satisify the isConsul interface. This interface is determines which retry method to use, and CTS's catalog-services was missing it so it never retried and returned on the first error.

Improving this way of determining the retry function is part of the work on standardizing the external dependency's modules interface that is on the roadmap for hashicat (a bit out at 0.5.0 though). I've created a ticket in hashicat to track thoughts on what would be needed for the module abstraction.

#67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lornasong lornasong lornasong approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@findkim @lornasong @eikenb