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

cli: Test API access using /status/leader in consul watch #10795

Merged
merged 2 commits into from
Aug 9, 2021

Conversation

blake
Copy link
Member

@blake blake commented Aug 5, 2021

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

@blake blake added the theme/cli Flags and documentation for the CLI interface label Aug 5, 2021
@blake blake requested a review from a team August 5, 2021 09:21
@blake blake self-assigned this Aug 5, 2021
Copy link
Contributor

@dnephin dnephin left a 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

@blake blake force-pushed the blake/cli-watch-namespace-9353 branch from 2dcaa1c to 0885780 Compare August 8, 2021 18:02
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging August 8, 2021 18:02 Inactive
@vercel vercel bot temporarily deployed to Preview – consul August 8, 2021 18:02 Inactive
@blake
Copy link
Member Author

blake commented Aug 8, 2021

Thanks for the suggestion, @dnephin. I've changed the code to query the /status/leader endpoint prior to attempting to initialize the watch.

@vercel vercel bot temporarily deployed to Preview – consul August 8, 2021 18:08 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging August 8, 2021 18:08 Inactive
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
@blake blake force-pushed the blake/cli-watch-namespace-9353 branch from df8b533 to c049479 Compare August 8, 2021 18:17
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging August 8, 2021 18:17 Inactive
@vercel vercel bot temporarily deployed to Preview – consul August 8, 2021 18:17 Inactive
@blake blake changed the title cli: Remove nodeName call when using consul watch cli: Test API access using /status/leader in consul watch Aug 8, 2021
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@blake blake merged commit 6a68bfc into main Aug 9, 2021
@blake blake deleted the blake/cli-watch-namespace-9353 branch August 9, 2021 16:00
@hc-github-team-consul-core
Copy link
Collaborator

🍒 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.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 6a68bfc onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Aug 9, 2021
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
@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 6a68bfc onto release/1.9.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Aug 9, 2021
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
@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 6a68bfc onto release/1.8.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Aug 9, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/cli Flags and documentation for the CLI interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consul watch within namespace
3 participants