Do not collapse ssl_crtd requests across reconfigurations 2 #228

eduard-bagdasaryan · 2023-09-14T12:25:48Z

Squid collapses ssl_crtd requests on a pending request with an identical
helper query. This collapsing “works” across Squid reconfigurations even
though the old helper responsible for the first request is replaced with
a new one during reconfiguration; that old helper must complete
servicing pending requests.

However, since the helper program itself could have been changed (just
prior to reconfiguration), it is conceptually wrong for
post-reconfiguration requests to reuse the old helper program response:
Squid must conservatively assume that the response may have changed
because the helper protocol does not allow Squid to validate the
freshness of the helper response (to a collapsed request). Such blind
reuse also creates runtime problems if a buggy helper never responds to
a request X, stalling all the new requests getting collapsed on X (until
a Squid restart).

To avoid this post-reconfiguration collapsing, we now switch to a new
GeneratorRequests object during reconfiguration, instead of keeping
all collapsed requests in a global GeneratorRequests object.

In this new approach each Ssl::Helper object has its own GeneratorRequests map (used for collapsing). Since a reconfiguration creates a new Sll::Helper, the collapsed request are not shared across reconfigurations.

rousskov · 2023-09-14T16:34:32Z

src/helper.cc

@@ -132,7 +132,7 @@ HelperServerBase::dropQueued()
        void *cbdata;
        if (cbdataReferenceValidDone(r->request.data, &cbdata)) {
            r->reply.result = Helper::Unknown;
-            r->request.callback(cbdata, r->reply);
+            parentHelper()->callBack(r->request.callback, cbdata, r->reply);


Instead of adding a virtual parentHelper() -- a red flag for several reasons -- I would rather add a Helper::Client reference as a dropQueued() parameter.

Done in PR229.

rousskov · 2023-09-14T16:35:56Z

src/helper.cc

-        result = Helper::Unknown;
+        callback(data, Helper::Reply(Helper::Unknown));
+    } else { // pretend the helper has responded with ERR
+        hlp->callBack(callback, data, Helper::Reply(Helper::Error));


Please do not send answers to never-submitted queries through the helper. Keep the old direct callback-calling code virtually unchanged here.

Left this code as-is in PR229..

rousskov · 2023-09-14T16:36:57Z

src/helper.cc

@@ -576,6 +574,12 @@ helper::submit(const char *buf, HLPCB * callback, void *data)
    debugs(84, DBG_DATA, Raw("buf", buf, strlen(buf)));
 }

+void
+helper::callBack(HLPCB *callback, void *data, const Helper::Reply &reply)


Most likely, this method should take just Xaction or Request+Reply parameters, not raw data+Reply.

Without SubmissionFailure() changes, we do not need these parameters of course. In PR229, callBack() takes a single Xaction * argument.

rousskov

I found one problem that, if confirmed, will require refactoring and research. The rest is minor style polishing.

rousskov · 2023-09-28T13:27:31Z

src/ssl/helper.h

 /**
 * Set of thread for ssl_crtd. This class is singleton.
 * This class use helper structure for threads management.
 */
-class Helper
+class Helper : public ::Helper::Client


Please use this formatting variation for new and modified class declarations:

Suggested change

class Helper : public ::Helper::Client

class Helper: public ::Helper::Client

src/ssl/helper.h

rousskov · 2023-09-28T13:30:08Z

src/ssl/helper.h

+    /// query:GeneratorRequest map
+    using GeneratorRequests = std::unordered_map<SBuf, GeneratorRequest*>;
+
+    explicit Helper(const char *name): Helper::Client(name) {}


Suggested change

explicit Helper(const char *name): Helper::Client(name) {}

explicit Helper(const char * const name): Helper::Client(name) {}

rousskov · 2023-09-28T13:31:33Z

src/ssl/helper.h

+    static Pointer Make(const char *name) { return new Helper(name); }
+
+    /// pending Helper requests (to all certificate generator helpers combined)
+    GeneratorRequests generatorRequests;


If possible, please make this and related type alias declaration private.

src/ssl/helper.h

src/ssl/helper.cc

and restored the (lost) removal of processed queries from generatorRequests map.

eduard-bagdasaryan

Done (up to 98d2d9a).

eduard-bagdasaryan · 2023-10-04T18:40:18Z

src/ssl/helper.h

 /**
 * Set of thread for ssl_crtd. This class is singleton.
 * This class use helper structure for threads management.
 */
-class Helper
+class Helper : public ::Helper::Client


src/ssl/helper.h

eduard-bagdasaryan · 2023-10-04T18:58:10Z

src/ssl/helper.h

+    /// query:GeneratorRequest map
+    using GeneratorRequests = std::unordered_map<SBuf, GeneratorRequest*>;
+
+    explicit Helper(const char *name): Helper::Client(name) {}


src/ssl/helper.h

eduard-bagdasaryan · 2023-10-04T19:03:33Z

src/ssl/helper.h

+    static Pointer Make(const char *name) { return new Helper(name); }
+
+    /// pending Helper requests (to all certificate generator helpers combined)
+    GeneratorRequests generatorRequests;


src/ssl/helper.cc

Now it is unlocked before it gets deleted in Ssl::HandleGeneratorReply(). With the previous approach, it was unlocked in Helper::Request destructor after Ssl::HandleGeneratorReply() - the memory would not leak in this case. The updated callBack() looks more in line with the base Helper::Client::callBack().

src/ssl/helper.cc

We must remove GeneratorRequest object (and clear its generatorRequests map entry) if Helper::Client::trySubmit() returns an error or throws.

Temporalily added compiler '-H' option to figure out why sbuf/Algorithms.h is missed on the buildserver.

eduard-bagdasaryan added 2 commits September 14, 2023 15:19

Do not collapse ssl_crtd requests across reconfigurations 2

6cb0ef8

In this new approach each Ssl::Helper object has its own GeneratorRequests map (used for collapsing). Since a reconfiguration creates a new Sll::Helper, the collapsed request are not shared across reconfigurations.

Added a missing helper stub

9f97993

rousskov requested changes Sep 14, 2023

View reviewed changes

eduard-bagdasaryan added 3 commits September 27, 2023 22:18

Merge from master and resolved conflicts

c722bae

Undone changes that were separated into dedicated PRs

f989919

Undone an out-of-scope change

b83bff3

rousskov requested changes Sep 28, 2023

View reviewed changes

eduard-bagdasaryan added 4 commits October 4, 2023 18:56

Merged from master

ac18101

Introduced Ssl::Helper::callBack()

05cbcce

and restored the (lost) removal of processed queries from generatorRequests map.

Polished

61265c9

Added an XXX regarding ssl_crtd turning nil during transaction

98d2d9a

eduard-bagdasaryan commented Oct 4, 2023

View reviewed changes

eduard-bagdasaryan and others added 3 commits October 4, 2023 22:52

Fixed unit tests

0226dac

Require that Ssl::Helper::callBack() got a valid GeneratorRequest

d746639

eduard-bagdasaryan commented Oct 12, 2023

View reviewed changes

src/ssl/helper.cc Show resolved Hide resolved

eduard-bagdasaryan and others added 5 commits October 17, 2023 16:21

Fixed leaks in Ssl::Helper::Submit()

ad816b3

We must remove GeneratorRequest object (and clear its generatorRequests map entry) if Helper::Client::trySubmit() returns an error or throws.

Missing header for a hash functor to support std::unordered_map<SBuf,*>

6ab8708

Build with showing all includes

88aa8c0

Temporalily added compiler '-H' option to figure out why sbuf/Algorithms.h is missed on the buildserver.

Merged from master

f4a11eb

Revert "Build with showing all includes"

ae13a0e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not collapse ssl_crtd requests across reconfigurations 2 #228

Do not collapse ssl_crtd requests across reconfigurations 2 #228

eduard-bagdasaryan commented Sep 14, 2023 •

edited

Loading

rousskov Sep 14, 2023

eduard-bagdasaryan Sep 18, 2023

rousskov Sep 14, 2023

eduard-bagdasaryan Sep 18, 2023

rousskov Sep 14, 2023 •

edited

Loading

eduard-bagdasaryan Sep 18, 2023

rousskov left a comment

rousskov Sep 28, 2023

eduard-bagdasaryan Oct 4, 2023

rousskov Sep 28, 2023

eduard-bagdasaryan Oct 4, 2023

rousskov Sep 28, 2023

eduard-bagdasaryan Oct 4, 2023

eduard-bagdasaryan left a comment

eduard-bagdasaryan Oct 4, 2023

eduard-bagdasaryan Oct 4, 2023

eduard-bagdasaryan Oct 4, 2023

	class Helper : public ::Helper::Client
	class Helper: public ::Helper::Client

	explicit Helper(const char *name): Helper::Client(name) {}
	explicit Helper(const char * const name): Helper::Client(name) {}

Do not collapse ssl_crtd requests across reconfigurations 2 #228

Are you sure you want to change the base?

Do not collapse ssl_crtd requests across reconfigurations 2 #228

Conversation

eduard-bagdasaryan commented Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan commented Sep 14, 2023 •

edited

Loading

rousskov Sep 14, 2023 •

edited

Loading