ca: fix a bug that caused a non blocking leaf cert query after a blocking leaf cert query to block #12820

rboyer · 2022-04-19T15:01:01Z

Fixes #12048

Fixes #12319

Regression introduced in #11693

Local reproduction steps:

consul agent -dev
curl -sLiv 'localhost:8500/v1/agent/connect/ca/leaf/web'
make note of the X-Consul-Index header returned
curl -sLi 'localhost:8500/v1/agent/connect/ca/leaf/web?index=<VALUE_FROM_STEP_3>'
Kill the above curl when it hangs with Ctrl-C
Repeat (2) and it should not hang.

agent/cache/cache.go

boxofrad

Looks great, left a couple of small comments/questions 🎉

agent/agent_endpoint_test.go

agent/cache/cache.go

…king leaf cert query to block Fixes #12048 Fixes #12319 Regression introduced in #11693

acpana · 2022-04-19T22:26:26Z

agent/cache/cache.go

@@ -376,6 +376,13 @@ func (c *Cache) getEntryLocked(
 	// Check if re-validate is requested. If so the first time round the
 	// loop is not a hit but subsequent ones should be treated normally.
 	if !tEntry.Opts.Refresh && info.MustRevalidate {
+		if entry.Fetching {


Ah, this is one of those lines that I hope doesn't surface code that was relying on side effects here.

AFAIK, (and I don't know much) there's not a lot of cache types that actually use MustRevalidate.

So my gut tells me that this should work beautifully for the bug in #12048 🤞🏼

But it would feel good to check in with @banks or @freddygv if they have more context here to get a third/ fourth pair of eyes 💯

acpana · 2022-04-19T22:38:21Z

agent/agent_endpoint_test.go

+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	go func() {


this is a non actionable comment:

for any of y'all wondering about testing that the blocking query does what we want for the leaf cert, that use case is here: https://github.com/hashicorp/consul/blob/main/agent/agent_endpoint_test.go#L6016-L6033

acpana · 2022-04-19T22:40:53Z

hey @rboyer thanks loads for taking this on 🥇 💯 ; that bug has been in the back of my mind 🪨

i see the backport/1.12 tag; I'll leave it up to you if you want to check this in for 1.12 or wait to chat with folks more familiar with the cache layer and have it in 1.12.1. Regardless, I made a milestone tag for 1.12.1 fyi .

kisunji · 2022-04-20T14:01:12Z

agent/agent_endpoint_test.go

+
+	// We just need to ensure that the above blocking query is in-flight before
+	// the next step, so do a little sleep.
+	time.Sleep(50 * time.Millisecond)


Could we make this deterministic with context.WithTimeout and calling the above code serially?
From a cursory look a cancelled request context should reach the cache machinery before ctx.Done() is ever checked.

I'm fine with merging without; just calling out potential alternatives to sleeping

Much like all of the other blocking query tests, the trick is figuring out how to initiate a blocking query that does not return, know it has reached the important part, and only THEN issue your followup query. We don't have any signaling mechanism to know exactly when something is actually blocking.

hc-github-team-consul-core · 2022-04-20T17:23:03Z

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

hc-github-team-consul-core · 2022-04-20T17:23:06Z

🍒✅ Cherry pick of commit f3ce353 onto release/1.12.x succeeded!

…king leaf cert query to block (#12820) Fixes #12048 Fixes #12319 Regression introduced in #11693 Local reproduction steps: 1. `consul agent -dev` 2. `curl -sLiv 'localhost:8500/v1/agent/connect/ca/leaf/web'` 3. make note of the `X-Consul-Index` header returned 4. `curl -sLi 'localhost:8500/v1/agent/connect/ca/leaf/web?index=<VALUE_FROM_STEP_3>'` 5. Kill the above curl when it hangs with Ctrl-C 6. Repeat (2) and it should not hang.

hc-github-team-consul-core · 2022-04-20T17:23:09Z

🍒✅ Cherry pick of commit f3ce353 onto release/1.11.x succeeded!

hc-github-team-consul-core · 2022-04-20T17:23:13Z

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

…king leaf cert query to block (#12820) Fixes #12048 Fixes #12319 Regression introduced in #11693 Local reproduction steps: 1. `consul agent -dev` 2. `curl -sLiv 'localhost:8500/v1/agent/connect/ca/leaf/web'` 3. make note of the `X-Consul-Index` header returned 4. `curl -sLi 'localhost:8500/v1/agent/connect/ca/leaf/web?index=<VALUE_FROM_STEP_3>'` 5. Kill the above curl when it hangs with Ctrl-C 6. Repeat (2) and it should not hang.

rboyer added backport/1.10 labels Apr 19, 2022

rboyer requested a review from a team April 19, 2022 15:01

rboyer self-assigned this Apr 19, 2022

github-actions bot added the theme/agent-cache Agent Cache label Apr 19, 2022

rboyer mentioned this pull request Apr 19, 2022

Test fixing cache revalidation behavior #12554

Closed

kisunji reviewed Apr 19, 2022

View reviewed changes

agent/cache/cache.go Outdated Show resolved Hide resolved

boxofrad approved these changes Apr 19, 2022

View reviewed changes

agent/agent_endpoint_test.go Outdated Show resolved Hide resolved

agent/agent_endpoint_test.go Show resolved Hide resolved

acpana reviewed Apr 19, 2022

View reviewed changes

agent/cache/cache.go Outdated Show resolved Hide resolved

acpana reviewed Apr 19, 2022

View reviewed changes

agent/cache/cache.go Outdated Show resolved Hide resolved

ca: fix a bug that caused a non blocking leaf cert query after a bloc…

19618c1

…king leaf cert query to block Fixes #12048 Fixes #12319 Regression introduced in #11693

rboyer force-pushed the fix-leaf-cert-blocking-queries branch from 0d1e0cc to 19618c1 Compare April 19, 2022 21:40

rboyer changed the title ~~ensure that a leaf cert non-blocking query after a blocking query does not block~~ ca: fix a bug that caused a non blocking leaf cert query after a blocking leaf cert query to block Apr 19, 2022

rboyer added 2 commits April 19, 2022 16:45

use httptest

804f426

add comment

b15007b

rboyer added the pr/no-metrics-test label Apr 19, 2022

rboyer requested review from boxofrad, kisunji and acpana April 19, 2022 21:54

kisunji approved these changes Apr 19, 2022

View reviewed changes

acpana reviewed Apr 19, 2022

View reviewed changes

kisunji reviewed Apr 20, 2022

View reviewed changes

boxofrad approved these changes Apr 20, 2022

View reviewed changes

rboyer merged commit f3ce353 into main Apr 20, 2022

rboyer deleted the fix-leaf-cert-blocking-queries branch April 20, 2022 17:21

iSchluff mentioned this pull request Jun 1, 2022

Watch of type connect_leaf blocks until timeout on service restart #13302

Closed

mikemorris mentioned this pull request Oct 17, 2022

initial scaffolding for consul-server-connection-manager integration hashicorp/consul-api-gateway#407

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ca: fix a bug that caused a non blocking leaf cert query after a blocking leaf cert query to block #12820

ca: fix a bug that caused a non blocking leaf cert query after a blocking leaf cert query to block #12820

rboyer commented Apr 19, 2022 •

edited

Loading

boxofrad left a comment

acpana Apr 19, 2022 •

edited

Loading

acpana Apr 19, 2022

acpana commented Apr 19, 2022

kisunji Apr 20, 2022 •

edited

Loading

rboyer Apr 20, 2022

hc-github-team-consul-core commented Apr 20, 2022

hc-github-team-consul-core commented Apr 20, 2022

hc-github-team-consul-core commented Apr 20, 2022

hc-github-team-consul-core commented Apr 20, 2022

ca: fix a bug that caused a non blocking leaf cert query after a blocking leaf cert query to block #12820

ca: fix a bug that caused a non blocking leaf cert query after a blocking leaf cert query to block #12820

Conversation

rboyer commented Apr 19, 2022 • edited Loading

boxofrad left a comment

Choose a reason for hiding this comment

acpana Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

acpana Apr 19, 2022

Choose a reason for hiding this comment

acpana commented Apr 19, 2022

kisunji Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

rboyer Apr 20, 2022

Choose a reason for hiding this comment

hc-github-team-consul-core commented Apr 20, 2022

hc-github-team-consul-core commented Apr 20, 2022

hc-github-team-consul-core commented Apr 20, 2022

hc-github-team-consul-core commented Apr 20, 2022

rboyer commented Apr 19, 2022 •

edited

Loading

acpana Apr 19, 2022 •

edited

Loading

kisunji Apr 20, 2022 •

edited

Loading