Add template skeletons #967

orenbm · 2019-10-24T21:29:49Z

What does this PR do (include background context, if relevant)?

Adds a template connector skeleton. It includes all the code that needs to be in a new connector with guidance to fill the rest

What ticket does this PR close?

Connected to #1211

izgeri · 2019-10-24T21:37:17Z

@orenbm it would be great if the connector plugin README referred to these templates for people who want to build a tcp or http connector

I'm also interested in feedback from the team about the location of the templates, as it could appear to someone that they are actual connectors based on their current location.

jonahx · 2019-10-24T21:41:45Z

test/template_connector/docker-compose.yml

+
+  test:
+    image: secretless-dev # this image is built in bin/build
+    command: go test -v ./test/template_connector


This current standard is to have a bash ./test file. Using go test would be an optional choice each connector integration folder could make.

Perhaps this would be a better standard, but it may not be flexible enough. In any case, that's a discussion we should have in the channel.

jonahx · 2019-10-24T21:42:51Z

test/template_connector/start

+
+./stop
+
+docker-compose up -d secretless


Add comment that they would add the other containers to start. Or maybe we just do docker-compose up to start them all?

i have this comment after the line above:
# This is where you should prepare the container of the platform you are connecting to secretless

you think it's not clear enough?

@orenbm I'd move the comment above

jonahx · 2019-10-24T21:43:46Z

test/template_connector/template_connector_test.go

+	_ "github.com/denisenkom/go-mssqldb"
+)
+
+func TestTemplateConnector(t *testing.T) {


Add comment to change name

suefran

Looks good from my perspective! Just a few edits. Thanks!

suefran · 2019-10-24T21:47:22Z

internal/plugin/connectors/http/template/plugin.go

+	}
+}
+
+// NewConnector returns an http.Connector that decorates each incoming http


.......that decorates each incoming HTTP......

suefran · 2019-10-24T21:49:40Z

pkg/secretless/plugin/sharedobj/internal_plugins.go

@@ -16,6 +16,7 @@ type InternalPluginLookupFunc func() (plugin.AvailablePlugins, error)

 // GetInternalPluginsFunc returns currently available built-in plugins.
 func GetInternalPluginsFunc() (plugin.AvailablePlugins, error) {
+	// New connectors should have an entry in the map below, according to their type (http/tcp)


we've been trying to standardize on HTTP and TCP when referring to them outside of a command that requires specific syntax.

suefran · 2019-10-24T21:51:30Z

test/template_connector/start

+
+docker-compose up -d secretless
+
+# This is where you should prepare the container of the platform you are connecting to secretless


add a period.

suefran · 2019-10-24T21:52:04Z

test/template_connector/template_connector_test.go

+
+func TestTemplateConnector(t *testing.T) {
+	t.Run("An empty test", func(t *testing.T) {
+		// We use go std test lib & testify/assert as our standard testing lib on secretless


add period.

sgnn7

@orenbm Some comments - mostly nits. Per comments from @izgeri I also believe that these template connectors may need a better place to live or be turned into "real example" plugins that are just pass-throughs as well as examples

sgnn7 · 2019-10-25T14:04:26Z

internal/plugin/connectors/http/template/connector.go

+	"github.com/cyberark/secretless-broker/pkg/secretless/log"
+	"github.com/cyberark/secretless-broker/pkg/secretless/plugin/connector"
+
+	gohttp "net/http"


the imports should be arranged:

<stdlib libraries> <third-party libs> <internal libs>

sgnn7 · 2019-10-25T14:05:25Z

internal/plugin/connectors/http/template/connector.go

@@ -0,0 +1,24 @@
+package template


This location for the template connector isn't optimal. Generally we don't want any unused code in internal/

sgnn7 · 2019-10-25T14:06:20Z

internal/plugin/connectors/http/template/connector.go

+	// TODO: add logic according to
+	//  https://github.com/cyberark/secretless-broker/blob/master/pkg/secretless/plugin/connector/README.md#http-connector
+
+	return nil


If you are really trying to create an inclusive example, giving the error a variable would be better:

var err error return err

sgnn7 · 2019-10-25T14:10:03Z

internal/plugin/connectors/http/template/plugin.go

+	// https://github.com/cyberark/secretless-broker/blob/master/pkg/secretless/plugin/connector/README.md#plugininfo
+	return map[string]string{
+		"pluginAPIVersion": "",
+		"type":             "",


Type prefix for http plugins is standardized: https://github.com/cyberark/secretless-broker/blob/master/internal/plugin/connectors/http/basicauth/plugin.go#L13

sgnn7 · 2019-10-25T14:10:52Z

internal/plugin/connectors/tcp/template/connector.go

+	// TODO: add logic according to
+	//  https://github.com/cyberark/secretless-broker/blob/master/pkg/secretless/plugin/connector/README.md#tcp-connector
+
+	return nil, nil


This skeleton should have a bit more stuff probably (it should connect to the backend and return a net.Conn).

how can i connect to a backend without knowing what it is?
Or do you think I should add it as a comment?

whenever i linked to a doc i didn't add more details so it won't be duplicated. wdyt? @sgnn7

You can add an example of it trying to parse "host" and "port" from credentialValuesByID

i couldn't find a generic way to write that. I updated the function description to better describe its process

sgnn7 · 2019-10-25T14:11:53Z

internal/plugin/connectors/tcp/template/plugin.go

+	// https://github.com/cyberark/secretless-broker/blob/master/pkg/secretless/plugin/connector/README.md#plugininfo
+	return map[string]string{
+		"pluginAPIVersion": "",
+		"type":             "",


This is standardized for tcp connectors: https://github.com/cyberark/secretless-broker/blob/master/internal/plugin/connectors/tcp/pg/plugin.go#L35

sgnn7 · 2019-10-25T14:12:57Z

test/template_connector/docker-compose.yml

+version: '3.0'
+
+services:
+  # TODO: add a service for the platform you want to connect with secretless


The wording here is odd - we don't want to connect a service to secretless but instead connect secretless to a service

sgnn7 · 2019-10-25T14:13:14Z

test/template_connector/docker-compose.yml

+  # TODO: add a service for the platform you want to connect with secretless
+
+  secretless:
+    image: secretless-broker # this image is built in bin/build


s/built in/built by/

sgnn7 · 2019-10-25T14:13:19Z

test/template_connector/docker-compose.yml

+      - ./secretless.yml:/secretless.yml
+
+  test:
+    image: secretless-dev # this image is built in bin/build


s/built in/built by/

sgnn7 · 2019-10-25T14:13:54Z

test/template_connector/secretless.yml

@@ -0,0 +1 @@
+# fill in according to https://docs.secretless.io/Latest/en/Content/Get%20Started/configuration.htm


I'm guessing this should have at least some starter skeleton config

orenbm · 2019-10-25T18:54:46Z

@izgeri @sgnn7 @jonahx I restructured the templates and added a README. please review :)

orenbm · 2019-10-25T18:59:52Z

@orenbm it would be great if the connector plugin README referred to these templates for people who want to build a tcp or http connector

sure, once these changes are merged i will link them in the README

I'm also interested in feedback from the team about the location of the templates, as it could appear to someone that they are actual connectors based on their current location.

I just rearranged them according to the feedback. Hope it's clearer now.

jonahx

Nice. I approved but noticed some other people had commented, so you may want to check with them before merging.

jonahx · 2019-10-26T13:54:22Z

templates/http/template/connector.go

+}
+
+// This function has access to the client http.Request and the credentials (as a map), and is expected to decorate the
+// request with Authorization headers.


line too long: wrap at 80 (you can set a hotkey in GoLand to make this easier)

Also, I'd say:

and is expected to modify the request so that it will authenticate. This typically means adding required authorization headers.

because some auth systems require other types of modifications, and in theory there is no limit on what they might require.

izgeri · 2019-10-28T15:17:03Z

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/cyberark/conjur-api-go v0.5.2
 	github.com/cyberark/conjur-authn-k8s-client v0.13.0
 	github.com/cyberark/summon v0.7.0
+	github.com/denisenkom/go-mssqldb v0.0.0-20191001013358-cfbb681360f0


the changes to go.* are unrelated to this code, afaict. do they need to be included in this PR?

izgeri · 2019-10-28T15:18:07Z

templates/README.md

+
+To add a new connector do the following:
+
+1. Copy the relevant template directory (HTTP/TCP) into `internal/plugin/connectors/<protocol>`. 


is protocol the language we're using for http vs tcp? it would be good to use consistent language to refer to this - I think I've seen it elsewhere as connector type

i didn't find something consistent. I see "handler type", "connection type", "config type"
I guess "connector type" makes sense

connector type 👍

izgeri · 2019-10-28T15:18:35Z

templates/README.md

+
+1. Copy the relevant template directory (HTTP/TCP) into `internal/plugin/connectors/<protocol>`. 
+    1. Inside each template directory you will find the required files & structs implemented, with
+instructions to fill them with the content of the new connector


are the instructions in the form of TODOs or otherwise? would be good to be clear about how to find the instructions here

izgeri · 2019-10-28T15:18:52Z

templates/README.md

+1. Copy the relevant template directory (HTTP/TCP) into `internal/plugin/connectors/<protocol>`. 
+    1. Inside each template directory you will find the required files & structs implemented, with
+instructions to fill them with the content of the new connector
+1.  Add an entry to the `Plugins` map defined in GetInternalPluginsFunc() of `internal_plugins.go`, 


this should be a link to a file, so the user doesn't have to search the code for this

izgeri · 2019-10-28T15:19:10Z

templates/README.md

+instructions to fill them with the content of the new connector
+1.  Add an entry to the `Plugins` map defined in GetInternalPluginsFunc() of `internal_plugins.go`, 
+according to their type (HTTP/TCP)
+1. Copy the `template_connector_test` directory into `test` and rename it to `<connector_name>_connector`


can you link to template_connector_test here?

izgeri · 2019-10-28T15:21:13Z

templates/README.md

@@ -0,0 +1,28 @@
+# Using templates to implement Secretless Connector Plugins


maybe this content should be moved to examples/connector_templates instead of putting it in templates? I'm basing this suggestion on the go project layout standards

izgeri

I left several comments for things that should be revised before we merge.

sgnn7

@orenbm Just one tiny nit from me

sgnn7 · 2019-10-28T15:59:25Z

templates/README.md

+ scripts
+ using
+   the `run_integration` script. In most cases, you will also call `junit` on
+   the xml file that `run_integration` outputs in your test's subdirectory.


This paragraph has some odd whitespace formatting/line breaks

fixed all requests

izgeri · 2019-10-28T18:09:17Z

examples/connector_templates/README.md

+
+To add a new connector do the following:
+
+1. Copy the relevant template directory (HTTP/TCP) into `internal/plugin/connectors/<connector type>`. 


how do you know which one to copy? is it worth referring to the connector docs to learn more if you're not sure which one you need to copy?

@izgeri is this line ok?

If you're not sure which connector type is suitable, please refer to the [connector technical overview](https://github.com/cyberark/secretless-broker/tree/master/pkg/secretless/plugin/connector#technical-overview)

izgeri

Added one more minor comment, address if you think it makes sense. Otherwise LGTM.

In the current structure it seems as if the template connector is an actual connector. This commit moves all the templates to a `examples/connector_templates` directory, with a README inside explaining what needs to be done

Add http & tcp connectors template code

6f15b71

orenbm requested a review from jonahx October 24, 2019 21:29

izgeri assigned orenbm Oct 24, 2019

izgeri requested a review from suefran October 24, 2019 21:35

orenbm force-pushed the add-template-skeletons branch from 8f618cd to 1b37d19 Compare October 24, 2019 21:36

jonahx reviewed Oct 24, 2019

View reviewed changes

suefran previously requested changes Oct 24, 2019

View reviewed changes

sgnn7 suggested changes Oct 25, 2019

View reviewed changes

izgeri mentioned this pull request Oct 25, 2019

Add a template connector skeleton #966

Closed

jonahx previously approved these changes Oct 26, 2019

View reviewed changes

orenbm dismissed jonahx’s stale review via 4a0d6be October 28, 2019 15:13

orenbm force-pushed the add-template-skeletons branch from 9d5bebb to 4a0d6be Compare October 28, 2019 15:13

izgeri reviewed Oct 28, 2019

View reviewed changes

izgeri suggested changes Oct 28, 2019

View reviewed changes

orenbm force-pushed the add-template-skeletons branch 4 times, most recently from 7f6bedc to 17706c2 Compare October 28, 2019 15:49

sgnn7 suggested changes Oct 28, 2019

View reviewed changes

orenbm force-pushed the add-template-skeletons branch 2 times, most recently from a3c3616 to 8a7c10a Compare October 28, 2019 16:36

Add template test folder

7ce640a

Implemented comments from PR

c4efa40

orenbm force-pushed the add-template-skeletons branch 2 times, most recently from 7436c4f to d8326b6 Compare October 28, 2019 16:47

sgnn7 previously approved these changes Oct 28, 2019

View reviewed changes

orenbm requested a review from izgeri October 28, 2019 18:05

izgeri reviewed Oct 28, 2019

View reviewed changes

izgeri previously approved these changes Oct 28, 2019

View reviewed changes

Restructure template files

80e322c

In the current structure it seems as if the template connector is an actual connector. This commit moves all the templates to a `examples/connector_templates` directory, with a README inside explaining what needs to be done

orenbm dismissed stale reviews from izgeri and sgnn7 via 80e322c October 28, 2019 18:19

orenbm force-pushed the add-template-skeletons branch from d8326b6 to 80e322c Compare October 28, 2019 18:19

izgeri approved these changes Oct 28, 2019

View reviewed changes

orenbm merged commit d6323b2 into master Oct 28, 2019

orenbm deleted the add-template-skeletons branch October 28, 2019 20:37


		docker-compose up -d secretless

		# This is where you should prepare the container of the platform you are connecting to secretless

		@@ -0,0 +1 @@
		# fill in according to https://docs.secretless.io/Latest/en/Content/Get%20Started/configuration.htm


		To add a new connector do the following:

		1. Copy the relevant template directory (HTTP/TCP) into `internal/plugin/connectors/<protocol>`.

		@@ -0,0 +1,28 @@
		# Using templates to implement Secretless Connector Plugins

Add template skeletons #967

Add template skeletons #967

Conversation

orenbm commented Oct 24, 2019

What does this PR do (include background context, if relevant)?

What ticket does this PR close?

izgeri commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suefran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgnn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm commented Oct 25, 2019

orenbm commented Oct 25, 2019

jonahx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgeri left a comment

Choose a reason for hiding this comment

sgnn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgeri left a comment

Choose a reason for hiding this comment