Skip to content

Commit

Permalink
Fix test-report to work with sudo requesting a password
Browse files Browse the repository at this point in the history
Signed-off-by: Jose Cortes <josecortes@datawire.io>
  • Loading branch information
josecv committed Jun 27, 2023
1 parent 51a264f commit ad4088b
Show file tree
Hide file tree
Showing 11 changed files with 203 additions and 55 deletions.
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ else
TELEPRESENCE_VERSION := ${TELEPRESENCE_VERSION}
endif

SHELL:=$(shell which bash)

$(if $(filter v2.%,$(TELEPRESENCE_VERSION)),\
$(info [make] TELEPRESENCE_VERSION=$(TELEPRESENCE_VERSION)),\
$(error TELEPRESENCE_VERSION variable is invalid: It must be a v2.* string, but is '$(TELEPRESENCE_VERSION)'))
Expand Down
2 changes: 1 addition & 1 deletion integration_test/itest/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func (s *cluster) ensureQuit(ctx context.Context) {
_, _, _ = Telepresence(ctx, "quit", "-s") //nolint:dogsled // don't care about any of the returns

// Ensure that the daemon-socket is non-existent.
_ = rmAsRoot(socket.RootDaemonPath(ctx))
_ = rmAsRoot(ctx, socket.RootDaemonPath(ctx))
}

func (s *cluster) ensureExecutable(ctx context.Context, errs chan<- error, wg *sync.WaitGroup) {
Expand Down
13 changes: 11 additions & 2 deletions integration_test/itest/rm_as_root_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,17 @@
package itest

//nolint:depguard // don't care about output or contexts
import "os/exec"
import (
"context"
"os/exec"

func rmAsRoot(file string) error {
"github.com/telepresenceio/telepresence/v2/pkg/proc"
)

func rmAsRoot(ctx context.Context, file string) error {
// We just wanna make sure that the credentials are cached in a uniform way.
if err := proc.CacheAdmin(ctx, ""); err != nil {
return err
}
return exec.Command("sudo", "rm", "-f", file).Run()
}
3 changes: 2 additions & 1 deletion integration_test/itest/rm_as_root_windows.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
package itest

import (
"context"
"os"

"golang.org/x/sys/windows"

"github.com/telepresenceio/telepresence/v2/pkg/shellquote"
)

func rmAsRoot(file string) error {
func rmAsRoot(ctx context.Context, file string) error {
cwd, _ := os.Getwd()
// UTF16PtrFromString can only fail if the argument contains a NUL byte. That will never happen here.
verbPtr, _ := windows.UTF16PtrFromString("runas")
Expand Down
10 changes: 10 additions & 0 deletions pkg/proc/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,13 @@ func IsAdmin() bool {
func Terminate(p *os.Process) error {
return terminate(p)
}

// CacheAdmin will ensure that the current process is able to invoke subprocesses with admin rights
// without having to ask for the password again. This is needed among other things to make sure the
// integration tests can see that a password is being asked for.
func CacheAdmin(ctx context.Context, prompt string) error {
// These logs will get picked up by the test-reporter to make sure the user is asked for the password.
dlog.Info(ctx, "Asking for admin credentials")
defer dlog.Info(ctx, "Admin credentials acquired")
return cacheAdmin(ctx, prompt)
}
60 changes: 35 additions & 25 deletions pkg/proc/exec_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,33 +41,43 @@ func startInBackground(includeEnv bool, args ...string) error {
return nil
}

func cacheAdmin(ctx context.Context, prompt string) error {
// If we're going to be prompting for the `sudo` password, we want to first provide
// the user with some info about exactly what we're prompting for. We don't want to
// use `sudo`'s `--prompt` flag for this because (1) we don't want it to be
// re-displayed if they typo their password, and (2) it might be ignored anyway
// depending on `passprompt_override` in `/etc/sudoers`. So we'll do a pre-flight
// `sudo --non-interactive true` to decide whether to display it.
//
// Note: Using `sudo --non-interactive --validate` does not work well in situations
// where the user has configured `myuser ALL=(ALL:ALL) NOPASSWD: ALL` in the sudoers
// file. Hence the use of `sudo --non-interactive true`. A plausible cause can be
// found in the first comment here:
// https://unix.stackexchange.com/questions/50584/why-sudo-timestamp-is-not-updated-when-nopasswd-is-set
needPwCmd := dexec.CommandContext(ctx, "sudo", "--non-interactive", "true")
needPwCmd.DisableLogging = true
if err := needPwCmd.Run(); err != nil {
if prompt != "" {
fmt.Println(prompt)
}
pwCmd := dexec.CommandContext(ctx, "sudo", "true")
pwCmd.DisableLogging = true
if err := pwCmd.Run(); err != nil {
return err
}
}
return nil
}

func startInBackgroundAsRoot(ctx context.Context, args ...string) error {
if !isAdmin() {
// If we're going to be prompting for the `sudo` password, we want to first provide
// the user with some info about exactly what we're prompting for. We don't want to
// use `sudo`'s `--prompt` flag for this because (1) we don't want it to be
// re-displayed if they typo their password, and (2) it might be ignored anyway
// depending on `passprompt_override` in `/etc/sudoers`. So we'll do a pre-flight
// `sudo --non-interactive true` to decide whether to display it.
//
// Note: Using `sudo --non-interactive --validate` does not work well in situations
// where the user has configured `myuser ALL=(ALL:ALL) NOPASSWD: ALL` in the sudoers
// file. Hence the use of `sudo --non-interactive true`. A plausible cause can be
// found in the first comment here:
// https://unix.stackexchange.com/questions/50584/why-sudo-timestamp-is-not-updated-when-nopasswd-is-set
needPwCmd := dexec.CommandContext(ctx, "sudo", "--non-interactive", "true")
needPwCmd.DisableLogging = true
if err := needPwCmd.Run(); err != nil {
fmt.Printf("Need root privileges to run: %s\n", shellquote.ShellString(args[0], args[1:]))
// `sudo` won't be able to read the password from the terminal when we run
// it with Setpgid=true, so do a pre-flight `sudo true` to read the
// password, and then enforce that being re-used by passing
// `--non-interactive`.
pwCmd := dexec.CommandContext(ctx, "sudo", "true")
pwCmd.DisableLogging = true
if err := pwCmd.Run(); err != nil {
return err
}
// `sudo` won't be able to read the password from the terminal when we run
// it with Setpgid=true, so do a pre-flight `sudo true` (i.e. cacheAdmin) to read the
// password, and then enforce that being re-used by passing
// `--non-interactive`.
prompt := fmt.Sprintf("Need root privileges to run: %s", shellquote.ShellString(args[0], args[1:]))
if err := CacheAdmin(ctx, prompt); err != nil {
return err
}
args = append([]string{"sudo", "--non-interactive"}, args...)
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/proc/exec_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ func createNewProcessGroup(cmd *exec.Cmd) {
cmd.SysProcAttr = &windows.SysProcAttr{CreationFlags: windows.CREATE_NEW_PROCESS_GROUP}
}

func cacheAdmin(_ context.Context, _ string) error {
// No-op on windows, there's no sudo caching. Runas will just pop a window open.
return nil
}

func startInBackground(_ bool, args ...string) error {
return shellExec("open", args[0], args[1:]...)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/vif/router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/datawire/dlib/dgroup"
"github.com/datawire/dlib/dlog"
"github.com/telepresenceio/telepresence/v2/pkg/dos"
"github.com/telepresenceio/telepresence/v2/pkg/proc"
"github.com/telepresenceio/telepresence/v2/pkg/routing"
"github.com/telepresenceio/telepresence/v2/pkg/subnet"
)
Expand Down Expand Up @@ -55,7 +56,7 @@ func (s *RoutingSuite) SetupSuite() {
s.Require().NoError(err)

// Run sudo to get a password prompt out of the way
err = dexec.CommandContext(context.Background(), "sudo", "true").Run()
err = proc.CacheAdmin(context.Background(), "")
s.Require().NoError(err)
}
// Make sure there's no existing route
Expand Down
95 changes: 95 additions & 0 deletions tools/src/test-report/logger.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package main

import (
"context"
"fmt"
"os"
"strings"
)

type Logger struct {
file *os.File
// writer *bufio.Writer
outCh chan string
doneCh chan struct{}
outputs map[TestID]string
}

func NewLogger(ctx context.Context, path string) (*Logger, error) {
f, err := os.Create(path)
if err != nil {
return nil, err
}
l := &Logger{
file: f,
// writer: bufio.NewWriter(f),
outCh: make(chan string, 1024),
doneCh: make(chan struct{}),
outputs: make(map[TestID]string),
}
go l.run(ctx)
return l, nil
}

func (l *Logger) run(ctx context.Context) {
defer close(l.doneCh)
for {
select {
case <-ctx.Done():
return
case out, ok := <-l.outCh:
if !ok {
return
}
l.file.WriteString(out)
}
}
}

func (l *Logger) CloseAndWait() {
close(l.outCh)
// l.writer.Flush()
l.file.Close()
<-l.doneCh
}

func (l *Logger) Report(line *Line) {
switch line.Action {
case RUN:
// There's an OUTPUT line that reports the RUN and the failures and stuff, we just really need formatting here.
l.outputs[line.TestID] = "\n"
parts := strings.Split(line.TestID.Test, "/")
for i := 0; i < len(parts)-1; i++ {
// If this is a subtest of a test, then we need to dump the output of the parent test so far.
parent := TestID{
Package: line.TestID.Package,
Test: strings.Join(parts[:i+1], "/"),
}
if output, ok := l.outputs[parent]; ok {
l.outCh <- output
l.outputs[parent] = ""
}
}
case PASS, FAIL, SKIP:
l.outputs[line.TestID] += "\n"
l.outCh <- l.outputs[line.TestID]
if line.Action != FAIL {
delete(l.outputs, line.TestID)
}
case OUTPUT:
l.outputs[line.TestID] += line.Output
}
}

func (l *Logger) ReportFailures() bool {
if len(l.outputs) == 0 {
return false
}
separator := strings.Repeat("=", 200)
fmt.Fprintf(os.Stderr, "\n\nFailed tests:\n")
for testID, output := range l.outputs {
fmt.Fprintf(os.Stderr, "\n%s.%s\n%s\n%s\n", testID.Package, testID.Test, output, separator)
}
return true

}
11 changes: 10 additions & 1 deletion tools/src/test-report/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"github.com/telepresenceio/telepresence/v2/pkg/dos"
)

const logsFileName = "tests.log"

type TestID struct {
Package string `json:"Package,omitempty"`
Test string `json:"Test,omitempty"`
Expand All @@ -36,6 +38,11 @@ func main() {
fmt.Fprintf(os.Stderr, "Failed to create reporter: %s\n", err)
os.Exit(1)
}
logger, err := NewLogger(ctx, logsFileName)
if err != nil {
fmt.Fprintf(os.Stderr, "Failed to create logger: %s\n", err)
os.Exit(1)
}
if metriton.IsDisabledByUser() {
fmt.Fprint(progressBar, "Reporting is disabled by user\n")
}
Expand All @@ -46,6 +53,7 @@ func main() {
scanner := bufio.NewScanner(os.Stdin)
defer func() {
reporter.CloseAndWait()
logger.CloseAndWait()
// This ends the progress bar and hence the program
progressBar.End()
cancel()
Expand All @@ -62,10 +70,11 @@ func main() {
}
reporter.Report(line)
progressBar.ReportCh <- line
logger.Report(line)
}
}()
progressBar.Wait()
if progressBar.PrintFailures() {
if logger.ReportFailures() {
os.Exit(1)
}
}
Loading

0 comments on commit ad4088b

Please sign in to comment.