Skip to content

Commit

Permalink
HPCC-32847 Code review comment changes
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@asselitx
asselitx committed Jan 15, 2025
1 parent 195b5d8 commit 915e318
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 38 deletions.
73 changes: 47 additions & 26 deletions esp/services/ws_workunits/ws_workunitsHelpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,11 @@

namespace ws_workunits {

// Same feature access as in ws_logaccess.ecm
#define WS_LOGACCESS "WsLogAccess"
#ifdef _CONTAINERIZED
static const char* LOG_ACCESS_FEATURE = "WsLogAccess";
#else
static const char* LOG_ACCESS_FEATURE = "ClusterTopologyAccess";
#endif

const char * const timerFilterText = "measure[time],source[global],depth[1,]"; // Does not include hthor subgraph timings
const char* zipFolder = "tempzipfiles" PATHSEPSTR;
Expand Down Expand Up @@ -3994,13 +3997,12 @@ void CWsWuFileHelper::cleanFolder(IFile* folder, bool removeFolder)
}

#ifndef _CONTAINERIZED
void CWsWuFileHelper::createProcessLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* process, const char* path)
void CWsWuFileHelper::createProcessLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* process, const char* path, bool hasLogsAccess)
{
BoolHash uniqueProcesses;
Owned<IPropertyTreeIterator> procs = cwu->getProcesses(process, NULL);
ForEach (*procs)
{
StringBuffer logSpec;
IPropertyTree& proc = procs->query();
const char* processName = proc.queryName();
if (isEmpty(processName))
Expand All @@ -4024,26 +4026,32 @@ void CWsWuFileHelper::createProcessLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo,
pid.appendf("%d", proc.getPropInt("@pid"));

fileName.append("_eclagent.log");
if (!hasLogsAccess)
throw makeStringExceptionV(ECLWATCH_ACCESS_TO_FILE_DENIED, "Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE);

winfo.getWorkunitEclAgentLog(processName, nullptr, pid.str(), mb, fileName.str());
}
else if (strieq(process, "Thor"))
{
fileName.append("_thormaster.log");
if (!hasLogsAccess)
throw makeStringExceptionV(ECLWATCH_ACCESS_TO_FILE_DENIED, "Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE);

winfo.getWorkunitThorMasterLog(processName, nullptr, mb, fileName.str());
}
}
catch(IException* e)
{
StringBuffer s;
e->errorMessage(s);
IERRLOG("Error accessing Process Log file %s: %s", logSpec.str(), s.str());
IERRLOG("Error accessing Process Log file %s: %s", processName, s.str());
writeToFile(fileName.str(), s.length(), s.str());
e->Release();
}
}
}

void CWsWuFileHelper::createThorSlaveLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* path)
void CWsWuFileHelper::createThorSlaveLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* path, bool hasLogsAccess)
{
if (cwu->getWuidVersion() == 0)
return;
Expand All @@ -4058,6 +4066,18 @@ void CWsWuFileHelper::createThorSlaveLogfile(IConstWorkUnit* cwu, WsWuInfo& winf
return;
}

// Since there is not already a handler inserting exceptions into the thor slave log,
// make a simple case here to insert a message about permission failure and write it to
// a file that could be recognized as a thor slave log.
if (!hasLogsAccess)
{
StringBuffer msg;
msg.appendf("Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE);
VStringBuffer fileName("%s%cthorslave.log", path, PATHSEPCHAR);
writeToFile(fileName.str(), msg.length(), msg.str());
return;
}

Owned<IThreadFactory> threadFactory = new CGetThorSlaveLogToFileThreadFactory();
Owned<IThreadPool> threadPool = createThreadPool("WsWuFileHelper GetThorSlaveLogToFile Thread Pool",
threadFactory, false, nullptr, thorSlaveLogThreadPoolSize, INFINITE);
Expand Down Expand Up @@ -4152,11 +4172,6 @@ void CWsWuFileHelper::createZAPInfoFile(CWsWuZAPInfoReq &request, IConstWorkUnit
writeZAPWUInfoToIOStream(outFile, "Problem: ", request.problemDesc);
writeZAPWUInfoToIOStream(outFile, "What Changed: ", request.whatChanged);
writeZAPWUInfoToIOStream(outFile, "Timing: ", request.whereSlow);
if (request.logsExcludedDueToNoAccess())
{
writeZAPWUInfoToIOStream(outFile, "Logs: ", "Excluded due to no access, need WsLogAccess:Read");
LOG(MCuserWarning, "ZAP report for workunit %s excludes logs due to access restrictions. Need WsLogAccess:READ", request.wuid.str());
}
}

void CWsWuFileHelper::writeZAPWUInfoToIOStream(IFileIOStream* outFile, const char* name, SCMStringBuffer& value)
Expand Down Expand Up @@ -4288,6 +4303,8 @@ void CWsWuFileHelper::readWULogToFile(const char *logFileName, WsWuInfo &winfo,
{
try
{
if (!zapLogFilterOptions.hasLogsAccess)
throw makeStringExceptionV(ECLWATCH_ACCESS_TO_FILE_DENIED, "Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE);
winfo.readWorkunitComponentLogs(logFileName, zapLogFilterOptions);
}
catch(IException* e)
Expand Down Expand Up @@ -4524,7 +4541,7 @@ int CWsWuFileHelper::zipAFolder(const char* folder, bool gzip, const char* zipFi
void CWsWuFileHelper::createWUZAPFile(IEspContext& context, IConstWorkUnit* cwu, CWsWuZAPInfoReq& request,
const char* tempDirName, StringBuffer& zapFileName, StringBuffer& zipFileNameWithFullPath, unsigned _thorSlaveLogThreadPoolSize)
{
request.hasLogsAccess = context.validateFeatureAccess(WS_LOGACCESS, SecAccess_Read, false);
request.hasLogsAccess = context.validateFeatureAccess(LOG_ACCESS_FEATURE, SecAccess_Read, false);
setZAPReportName(request.zapFileName, cwu->queryWuid(), zapFileName);

zipFileNameWithFullPath.set(tempDirName).append(PATHSEPCHAR).append("zapreport.zip");
Expand All @@ -4540,17 +4557,14 @@ void CWsWuFileHelper::createWUZAPFile(IEspContext& context, IConstWorkUnit* cwu,

StringArray localFiles;
createZAPWUQueryAssociatedFiles(cwu, tempDirName, localFiles, request.hasLogsAccess);
if (request.hasLogsAccess)
{
#ifndef _CONTAINERIZED
createProcessLogfile(cwu, winfo, "EclAgent", tempDirName);
createProcessLogfile(cwu, winfo, "Thor", tempDirName);
createProcessLogfile(cwu, winfo, "EclAgent", tempDirName, request.hasLogsAccess);
createProcessLogfile(cwu, winfo, "Thor", tempDirName, request.hasLogsAccess);
if (request.includeThorSlaveLog.isEmpty() || strieq(request.includeThorSlaveLog.str(), "on"))
createThorSlaveLogfile(cwu, winfo, tempDirName);
createThorSlaveLogfile(cwu, winfo, tempDirName, request.hasLogsAccess);
#else
readWULogToFiles(cwu, winfo, tempDirName, request);
#endif
}

//Write out to ZIP file
zipAllZAPFiles(tempDirName, localFiles, request.password, zipFileNameWithFullPath);
Expand All @@ -4573,12 +4587,24 @@ void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const
cur.getName(name);
cur.getIp(ip);

if (!hasLogsAccess && endsWith(name.str(), ".eclcc.log"))
continue;

RemoteFilename rfn;
SocketEndpoint ep(ip.str());
rfn.setPath(ep, name.str());
StringBuffer fileName(name.str());
getFileNameOnly(fileName, false);

VStringBuffer outFileName("%s%c%s", tempDirName, PATHSEPCHAR, fileName.str());

// Since users are only accustomed to seeing error messages replacing log file contents,
// special case an access denied error message for the one log file processed here.
if (!hasLogsAccess && endsWith(name.str(), ".eclcc.log"))
{
StringBuffer msg;
msg.appendf("Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE);
writeToFile(outFileName.str(), msg.length(), msg.str());
continue;
}

if (rfn.isLocal())
{
localFiles.append(name.str());
Expand All @@ -4592,11 +4618,6 @@ void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const
continue;
}

StringBuffer fileName(name.str());
getFileNameOnly(fileName, false);

VStringBuffer outFileName("%s%c%s", tempDirName, PATHSEPCHAR, fileName.str());

OwnedIFile outFile = createIFile(outFileName);
if (!outFile)
{
Expand Down
14 changes: 2 additions & 12 deletions esp/services/ws_workunits/ws_workunitsHelpers.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,16 +383,6 @@ struct CWsWuZAPInfoReq
logFilter.populateLogFilter(wuid.str(), httpRequest);
}

// True when logs _would have been included_ if the user had access
bool logsExcludedDueToNoAccess()
{
#ifdef _CONTAINERIZED
return (includeRelatedLogs || includePerComponentLogs) && !hasLogsAccess;
#else
// Bare metal historically always includes Thor and EclAgent logs
return !hasLogsAccess;
#endif
}
};

class WsWuInfo
Expand Down Expand Up @@ -908,8 +898,8 @@ class CWsWuFileHelper
void createZAPWUQueryAssociatedFiles(IConstWorkUnit *cwu, const char *pathToCreate, StringArray &localFiles, bool hasLogsAccess);
void createZAPWUGraphProgressFile(const char *wuid, const char *pathNameStr);
#ifndef _CONTAINERIZED
void createProcessLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *process, const char *path);
void createThorSlaveLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *path);
void createProcessLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *process, const char *path, bool hasLogsAccess);
void createThorSlaveLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *path, bool hasLogsAccess);
#endif
LogAccessLogFormat getComponentLogFormatFromLogName(const char *log);
void readWULogToFiles(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *path, CWsWuZAPInfoReq &zapLogFilterOptions);
Expand Down

0 comments on commit 915e318

Please sign in to comment.