Add Secrets Provider refresh interval annotations and timer #426

rpothier · 2022-01-18T19:38:04Z

Desired Outcome

Add annotations and a Go timer to periodically refresh secrets.

Implemented Changes

Two new annotations have been added for provider refresh.
conjur.org/secrets-refresh-enabled
conjur.org/secrets-refresh-interval
also, side-car has been added to conjur.org/container-mode

Adds a go timer routine to call periodicProvider.
This is configured by adding a wrapper function to provideSecrets, if Secrets Provider
is configured in side-car mode a new goRouting is invoked that waits for a
timer tick that was configured with secrets-refresh-interval, and will update
the secrets on each tick.

Also had to optimize ValidateSecretsProviderSettings due to CodeClimate complaining
there were too many lines in the function.

Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue link: ONYX-15538
ONYX-16504

Definition of Done

Add support for new Annotations conjur.org/secrets-refresh-interval and conjur.org/secrets-refresh-enabled.
In Secrets Provider, call the provideSecrets() function periodically based on the new configuration.
Add UT for the parsing of the new Annotation.

Changelog

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
A follow-up issue to update official docs has been filed here: insert issue ID
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

pkg/secrets/provide_conjur_secrets.go

pkg/secrets/config/config.go

szh

Looking great! Left a couple of questions

pkg/secrets/provide_conjur_secrets_test.go

cmd/secrets-provider/main.go

codeclimate · 2022-02-01T20:32:08Z

pkg/secrets/config/config.go

+	return storeType, err
+}
+
+func validRefreshInterval(intervalStr string, enableStr string, containerMode string) error {


Function validRefreshInterval has 5 return statements (exceeds 4 allowed).

szh

Looks great! Just one comment

szh · 2022-02-02T14:14:29Z

PUSH_TO_FILE.md

 | `conjur.org/secrets-destination`    | `SECRETS_DESTINATION` | Allowed values: <ul><li>`file`</li><li>`k8s_secrets`</li></ul> |
 | `conjur.org/k8s-secrets`            | `K8S_SECRETS`         | This list is ignored when `conjur.org/secrets-destination` annotation is set to **`file`** |
 | `conjur.org/retry-count-limit`      | `RETRY_COUNT_LIMIT`   | Defaults to 5
 | `conjur.org/retry-interval-sec`     | `RETRY_INTERVAL_SEC`  | Defaults to 1 (sec)              |
 | `conjur.org/debug-logging`          | `DEBUG`               | Defaults to `false`              |
+| `conjur.org/secrets-refresh-enabled`| Note\* | Defaults to `false`. Secrets Provider will exit with error if this is explicitly set to `false` and `conjur.org/secrets-rotation-interval` is explicitly set. |
+| `conjur.org/secrets-refresh-interval`           | Note\* | Set to a valid duration string as defined [here](https://pkg.go.dev/time#ParseDuration). Valid time units are `s`, `m`, and `h` (for seconds, minutes, and hours, respectively). Some examples of valid duration strings:<ul><li>`5m`</li><li>`2h30m`</li><li>`48h`</li></ul>The minimum refresh interval is 1 second. A refresh interval of 0 seconds is treated as a fatal configuration error. The maximum refresh interval is approximately 290 years. |


Suggested change

| `conjur.org/secrets-refresh-interval` | Note\* | Set to a valid duration string as defined [here](https://pkg.go.dev/time#ParseDuration). Valid time units are `s`, `m`, and `h` (for seconds, minutes, and hours, respectively). Some examples of valid duration strings:<ul><li>`5m`</li><li>`2h30m`</li><li>`48h`</li></ul>The minimum refresh interval is 1 second. A refresh interval of 0 seconds is treated as a fatal configuration error. The maximum refresh interval is approximately 290 years. |

| `conjur.org/secrets-refresh-interval` | Note\* | Set to a valid duration string as defined [here](https://pkg.go.dev/time#ParseDuration). Valid time units are `s`, `m`, and `h` (for seconds, minutes, and hours, respectively). Some examples of valid duration strings:<ul><li>`5m`</li><li>`2h30m`</li><li>`48h`</li></ul>The minimum refresh interval is 1 second. A refresh interval of 0 seconds is treated as a fatal configuration error. The maximum refresh interval is 100 years. |

to match https://github.com/cyberark/secrets-provider-for-k8s/pull/426/files#diff-1a2f43d8162d071687f1a5864ce5b6b825e9f25d5a74a1df1c218bbbfe597a33R25

Hmm, would it be better to leave this and change the default timeout?

I don't think it makes much of a difference, 100 or 290.

Based on other offline conversation, I'm removing the timeout so this is no longer an issue.

diverdane · 2022-02-03T14:03:42Z

PUSH_TO_FILE.md

@@ -323,12 +323,14 @@ for a description of each environment variable setting:
 | K8s Annotation  | Equivalent<br>Environment Variable | Description, Notes |
 |-----------------------------------------|---------------------|----------------------------------|
 | `conjur.org/authn-identity`         | `CONJUR_AUTHN_LOGIN`  | Required value. Example: `host/conjur/authn-k8s/cluster/apps/inventory-api` |
-| `conjur.org/container-mode`         | `CONTAINER_MODE`      | Allowed values: <ul><li>`init`</li><li>`application`</li></ul>Defaults to `init`.<br>Must be set (or default) to `init` for Push to File mode.|
+| `conjur.org/container-mode`         | `CONTAINER_MODE`      | Allowed values: <ul><li>`init`</li><li>`application`</li><li>`side-car`</li></ul>Defaults to `init`.<br>Must be set (or default) to `init` or `side-car`for Push to File mode.|


Typo, side-car should be single word sidecar. Same throughout the rest of the changes.
Also, we should consider making the sidecar option the default, but probably should discuss with Alex.

thanks for catching that, updated.

diverdane · 2022-02-03T16:13:43Z

cmd/secrets-provider/main.go

+		getContainerMode(),
+		provideSecrets,
+		// Set a large timeout, basically forever
+		secretsConfigProvider.MaxRefreshTimeout,


Is this MaxRefreshTimeout used for implementing a close proximity for "sleep forever" functionality? If so, I think we can do this differently, and have the SP block forever.

pkg/secrets/provide_conjur_secrets.go

diverdane · 2022-02-03T17:03:11Z

PUSH_TO_FILE.md

 | `conjur.org/secrets-destination`    | `SECRETS_DESTINATION` | Allowed values: <ul><li>`file`</li><li>`k8s_secrets`</li></ul> |
 | `conjur.org/k8s-secrets`            | `K8S_SECRETS`         | This list is ignored when `conjur.org/secrets-destination` annotation is set to **`file`** |
 | `conjur.org/retry-count-limit`      | `RETRY_COUNT_LIMIT`   | Defaults to 5
 | `conjur.org/retry-interval-sec`     | `RETRY_INTERVAL_SEC`  | Defaults to 1 (sec)              |
 | `conjur.org/debug-logging`          | `DEBUG`               | Defaults to `false`              |
+| `conjur.org/secrets-refresh-enabled`| Note\* | Defaults to `false`. Secrets Provider will exit with error if this is explicitly set to `false` and `conjur.org/secrets-rotation-interval` is explicitly set. |


I prefer what you have here: having the secrets-refresh-enabled setting simply default to false, rather then defaulting to true or false depending upon whether secrets-refresh-enabled is explicitly set. The latter way is more convenient in some ways, but it's MUCH more difficult to document. I think we need to get an okay from Alex on this, since I think his expectation is the latter.

The implementation does not require a true if the secrets-refresh-interval set. So this needs to be updated still.

diverdane · 2022-02-03T17:04:35Z

PUSH_TO_FILE.md

 | `conjur.org/secrets-destination`    | `SECRETS_DESTINATION` | Allowed values: <ul><li>`file`</li><li>`k8s_secrets`</li></ul> |
 | `conjur.org/k8s-secrets`            | `K8S_SECRETS`         | This list is ignored when `conjur.org/secrets-destination` annotation is set to **`file`** |
 | `conjur.org/retry-count-limit`      | `RETRY_COUNT_LIMIT`   | Defaults to 5
 | `conjur.org/retry-interval-sec`     | `RETRY_INTERVAL_SEC`  | Defaults to 1 (sec)              |
 | `conjur.org/debug-logging`          | `DEBUG`               | Defaults to `false`              |
+| `conjur.org/secrets-refresh-enabled`| Note\* | Defaults to `false`. Secrets Provider will exit with error if this is explicitly set to `false` and `conjur.org/secrets-rotation-interval` is explicitly set. |
+| `conjur.org/secrets-refresh-interval`           | Note\* | Set to a valid duration string as defined [here](https://pkg.go.dev/time#ParseDuration). Valid time units are `s`, `m`, and `h` (for seconds, minutes, and hours, respectively). Some examples of valid duration strings:<ul><li>`5m`</li><li>`2h30m`</li><li>`48h`</li></ul>The minimum refresh interval is 1 second. A refresh interval of 0 seconds is treated as a fatal configuration error. The maximum refresh interval is approximately 290 years. |


Before "The minimum refresh interval is 1 second", it should also say "The default refresh interval is 5 minutes."

diverdane · 2022-02-04T21:58:15Z

cmd/secrets-provider/main.go

@@ -186,6 +184,12 @@ func retryableSecretsProvider(
 		secretsConfig.RetryCountLimit,
 		provideSecrets,
 	)
+
+	provideSecrets = secrets.PeriodicSecretProvider(


Since this function is now returning a PeriodicSecretProvider, should the function be renamed periodicSecretsProvider or similar? Or alternatively, move the call to secrets.PeriodicSecretProvider outside of this retryableSecretsProvider() function.... unless you're adding this to get by some CodeClimate limitation e.g. on number of return statements in another function?

Or maybe just rename it to something very generic e.g. "SecretProvider()"? Not sure what makes sense here.

yea, as it's not always periodic, we could go with SecretProvider()

diverdane · 2022-02-04T22:06:22Z

cmd/secrets-provider/main.go

-		logError(errStr)
+	go provideSecrets()
+	select {
+	case err = <-secrets.SecretProviderError:


Looks like secrets.SecretProviderError is currently a global variable. Can we change this so that the error channel is created dynamically in secrets.PeriodicSecretProvider(), and passed up the call stack and returned to this function when we call retryableSecretsProvider() on line 83 above?

Looking at this again, I think the logic of calling a goroutine and providing an error channel should be moved to the pkg/secrets package. Generally, we want the main.go to be as simple as possible, and I think that this implementation detail for periodic secrets provider function should be abstracted away from main.go.

We would still need to do exactly what you're doing here inside the test code, since that will need to have a goroutine that can either run once and return, run once and sleep, or run periodically... and be able to stop that goroutine from the test thread.

To see what I'm suggesting, take a look at these diffs that I created on a branch based on your PR branch:
d61c96c

Patched in the diffs.

diverdane · 2022-02-05T00:06:59Z

pkg/secrets/config/config.go

@@ -46,17 +52,38 @@ type annotationRestraints struct {
 	allowedValues []string
 }

+// Annotations used to support Conjur secrets rotation.


This comment needs to be updated. The Annotations listed aren't just for secrets rotation, but for all SP non-push-to-file Annotations.

diverdane · 2022-02-05T00:15:37Z

pkg/secrets/config/config.go

 	storeType := ""
+	var err error

 	if annotStoreType == "" {


Your call on this, but since you're rewriting some of this already.... Golang coding style recommends using switch block rather than if/else if/else block. So this section would have the form:

switch { case annotStoreType == "": // Do things case validStoreType(annotStoreType): // Do other things default: // Log an error }

diverdane · 2022-02-07T13:35:44Z

cmd/secrets-provider/main.go

-		logError(errStr)
+	go provideSecrets()
+	select {
+	case err = <-secrets.SecretProviderError:


Looking at this again, I think the logic of calling a goroutine and providing an error channel should be moved to the pkg/secrets package. Generally, we want the main.go to be as simple as possible, and I think that this implementation detail for periodic secrets provider function should be abstracted away from main.go.

We would still need to do exactly what you're doing here inside the test code, since that will need to have a goroutine that can either run once and return, run once and sleep, or run periodically... and be able to stop that goroutine from the test thread.

To see what I'm suggesting, take a look at these diffs that I created on a branch based on your PR branch:
d61c96c

diverdane · 2022-02-07T13:40:23Z

cmd/secrets-provider/main.go

@@ -186,6 +184,12 @@ func retryableSecretsProvider(
 		secretsConfig.RetryCountLimit,
 		provideSecrets,
 	)
+
+	provideSecrets = secrets.PeriodicSecretProvider(


Or maybe just rename it to something very generic e.g. "SecretProvider()"? Not sure what makes sense here.

diverdane · 2022-02-07T13:42:59Z

pkg/secrets/provide_conjur_secrets.go

+		}
+		if mode == "sidecar" && secretRefreshInterval > 0{
+			TickerQuit <- true
+			time.Sleep(1 * time.Second) // give a second for the go routine to shut down


Might be better to define this as a const at the top of the file so it's easy to find/change.

diverdane · 2022-02-07T13:47:53Z

pkg/secrets/provide_conjur_secrets.go

@@ -97,3 +96,55 @@ func RetryableSecretProvider(
 		return err
 	}
 }
+
+var TickerQuit = make(chan bool)


These three channels don't need to be defined/declared as global variables.

To make this testable, the "ProviderDone" channel should passed in from the caller to PeriodicSecretProvider (in main.go, we'll pass it a channel that's not used for now, but in the future we can add a signal handler that can use this channel to stop the SP).

See diffs here:
d61c96c#diff-c083eb56e1c331264db79fbb96d5fbb5ecfd8f082e9dde16e1a0e28f5f0f4230L100-R119

made suggested changes.

diverdane · 2022-02-07T13:51:01Z

pkg/secrets/provide_conjur_secrets.go

+	}
+}
+
+func periodicSecretProvider(provideSecrets ProviderFunc, ticker *time.Ticker) {


You can pass in the error channel (for output) and the quit channel (for input) to this function, so that you won't have to use global variables.

See: d61c96c#diff-c083eb56e1c331264db79fbb96d5fbb5ecfd8f082e9dde16e1a0e28f5f0f4230L136-R164

diverdane · 2022-02-07T13:57:20Z

pkg/secrets/provide_conjur_secrets_test.go

+const (
+	providerDelayTime = 500
+)
+var providerCount = 0


I think the reason that we're seeing some flaky test cases below (the ones that are commented out) is that we're using one common providerCount gobal variable for all test cases. It makes testing more complex when you're testing slow provider functionality because you have to wait for some grace period at the end of each test case for the previous test case's slow provider to finish.

Generally, we want each test case to have its own state. So this should be implemented as a structure that has a provide() method, and instantiate one of those for each test case.

To see what I mean, check out this mocking:
d61c96c#diff-c0314624500148cb0ccac2409ef0d4dc9a8cddf7603e8879bae326cc2d96d7d5R17-R52

This mocking works, except that I haven't reconciled it with the eventualProvider() function, which depends upon those base objects.

diverdane · 2022-02-07T13:58:43Z

pkg/secrets/provide_conjur_secrets_test.go

@@ -14,11 +14,31 @@ import (
 	"github.com/stretchr/testify/assert"
 )

+const (
+	providerDelayTime = 500


I think we can get by with 10s of milliseconds instead of 100s.
For example, this seems to work:
d61c96c#diff-c0314624500148cb0ccac2409ef0d4dc9a8cddf7603e8879bae326cc2d96d7d5L132-R176

made the change.

codeclimate · 2022-02-07T18:47:22Z

Code Climate has analyzed commit 50bb93e and detected 0 issues on this pull request.

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

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

View more on Code Climate.

szh

🎉

rpothier force-pushed the ONYX-15538-secrets-rotation branch 4 times, most recently from 1f62430 to 26f25ca Compare January 25, 2022 21:09

codeclimate bot reviewed Jan 25, 2022

View reviewed changes

pkg/secrets/provide_conjur_secrets.go Show resolved Hide resolved

rpothier force-pushed the ONYX-15538-secrets-rotation branch from 26f25ca to 2d5edd8 Compare January 27, 2022 17:42

codeclimate bot reviewed Jan 27, 2022

View reviewed changes

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

rpothier changed the title ~~Add Secrets Provider refresh interval~~ Add Secrets Provider refresh interval annotations and timer Jan 27, 2022

rpothier force-pushed the ONYX-15538-secrets-rotation branch from 2d5edd8 to 26c7838 Compare January 27, 2022 20:16

codeclimate bot reviewed Jan 27, 2022

View reviewed changes

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

rpothier force-pushed the ONYX-15538-secrets-rotation branch 3 times, most recently from d181ae8 to f53cc0a Compare January 28, 2022 18:46

rpothier self-assigned this Jan 28, 2022

rpothier force-pushed the ONYX-15538-secrets-rotation branch 3 times, most recently from f04c267 to 7f43c99 Compare January 28, 2022 21:43

codeclimate bot reviewed Jan 28, 2022

View reviewed changes

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

rpothier force-pushed the ONYX-15538-secrets-rotation branch 7 times, most recently from 27f9b0f to b57062d Compare January 31, 2022 20:32

rpothier marked this pull request as ready for review January 31, 2022 20:39

rpothier requested a review from a team as a code owner January 31, 2022 20:39

rpothier force-pushed the ONYX-15538-secrets-rotation branch 2 times, most recently from bb49364 to 319786f Compare February 1, 2022 18:45

szh reviewed Feb 1, 2022

View reviewed changes

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

cmd/secrets-provider/main.go Outdated Show resolved Hide resolved

rpothier force-pushed the ONYX-15538-secrets-rotation branch from 319786f to 94a1caa Compare February 1, 2022 20:25

codeclimate bot reviewed Feb 1, 2022

View reviewed changes

rpothier force-pushed the ONYX-15538-secrets-rotation branch 2 times, most recently from 9aa12a7 to da0db49 Compare February 2, 2022 01:38

szh reviewed Feb 2, 2022

View reviewed changes

rpothier force-pushed the ONYX-15538-secrets-rotation branch 4 times, most recently from d35c8e6 to a3d4ed4 Compare February 3, 2022 14:34

diverdane reviewed Feb 3, 2022

View reviewed changes

rpothier force-pushed the ONYX-15538-secrets-rotation branch from a3d4ed4 to 3d2ba81 Compare February 3, 2022 15:40

diverdane reviewed Feb 3, 2022

View reviewed changes

rpothier force-pushed the ONYX-15538-secrets-rotation branch 4 times, most recently from 58cbe8e to ed8b6cb Compare February 4, 2022 20:26

diverdane reviewed Feb 5, 2022

View reviewed changes

diverdane reviewed Feb 7, 2022

View reviewed changes

rpothier force-pushed the ONYX-15538-secrets-rotation branch from ed8b6cb to 11991d4 Compare February 7, 2022 15:22

Add Secrets Provider refresh interval

50bb93e

rpothier force-pushed the ONYX-15538-secrets-rotation branch from 11991d4 to 50bb93e Compare February 7, 2022 18:42

rpothier marked this pull request as draft February 7, 2022 18:43

rpothier marked this pull request as ready for review February 7, 2022 19:39

szh approved these changes Feb 7, 2022

View reviewed changes

rpothier merged commit c3f02ce into main Feb 7, 2022

rpothier deleted the ONYX-15538-secrets-rotation branch February 7, 2022 21:05

john-odonnell mentioned this pull request May 17, 2022

Update suite.yml for v1.17.6+suite.1 release cyberark/conjur-oss-suite-release#247

Merged

gl-johnson mentioned this pull request Aug 23, 2022

Update suite.yml for v18.0.0+suite1 release cyberark/conjur-oss-suite-release#250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Secrets Provider refresh interval annotations and timer #426

Add Secrets Provider refresh interval annotations and timer #426

rpothier commented Jan 18, 2022 •

edited

Loading

szh left a comment

codeclimate bot Feb 1, 2022

szh left a comment

szh Feb 2, 2022

rpothier Feb 2, 2022

szh Feb 2, 2022

rpothier Feb 4, 2022

diverdane Feb 3, 2022

rpothier Feb 3, 2022

diverdane Feb 3, 2022

diverdane Feb 3, 2022

rpothier Feb 4, 2022

diverdane Feb 3, 2022

rpothier Feb 7, 2022

diverdane Feb 4, 2022

diverdane Feb 7, 2022

rpothier Feb 7, 2022

diverdane Feb 4, 2022

diverdane Feb 7, 2022

rpothier Feb 7, 2022

diverdane Feb 5, 2022

diverdane Feb 5, 2022

rpothier Feb 7, 2022

diverdane Feb 7, 2022

diverdane Feb 7, 2022

diverdane Feb 7, 2022

diverdane Feb 7, 2022

rpothier Feb 7, 2022

diverdane Feb 7, 2022

diverdane Feb 7, 2022

diverdane Feb 7, 2022

rpothier Feb 7, 2022

codeclimate bot commented Feb 7, 2022

szh left a comment

Add Secrets Provider refresh interval annotations and timer #426

Add Secrets Provider refresh interval annotations and timer #426

Conversation

rpothier commented Jan 18, 2022 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

szh left a comment

Choose a reason for hiding this comment

codeclimate bot Feb 1, 2022

Choose a reason for hiding this comment

szh 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Feb 7, 2022

szh left a comment

Choose a reason for hiding this comment

rpothier commented Jan 18, 2022 •

edited

Loading