Merge pull request #4077 from apostasie/tigron-2-debugging

[Tigron]: command env WhiteList support and cosmetic changes

Author: AkihiroSuda

mod/tigron/.golangci.yml

@@ -12,25 +12,81 @@ issues:
 
 linters:
   default: all
+  enable:
+    # These are the default set of golangci (errcheck is disabled, see below)
+    - govet             # Vet examines Go source code and reports suspicious constructs. It is roughly the same as 'go vet' and uses its passes.
+    - ineffassign       # Detects when assignments to existing variables are not used.
+    - staticcheck       # It's the set of rules from staticcheck.
+    - unused            # Checks Go code for unused constants, variables, functions and types.
+    # These are the linters we knowingly want enabled in addition to the default set
+    - containedctx      # avoid embedding context into structs
+    - depguard          # Allows to explicitly allow or disallow third party modules
+    - err113            # encourage static errors
+    - gochecknoglobals  # globals should be avoided as much as possible
+    - godot             # forces dot at the end of comments
+    - gosec             # various security checks
+    - interfacebloat    # limit complexity in public APIs
+    - paralleltest      # enforces tests using parallel
+    - revive            # meta linter (see settings below)
+    - testpackage       # test packages should be separate from the package they test (eg: name them package_test)
+    - testableexamples  # makes sure that examples are testable (have an expected output)
+    - thelper           # enforces use of t.Helper()
+    - varnamelen        # encourage readable descriptive names for variables instead of x, y, z
   disable:
     # These are the linters that we know we do not want
-    - cyclop          # provided by revive
-    - exhaustruct     # does not serve much of a purpose
-    - funlen          # provided by revive
-    - gocognit        # provided by revive
-    - goconst         # provided by revive
-    - godox           # not helpful unless we could downgrade it to warning / info
-    - ginkgolinter    # no ginkgo
-    - gomodguard      # we use depguard instead
-    - ireturn         # too annoying with not enough value
-    - lll             # provided by golines
-    - nonamedreturns  # named returns are occasionally useful
-    - prealloc        # premature optimization
-    - promlinter      # no prometheus
-    - sloglint        # no slog
-    - testifylint     # no testify
-    - zerologlint     # no zerolog
+    - cyclop            # provided by revive
+    - exhaustruct       # does not serve much of a purpose
+    - errcheck          # provided by revive
+    - forcetypeassert   # provided by revive
+    - funlen            # provided by revive
+    - gocognit          # provided by revive
+    - goconst           # provided by revive
+    - godox             # not helpful unless we could downgrade it to warning / info
+    - ginkgolinter      # no ginkgo
+    - gomodguard        # we use depguard instead
+    - ireturn           # too annoying with not enough value
+    - lll               # provided by golines
+    - nestif            # already provided ten different ways with revive cognitive complexity, etc
+    - nonamedreturns    # named returns are occasionally useful
+    - prealloc          # premature optimization
+    - promlinter        # no prometheus
+    - sloglint          # no slog
+    - testifylint       # no testify
+    - zerologlint       # no zerolog
   settings:
+    interfacebloat:
+      # Default is 10
+      max: 13
+    revive:
+      enable-all-rules: true
+      rules:
+        - name: cognitive-complexity
+          # Default is 7
+          arguments: [60]
+        - name: function-length
+          # Default is 50, 75
+          arguments: [80, 180]
+        - name: cyclomatic
+          # Default is 10
+          arguments: [30]
+        - name: add-constant
+          arguments:
+            - allowInts: "0,1,2"
+              allowStrs: '""'
+        - name: flag-parameter
+          # Not sure why this is valuable.
+          disabled: true
+        - name: line-length-limit
+          # Formatter `golines` takes care of this.
+          disabled: true
+        - name: unhandled-error
+          arguments:
+            - "fmt.Print"
+            - "fmt.Println"
+            - "fmt.Printf"
+            - "fmt.Fprint"
+            - "fmt.Fprintln"
+            - "fmt.Fprintf"
     depguard:
       rules:
         main:

mod/tigron/expect/comparators_test.go

@@ -14,8 +14,11 @@
    limitations under the License.
 */
 
+//revive:disable:add-constant
 package expect_test
 
+// TODO: add a lot more tests including failure conditions with mimicry
+
 import (
 	"regexp"
 	"testing"

mod/tigron/internal/com/command.go

@@ -43,16 +43,14 @@ var (
 	ErrFailedStarting = errors.New("command failed starting")
 	// ErrSignaled is returned by Wait() if a signal was sent to the command while running.
 	ErrSignaled = errors.New("command execution signaled")
-	// ErrExecutionFailed is returned by Wait() when a command executes but returns a non-zero error
-	// code.
+	// ErrExecutionFailed is returned by Wait() when a command executes but returns a non-zero error code.
 	ErrExecutionFailed = errors.New("command returned a non-zero exit code")
 	// ErrFailedSendingSignal may happen if sending a signal to an already terminated process.
 	ErrFailedSendingSignal = errors.New("failed sending signal")
 
 	// ErrExecAlreadyStarted is a system error normally indicating a bogus double call to Run().
 	ErrExecAlreadyStarted = errors.New("command has already been started (double `Run`)")
-	// ErrExecNotStarted is a system error normally indicating that Wait() has been called without
-	// first calling Run().
+	// ErrExecNotStarted is a system error normally indicating that Wait() has been called without first calling Run().
 	ErrExecNotStarted = errors.New("command has not been started (call `Run` first)")
 	// ErrExecAlreadyFinished is a system error indicating a double call to Wait().
 	ErrExecAlreadyFinished = errors.New("command is already finished")
@@ -75,7 +73,7 @@ type Result struct {
 }
 
 type execution struct {
-	//nolint:containedctx
+	//nolint:containedctx // Is there a way around this?
 	context context.Context
 	cancel  context.CancelFunc
 	command *exec.Cmd
@@ -93,10 +91,10 @@ type Command struct {
 	WrapArgs    []string
 	Timeout     time.Duration
 
-	WorkingDir string
-	Env        map[string]string
-	// FIXME: EnvBlackList might change for a better mechanism (regexp and/or whitelist + blacklist)
+	WorkingDir   string
+	Env          map[string]string
 	EnvBlackList []string
+	EnvWhiteList []string
 
 	writers []func() io.Reader
 
@@ -122,6 +120,7 @@ func (gc *Command) Clone() *Command {
 		WorkingDir:   gc.WorkingDir,
 		Env:          map[string]string{},
 		EnvBlackList: append([]string(nil), gc.EnvBlackList...),
+		EnvWhiteList: append([]string(nil), gc.EnvWhiteList...),
 
 		writers: append([]func() io.Reader(nil), gc.writers...),
 
@@ -137,27 +136,24 @@ func (gc *Command) Clone() *Command {
 	return com
 }
 
-// WithPTY requests that the command be executed with a pty for std streams. Parameters allow
-// showing which streams
-// are to be tied to the pty.
+// WithPTY requests that the command be executed with a pty for std streams.
+// Parameters allow showing which streams are to be tied to the pty.
 // This command has no effect if Run has already been called.
 func (gc *Command) WithPTY(stdin, stdout, stderr bool) {
 	gc.ptyStdout = stdout
 	gc.ptyStderr = stderr
 	gc.ptyStdin = stdin
 }
 
-// WithFeeder ensures that the provider function will be executed and its output fed to the command
-// stdin. WithFeeder, like Feed, can be used multiple times, and writes will be performed
-// sequentially, in order.
+// WithFeeder ensures that the provider function will be executed and its output fed to the command stdin.
+// WithFeeder, like Feed, can be used multiple times, and writes will be performed sequentially, in order.
 // This command has no effect if Run has already been called.
 func (gc *Command) WithFeeder(writers ...func() io.Reader) {
 	gc.writers = append(gc.writers, writers...)
 }
 
 // Feed ensures that the provider reader will be copied on the command stdin.
-// Feed, like WithFeeder, can be used multiple times, and writes will be performed in sequentially,
-// in order.
+// Feed, like WithFeeder, can be used multiple times, and writes will be performed in sequentially, in order.
 // This command has no effect if Run has already been called.
 func (gc *Command) Feed(reader io.Reader) {
 	gc.writers = append(gc.writers, func() io.Reader {
@@ -197,7 +193,6 @@ func (gc *Command) Run(parentCtx context.Context) error {
 
 	// Create a contextual command, set the logger
 	cmd = gc.buildCommand(ctx)
-
 	// Get a debug-logger from the context
 	var (
 		log logger.Logger
@@ -338,8 +333,7 @@ func (gc *Command) wrap() error {
 		err      error
 	)
 
-	// XXXgolang: this is troubling. cmd.ProcessState.ExitCode() is always fine, even if
-	// cmd.ProcessState is nil.
+	// XXXgolang: this is troubling. cmd.ProcessState.ExitCode() is always fine, even if cmd.ProcessState is nil.
 	exitCode = cmd.ProcessState.ExitCode()
 
 	if cmd.ProcessState != nil {
@@ -356,7 +350,7 @@ func (gc *Command) wrap() error {
 		}
 	}
 
-	// Catch-up on the context
+	// Catch-up on the context.
 	switch ctx.Err() {
 	case context.DeadlineExceeded:
 		err = ErrTimeout
@@ -365,7 +359,7 @@ func (gc *Command) wrap() error {
 	default:
 	}
 
-	// Stuff everything in Result and return err
+	// Stuff everything in Result and return err.
 	gc.result = &Result{
 		ExitCode: exitCode,
 		Stdout:   pipes.fromStdout,
@@ -382,7 +376,7 @@ func (gc *Command) wrap() error {
 }
 
 func (gc *Command) buildCommand(ctx context.Context) *exec.Cmd {
-	// Build arguments and binary
+	// Build arguments and binary.
 	args := gc.Args
 	if gc.PrependArgs != nil {
 		args = append(gc.PrependArgs, args...)
@@ -399,26 +393,55 @@ func (gc *Command) buildCommand(ctx context.Context) *exec.Cmd {
 	//nolint:gosec
 	cmd := exec.CommandContext(ctx, binary, args...)
 
-	// Add dir
+	// Add dir.
 	cmd.Dir = gc.WorkingDir
 
-	// Set wait delay after waits returns
+	// Set wait delay after waits returns.
 	cmd.WaitDelay = delayAfterWait
 
-	// Build env
+	// Build env.
 	cmd.Env = []string{}
-	// TODO: replace with regexps? and/or whitelist?
+
+	const (
+		star  = "*"
+		equal = "="
+	)
+
 	for _, envValue := range os.Environ() {
 		add := true
 
-		for _, b := range gc.EnvBlackList {
-			if b == "*" || strings.HasPrefix(envValue, b+"=") {
+		for _, needle := range gc.EnvBlackList {
+			if strings.HasSuffix(needle, star) {
+				needle = strings.TrimSuffix(needle, star)
+			} else if needle != star && !strings.Contains(needle, equal) {
+				needle += equal
+			}
+
+			if needle == star || strings.HasPrefix(envValue, needle) {
 				add = false
 
 				break
 			}
 		}
 
+		if len(gc.EnvWhiteList) > 0 {
+			add = false
+
+			for _, needle := range gc.EnvWhiteList {
+				if strings.HasSuffix(needle, star) {
+					needle = strings.TrimSuffix(needle, star)
+				} else if needle != star && !strings.Contains(needle, equal) {
+					needle += equal
+				}
+
+				if needle == star || strings.HasPrefix(envValue, needle) {
+					add = true
+
+					break
+				}
+			}
+		}
+
 		if add {
 			cmd.Env = append(cmd.Env, envValue)
 		}
@@ -429,12 +452,12 @@ func (gc *Command) buildCommand(ctx context.Context) *exec.Cmd {
 		cmd.Env = append(cmd.Env, k+"="+v)
 	}
 
-	// Attach platform ProcAttr and get optional custom cancellation routine
+	// Attach platform ProcAttr and get optional custom cancellation routine.
 	if cancellation := addAttr(cmd); cancellation != nil {
 		cmd.Cancel = func() error {
 			gc.exec.log.Log("command cancelled")
 
-			// Call the platform dependent cancellation routine
+			// Call the platform dependent cancellation routine.
 			return cancellation()
 		}
 	}

mod/tigron/internal/com/command_other.go

@@ -24,10 +24,10 @@ import (
 )
 
 func addAttr(cmd *exec.Cmd) func() error {
-	// Default shutdown will leave child processes behind in certain circumstances
+	// Default shutdown will leave child processes behind in certain circumstances.
 	cmd.SysProcAttr = &syscall.SysProcAttr{
 		Setsid: true,
-		// FIXME: understand why we would want that
+		// FIXME: understand why we would want that.
 		// Setctty: true,
 	}