-
Notifications
You must be signed in to change notification settings - Fork 513
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
Support ttl=N from helpers #1662
base: master
Are you sure you want to change the base?
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.
I support adding this feature.
@@ -232,9 +232,13 @@ Helper::Reply::parseResponseKeys() | |||
// TODO: Convert the above code to use Tokenizer and SBuf | |||
const SBuf parsedKey(key); | |||
const SBuf parsedValue(v); // allow empty values (!v or !*v) | |||
CheckReceivedKey(parsedKey, parsedValue); | |||
notes.add(parsedKey, parsedValue); |
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.
I see no good reason to make this old code conditional on the annotation not being called "ttl". The official code already handles existing Squid-specific helper annotations like "group" and "tag". Why make the new ttl
special in that regard?
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.
This key pertains to the re-use of the exact helper response that it is part of. Other annotations are relevant in higher levels of abstraction and make sense preserving across the potentially multiple helpers a transaction can use.
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.
This key pertains to the re-use of the exact helper response that it is part of. Other annotations are relevant in higher levels of abstraction and make sense preserving across the potentially multiple helpers a transaction can use.
A PR may propose a new from-helper annotation categorization scheme, of course, but this should be done explicitly (e.g., by adding a clear definition of categories using C++ comments), and the categorization should be applied to existing/old supported annotations. Just adding do not do this for annotations named foo
code is not good enough because it is not clear whether the new code should apply to some old annotations and whether some future bar
annotation would need to be added to that code or excluded from it. Addressing this change request requires PR code modifications.
This change request is about (and is attached to) two independent features, represented by two old code lines:
-
The alleged "ttl vs. other annotations" differences are not relevant to CheckReceivedKey() at all.
-
As for (not) adding the new annotation to
notes
, some of the existing/old annotations have the same "single helper response" scope AFAICT; once you define the new category, please double check whether any of the existing annotations belong to it. Also, if this PR starts excluding some annotation categories fromnotes
, then allnotes
uses must be checked to make sure the exclusion is valid/desirable. I would not be surprised if that audit results in preservation of the old code updatingnotes
for all annotation categories and/or code to preserve/replicate the oldnotes
functionality for the new category of annotations.
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.
I believe our weekly meeting discussions and PR modifications have sufficiently resolved this request item.
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.
I did not see any relevant recent code changes; please point me to them if I missed them. Those discussions happened more than three months ago, right? Could you please summarize what you think we agreed on back then? FWIW, the issues flagged by this change request still appear real/important to me.
47d75af
to
4365dcd
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.
This is as far as I could get without an expires
data member description.
I have adjusted the following phrase in PR description because we still support credentialsttl configuration, even for Basic authentication, AFAICT: - replaces the auth_param credentialsttl configuration.
+ overwrites the auth_param credentialsttl configuration. |
Co-authored-by: Alex Rousskov <[email protected]>
Co-authored-by: Alex Rousskov <[email protected]>
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.
Documenting the new expires
member helped more this PR review further, thank you! We are not out of the woods yet, but I hope this review iteration helps resolve the primary conflicts between (some) PR code and that documentation.
I also marked a couple of other problems to facilitate PR progress.
/* current time for timeouts */ | ||
lb->expiretime = current_time.tv_sec; | ||
lb->expires = current_time.tv_sec; |
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.
The (unchanged) C++ comment may have been correct before PR changes, but the (changed) assignment appears to violate new expires
semantics (which is not current/caching time but future/expiration time). All newly cached entries cannot expire "immediately" or "now" (which is what this code code says if we go by the now-documented latest or maximum expiration time semantics of expires
).
The same concern applies to some other PR code snippets, but it is probably best to focus on one or two at a time (for now).
* | ||
* Scheme specific criteria may cause credentials to become invalid | ||
* before this timestamp. Use `ttl()` to access the scheme-specific | ||
* checks of whether the credentials are actually valid. |
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.
The ttl() method is documented in terms of entry expiration/freshness rather than validity. Let's use that terminology for consistency sake:
* checks of whether the credentials are actually valid. | |
* checks of whether the credentials have expired. |
or
* checks of whether the credentials are actually valid. | |
* checks of whether the credentials are still fresh. |
or
* checks of whether the credentials are actually valid. | |
* checks of whether the credentials are stale. |
* Scheme specific criteria may cause credentials to become invalid | ||
* before this timestamp. Use `ttl()` to access the scheme-specific | ||
* checks of whether the credentials are actually valid. |
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.
This design feels very dangerous to me: It is so easy for a developer to forget to check ttl() and end up with expires
-checking code that uses a stale entry. AFAICT, some PR code does use expires
without checking ttl() already. A reviewer can easily miss such mistakes as well.
Can we improve the situation by making ttl() to be always in sync with the value stored in this new public data member? If some scheme-specific logic requires earlier expiration, can that scheme adjust the expires member accordingly? If this is not doable, then perhaps this data member should have (a different name and) safe "absolute cache addition or revalidation time" semantics rather than dangerous "maximum absolute expiration time" semantics that requires ttl() checks to find the actual expiration time?
int32_t basic_ttl = expiretime - squid_curtime + static_cast<Auth::Basic::Config*>(config)->credentialsTTL; | ||
int32_t global_ttl = static_cast<int32_t>(expiretime - squid_curtime + Auth::TheConfig.credentialsTtl); | ||
int32_t basic_ttl = expires - current_time.tv_sec; | ||
const auto global_ttl = static_cast<int32_t>(basic_ttl + Auth::TheConfig.credentialsTtl); |
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.
This code does not seem to match the new expires
data member documentation: This code extends or prolong freshness beyond expires
(by credentialsTtl seconds) while documentation says expires
is the maximum possible time (i.e., "the latest timestamp") (and ttl() may only shorten freshness time, making the entry expire earlier than expires
).
|
||
return min(basic_ttl, global_ttl); | ||
} | ||
|
||
bool | ||
Auth::Basic::User::authenticated() const | ||
{ | ||
if ((credentials() == Auth::Ok) && (expiretime + static_cast<Auth::Basic::Config*>(config)->credentialsTTL > squid_curtime)) | ||
if (credentials() == Auth::Ok && expires > current_time.tv_sec) |
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.
expires
documentation says we should check ttl() instead of relying on expires
maximum, but this code does not use ttl(). Why is this code not written using ttl()? For example,
if (credentials() == Auth::Ok && expires > current_time.tv_sec) | |
if (credentials() == Auth::Ok && ttl() > 0) |
or
if (credentials() == Auth::Ok && expires > current_time.tv_sec) | |
if (credentials() == Auth::Ok && ttl() >= 0) |
If this code is correct, please add a C++ comment explaining why we should not use ttl() here despite the documentation implying that we not just may use ttl(), but must use ttl() to determine correct expiration time.
The same concern applies to some other PR code snippets, but it is probably best to focus on one or two at a time (for now).
@@ -232,9 +232,13 @@ Helper::Reply::parseResponseKeys() | |||
// TODO: Convert the above code to use Tokenizer and SBuf | |||
const SBuf parsedKey(key); | |||
const SBuf parsedValue(v); // allow empty values (!v or !*v) | |||
CheckReceivedKey(parsedKey, parsedValue); | |||
notes.add(parsedKey, parsedValue); |
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.
I did not see any relevant recent code changes; please point me to them if I missed them. Those discussions happened more than three months ago, right? Could you please summarize what you think we agreed on back then? FWIW, the issues flagged by this change request still appear real/important to me.
|
||
if (v && parsedKey.cmp("ttl") == 0) { | ||
if (parsedValue.findFirstNotOf(CharacterSet::DIGIT) != SBuf::npos) { | ||
debugs(84, DBG_IMPORTANT, "WARNING: Unexpected from-helper value: ttl=" << parsedValue); |
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.
Should not we reject empty values as well? Please keep in mind the v
part in the outer if
statement if you refactor to reject all invalid ttl=
values, and not just values that contain non-digits.
continue; | ||
} | ||
char *end = v + parsedValue.length(); | ||
expires = current_time.tv_sec + strtoll(v, &end, 10); |
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.
This code implies that strtoll() stops at end
. AFAICT, it does not. The second strtoll() parameter is an output parameter only.
It should be (initialized to nil, supplied to strtoll(), and then) used to detect invalid values.
@@ -81,6 +81,8 @@ redirectHandleReply(void *data, const Helper::Reply &reply) | |||
{ | |||
RedirectStateData *r = static_cast<RedirectStateData *>(data); | |||
debugs(61, 5, "reply=" << reply); | |||
if (reply.expires.has_value()) | |||
debugs(61, 5, "ignoring unexpected ttl=" << (reply.expires.value() - current_time.tv_sec)); |
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.
If possible, please enhance/configure the right helper
class objects to honor TTLs (and warn when they are supplied but not honored) instead of hoping that we will remember to copy code snippets like this to every new helper response handler that does not support TTLs.
debugs(61, 5,"StoreId helper: reply=" << reply); | ||
debugs(61, 5, "reply=" << reply); | ||
if (reply.expires.has_value()) | ||
debugs(61, 5, "ignoring unexpected ttl=" << (reply.expires.value() - current_time.tv_sec)); |
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.
Should we warn the admin (at debug level 1, possibly with a FadingCounter) that their helper is sending TTLs that Squid is ignoring?
Currently only used for the Basic authentication helpers where it
overwrites the auth_param credentialsttl configuration.