Succeeded: Change GetValue to GetValues for Providers to allow batch retrieval #1344

doodlesbykumbi · 2020-09-24T12:21:34Z

GetValues works exactly the same as GetValue, however GetValues gives providers the ability to implement arbitrary batch retrieval mechanisms.

Succeeded by #1356 (Cobertura got in the way)

What does this PR do?

What's changed? Why were these changes made?
How should the reviewer approach this PR, especially if manual tests are required?
Are there relevant screenshots you can add to the PR description?

What ticket does this PR close?

Connected to #[relevant GitHub issues, eg 76]

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

This PR does not require updating any documentation, or
Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs

(For releases only) Manual tests

Manually tested Keychain provider
Manually run Kubernetes-Conjur demo with a local Secretless Broker image build of your branch

internal/plugin/resolver.go

internal/providers/awssecrets/provider.go

internal/providers/conjur/provider.go

internal/providers/env/provider.go

internal/providers/file/provider.go

sgnn7

@doodlesbykumbi Great progress forward with some mostly-small comments. I would like to see multi-value fetch tests for each provider though in the e2e tests too.

k8s-ci/k8s_crds/test

internal/summon/command/run.go

internal/providers/vault/provider.go

internal/plugin/v1/provider.go

sgnn7 · 2020-10-05T15:46:22Z

internal/plugin/resolver.go

+				continue
+			}
+		}
+		if hasErrors {


This brings an interesting question: do we care about finishing resolution of all providers if we hit an error in any provider? My guess is that a single error in fetching variables is enough to throw an error but I'd like to hear your thoughts on this.

I think the developer UX of getting all the useful information about errors would be better here. There's no added cost because secret fetching is delegated to the provider and it is at the provider's discretion to fail fast or not. If we have the additional error info I think we ought to show it.

internal/plugin/resolver.go

test/providers/vault/vault_provider_test.go

test/connector/http/conjur/conjur_provider_test.go

internal/plugin/v1/testutils/testutils.go

sgnn7

@doodlesbykumbi Just a few more comments but it's almost there!

sgnn7 · 2020-10-13T14:32:33Z

go.sum

@@ -29,6 +29,8 @@ github.com/containerd/containerd v1.3.2 h1:ForxmXkA6tPIvffbrDAcPUIB32QgXkt2XFj+F
 github.com/containerd/containerd v1.3.2/go.mod h1:bC6axHOhabU15QhwfG7w5PipXdVtMXFTttgp+kVtyUA=
 github.com/cyberark/conjur-api-go v0.5.2 h1:8ntk07YNRz5bBwjNXkDEAPR70Yr+J2MN8NGlkhaMC3k=
 github.com/cyberark/conjur-api-go v0.5.2/go.mod h1:hwaReWirzgKor+JtH6vbwZaASDXulvd0SzGCloC5uOc=
+github.com/cyberark/conjur-authn-k8s-client v0.13.0/go.mod h1:JTeGIeRO59J7mMEc5yF6FPtk1QnaAzs4GyZa4WldqZc=


Hmmm.. why do we have both v0.13.0 and v0.19.0 of authn-k8s module here?

sgnn7 · 2020-10-13T14:32:49Z

go.sum

@@ -244,6 +246,7 @@ github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
+go.mongodb.org/mongo-driver v1.4.2 h1:WlnEglfTg/PfPq4WXs2Vkl/5ICC6hoG8+r+LraPmGk4=


I think this is leftover from your other PR

Definitely. Taken out

internal/plugin/resolver.go

internal/plugin/v1/provider.go

internal/summon/command/run.go

k8s-ci/k8s_crds/test

internal/plugin/v1/testutils/testutils.go

sgnn7

@doodlesbykumbi Great work! There's only a few more things in the new code added that need a bit of attention.

internal/summon/command/temp_factory.go

sgnn7 · 2020-10-15T15:21:02Z

internal/summon/command/run.go

 	}
-	return
+
+	if atLeastOneVar := len(varSecretsSpecPaths) > 0; !atLeastOneVar {


Not a blocker but this could have been simpler with just the condition:

# If there are no variables to resolve, return what we have if len(varSecretsSpecPaths) == 0 {

sgnn7 · 2020-10-15T15:25:51Z

internal/summon/command/temp_factory.go

 	}
-	tf = nil


Two things here:

If we're not clearing the object (which I agree is weird), we need to at least reset tf.files[] to an empty array so that repeated invocation won't try to delete already-deleted files.

If we're not clearing the object here too, it is a bit problematic that we remove the directory in cleanup since it allows for calling Push() again but the directory won't be there.

There is no such thing as clearing an object in Go as far as I know. But tf.files = nil is a worthwhile thing.

tf.files = nil is a worthwhile thing.

This may need to be a new empty array though instead of nil

sgnn7 · 2020-10-15T15:26:13Z

internal/summon/command/temp_factory_test.go

+		So(err, ShouldNotBeNil)
+		So(err.Error(), ShouldContainSubstring, "no such file or directory")
+	})
+


nit: Stray newline

internal/plugin/resolver.go

BradleyBoutcher · 2020-10-15T18:38:35Z

internal/plugin/resolver.go

+		// Sort the error strings to provide deterministic output
+		sort.Strings(errorStrings)
+
+		err = fmt.Errorf(strings.Join(errorStrings, "; "))


What will this look like printed out? Should we separate with newlines as well?

Here's an example,ERROR: Resolving credentials from provider 'env' failed: env cannot find environment variable 'something-also-not-in-env', env cannot find environment variable 'something-not-in-env'; ERROR: Resolving credentials from provider 'file' failed: open something-not-on-file: no such file or directory. Hmm maybe newlines are a better option

BradleyBoutcher

Nit: Don't forget to update the changelog and any applicable README's!

GetValues works exactly the same as GetValue, however GetValues gives providers the ability to implement arbitrary batch retrieval mechanisms.

This return type provides Value and Error for each variable, and simplifies mapping from input ids to response without relying on order

codeclimate · 2020-10-16T20:59:42Z

Code Climate has analyzed commit 26246a8 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 90.9% (50% is the threshold).

This pull request will bring the total coverage in the repository to 53.0% (2.7% change).

View more on Code Climate.

doodlesbykumbi · 2020-10-16T20:59:56Z

@BradleyBoutcher re:

Nit: Don't forget to update the changelog and any applicable README's!.

This is entirely an internal change so I think neither of these are needed

doodlesbykumbi · 2020-10-19T15:10:00Z

Succeeded by #1356 (Cobertura got in the way)

codeclimate bot reviewed Sep 24, 2020

View reviewed changes

doodlesbykumbi mentioned this pull request Sep 24, 2020

Support multi-value dynamic credentials #1337

Open

doodlesbykumbi force-pushed the batch-provider branch 3 times, most recently from 7fc7a42 to 683407f Compare September 24, 2020 13:57

sgnn7 suggested changes Oct 5, 2020

View reviewed changes

doodlesbykumbi force-pushed the batch-provider branch 4 times, most recently from 4d5cc41 to edb41de Compare October 13, 2020 00:46

codeclimate bot reviewed Oct 13, 2020

View reviewed changes

test/providers/vault/vault_provider_test.go Outdated Show resolved Hide resolved

test/connector/http/conjur/conjur_provider_test.go Outdated Show resolved Hide resolved

test/connector/http/conjur/conjur_provider_test.go Outdated Show resolved Hide resolved

doodlesbykumbi marked this pull request as ready for review October 13, 2020 09:59

doodlesbykumbi requested a review from a team as a code owner October 13, 2020 09:59

codeclimate bot reviewed Oct 13, 2020

View reviewed changes

internal/plugin/v1/testutils/testutils.go Outdated Show resolved Hide resolved

doodlesbykumbi force-pushed the batch-provider branch from c776af8 to a1d8733 Compare October 13, 2020 12:56

sgnn7 suggested changes Oct 13, 2020

View reviewed changes

doodlesbykumbi force-pushed the batch-provider branch 2 times, most recently from ff68070 to 4909a67 Compare October 14, 2020 14:21

izgeri mentioned this pull request Oct 14, 2020

Secretless stability tests have been run with v1.7.1 #1353

Closed

doodlesbykumbi force-pushed the batch-provider branch 2 times, most recently from 6d9302e to e9eeaf1 Compare October 15, 2020 11:41

doodlesbykumbi changed the title ~~WIP: Accommodate batch retrieval for Providers~~ Change GetValue to GetValues for Providers to allow batch retrieval Oct 15, 2020

sgnn7 suggested changes Oct 15, 2020

View reviewed changes

BradleyBoutcher reviewed Oct 15, 2020

View reviewed changes

doodlesbykumbi force-pushed the batch-provider branch 2 times, most recently from dacda87 to 7b86082 Compare October 16, 2020 19:06

doodlesbykumbi added 3 commits October 16, 2020 21:23

Update Provider interface to use GetValues instead of GetValue

291b034

GetValues works exactly the same as GetValue, however GetValues gives providers the ability to implement arbitrary batch retrieval mechanisms.

Find out why CRDs are failing

452f949

Use (map[id]{Value, Error}, error) for Provider#GetValues return type

1f23704

This return type provides Value and Error for each variable, and simplifies mapping from input ids to response without relying on order

doodlesbykumbi added 4 commits October 16, 2020 21:23

Add tests to support Provider#GetValues

a4c0e52

Add testutils to DRY up provider tests

bcf0df1

Ensure each provider can provide multiple values

6c2c459

Improve unit tests for internal/summon/command

26246a8

doodlesbykumbi force-pushed the batch-provider branch from 7b86082 to 26246a8 Compare October 16, 2020 20:58

doodlesbykumbi mentioned this pull request Oct 19, 2020

Change GetValue to GetValues for Providers to allow batch retrieval #1356

Merged

8 tasks

doodlesbykumbi changed the title ~~Change GetValue to GetValues for Providers to allow batch retrieval~~ Stale: Change GetValue to GetValues for Providers to allow batch retrieval Oct 19, 2020

doodlesbykumbi closed this Oct 19, 2020

doodlesbykumbi deleted the batch-provider branch October 19, 2020 15:10

doodlesbykumbi changed the title ~~Stale: Change GetValue to GetValues for Providers to allow batch retrieval~~ Succeeded: Change GetValue to GetValues for Providers to allow batch retrieval Oct 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Succeeded: Change GetValue to GetValues for Providers to allow batch retrieval #1344

Succeeded: Change GetValue to GetValues for Providers to allow batch retrieval #1344

doodlesbykumbi commented Sep 24, 2020 •

edited

Loading

sgnn7 left a comment

sgnn7 Oct 5, 2020

doodlesbykumbi Oct 12, 2020

sgnn7 left a comment

sgnn7 Oct 13, 2020

doodlesbykumbi Oct 15, 2020

sgnn7 Oct 13, 2020

doodlesbykumbi Oct 15, 2020

sgnn7 left a comment

sgnn7 Oct 15, 2020

sgnn7 Oct 15, 2020

doodlesbykumbi Oct 16, 2020

sgnn7 Oct 16, 2020

sgnn7 Oct 15, 2020

BradleyBoutcher Oct 15, 2020

doodlesbykumbi Oct 16, 2020

BradleyBoutcher left a comment

codeclimate bot commented Oct 16, 2020

doodlesbykumbi commented Oct 16, 2020

doodlesbykumbi commented Oct 19, 2020

Succeeded: Change GetValue to GetValues for Providers to allow batch retrieval #1344

Succeeded: Change GetValue to GetValues for Providers to allow batch retrieval #1344

Conversation

doodlesbykumbi commented Sep 24, 2020 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

(For releases only) Manual tests

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

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

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

BradleyBoutcher left a comment

Choose a reason for hiding this comment

codeclimate bot commented Oct 16, 2020

doodlesbykumbi commented Oct 16, 2020

doodlesbykumbi commented Oct 19, 2020

doodlesbykumbi commented Sep 24, 2020 •

edited

Loading