From 919935beec95cbf21489ed04e5b2a1f4065dc732 Mon Sep 17 00:00:00 2001 From: Random-Liu Date: Thu, 18 Aug 2016 21:50:33 -0700 Subject: [PATCH] Remove sudo in test suite and run test with sudo. --- test/e2e_node/apparmor_test.go | 1 + test/e2e_node/e2e_node_suite_test.go | 7 +- test/e2e_node/remote/remote.go | 78 ++++++++++++++--------- test/e2e_node/runner/local/run_local.go | 2 +- test/e2e_node/runner/remote/run_remote.go | 2 +- test/e2e_node/services/server.go | 16 +---- test/e2e_node/services/services.go | 24 ++----- test/e2e_node/system/kernel_validator.go | 5 +- 8 files changed, 65 insertions(+), 70 deletions(-) diff --git a/test/e2e_node/apparmor_test.go b/test/e2e_node/apparmor_test.go index 440ec9068fe..7e33a2d97a5 100644 --- a/test/e2e_node/apparmor_test.go +++ b/test/e2e_node/apparmor_test.go @@ -119,6 +119,7 @@ func loadTestProfiles() error { return fmt.Errorf("failed to write profiles to file: %v", err) } + // TODO(random-liu): The test is run as root now, no need to use sudo here. cmd := exec.Command("sudo", "apparmor_parser", "-r", "-W", f.Name()) stderr := &bytes.Buffer{} cmd.Stderr = stderr diff --git a/test/e2e_node/e2e_node_suite_test.go b/test/e2e_node/e2e_node_suite_test.go index 821089e635e..b5d275034ee 100644 --- a/test/e2e_node/e2e_node_suite_test.go +++ b/test/e2e_node/e2e_node_suite_test.go @@ -172,12 +172,11 @@ func validateSystem() error { if err != nil { return fmt.Errorf("can't get current binary: %v", err) } - // TODO(random-liu): Remove sudo in containerize PR. - output, err := exec.Command("sudo", testBin, "--system-validate-mode").CombinedOutput() + output, err := exec.Command(testBin, append([]string{"--system-validate-mode"}, os.Args[1:]...)...).CombinedOutput() // The output of system validation should have been formatted, directly print here. fmt.Print(string(output)) if err != nil { - return fmt.Errorf("system validation failed") + return fmt.Errorf("system validation failed: %v", err) } return nil } @@ -190,7 +189,7 @@ func maskLocksmithdOnCoreos() { return } if bytes.Contains(data, []byte("ID=coreos")) { - output, err := exec.Command("sudo", "systemctl", "mask", "--now", "locksmithd").CombinedOutput() + output, err := exec.Command("systemctl", "mask", "--now", "locksmithd").CombinedOutput() Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("should be able to mask locksmithd - output: %q", string(output))) glog.Infof("Locksmithd is masked successfully") } diff --git a/test/e2e_node/remote/remote.go b/test/e2e_node/remote/remote.go index 53fd68d0299..bcc2cf353f8 100644 --- a/test/e2e_node/remote/remote.go +++ b/test/e2e_node/remote/remote.go @@ -161,7 +161,7 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string if err != nil { return "", false, fmt.Errorf("could not find username: %v", err) } - output, err := RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sudo", "usermod", "-a", "-G", "docker", uname.Username) + output, err := SSH(host, "usermod", "-a", "-G", "docker", uname.Username) if err != nil { return "", false, fmt.Errorf("instance %s not running docker daemon - Command failed: %s", host, output) } @@ -172,14 +172,15 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string dirName := fmt.Sprintf("gcloud-e2e-%d", rand.Int31()) tmp := fmt.Sprintf("/tmp/%s", dirName) - _, err := RunSshCommand("ssh", GetHostnameOrIp(host), "--", "mkdir", tmp) + // Do not sudo here, so that we can use scp to copy test archive to the directdory. + _, err := SSHNoSudo(host, "mkdir", tmp) if err != nil { // Exit failure with the error return "", false, err } if cleanup { defer func() { - output, err := RunSshCommand("ssh", GetHostnameOrIp(host), "--", "rm", "-rf", tmp) + output, err := SSH(host, "rm", "-rf", tmp) if err != nil { glog.Errorf("failed to cleanup tmp directory %s on host %v. Output:\n%s", tmp, err, output) } @@ -188,57 +189,62 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string // Install the cni plugin. cniPath := filepath.Join(tmp, CNIDirectory) - if _, err := RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sh", "-c", - getSshCommand(" ; ", fmt.Sprintf("sudo mkdir -p %s", cniPath), - fmt.Sprintf("sudo wget -O - %s | sudo tar -xz -C %s", CNIURL, cniPath))); err != nil { + cmd := getSSHCommand(" ; ", + fmt.Sprintf("mkdir -p %s", cniPath), + fmt.Sprintf("wget -O - %s | tar -xz -C %s", CNIURL, cniPath), + ) + if _, err := SSH(host, "sh", "-c", cmd); err != nil { // Exit failure with the error return "", false, err } // Configure iptables firewall rules // TODO: consider calling bootstrap script to configure host based on OS - cmd := getSshCommand("&&", + cmd = getSSHCommand("&&", `iptables -L INPUT | grep "Chain INPUT (policy DROP)"`, "(iptables -C INPUT -w -p TCP -j ACCEPT || iptables -A INPUT -w -p TCP -j ACCEPT)", "(iptables -C INPUT -w -p UDP -j ACCEPT || iptables -A INPUT -w -p UDP -j ACCEPT)", "(iptables -C INPUT -w -p ICMP -j ACCEPT || iptables -A INPUT -w -p ICMP -j ACCEPT)") - output, err := RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sudo", "sh", "-c", cmd) + output, err := SSH(host, "sh", "-c", cmd) if err != nil { glog.Errorf("Failed to configured firewall: %v output: %v", err, output) } - cmd = getSshCommand("&&", + cmd = getSSHCommand("&&", `iptables -L FORWARD | grep "Chain FORWARD (policy DROP)" > /dev/null`, "(iptables -C FORWARD -w -p TCP -j ACCEPT || iptables -A FORWARD -w -p TCP -j ACCEPT)", "(iptables -C FORWARD -w -p UDP -j ACCEPT || iptables -A FORWARD -w -p UDP -j ACCEPT)", "(iptables -C FORWARD -w -p ICMP -j ACCEPT || iptables -A FORWARD -w -p ICMP -j ACCEPT)") - output, err = RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sudo", "sh", "-c", cmd) + output, err = SSH(host, "sh", "-c", cmd) if err != nil { glog.Errorf("Failed to configured firewall: %v output: %v", err, output) } // Copy the archive to the staging directory - _, err = RunSshCommand("scp", archive, fmt.Sprintf("%s:%s/", GetHostnameOrIp(host), tmp)) + _, err = runSSHCommand("scp", archive, fmt.Sprintf("%s:%s/", GetHostnameOrIp(host), tmp)) if err != nil { // Exit failure with the error return "", false, err } // Kill any running node processes - cmd = getSshCommand(" ; ", - "sudo pkill kubelet", - "sudo pkill kube-apiserver", - "sudo pkill etcd", + cmd = getSSHCommand(" ; ", + "pkill kubelet", + "pkill kube-apiserver", + "pkill etcd", ) // No need to log an error if pkill fails since pkill will fail if the commands are not running. // If we are unable to stop existing running k8s processes, we should see messages in the kubelet/apiserver/etcd // logs about failing to bind the required ports. glog.Infof("Killing any existing node processes on %s", host) - RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sh", "-c", cmd) + SSH(host, "sh", "-c", cmd) // Extract the archive - cmd = getSshCommand(" && ", fmt.Sprintf("cd %s", tmp), fmt.Sprintf("tar -xzvf ./%s", archiveName)) + cmd = getSSHCommand(" && ", + fmt.Sprintf("cd %s", tmp), + fmt.Sprintf("tar -xzvf ./%s", archiveName), + ) glog.Infof("Extracting tar on %s", host) - output, err = RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sh", "-c", cmd) + output, err = SSH(host, "sh", "-c", cmd) if err != nil { // Exit failure with the error return "", false, err @@ -261,7 +267,7 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string } // Determine if tests will run on a GCI node. - output, err = RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sh", "-c", "'cat /etc/os-release'") + output, err = SSH(host, "sh", "-c", "'cat /etc/os-release'") if err != nil { glog.Errorf("Issue detecting node's OS via node's /etc/os-release. Err: %v, Output:\n%s", err, output) return "", false, fmt.Errorf("Issue detecting node's OS via node's /etc/os-release. Err: %v, Output:\n%s", err, output) @@ -270,7 +276,7 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string // Note this implicitly requires the script to be where we expect in the tarball, so if that location changes the error // here will tell us to update the remote test runner. mounterPath := filepath.Join(tmp, "cluster/gce/gci/mounter/mounter") - output, err = RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sh", "-c", fmt.Sprintf("'chmod 544 %s'", mounterPath)) + output, err = SSH(host, "sh", "-c", fmt.Sprintf("'chmod 544 %s'", mounterPath)) if err != nil { glog.Errorf("Unable to chmod 544 GCI mounter script. Err: %v, Output:\n%s", err, output) return "", false, err @@ -284,7 +290,7 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string } // Run the tests - cmd = getSshCommand(" && ", + cmd = getSSHCommand(" && ", fmt.Sprintf("cd %s", tmp), fmt.Sprintf("timeout -k 30s %fs ./ginkgo %s ./e2e_node.test -- --logtostderr --v 4 --node-name=%s --report-dir=%s/results --report-prefix=%s %s", testTimeoutSeconds.Seconds(), ginkgoFlags, host, tmp, junitFilePrefix, testArgs), @@ -292,7 +298,7 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string aggErrs := []error{} glog.Infof("Starting tests on %s", host) - output, err = RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sh", "-c", cmd) + output, err = SSH(host, "sh", "-c", cmd) if err != nil { aggErrs = append(aggErrs, err) @@ -313,10 +319,10 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string // Try getting the system logs from journald and store it to a file. // Don't reuse the original test directory on the remote host because // it could've be been removed if the node was rebooted. - _, err := RunSshCommand("ssh", GetHostnameOrIp(host), "--", "sh", "-c", fmt.Sprintf("'sudo journalctl --system --all > %s'", logPath)) + _, err := SSH(host, "sh", "-c", fmt.Sprintf("'journalctl --system --all > %s'", logPath)) if err == nil { glog.Infof("Got the system logs from journald; copying it back...") - if _, err := RunSshCommand("scp", fmt.Sprintf("%s:%s", GetHostnameOrIp(host), logPath), destPath); err != nil { + if _, err := runSSHCommand("scp", fmt.Sprintf("%s:%s", GetHostnameOrIp(host), logPath), destPath); err != nil { glog.Infof("Failed to copy the log: err: %v", err) } } else { @@ -334,26 +340,38 @@ func RunRemote(archive string, host string, cleanup bool, junitFilePrefix string } func getTestArtifacts(host, testDir string) error { - _, err := RunSshCommand("scp", "-r", fmt.Sprintf("%s:%s/results/", GetHostnameOrIp(host), testDir), fmt.Sprintf("%s/%s", *resultsDir, host)) + _, err := runSSHCommand("scp", "-r", fmt.Sprintf("%s:%s/results/", GetHostnameOrIp(host), testDir), fmt.Sprintf("%s/%s", *resultsDir, host)) if err != nil { return err } // Copy junit to the top of artifacts - _, err = RunSshCommand("scp", fmt.Sprintf("%s:%s/results/junit*", GetHostnameOrIp(host), testDir), fmt.Sprintf("%s/", *resultsDir)) + _, err = runSSHCommand("scp", fmt.Sprintf("%s:%s/results/junit*", GetHostnameOrIp(host), testDir), fmt.Sprintf("%s/", *resultsDir)) if err != nil { return err } return nil } -// getSshCommand handles proper quoting so that multiple commands are executed in the same shell over ssh -func getSshCommand(sep string, args ...string) string { +// getSSHCommand handles proper quoting so that multiple commands are executed in the same shell over ssh +func getSSHCommand(sep string, args ...string) string { return fmt.Sprintf("'%s'", strings.Join(args, sep)) } -// runSshCommand executes the ssh or scp command, adding the flag provided --ssh-options -func RunSshCommand(cmd string, args ...string) (string, error) { +// SSH executes ssh command with runSSHCommand as root. The `sudo` makes sure that all commands +// are executed by root, so that there won't be permission mismatch between different commands. +func SSH(host string, cmd ...string) (string, error) { + return runSSHCommand("ssh", append([]string{GetHostnameOrIp(host), "--", "sudo"}, cmd...)...) +} + +// SSHNoSudo executes ssh command with runSSHCommand as normal user. Sometimes we need this, +// for example creating a directory that we'll copy files there with scp. +func SSHNoSudo(host string, cmd ...string) (string, error) { + return runSSHCommand("ssh", append([]string{GetHostnameOrIp(host), "--"}, cmd...)...) +} + +// runSSHCommand executes the ssh or scp command, adding the flag provided --ssh-options +func runSSHCommand(cmd string, args ...string) (string, error) { if env, found := sshOptionsMap[*sshEnv]; found { args = append(strings.Split(env, " "), args...) } diff --git a/test/e2e_node/runner/local/run_local.go b/test/e2e_node/runner/local/run_local.go index 241c060eb7f..3371f533515 100644 --- a/test/e2e_node/runner/local/run_local.go +++ b/test/e2e_node/runner/local/run_local.go @@ -56,7 +56,7 @@ func main() { func runCommand(name string, args ...string) error { glog.Infof("Running command: %v %v", name, strings.Join(args, " ")) - cmd := exec.Command("sh", "-c", strings.Join(append([]string{name}, args...), " ")) + cmd := exec.Command("sudo", "sh", "-c", strings.Join(append([]string{name}, args...), " ")) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr return cmd.Run() diff --git a/test/e2e_node/runner/remote/run_remote.go b/test/e2e_node/runner/remote/run_remote.go index 193e697479f..045a4278ee6 100644 --- a/test/e2e_node/runner/remote/run_remote.go +++ b/test/e2e_node/runner/remote/run_remote.go @@ -507,7 +507,7 @@ func createInstance(imageConfig *internalGCEImage) (string, error) { remote.AddHostnameIp(name, externalIp) } var output string - output, err = remote.RunSshCommand("ssh", remote.GetHostnameOrIp(name), "--", "sudo", "docker", "version") + output, err = remote.SSH(name, "docker", "version") if err != nil { err = fmt.Errorf("instance %s not running docker daemon - Command failed: %s", name, output) continue diff --git a/test/e2e_node/services/server.go b/test/e2e_node/services/server.go index 18a9d0a31eb..2911a162104 100644 --- a/test/e2e_node/services/server.go +++ b/test/e2e_node/services/server.go @@ -44,7 +44,7 @@ type server struct { // startCommand is the command used to start the server startCommand *exec.Cmd // killCommand is the command used to stop the server. It is not required. If it - // is not specified, `sudo kill` will be used to stop the server. + // is not specified, `kill` will be used to stop the server. killCommand *exec.Cmd // restartCommand is the command used to restart the server. If provided, it will be used // instead of startCommand when restarting the server. @@ -338,19 +338,7 @@ func (s *server) kill() error { const timeout = 10 * time.Second for _, signal := range []string{"-TERM", "-KILL"} { glog.V(2).Infof("Killing process %d (%s) with %s", pid, name, signal) - cmd := exec.Command("sudo", "kill", signal, strconv.Itoa(pid)) - - // Run the 'kill' command in a separate process group so sudo doesn't ignore it - attrs := &syscall.SysProcAttr{} - // Hack to set unix-only field without build tags. - setpgidField := reflect.ValueOf(attrs).Elem().FieldByName("Setpgid") - if setpgidField.IsValid() { - setpgidField.Set(reflect.ValueOf(true)) - } else { - return fmt.Errorf("Failed to set Setpgid field (non-unix build)") - } - cmd.SysProcAttr = attrs - + cmd := exec.Command("kill", signal, strconv.Itoa(pid)) _, err := cmd.Output() if err != nil { glog.Errorf("Error signaling process %d (%s) with %s: %v", pid, name, signal, err) diff --git a/test/e2e_node/services/services.go b/test/e2e_node/services/services.go index 4570c372e9b..040b506a6f7 100644 --- a/test/e2e_node/services/services.go +++ b/test/e2e_node/services/services.go @@ -148,17 +148,7 @@ func (e *E2EServices) startInternalServices() (*server, error) { if err != nil { return nil, fmt.Errorf("can't get current binary: %v", err) } - startCmd := exec.Command("sudo", testBin, - // TODO(mtaufen): Flags e.g. that target the TestContext need to be manually forwarded to the - // test binary when we start it up in run-services mode. This is not ideal. - // Very unintuitive because it prevents any flags NOT manually forwarded here - // from being set via TEST_ARGS when running tests from the command line. - "--run-services-mode", - "--server-start-timeout", serverStartTimeout.String(), - "--feature-gates", framework.TestContext.FeatureGates, - "--logtostderr", - "--vmodule=*="+LOG_VERBOSITY_LEVEL, - ) + startCmd := exec.Command(testBin, append([]string{"--run-services-mode"}, os.Args[1:]...)...) server := newServer("services", startCmd, nil, nil, getServicesHealthCheckURLs(), servicesLogFile, e.monitorParent, false) return server, server.start() } @@ -184,8 +174,8 @@ func (e *E2EServices) startKubelet() (*server, error) { // sense to test it that way unitName := fmt.Sprintf("kubelet-%d.service", rand.Int31()) cmdArgs = append(cmdArgs, systemdRun, "--unit="+unitName, "--remain-after-exit", builder.GetKubeletServerBin()) - killCommand = exec.Command("sudo", "systemctl", "kill", unitName) - restartCommand = exec.Command("sudo", "systemctl", "restart", unitName) + killCommand = exec.Command("systemctl", "kill", unitName) + restartCommand = exec.Command("systemctl", "restart", unitName) e.logFiles["kubelet.log"] = logFileData{ journalctlCommand: []string{"-u", unitName}, } @@ -251,7 +241,7 @@ func (e *E2EServices) startKubelet() (*server, error) { "--network-plugin-dir", filepath.Join(cwd, "cni", "bin")) // Enable kubenet } - cmd := exec.Command("sudo", cmdArgs...) + cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...) server := newServer( "kubelet", cmd, @@ -286,7 +276,7 @@ func (e *E2EServices) getLogFiles() { continue } glog.Infof("Get log file %q with journalctl command %v.", targetFileName, logFileData.journalctlCommand) - out, err := exec.Command("sudo", append([]string{"journalctl"}, logFileData.journalctlCommand...)...).CombinedOutput() + out, err := exec.Command("journalctl", logFileData.journalctlCommand...).CombinedOutput() if err != nil { glog.Errorf("failed to get %q from journald: %v, %v", targetFileName, string(out), err) } else { @@ -319,10 +309,10 @@ func isJournaldAvailable() bool { func copyLogFile(src, target string) error { // If not a journald based distro, then just symlink files. - if out, err := exec.Command("sudo", "cp", src, target).CombinedOutput(); err != nil { + if out, err := exec.Command("cp", src, target).CombinedOutput(); err != nil { return fmt.Errorf("failed to copy %q to %q: %v, %v", src, target, out, err) } - if out, err := exec.Command("sudo", "chmod", "a+r", target).CombinedOutput(); err != nil { + if out, err := exec.Command("chmod", "a+r", target).CombinedOutput(); err != nil { return fmt.Errorf("failed to make log file %q world readable: %v, %v", target, out, err) } return nil diff --git a/test/e2e_node/system/kernel_validator.go b/test/e2e_node/system/kernel_validator.go index f71d7072e32..e98cd46510f 100644 --- a/test/e2e_node/system/kernel_validator.go +++ b/test/e2e_node/system/kernel_validator.go @@ -208,14 +208,13 @@ func (k *KernelValidator) getKernelConfigReader() (io.Reader, error) { } // If the kernel config file is not found, try to load the kernel // config module and check again. - // TODO(random-liu): Remove "sudo" in containerize test PR #31093 - output, err := exec.Command("sudo", modprobeCmd, configsModule).CombinedOutput() + output, err := exec.Command(modprobeCmd, configsModule).CombinedOutput() if err != nil { return nil, fmt.Errorf("unable to load kernel module %q: output - %q, err - %v", configsModule, output, err) } // Unload the kernel config module to make sure the validation have no side effect. - defer exec.Command("sudo", modprobeCmd, "-r", configsModule).Run() + defer exec.Command(modprobeCmd, "-r", configsModule).Run() loadModule = true } return nil, fmt.Errorf("no config path in %v is available", possibePaths)