Mcol 520 #587
Mcol 520 #587
Conversation
| cmd = "sudo echo " + entry + " >> /etc/fstab"; | ||
| } | ||
|
|
||
| cmd = "echo " + entry + " >> /etc/fstab"; | ||
| system(cmd.c_str()); |
There was a problem hiding this comment.
won't work as not-root.
| @@ -2607,7 +2582,7 @@ pid_t ProcessMonitor::startProcess(string processModuleType, string processName, | |||
|
|
|||
| string logdir("/var/log/mariadb/columnstore"); | |||
|
|
|||
| if (access(logdir.c_str(), W_OK) != 0) logdir = "/tmp"; | |||
| if (access(logdir.c_str(), W_OK) != 0) logdir = tmpLogDir; | |||
|
|
|||
| string cmd = startup::StartUp::installDir() + "/bin/reset_locks > " + logdir + "/reset_locks.log1 2>&1"; | |||
There was a problem hiding this comment.
This seems dangerous; why are we doing this here?
| system(cmd.c_str()); | ||
| cmd = "mv " + errFileName + " " + saveerrFileName; | ||
| cmd = "mv " + errFileName + " " + saveerrFileName + " > /dev/null 2>&1"; | ||
| system(cmd.c_str()); | ||
| } |
There was a problem hiding this comment.
looks like we're doing the same thing whether LogFile == 'off' or not.
| @@ -3128,7 +3103,7 @@ int ProcessMonitor::updateLog(std::string action, std::string level) | |||
| //if non-root, change file permissions so we can update it | |||
| if ( !rootUser) | |||
| { | |||
| string cmd = "sudo chmod 666 " + fileName + " > /dev/null"; | |||
| string cmd = "chmod 666 " + fileName + " > /dev/null 2>&1"; | |||
There was a problem hiding this comment.
we shouldn't make the log config world-writable. Also this won't work as not-root.
| for ( int i = 0 ; i < 10 ; i++ ) | ||
| { | ||
| //run upgrade script | ||
| string cmd = startup::StartUp::installDir() + "/bin/upgrade-columnstore.sh doupgrade --password=" + |
| return oam::API_FAILURE; | ||
| } | ||
| */ | ||
|
|
There was a problem hiding this comment.
I assume we replaced this with something else, but I don't think I've come across it yet. Safe to get rid of this?
| @@ -5217,16 +5150,10 @@ int ProcessMonitor::changeMyCnf(std::string type) | |||
| // set owner and permission | |||
| string cmd = "chmod 664 " + mycnfFile + " >/dev/null 2>&1"; | |||
|
|
|||
There was a problem hiding this comment.
should this be 664 or 644? But also why do we need to change the perms?
| system(cmd.c_str()); | ||
|
|
||
| cmd = "chown mysql:mysql " + mycnfFile + " >/dev/null 2>&1"; | ||
|
|
||
| if ( !rootUser) | ||
| cmd = "sudo chown mysql:mysql " + mycnfFile + " >/dev/null 2>&1"; | ||
|
|
||
| system(cmd.c_str()); |
There was a problem hiding this comment.
won't work as not-root, but probably also doesn't have to be done....?
| @@ -5677,12 +5605,12 @@ int ProcessMonitor::runMasterDist(std::string& password, std::string& slaveModul | |||
| { | |||
| string ipAddr = (*pt1).IPAddr; | |||
|
|
|||
| string cmd = startup::StartUp::installDir() + "/bin/rsync.sh " + ipAddr + " " + password + " " + startup::StartUp::installDir() + " 1 > /tmp/master-dist_" + moduleName + ".log"; | |||
| string logFile = tmpLogDir + "/master-dist_" + moduleName + ".log"; | |||
| string cmd = startup::StartUp::installDir() + "/bin/rsync.sh " + ipAddr + " " + password + " " + startup::StartUp::installDir() + " 1 > " + logFile; | |||
There was a problem hiding this comment.
insert password complaint here.
|
|
||
| string logFile = "/tmp/master-dist_" + slaveModule + ".log"; | ||
| string cmd = startup::StartUp::installDir() + "/bin/rsync.sh " + ipAddr + " " + password + " " + startup::StartUp::installDir() + " 1 > " + logFile; |
| else | ||
| cmd = "sudo mount " + deviceName + " " + startup::StartUp::installDir() + "/mysql/db -t ext2 -o defaults > /tmp/um_mount.log"; | ||
| string mountLog = tmpLogDir + "/um_mount.log"; | ||
| cmd = "mount " + deviceName + " " + startup::StartUp::installDir() + "/mysql/db -t ext2 -o defaults > " + mountLog; |
There was a problem hiding this comment.
won't work as not-root.
| dbrootID + " " + startup::StartUp::installDir() + "/data" + dbrootID + " > /tmp/glusterAssign.txt 2>&1"; | ||
| } | ||
| string tmpLog = tmpLogDir + "/glusterAssign.log"; | ||
| command = "mount -tglusterfs -odirect-io-mode=enable " + moduleIPAddr + ":/dbroot" + dbrootID + " " + startup::StartUp::installDir() + "/data" + dbrootID + " > " + tmpLog + " 2>&1"; | ||
|
|
There was a problem hiding this comment.
lots of mounting and unmounting going on above, none if it will work as not-root.
|
|
||
| string tmpLog = tmpLogDir + "/glusterUnassign.log"; | ||
|
|
||
| command = "umount -f " + startup::StartUp::installDir() + "/data" + dbrootID + " > " + tmpLog + " 2>&1"; | ||
|
|
||
| int ret = system(command.c_str()); |
There was a problem hiding this comment.
won't work as not-root.
| { | ||
| log.writeLog(__LINE__, "glusterUnassign failed.", LOG_TYPE_ERROR); | ||
| system("mv -f /tmp/glusterUnassign.txt /tmp/glusterUnassign_failed.txt"); | ||
|
|
||
| string cmd = "mv -f " + tmpLog + " " + tmpLog + "failed"; |
There was a problem hiding this comment.
doesn't matter as much as the other stuff, but the before and after txt doesn't match. old ver is a string literal, so easy to see it. If tmpLog is 'log.txt', then the new version will construct 'mv -f log.txt log.txtfailed'
| @@ -239,7 +238,6 @@ int main(int argc, char** argv) | |||
| semDoit(BrmKeys.PROCESSSTATUS_SYSVKEY, "PROC_STAT "); | |||
| semDoit(BrmKeys.SYSTEMSTATUS_SYSVKEY, "SYS_STAT "); | |||
| semDoit(BrmKeys.SWITCHSTATUS_SYSVKEY, "SW_STAT "); | |||
| semDoit(BrmKeys.STORAGESTATUS_SYSVKEY, "STORE_STAT "); | |||
| semDoit(BrmKeys.NICSTATUS_SYSVKEY, "NIC_STAT "); | |||
There was a problem hiding this comment.
I took these out after finding no reference in the code to them.
| @@ -18,6 +18,9 @@ if [ -z "$COLUMNSTORE_INSTALL_DIR" ]; then | |||
| COLUMNSTORE_INSTALL_DIR=/usr/local/mariadb/columnstore | |||
| fi | |||
|
|
|||
| #get temp directory | |||
| tmpDir=`$COLUMNSTORE_INSTALL_DIR/bin/getConfig SystemConfig SystemTempFileDir` | |||
There was a problem hiding this comment.
Right now there seems to be a 2nd temp dir and a lock dir that should either be tested, or what's better imo is consolidating all of those.
| @@ -538,7 +540,7 @@ checkLocalSELINUX() | |||
| pass=true | |||
| #check local SELINUX | |||
| if [ -f /etc/selinux/config ]; then | |||
| `cat /etc/selinux/config | grep SELINUX | grep enforcing > /tmp/selinux_check 2>&1` | |||
| `cat /etc/selinux/config | grep SELINUX | grep enforcing > ${tmpDir}/selinux_check 2>&1` | |||
There was a problem hiding this comment.
do we require users to disable selinux?
| if [ "$?" -eq 0 ]; then | ||
| `$COLUMNSTORE_INSTALL_DIR/bin/remote_scp_get.sh $ipadd Calpont1 test.log >> /tmp/remote_scp_get 2>&1` | ||
| `$COLUMNSTORE_INSTALL_DIR/bin/remote_scp_get.sh $ipadd Calpont1 test.log >> ${tmpDir}/remote_scp_get 2>&1` |
| if [ "$?" -eq 0 ]; then | ||
| echo "${bold}Warning${normal}, $ipadd Node $firewall service is Active, check port test results" | ||
| pass=false | ||
| else | ||
| `$COLUMNSTORE_INSTALL_DIR/bin/remote_command.sh $ipadd $PASSWORD "systemctl status '$firewall' > /tmp/firewall_check 2>&1" 1 > /tmp/remote_command_check` | ||
| `$COLUMNSTORE_INSTALL_DIR/bin/remote_command.sh $ipadd $PASSWORD "systemctl status '$firewall' > ${tmpDir}/firewall_check 2>&1" 1 > ${tmpDir}/remote_command_check` | ||
| if [ "$?" -eq 0 ]; then | ||
| echo "${bold}Warning${normal}, $ipadd Node $firewall service is Active, check port test results" | ||
| pass=false |
There was a problem hiding this comment.
are we failing a config check b/c a firewall exists? Can we replace these checks with attempts to connect on CS ports instead?
| @@ -728,12 +730,12 @@ checkPorts() | |||
| pass=true | |||
| for ipadd in "${NODE_IPADDRESS[@]}"; do | |||
|
|
|||
| `sudo nmap $ipadd -p 8600-8630,8700,8800,3306 | grep 'filtered' > /tmp/port_test` | |||
| `nmap $ipadd -p 8600-8630,8700,8800,3306 | grep 'filtered' > ${tmpDir}/port_test` | |||
There was a problem hiding this comment.
fwiw, my ubuntu CS installations don't have nmap; there should be an existence check first
| @@ -728,12 +730,12 @@ checkPorts() | |||
| pass=true | |||
| for ipadd in "${NODE_IPADDRESS[@]}"; do | |||
|
|
|||
| `sudo nmap $ipadd -p 8600-8630,8700,8800,3306 | grep 'filtered' > /tmp/port_test` | |||
| `nmap $ipadd -p 8600-8630,8700,8800,3306 | grep 'filtered' > ${tmpDir}/port_test` | |||
| if [ "$?" -ne 0 ]; then | |||
| echo $ipadd " Node Passed port test" | |||
| else | |||
| echo $ipadd " Node ${bold}Failed${normal} port test, check and disable any firewalls or open ports in firewall" | |||
There was a problem hiding this comment.
refer the user to the doc specifying which ports.
| @@ -50,12 +50,16 @@ namespace config | |||
|
|
|||
| void WriteOnceConfig::initializeDefaults() | |||
| { | |||
| string tmpDir = startup::StartUp::tmpDir(); | |||
|
|
|||
| fLBID_Shift = make_pair("13", false); | |||
| fDBRootCount = make_pair("1", false); | |||
| fDBRMRoot = make_pair("/mnt/OAM/dbrm/BRM_saves", false); | |||
There was a problem hiding this comment.
what is this writeonce thing for?
| @@ -76,6 +80,8 @@ void IDBPolicy::init( bool bEnableLogging, bool bUseRdwrMemBuffer, const string& | |||
| } | |||
| else | |||
| { | |||
| cout << tmpfilepath << endl; | |||
There was a problem hiding this comment.
looks like a debugging printout, can get rid of it.
| #else | ||
|
|
||
| //check for non-root user | ||
| const char* p = getenv("HOME"); |
There was a problem hiding this comment.
eh, better to check the user id directly with getuid()
|
|
||
| fTmpDirp = new string("/tmp"); | ||
|
|
||
| *fTmpDirp = *fTmpDirp + TempFileDir; |
There was a problem hiding this comment.
Looks like this will end up being /tmp/TempFileDir, which doesn't make sense.
| // non-root user | ||
| fTmpDirp = new string(homedir); | ||
|
|
||
| *fTmpDirp = *fTmpDirp + "/.tmp"; |
There was a problem hiding this comment.
remind me why we're hiding the tmp dir? But also shouldn't we just return what's in the config file?
| } | ||
| catch (exception &e) { | ||
| ostringstream o; | ||
| o << "BRM caught an exception attaching to a shared memory segment (" << keyName << "): " << e.what(); |
There was a problem hiding this comment.
Oops! My fault. Missing the log call.
|
|
||
| if not os.access('.', os.W_OK): | ||
| os.chdir('/tmp') | ||
| logger.warn('Changing to /tmp to have permission to write files') |
There was a problem hiding this comment.
I'm not sure why this was here or why it was removed. Does this write to the current directory? If it does we should put this check back in these scripts.
| string hdfsRdwrScratch = cf->getConfig("SystemConfig", "hdfsRdwrScratch"); | ||
| hdfsRdwrScratch = TmpFileDir + hdfsRdwrScratch; |
There was a problem hiding this comment.
This seems redundant. I assume hdfsRdwrScratch an absolute path.
There was a problem hiding this comment.
Wow it took 3 long days to get through all of this. I can't say that I caught everything. Things that weren't changed are obviously not in this diff. Also, given the sheer quantity, I must have missed some stuff.
Several big concerns jump out.
- We were and possibly are putting root passwords in clear text in source files and/or config files.
- There's very little error checking.
- Most of those 'sudo something' to 'something' changes will fail, and we won't know about it unless it affects system operation.
- A lot of the things the installer parts do aren't necessary or are unwise, so when those ops fail it's not such a bad thing.
- Gluster, Amazon, and HDFS configs probably are broken when run as not-root, because some of the things it does look important, and do require root to do them. We likely won't notice the failure(s) and will tell the user everything is fine.
Despite all of the individual failures that might happen, things may still work for simple configurations, because a lot of what it does isn't strictly necessary (editing fstab, mounting things, formatting things, editing init scripts, etc), and we won't notice the failures.
No description provided.