-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
cli: Test API access using /status/leader in consul watch #10795
Conversation
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 working on this fix! I think this change would indeed fix the problem, but I wonder why this was done in the first place.
I looked through git history, but it seems like this was added as part of the first commit for this command, so there's no real context.
I have a vague recollection (which might be wrong) of a conversation about this, someone had suggested we did this kind of thing in a watch as a way of catching connection problems before going into the blocking loop.
I wonder if that kind of pre-check before starting a blocking loop would still be valuable. It's unfortunate that we don't have a /v1/ping
endpoint that we can use to check basic connectivity.
I recently learned that /v1/status/leader
(Status().Leader()
from the api client) will never return an error unless there is a connection problem. That API endpoint also requires no authorization.
Maybe we could replace the Agent().NodeName()
call with Status().Leader()
and document why we are using it. Something along the lines of:
- there is no ping endpoint
- we want a ping before going into the blocking loop
- /status/leader can act as a substitute for
/ping
because it requires no ACL permissions
2dcaa1c
to
0885780
Compare
Thanks for the suggestion, @dnephin. I've changed the code to query the |
Replace call to /agent/self with /status/leader to verify agent reachability before initializing a watch. This endpoint is not guarded by ACLs, and as such can be queried by any unauthenticated API client. Fixes #9353
df8b533
to
c049479
Compare
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.
LGTM, thank you!
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/423988. |
🍒✅ Cherry pick of commit 6a68bfc onto |
Replace call to /agent/self with /status/leader to verify agent reachability before initializing a watch. This endpoint is not guarded by ACLs, and as such can be queried by any API client regardless of their permissions. Fixes #9353
🍒✅ Cherry pick of commit 6a68bfc onto |
Replace call to /agent/self with /status/leader to verify agent reachability before initializing a watch. This endpoint is not guarded by ACLs, and as such can be queried by any API client regardless of their permissions. Fixes #9353
🍒✅ Cherry pick of commit 6a68bfc onto |
Replace call to /agent/self with /status/leader to verify agent reachability before initializing a watch. This endpoint is not guarded by ACLs, and as such can be queried by any API client regardless of their permissions. Fixes #9353
Replace call to /agent/self with /status/leader to verify agent reachability before initializing a watch. This endpoint is not guarded by ACLs, and as such can be queried by any unauthenticated API client.
Fixes #9353