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

check: fixes updates for checks in namespaces #259

Merged
merged 1 commit into from
May 6, 2024

Conversation

t-davies
Copy link
Contributor

@t-davies t-davies commented Apr 26, 2024

Description

Fixes a bug where if a check existed in a namespace, although the check would run - the status would not be updated as ESM incorrectly determined that the check had been deregistered.

/v1/health/node/:node will not return checks in all namespaces by default, therefore we were incorrectly returning early from handleUpdateCheck if the check we were looking for was associated to a service in a different namespace from our Consul token.

We now pass the namespace stored against the check when querying to make sure the check still exists. This should be safe for both Consul OSS and Enterprise, checks for OSS will just send "default" as the namespace for every call - need to double check though as some endpoints will return a 400 when passed the ns query param.

Testing & reproduction steps

Tricky to add test coverage for this, since it requires Consul Enterprise. Tested in a real deployment with Consul Enterprise and verified that this resolves the issue.

To reproduce original bug

  1. Register a node, e.g. "node1".
  2. Create a new namespace, e.g. "ns1".
  3. Register a service, with a check, in "ns1" and associate with the "node1".
  4. Run Consul ESM, the check will be detected and executed - but the status is never updated and nothing is logged.

Links

/v1/health/node/:node will not return checks in all namespaces by
default, therefore we were incorrectly returning early from
handleUpdateCheck if the check we were looking for was associated
to a service in a different namespace from our Consul token.
@ndhanushkodi ndhanushkodi merged commit 4ae7b11 into hashicorp:main May 6, 2024
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants