From 7eb82b680f6d92b764ddc85021bf09cf7a25e1bd Mon Sep 17 00:00:00 2001 From: Kayla McKay <39921134+kaymckay@users.noreply.github.com> Date: Mon, 7 Mar 2022 11:49:29 -0600 Subject: [PATCH] add new config and clean (#14) --- .golangci.yml | 163 ++++++++++++++---------- cmd/vela-scp/main.go | 4 + cmd/vela-ssh/main.go | 4 + internal/openssh/openssh.go | 5 + internal/scp/scp.go | 10 ++ internal/scp/scp_test.go | 6 +- internal/ssh/ssh.go | 9 ++ internal/ssh/ssh_test.go | 6 +- internal/testutils/testutils.go | 6 + pkg/binarywrapper/binarywrapper.go | 8 ++ pkg/binarywrapper/binarywrapper_test.go | 9 ++ 11 files changed, 161 insertions(+), 69 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index beb5f17..97b6d15 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -28,19 +28,16 @@ linters-settings: # https://github.com/ultraware/funlen funlen: - lines: 100 - statements: 50 + # accounting for comments + lines: 160 + statements: 70 - # https://github.com/tommy-muehle/go-mnd - gomnd: - settings: - mnd: - # don't include the "operation" and "assign" - checks: - - argument - - case - - condition - - return + # https://github.com/denis-tingaikin/go-header + goheader: + template: |- + Copyright (c) {{ YEAR }} Target Brands, Inc. All rights reserved. + + Use of this source code is governed by the LICENSE file in this repository. # https://github.com/client9/misspell misspell: @@ -48,10 +45,10 @@ linters-settings: # https://github.com/golangci/golangci-lint/blob/master/pkg/golinters/nolintlint nolintlint: - allow-leading-space: true # allow non-"machine-readable" format (ie. with leading space) - allow-unused: false # allow nolint directives that don't address a linting issue - require-explanation: true # require an explanation for nolint directives - require-specific: true # require nolint directives to be specific about which linter is being skipped + allow-leading-space: true # allow non-"machine-readable" format (ie. with leading space) + allow-unused: false # allow nolint directives that don't address a linting issue + require-explanation: true # require an explanation for nolint directives + require-specific: true # require nolint directives to be specific about which linter is being skipped # This section provides the configuration for which linters # golangci will execute. Several of them were disabled by @@ -62,56 +59,91 @@ linters: # enable a specific set of linters to run enable: - - bodyclose - - deadcode # enabled by default - - dupl - - errcheck # enabled by default - - funlen - - goconst - - gocyclo - - godot - - gofmt - - goimports - - revive - - gomnd - - goprintffuncname - - gosec - - gosimple # enabled by default - - govet # enabled by default - - ineffassign # enabled by default - - misspell - - nakedret - - nolintlint - - staticcheck # enabled by default - - structcheck # enabled by default - - stylecheck - - typecheck # enabled by default - - unconvert - - unparam - - unused # enabled by default - - varcheck # enabled by default - - whitespace - + - bidichk # checks for dangerous unicode character sequences + - bodyclose # checks whether HTTP response body is closed successfully + - contextcheck # check the function whether use a non-inherited context + - deadcode # finds unused code + - dupl # code clone detection + - errcheck # checks for unchecked errors + - errorlint # find misuses of errors + - exportloopref # check for exported loop vars + - funlen # detects long functions + - goconst # finds repeated strings that could be replaced by a constant + - gocyclo # computes and checks the cyclomatic complexity of functions + - godot # checks if comments end in a period + - gofmt # checks whether code was gofmt-ed + - goheader # checks is file header matches to pattern + - goimports # fixes imports and formats code in same style as gofmt + - gomoddirectives # manage the use of 'replace', 'retract', and 'excludes' directives in go.mod + - goprintffuncname # checks that printf-like functions are named with f at the end + - gosec # inspects code for security problems + - gosimple # linter that specializes in simplifying a code + - govet # reports suspicious constructs, ex. Printf calls whose arguments don't align with the format string + - ineffassign # detects when assignments to existing variables aren't used + - makezero # finds slice declarations with non-zero initial length + - misspell # finds commonly misspelled English words in comments + - nakedret # finds naked returns in functions greater than a specified function length + - nilerr # finds the code that returns nil even if it checks that the error is not nil + - noctx # noctx finds sending http request without context.Context + - nolintlint # reports ill-formed or insufficient nolint directives + - revive # linter for go + - staticcheck # applies static analysis checks, go vet on steroids + - structcheck # finds unused struct fields + - stylecheck # replacement for golint + - tenv # analyzer that detects using os.Setenv instead of t.Setenv since Go1.17 + - typecheck # parses and type-checks go code, like the front-end of a go compiler + - unconvert # remove unnecessary type conversions + - unparam # reports unused function parameters + - unused # checks for unused constants, variables, functions and types + - varcheck # finds unused global variables and constants + - whitespace # detects leading and trailing whitespace + - wsl # forces code to use empty lines + # static list of linters we know golangci can run but we've # chosen to leave disabled for now - # - asciicheck - # - depguard - # - dogsled - # - exhaustive - # - gochecknoinits - # - gochecknoglobals - # - gocognit - # - gocritic - # - godox - # - goerr113 - # - interfacer - # - nestif - # - noctx - # - prealloc - # - rowserrcheck - # - scopelint - # - testpackage - # - wsl + # - asciicheck - non-critical + # - cyclop - unused complexity metric + # - depguard - unused + # - dogsled - blanks allowed + # - durationcheck - unused + # - errname - unused + # - exhaustive - unused + # - exhaustivestruct - style preference + # - forbidigo - unused + # - forcetypeassert - unused + # - gci - use goimports + # - gochecknoinits - unused + # - gochecknoglobals - global variables allowed + # - gocognit - unused complexity metric + # - gocritic - style preference + # - godox - to be used in the future + # - goerr113 - to be used in the future + # - golint - archived, replaced with revive + # - gofumpt - use gofmt + # - gomnd - get too many false-positives + # - gomodguard - unused + # - ifshort - use both styles + # - ireturn - allow interfaces to be returned + # - importas - want flexibility with naming + # - lll - not too concerned about line length + # - interfacer - archived + # - nestif - non-critical + # - nilnil - style preference + # - nlreturn - style preference + # - maligned - archived, replaced with govet 'fieldalignment' + # - paralleltest - false-positives + # - prealloc - don't use + # - predeclared - unused + # - promlinter - style preference + # - rowserrcheck - unused + # - scopelint - deprecated - replaced with exportloopref + # - sqlclosecheck - unused + # - tagliatelle - use a mix of variable naming + # - testpackage - don't use this style of testing + # - thelper - false-positives + # - varnamelen - unused + # - wastedassign - duplicate functionality + # - wrapcheck - style preference # This section provides the configuration for how golangci # will report the issues it finds. @@ -125,6 +157,3 @@ issues: - funlen - goconst - gocyclo - - gomnd - - lll - - revive diff --git a/cmd/vela-scp/main.go b/cmd/vela-scp/main.go index 83144ff..564d002 100644 --- a/cmd/vela-scp/main.go +++ b/cmd/vela-scp/main.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + package main import ( diff --git a/cmd/vela-ssh/main.go b/cmd/vela-ssh/main.go index ce9c760..7cf81c8 100644 --- a/cmd/vela-ssh/main.go +++ b/cmd/vela-ssh/main.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + package main import ( diff --git a/internal/openssh/openssh.go b/internal/openssh/openssh.go index c4092ea..ac0dd4c 100644 --- a/internal/openssh/openssh.go +++ b/internal/openssh/openssh.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + package openssh import ( @@ -86,5 +90,6 @@ func CreateRestrictedFile(fs afero.Fs, fileprefix string, contents string) (stri if err := fs.Chmod(file.Name(), TempFilePermissions); err != nil { return "", fmt.Errorf("couldn't set file permissions: %w", err) } + return file.Name(), nil } diff --git a/internal/scp/scp.go b/internal/scp/scp.go index 49c050f..95d5e84 100644 --- a/internal/scp/scp.go +++ b/internal/scp/scp.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + // This package wraps the scp portion of the OpenSSH binaries to allow // for copying files to/from remote and local file systems. @@ -116,12 +120,15 @@ func (c *Config) Setup() error { tempSCPPath := fmt.Sprintf("%s/scp", path) tempSSHPath := fmt.Sprintf("%s/ssh", path) tempSSHPassPath := fmt.Sprintf("%s/sshpass", path) + if ok, _ := afero.Exists(c.fs, tempSCPPath); ok && len(c.locationSCPbinary) == 0 { c.locationSCPbinary = tempSCPPath } + if ok, _ := afero.Exists(c.fs, tempSSHPath); ok && len(c.locationSSHbinary) == 0 { c.locationSSHbinary = tempSSHPath } + if ok, _ := afero.Exists(c.fs, tempSSHPassPath); ok && len(c.locationSSHPASSbinary) == 0 { c.locationSSHPASSbinary = tempSSHPassPath } @@ -144,6 +151,7 @@ func (c *Config) Setup() error { if err != nil { return err } + c.IdentityFilePath = append([]string{filename}, c.IdentityFilePath...) } @@ -152,6 +160,7 @@ func (c *Config) Setup() error { if err != nil { return err } + c.locationPasswordFile = filename } @@ -160,6 +169,7 @@ func (c *Config) Setup() error { if err != nil { return err } + c.locationPassphraseFile = filename } diff --git a/internal/scp/scp_test.go b/internal/scp/scp_test.go index d42beab..951e2f9 100644 --- a/internal/scp/scp_test.go +++ b/internal/scp/scp_test.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + package scp import ( @@ -80,7 +84,7 @@ func TestValidateErrors(t *testing.T) { t.Run(name, func(t *testing.T) { if err := test.config.Validate(); err == nil { t.Errorf("Validate() should have raised an error") - } else if (test.wantErr != nil) && (err != test.wantErr) { + } else if (test.wantErr != nil) && !errors.Is(err, test.wantErr) { t.Errorf("Validate() returned wrong error\ngot: %s\nwanted: %s", err, test.wantErr) } }) diff --git a/internal/ssh/ssh.go b/internal/ssh/ssh.go index 5f681d1..1b03e03 100644 --- a/internal/ssh/ssh.go +++ b/internal/ssh/ssh.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + // This package wraps the ssh portion of the OpenSSH binaries to allow // for executing scripts and commands on remote systems. @@ -113,9 +117,11 @@ func (c *Config) Setup() error { for _, path := range openssh.BinSearchLocations { tempSSHPath := fmt.Sprintf("%s/ssh", path) tempSSHPassPath := fmt.Sprintf("%s/sshpass", path) + if ok, _ := afero.Exists(c.fs, tempSSHPath); ok && len(c.locationSSHbinary) == 0 { c.locationSSHbinary = tempSSHPath } + if ok, _ := afero.Exists(c.fs, tempSSHPassPath); ok && len(c.locationSSHPASSbinary) == 0 { c.locationSSHPASSbinary = tempSSHPassPath } @@ -134,6 +140,7 @@ func (c *Config) Setup() error { if err != nil { return err } + c.IdentityFilePath = append([]string{filename}, c.IdentityFilePath...) } @@ -142,6 +149,7 @@ func (c *Config) Setup() error { if err != nil { return err } + c.locationPasswordFile = filename } @@ -150,6 +158,7 @@ func (c *Config) Setup() error { if err != nil { return err } + c.locationPassphraseFile = filename } diff --git a/internal/ssh/ssh_test.go b/internal/ssh/ssh_test.go index cc87ccc..6069956 100644 --- a/internal/ssh/ssh_test.go +++ b/internal/ssh/ssh_test.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + package ssh import ( @@ -80,7 +84,7 @@ func TestValidateErrors(t *testing.T) { t.Run(name, func(t *testing.T) { if err := test.config.Validate(); err == nil { t.Errorf("Validate() should have raised an error") - } else if (test.wantErr != nil) && (err != test.wantErr) { + } else if (test.wantErr != nil) && !errors.Is(err, test.wantErr) { t.Errorf("Validate() returned wrong error\ngot: %s\nwanted: %s", err, test.wantErr) } }) diff --git a/internal/testutils/testutils.go b/internal/testutils/testutils.go index c7e277e..2314837 100644 --- a/internal/testutils/testutils.go +++ b/internal/testutils/testutils.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + package testutils import ( @@ -29,6 +33,7 @@ func CreateMockFiles(t *testing.T, filename ...string) afero.Fs { t.FailNow() } } + return fs } @@ -70,6 +75,7 @@ func FlattenArguments(args ...interface{}) (flatArgs []string) { default: } } + return } diff --git a/pkg/binarywrapper/binarywrapper.go b/pkg/binarywrapper/binarywrapper.go index 2e37f8b..72442aa 100644 --- a/pkg/binarywrapper/binarywrapper.go +++ b/pkg/binarywrapper/binarywrapper.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + // Package binarywrapper is a utility package that makes wrapping binaries a little easier // as it aims to provide a common structure to use for converting binaries // into plugins. Along the way it allows for some setup tasks, validation, @@ -107,6 +111,7 @@ func (p *Plugin) Exec() error { // here to NOT expand the arguments with environmental variables yet // as those might contain secrets or other information we don't want to leak. pluginArguments := p.Arguments() + logrus.WithFields(logrus.Fields{ "binary": p.Binary(), "arguments": p.Arguments(), @@ -152,15 +157,18 @@ func (p *Plugin) Exec() error { if outBuffer.Len() > 0 { logrus.Info(outBuffer.String()) } + if errorBuffer.Len() > 0 { logrus.Error(errorBuffer.String()) } + return fmt.Errorf("%w: %s", ErrExec, err) } if outBuffer.Len() > 0 { logrus.Info(outBuffer.String()) } + if errorBuffer.Len() > 0 { logrus.Error(errorBuffer.String()) } diff --git a/pkg/binarywrapper/binarywrapper_test.go b/pkg/binarywrapper/binarywrapper_test.go index 0e14f8a..f0e02a6 100644 --- a/pkg/binarywrapper/binarywrapper_test.go +++ b/pkg/binarywrapper/binarywrapper_test.go @@ -1,3 +1,7 @@ +// Copyright (c) 2022 Target Brands, Inc. All rights reserved. +// +// Use of this source code is governed by the LICENSE file in this repository. + // binarywrapper_test was chosen to be blackbox testing of the wrapper // to try and better understand the behavior that end consumers of the wrapper // might experience as they're using this. That, and there isn't much "internal" @@ -32,12 +36,14 @@ func TestMain(m *testing.M) { if strings.Contains(strings.Join(os.Args, ""), "$") { os.Exit(1) } + os.Exit(0) case testMainSuccessOutput: if len(os.Args) != 4 { fmt.Printf("invalid os.Args: %s", strings.Join(os.Args, " ")) os.Exit(2) } + fmt.Println(os.Args[2]) fmt.Fprint(os.Stderr, os.Args[3]) os.Exit(0) @@ -46,6 +52,7 @@ func TestMain(m *testing.M) { fmt.Printf("invalid os.Args: %s", strings.Join(os.Args, " ")) os.Exit(3) } + fmt.Println(os.Args[2]) fmt.Fprint(os.Stderr, os.Args[3]) os.Exit(4) @@ -84,6 +91,7 @@ func (m *mockExecConfig) Arguments() []string { if len(m.arguments) > 0 { return m.arguments } + return []string{} } @@ -91,6 +99,7 @@ func (m *mockExecConfig) Environment() map[string]string { if m.environment != nil { return m.environment } + return map[string]string{} }