adding initial functionality for scp and ssh plugins #1

GregoryDosh · 2022-01-19T16:44:53Z

This contains the initial logic for the SCP & SSH plugins. These are a rewrite to wrap the respective scp and ssh binaries in a thin plugin initialization app so that folks can generally use these in their pipelines as they would from the command line.

The parameter names and formats were to be as close as possible to the OpenSSH usage manuals so that it is easier to crossreference what each parameter is or does. There are a few things that the plugin takes care of such as taking the contents of a private identity file and placing them in the plugin container and changing the permissions so that the ssh and scp binaries can use them during the plugin execution.

E.g. scp -i ~/.ssh/id_rsa file1 file2 remote-host:/some/location would look like a plugin use of

steps:
  - name: scp to some server
    image: target/vela-scp
    secrets: [ identity_file_contents ]
    parameters:
      source:
      - file1
      - file2
      target: remote-host:/some/location

And for an ssh action of ssh ~/.ssh/id_rsa some-user@some-host some-command it would look like

steps:
  - name: ssh to some server
    image: target/vela-ssh
    secrets: [ identity_file_contents ]
    parameters:
      destination: some-user@some-host
      command:
      - echo This is in a command block which is maybe confusing to folks about the commands block so it also is aliased with script if needed for doc purposes.

internal/openssh/openssh.go

codecov · 2022-01-19T16:45:51Z

Codecov Report

❗ No coverage uploaded for pull request base (main@8de474d). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #1   +/-   ##
=======================================
  Coverage        ?   93.40%           
=======================================
  Files           ?        3           
  Lines           ?      288           
  Branches        ?        0           
=======================================
  Hits            ?      269           
  Misses          ?       13           
  Partials        ?        6

add integration tests

kaymckay

Looks good! holy testing 🤯

.github/CODE_OF_CONDUCT.md

Makefile

kaymckay

lgtm 👍

GregoryDosh · 2022-02-02T19:03:12Z

For others looking at this PR in the future. There was also some feedback via the Gopher Slack channel that had a bit of feedback.

wass3r

first pass - thanks for all the work. love the thorough commenting and testing! see inline comments for more.

wass3r · 2022-02-09T17:46:51Z

internal/openssh/openssh.go

+	// would be in its own package somewhere so it wasn't tied to any particular plugin.
+	// If this gets changed in the future, be sure to update the Makefile references
+	// for the build-time injection of the Git Tag, Commit, etc.
+	"github.com/go-vela/vela-kaniko/version"


agree.. a vela-plugin-utils-go would do nicely - i know there's been conversation elsewhere on this. it doesn't feel right to depend on a "random" package's version implementation. if we move forward with this for now, i'd vote to create a story (if it doesn't exist) to create this new repo and update this and other plugins.

Absolutely agree and I don't mind taking on the creation and movement and orchestration of all the other repos to get them pointing at this updated utils if we go that route. For now it was just to move forward with something and highlight an area of potential future improvement.

wass3r · 2022-02-09T17:52:01Z

pkg/binarywrapper/binarywrapper.go

+
+	// Log some good debugging information here. There is a purposeful choice
+	// here to NOT expand the arguments with environmental variables yet
+	// as those might contain secrets or other information we don't want to leak.


internal/scp/scp.go

internal/ssh/ssh.go

internal/scp/scp.go

internal/ssh/ssh.go

wass3r · 2022-02-09T22:02:33Z

pkg/binarywrapper/binarywrapper.go

+// 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,
+// and execution.
+package binarywrapper


can definitely see this as part of something like vela-plugin-utils-go like version

wass3r · 2022-02-09T22:13:25Z

pkg/binarywrapper/binarywrapper_test.go

+		"formats arguments with env vars before executing": {
+			execStyle: binarywrapper.OSExecCommand,
+			config: &mockExecConfig{
+				binaryPath: os.Args[0],
+				arguments:  []string{"$SOME_TEST"},
+				environment: map[string]string{
+					"GO_MAIN_TEST_CASE": testMainEnvVar,
+					"SOME_TEST":         "Howdy!",
+				},
+			},
+		},


i'm more familiar with having the first field in the struct be name and then using that for t.Run (eg: https://github.com/go-vela/server/blob/master/secret/vault/delete_test.go#L38-L51) instead of using a map. that said, we definitely have table tests without any naming, so this better than that. however, now we have a total of three styles: no name, name struct field, and this map approach. we should probably standardize.

I'm not hard on any stance other than the fact that named tests are easier to debug when they fail. I've been leaning and experimenting more with map[string]struct style tests because of some of the points made on this article talking about table drive tests. Map iteration is randomized each passthrough so if there is any reliance between tests it should come out in repeated tests. Doesn't matter to me much either way though and can refactor and fix things here along the way.

that's fair and hard to argue against. seems like a safe(r) approach, probably. something we should probably adopt across the board and somehow document for contributors as well?

plyr4

lots of code, so the initial pass LGTM. I checked for target IP + obvious issues.

initial copy & paste from GregoryDosh/vela-openssh

2a3712d

github-actions bot reviewed Jan 19, 2022

View reviewed changes

internal/openssh/openssh.go Outdated Show resolved Hide resolved

GregoryDosh and others added 9 commits January 19, 2022 10:52

fix: move octal permissions magic number to constant

c6dfdfb

feat: add integration tests

8a4dc3b

fix: remove unneeded uses

fb40e5d

fix: move integration test to new step

2cd241a

ci: force break CI to ensure actions run correctly

929594a

test: move integration tests into script from Makefile

acad3b5

test: use docker-compose run instead of 'up -d'

95c82ce

fix: use correct password to make sure CI integration tests pass

bd23676

Merge pull request #2 from go-vela/integration-test

600fa07

add integration tests

kaymckay reviewed Jan 24, 2022

View reviewed changes

.github/CODE_OF_CONDUCT.md Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

kaymckay and others added 5 commits January 24, 2022 13:50

Merge branch 'main' into ready-for-pr

63f68ca

fix: update opensource email

a9c850f

fix(cmd): remove VELA prefix from some env vars

4a5860c

ci: add additional integration tests and make testing easier

aff1da4

docs: add initial usage docs

568557b

GregoryDosh changed the title ~~initial copy & paste from GregoryDosh/vela-openssh~~ adding initial functionality for scp and ssh plugins Feb 1, 2022

docs: fix typo in secret ref

a623f45

kaymckay previously approved these changes Feb 2, 2022

View reviewed changes

wass3r requested changes Feb 9, 2022

View reviewed changes

GregoryDosh added 2 commits February 11, 2022 08:59

docs: add missing comments

ba970df

fix(sshpass): change passphrase detection to 'assphrase'

96880e8

GregoryDosh dismissed kaymckay’s stale review via 96880e8 February 11, 2022 15:00

fix(sshpass): change passphrase detection to 'assphrase'

e91bce8

plyr4 approved these changes Feb 14, 2022

View reviewed changes

wass3r approved these changes Feb 14, 2022

View reviewed changes

GregoryDosh merged commit 72d2d6e into main Feb 15, 2022

GregoryDosh deleted the ready-for-pr branch February 15, 2022 14:39

adding initial functionality for scp and ssh plugins #1

adding initial functionality for scp and ssh plugins #1

Uh oh!

Conversation

GregoryDosh commented Jan 19, 2022

Uh oh!

Uh oh!

codecov bot commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kaymckay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaymckay left a comment

Choose a reason for hiding this comment

Uh oh!

GregoryDosh commented Feb 2, 2022

Uh oh!

wass3r left a comment

Choose a reason for hiding this comment

Uh oh!

wass3r Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

GregoryDosh Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wass3r Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wass3r Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

wass3r Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

GregoryDosh Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

wass3r Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

plyr4 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jan 19, 2022 •

edited

Loading

GregoryDosh Feb 9, 2022 •

edited

Loading