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

Support ttl=N from helpers #1662

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Feb 6, 2024

Currently only used for the Basic authentication helpers where it
overwrites the auth_param credentialsttl configuration.

Copy link
Contributor

@rousskov rousskov left a 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.

src/helper/Reply.h Outdated Show resolved Hide resolved
src/helper/Reply.cc Outdated Show resolved Hide resolved
src/cf.data.pre Show resolved Hide resolved
src/cf.data.pre Show resolved Hide resolved
src/auth/basic/UserRequest.cc Outdated Show resolved Hide resolved
@@ -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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 from notes, then all notes 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 updating notes for all annotation categories and/or code to preserve/replicate the old notes functionality for the new category of annotations.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

src/helper/Reply.h Outdated Show resolved Hide resolved
src/helper/Reply.cc Outdated Show resolved Hide resolved
src/helper/Reply.cc Outdated Show resolved Hide resolved
src/auth/basic/UserRequest.cc Show resolved Hide resolved
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Feb 6, 2024
@yadij yadij marked this pull request as draft February 7, 2024 06:35
@yadij yadij marked this pull request as ready for review February 14, 2024 19:54
@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Feb 14, 2024
@yadij yadij requested a review from rousskov February 14, 2024 19:54
@yadij yadij added the feature maintainer needs documentation updates for merge label Feb 14, 2024
Copy link
Contributor

@rousskov rousskov left a 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.

doc/release-notes/release-7.sgml.in Outdated Show resolved Hide resolved
src/auth/User.h Show resolved Hide resolved
src/auth/User.cc Show resolved Hide resolved
src/auth/basic/UserRequest.cc Show resolved Hide resolved
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Feb 19, 2024
@rousskov
Copy link
Contributor

rousskov commented Feb 19, 2024

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.

@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jun 4, 2024
@yadij yadij requested a review from rousskov June 4, 2024 18:41
Copy link
Contributor

@rousskov rousskov left a 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.

Comment on lines 275 to +276
/* current time for timeouts */
lb->expiretime = current_time.tv_sec;
lb->expires = current_time.tv_sec;
Copy link
Contributor

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.
Copy link
Contributor

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:

Suggested change
* checks of whether the credentials are actually valid.
* checks of whether the credentials have expired.

or

Suggested change
* checks of whether the credentials are actually valid.
* checks of whether the credentials are still fresh.

or

Suggested change
* checks of whether the credentials are actually valid.
* checks of whether the credentials are stale.

Comment on lines +57 to +59
* 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.
Copy link
Contributor

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);
Copy link
Contributor

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)
Copy link
Contributor

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,

Suggested change
if (credentials() == Auth::Ok && expires > current_time.tv_sec)
if (credentials() == Auth::Ok && ttl() > 0)

or

Suggested change
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);
Copy link
Contributor

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);
Copy link
Contributor

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);
Copy link
Contributor

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));
Copy link
Contributor

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));
Copy link
Contributor

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?

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jun 4, 2024
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature maintainer needs documentation updates for merge S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants