HPCC-32847 Require WsLogAccess to include logs in ZAP #19389
Conversation
Require the same WsLogAccess:READ permission to include log files in a ZAP as would be required to view them. When log files are excluded from the ZAP due to lack of permission, the ZAP info file (<wuid>.txt) includes a line explaining their absence. Signed-off-by: Terrence Asselin <[email protected]>
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32847 Jirabot Action Result: |
| writeZAPWUInfoToIOStream(outFile, "Timing: ", request.whereSlow); | ||
| if (request.logsExcludedDueToNoAccess()) | ||
| { | ||
| writeZAPWUInfoToIOStream(outFile, "Logs: ", "Excluded due to no access, need WsLogAccess:Read"); |
There was a problem hiding this comment.
Very minor, but potentially very helpful would be to add "user lacks 'WsLogAccess:Read' feature access, please contact admin" or something to that affect.
| @@ -4563,6 +4573,9 @@ void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const | |||
| cur.getName(name); | |||
| cur.getIp(ip); | |||
|
|
|||
| if (!hasLogsAccess && endsWith(name.str(), ".eclcc.log")) | |||
There was a problem hiding this comment.
I'm not sure I follow what's special about *.eclcc.log files, this might need a code comment
There was a problem hiding this comment.
The eclcc log files are included as "Associated Files" for bare-metal clusters. I'm checking for them specifically here to exclude if the user doesn't have access. I considered checking for any *.log file but thought it might be best to limit the exclusion to just the log file I'd seen could be included here.
There was a problem hiding this comment.
Thanks @asselitx, as mentioned offline, eclcc.log files aren't typically creating via the logAccess feature, and the logAccess feature level flag might not apply here.
| @@ -381,6 +382,17 @@ struct CWsWuZAPInfoReq | |||
| { | |||
| logFilter.populateLogFilter(wuid.str(), httpRequest); | |||
| } | |||
|
|
|||
| // True when logs _would have been included_ if the user had access | |||
| bool logsExcludedDueToNoAccess() | |||
There was a problem hiding this comment.
minor, but for some reason the current method name misleads me into thinking the logs have already been excluded, but I think the method is determining if the logs should be excluded, does it make sense to rename to excludeLogsDueToNoAccess()?
* Vary the permission feature required to access logs to match the one used for the cluster deployment- containerized or bare metal. * The WsLogAccess permission is used when pulling all containerized log files, including eclcc, because the remote log accessor is required to pull them all. * Because users are used to seeing errors in log file stubs, instead of actual log entries, continue that behavior here. When access to log files is denied, insert a message into each (otherwise empty) log file included in the ZAP. * Some code paths don't currently replace files with error messages, so to maintain current behavior and user expectations don't insert error messages into any file other than a log file. Signed-off-by: Terrence Asselin <[email protected]>
|
I've re-worked this change based on our conversation. Please see my second commit comments for some details. |
Require the same WsLogAccess:READ permission to include log files in a ZAP as would be required to view them. When log files are excluded from the ZAP due to lack of permission, the ZAP info file (.txt) includes a line explaining their absence.
Type of change:
Checklist:
Smoketest:
Testing:
Tested manually on laptop both bare metal and containerized deployments. Checked cases with and without access both requesting and not requesting logs.