Populate XrdSciTokens with more detailed log messages. #1644
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This introduces configuration-file-controlled tracing of SciTokens- related messages and a number of detailed log messages. As typical, this is controlled by a new config variable: ``` scitokens.trace [level] [level] ... ``` Where level can be one of `all`, `none`, `debug`, `info`, `warning`, or `error`. Roughly, the levels imply: - Error: fatal issue in plugin configuration - Warning: Potentially-problematic configuration or token that is invalid - Info: Informational messages about the plugin. Shouldn't have any logging per-request - Debug: Verbose, per-request details about the tokens. The default is error + warning; debug is not recommended for continuous production settings but is immensely helpful in solving problems.
abh3
requested changes
Mar 16, 2022
|
(Just as a quick update - no disagreements with the PR feedback above. I was just on vacation for a week and haven't had time to cycle back to this.) |
|
Ok, I think the various requested changes are in. Please re-review! |
abh3
approved these changes
May 4, 2022
| @@ -296,7 +391,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper | |||
| pthread_rwlock_init(&m_config_lock, nullptr); | |||
There was a problem hiding this comment.
You should consider using XrdSysRWLock as this would be very helpful in debugging lock/unlock problems in the future.
| @@ -465,7 +574,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper | |||
| pthread_rwlock_unlock(&m_config_lock); | |||
| @@ -602,7 +712,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper | |||
| auto enf = enforcer_create(token_issuer.c_str(), &m_audiences_array[0], &err_msg); | |||
| pthread_rwlock_unlock(&m_config_lock); | |||
| @@ -636,15 +746,15 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper | |||
| auto iter = m_issuers.find(token_issuer); | |||
| if (iter == m_issuers.end()) { | |||
| pthread_rwlock_unlock(&m_config_lock); | |||
| @@ -636,15 +746,15 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper | |||
| auto iter = m_issuers.find(token_issuer); | |||
| if (iter == m_issuers.end()) { | |||
| pthread_rwlock_unlock(&m_config_lock); | |||
| m_log.Emsg("GenerateAcls", "Authorized issuer without a config."); | |||
| m_log.Log(LogMask::Warning, "GenerateAcls", "Authorized issuer without a config."); | |||
| scitoken_destroy(token); | |||
| return false; | |||
| } | |||
| const auto &config = iter->second; | |||
| value = nullptr; | |||
| if (scitoken_get_claim_string(token, "sub", &value, &err_msg)) { | |||
| pthread_rwlock_unlock(&m_config_lock); | |||
| @@ -942,7 +1093,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper | |||
| } | |||
|
|
|||
| if (issuers.empty()) { | |||
| m_log.Emsg("Reconfig", "No issuers configured."); | |||
| m_log.Log(LogMask::Warning, "Reconfig", "No issuers configured."); | |||
| } | |||
|
|
|||
| pthread_rwlock_wrlock(&m_config_lock); | |||
| @@ -87,6 +122,46 @@ XrdAccPrivs AddPriv(Access_Operation op, XrdAccPrivs privs) | |||
| return static_cast<XrdAccPrivs>(new_privs); | |||
| } | |||
|
|
|||
| const std::string OpToName(Access_Operation op) { | |||
There was a problem hiding this comment.
Likely this should be made a generic utility in XrdAcc as another implementation of the same thing exists there. Something to put in the todo list.
| @@ -29,6 +32,38 @@ XrdVERSIONINFO(XrdAccAuthorizeObjAdd, XrdAccSciTokens); | |||
|
|
|||
| namespace { | |||
|
|
|||
| enum LogMask { | |||
There was a problem hiding this comment.
This has gotten a life of its own and I see the same implementation in three other modules (XrdMacaroonsHandler.hh, XrdTpcTPC.hh, and XrdVomsMapfile.hh) making this the fourth sych implementation. Shouldn't this be abstracted into a utility function?
bbockelm
pushed a commit
to opensciencegrid/Software-Redhat
that referenced
this pull request
Jun 22, 2022
… more detailed log messages") git-svn-id: https://vdt.cs.wisc.edu/svn/native/redhat/branches/osg-3.6@27029 4e558342-562e-0410-864c-e07659590f8c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces configuration-file-controlled tracing of SciTokens-related messages and a number of detailed log messages.
As typical, this is controlled by a new config variable:
Where level can be one of
all,none,debug,info,warning, orerror.Roughly, the levels imply:
The default is error + warning; debug is not recommended for continuous production settings but is immensely helpful in solving problems.