Add startup logic to enable Push-to-File #377

john-odonnell · 2021-10-22T15:36:33Z

What does this PR do?

Adds new type ProviderFunc func() error
- Represents an ambiguous style of secret provider, pushing secrets either to K8s Secrets or to File
func NewProviderForType(...) (ProviderFunc, []error)
- Returns a ProviderFunc given SP configuration
func RetryableSecretProvider(...) ProviderFunc
- Wraps a given ProviderFunc in a limitedBackOff regulated Retry loop
- Maintains the general ProviderFunc type
New provider type structs in k8s_secrets_storage and pushtofile packages
- NewProvider(...) defines dependencies required by each process
- Each provider has an attached Provide() method which Implements the ProviderFunc type
Implementation details of retrieving secrets from Conjur, specifically generating an access token, is relegated to the conjur package
The above changes are reflected in main.go

What ticket does this PR close?

ONYX-12537

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

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

pkg/secrets/provide_conjur_secrets.go

imheresamir · 2021-10-26T14:56:45Z

cmd/secrets-provider/main.go

-	if _, err := os.Stat(annotationsFile); err == nil {
-		annotationsMap, err = annotations.NewAnnotationsFromFile(annotationsFile)
+	// Only attempt to populate from annotations if the annotations file exists
+	// TODO: Figure out strategy for dealing with explicit annotation file path


Why would the annotation file path be anything other than /conjur/podinfo ? I don't think it makes sense that the file path would be user configurable...

This comment is actually from #367 by @doodlesbykumbi. GH did a weird auto-rebase on my branch, but I've corrected it since.

I think I would agree with you here, though. The location of the annotations file seems like an internal detail rather than something the user should be concerned with. I feel differently about a configurable secrets file base path, though.

Both of the fixed paths /conjur/podinfo and the /conjur/secrets are from the perspective of the Secrets Provider container only. For the application container, the user can mount the corresponding volumes at any arbitrary mountPaths (though they might not care about the podinfo volume).

We intentionally left out "config knobs" for these SP mountPaths to simplify the user experience and so that we have two less things that can get misconfigured.

With that in mind, we can probably change defaultAnnotationsFile back to annotationsFile, and defaultSecretBasePath to secretBasePath.

My motivation for making these values configurable is for

For testing purposes we should be able to specify arbitrary mouth paths.

Potentially supporting dry run capabilities. I acknowledge this as a nice-to-have, and something that might require a separate dedicated discussion.

We don't have to expose these as config knobs to the user, but I think the hardcoded value should only be present in ./main.go.
I opted for defaultAnnotationsFile vs annotationsFile to specifically call out the fact that this is the default, but perhaps that causes some confusion due to the word "default" implying that there are other options. Perhaps a better name would be fixedAnnotationsFilePath.

Changed to annotationsFilePath and secretBasePath. When they're made "configurable" in the context of testing/dry run, then we can change it to something more suitable.

pkg/secrets/pushtofile/provide_conjur_secrets_test.go

codeclimate · 2021-10-26T16:05:03Z

pkg/secrets/provide_conjur_secrets.go

-		provideConjurSecretFunc = k8s_secrets_storage.ProvideConjurSecretsToK8sSecrets
+// NewProviderForType returns a ProviderFunc responsible for retrieving and providing secrets in a given StoreType mode.
+func NewProviderForType(
+	retrievek8sSecret k8s.RetrieveK8sSecretFunc,


Function NewProviderForType has 7 arguments (exceeds 4 allowed). Consider refactoring.

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go

codeclimate · 2021-10-26T16:05:03Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go

+
+// NewProvider creates a new secret provider for K8s Secrets mode.
+func NewProvider(
+	retrievek8sSecret k8s.RetrieveK8sSecretFunc,


Function NewProvider has 5 arguments (exceeds 4 allowed). Consider refactoring.

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go

codeclimate · 2021-10-26T20:08:15Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go

+}
+
+// Provide implements a ProviderFunc to retrieve and push secrets to K8s secrets.
+func (p k8sProvider) Provide() error {


Method k8sProvider.Provide has 6 return statements (exceeds 4 allowed).

diverdane

Looks good!!! Nice work on the provider retry wrapper, and on adding testability for the retry mechanism!

diverdane · 2021-10-27T01:15:40Z

cmd/secrets-provider/main.go

-	if _, err := os.Stat(annotationsFile); err == nil {
-		annotationsMap, err = annotations.NewAnnotationsFromFile(annotationsFile)
+	// Only attempt to populate from annotations if the annotations file exists
+	// TODO: Figure out strategy for dealing with explicit annotation file path


Both of the fixed paths /conjur/podinfo and the /conjur/secrets are from the perspective of the Secrets Provider container only. For the application container, the user can mount the corresponding volumes at any arbitrary mountPaths (though they might not care about the podinfo volume).

We intentionally left out "config knobs" for these SP mountPaths to simplify the user experience and so that we have two less things that can get misconfigured.

With that in mind, we can probably change defaultAnnotationsFile back to annotationsFile, and defaultSecretBasePath to secretBasePath.

diverdane · 2021-10-27T01:17:40Z

cmd/secrets-provider/main.go

 	secretsConfig := setupSecretsConfig()

-	provideConjurSecrets, err := secrets.GetProvideConjurSecretFunc(secretsConfig.StoreType)
+	// Initialize a Conjur Secret Fetcher
+	secretRetriver, err := conjur.NewConjurSecretRetriever(*authnConfig)


Typo: secretRetriver should be secretRetriever.

diverdane · 2021-10-27T01:20:34Z

cmd/secrets-provider/main.go

+	)
+	logErrorsAndConditionalExit(errs, nil, messages.CSPFK053E)
+
+	provideSecrets = secrets.RetryableSecretProvider(


Nice, I like the RetryableSecretProvider(...) wrapper!

diverdane · 2021-10-27T01:24:15Z

pkg/secrets/clients/conjur/conjur_secrets_retriever.go

+		return nil, log.RecordedError(messages.CSPFK010E)
+	}
+
+	// NOTE: the token is cleanup up by whoever created it!


Should be "the token is cleaned up"?

Removed this comment entirely - seems repetitive given the comment a few lines later:

// Always delete the access token. The deletion is idempotent and never fails defer authn.AccessToken.Delete()

diverdane · 2021-10-27T01:24:41Z

pkg/secrets/clients/conjur/conjur_secrets_retriever.go

+	if err != nil {
+		return nil, log.RecordedError(messages.CSPFK002E)
+	}
+	// Always delete the access token. The deletion idempotent and never fails


Should be "The deletion is idempotent"?

diverdane · 2021-10-27T01:27:42Z

pkg/log/messages/error_messages.go

@@ -74,3 +74,6 @@ const CSPFK049E string = "CSPFK049E Failed to validate Pod annotations"
 const CSPFK050E string = "CSPFK050E Failed to unmarshal Push-to-File secrets. Reason: %s"
 const CSPFK051E string = "CSPFK051E Unknown file format '%s'"
 const CSPFK052E string = "CSPFK052E Failed to retrieve secrets. Reason: %s"
+
+const CSPFK053E string = "CSPFK053E Unable to initialize Secrets Provider: unable to create secret group collection"


Oh-oh, I used the same CSPFK053E error code. Looks like we're going to need to arm wrestle for it.

Branching thought off of this...

This file has all error messages numbered sequentially, but it also has them loosely grouped by area of concern, separated by comments, ie. // General, // Push to File, etc.

I've added CSPFK054E, which arises when SP fails for a bad StoreType - which is more of a // General failure. Moving this error to the // General section would require changing the identifiers for every error message declared afterwords. Updating the numbering style might help the error messages to be more extensible and static through future changes.

I think if the messages here are to be grouped by area of concern, then maybe they should be numbered within their area of concern:

const CSPFK-A001E string = "first message concerned with aspect A" const CSPFK-A002E string = "second message concerned with aspect A" const CSPFK-B001E string = "first message concerned with aspect B"

This also might help to avoid collisions like this one! (EDIT: not this one exactly, because we're both adding to the same concern group w/ 053E, but similar cases)

diverdane · 2021-10-27T01:53:40Z

pkg/secrets/provide_conjur_secrets_test.go

+
+	for _, tc := range TestCases {
+		var logBuffer bytes.Buffer
+		logger.InfoLogger = log.New(&logBuffer, "", 0)


logger.InfoLogger is a global, exported variable for the authn-k8s-client log package. So if we ever run tests in parallel, I think we could have multiple tests contending for this global setting?

At some point, we'll probably want to revisit how this is done in the conjur-authn-client log package... maybe have it return the info logger to the entity that's using the log package, rather than storing the info logger is as a global, exported variable?

ProviderFunc will represent ambiguously present a func which is responsible for prividing secrets, either to K8s Secrets or to Files depending on configuration.

codeclimate · 2021-10-27T14:34:08Z

Code Climate has analyzed commit bc9fffa and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3

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

This pull request will bring the total coverage in the repository to 87.6% (-1.6% change).

View more on Code Climate.

diverdane

LGTM!
Thanks for the final push in getting us across the E2E goal line!
I think we should merge this as is, and handle these with separate, smaller PRs:

Issue with having to specify full secret file path
Code Climate issues
I can work on the CC issues if you'd like... your choice.

john-odonnell force-pushed the startup-logic branch 2 times, most recently from c314d85 to ed71869 Compare October 22, 2021 21:35

imheresamir reviewed Oct 22, 2021

View reviewed changes

pkg/secrets/provide_conjur_secrets.go Outdated Show resolved Hide resolved

doodlesbykumbi force-pushed the ONYX-11974 branch from 168e04b to d6719a8 Compare October 26, 2021 10:53

Base automatically changed from ONYX-11974 to main October 26, 2021 14:17

imheresamir reviewed Oct 26, 2021

View reviewed changes

pkg/secrets/pushtofile/provide_conjur_secrets_test.go Outdated Show resolved Hide resolved

john-odonnell force-pushed the startup-logic branch from ed71869 to ff3e2bf Compare October 26, 2021 15:57

codeclimate bot reviewed Oct 26, 2021

View reviewed changes

john-odonnell force-pushed the startup-logic branch from ff3e2bf to 3c577a7 Compare October 26, 2021 18:06

john-odonnell marked this pull request as ready for review October 26, 2021 18:51

john-odonnell requested review from a team as code owners October 26, 2021 18:51

john-odonnell force-pushed the startup-logic branch from 8ec6f5b to de875b2 Compare October 26, 2021 20:03

codeclimate bot reviewed Oct 26, 2021

View reviewed changes

diverdane reviewed Oct 27, 2021

View reviewed changes

john-odonnell added 7 commits October 27, 2021 09:27

Relegate AccessToken creation to Conjur package

5ba1b31

Create ProviderFunc function type

112fb53

ProviderFunc will represent ambiguously present a func which is responsible for prividing secrets, either to K8s Secrets or to Files depending on configuration.

Use new ambiguous ProviderFunc in main

4c814a9

Fix K8s Secrets mode

fa6ee2d

Fix Retry Interval assignment

e4f9f9d

Add RetryableSecretProvider unit tests

3409843

Add unit testing for Push-to-File provider

61a55ae

john-odonnell force-pushed the startup-logic branch 2 times, most recently from 015eb46 to f2d66a1 Compare October 27, 2021 13:34

john-odonnell added 2 commits October 27, 2021 09:48

PR Review Fixes

f1818dc

Fix CodeClimate errors

bc9fffa

john-odonnell force-pushed the startup-logic branch from f2d66a1 to bc9fffa Compare October 27, 2021 13:48

john-odonnell requested a review from diverdane October 27, 2021 16:16

diverdane approved these changes Oct 29, 2021

View reviewed changes

john-odonnell merged commit c0bc31f into main Oct 29, 2021

john-odonnell deleted the startup-logic branch October 29, 2021 13:33

jtuttle mentioned this pull request Nov 12, 2021

Bump version for v1.14.1+suite.1 release cyberark/conjur-oss-suite-release#237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add startup logic to enable Push-to-File #377

Add startup logic to enable Push-to-File #377

john-odonnell commented Oct 22, 2021 •

edited

Loading

imheresamir Oct 26, 2021

john-odonnell Oct 26, 2021

diverdane Oct 27, 2021

doodlesbykumbi Oct 27, 2021

john-odonnell Oct 27, 2021

codeclimate bot Oct 26, 2021

codeclimate bot Oct 26, 2021

codeclimate bot Oct 26, 2021

diverdane left a comment

diverdane Oct 27, 2021

diverdane Oct 27, 2021

john-odonnell Oct 27, 2021

diverdane Oct 27, 2021

diverdane Oct 27, 2021

john-odonnell Oct 27, 2021

diverdane Oct 27, 2021

john-odonnell Oct 27, 2021

diverdane Oct 27, 2021

john-odonnell Oct 27, 2021 •

edited

Loading

diverdane Oct 27, 2021

codeclimate bot commented Oct 27, 2021

diverdane left a comment

Add startup logic to enable Push-to-File #377

Add startup logic to enable Push-to-File #377

Conversation

john-odonnell commented Oct 22, 2021 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

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

codeclimate bot Oct 26, 2021

Choose a reason for hiding this comment

codeclimate bot Oct 26, 2021

Choose a reason for hiding this comment

codeclimate bot Oct 26, 2021

Choose a reason for hiding this comment

diverdane 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

john-odonnell Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Oct 27, 2021

diverdane left a comment

Choose a reason for hiding this comment

john-odonnell commented Oct 22, 2021 •

edited

Loading

john-odonnell Oct 27, 2021 •

edited

Loading