Implement detection of changes in K8s Secret content with UT #448

rpothier · 2022-03-15T19:35:10Z

Desired Outcome

The Kubernetes secrets should only be updated when there is new content.

Implemented Changes

Updated to detect changes the k8s secrets values and Conjur secrets.

Connected Issue/Story

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

CyberArk internal issue link: ONYX-17475

Definition of Done

[] K8s secrets are updated only when there are changes in the k8s secrets content or Conjur secrets.

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/log/messages/info_messages.go

szh · 2022-03-21T18:13:45Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

@@ -372,6 +375,7 @@ func TestProvide(t *testing.T) {
 		for secretName, secretData := range tc.k8sSecrets {
 			mocks.kubeClient.AddSecret(secretName, secretData)
 		}
+		prevSecretsChecksums = map[string]utils.Checksum{}


Doesn't look like this is used anywhere

This is needed for TestProvide() to zero out prevSecretsChecksums so that it writes values in this test.

Oooh.... I'm glad @szh asked this question!!! I don't think we should be using a global variable for prevSecretsChecksums here, or for the Push-to-File provider either. The issue that we might face with using global variables is that if our UT test cases are ever run in parallel, then one test case could potentially stomp on another test case's values in the global prevSecretsChecksums.

I believe the prevSecretsChecksums should be moved to the K8sProvider struct. Each UT test case should be creating its own K8sProvider to test with, so each one should be created with its own prevSecretsChecksums. Thinking about this a bit more, this would also help in the future if we should ever need to run multiple K8sProviders in parallel.

We need to fix this for the Push-to-File provider as well. I don't know if you want to tackle that here or separately.

szh · 2022-03-21T18:22:16Z

pkg/utils/checksum.go

+)
+
+type Checksum []byte
+func FileChecksum(buf *bytes.Buffer) (Checksum, error) {


I noticed this PR decreases the code coverage a bit. Should we add tests for this file?

diverdane · 2022-03-21T22:32:02Z

ROTATION.md

@@ -135,7 +141,6 @@ This feature is a **Community** level project that is still under development.
 There could be changes to the documentation so please check back often for updates and additions.
 Future enhancements to this feature will include:

- Support for secrets rotation of Kubernetes secrets
 - atomic writes for multiple Conjur secret values
 - reporting of multiple errored secret variables for bulk Conjur secret retrieval and selective deletion of secret values from files
 - atomic write of secret files


The "atomic write of secret files" is implemented now as well, so we might as well delete that item from this list of future enhancements.

diverdane · 2022-03-21T23:04:10Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

@@ -372,6 +375,7 @@ func TestProvide(t *testing.T) {
 		for secretName, secretData := range tc.k8sSecrets {
 			mocks.kubeClient.AddSecret(secretName, secretData)
 		}
+		prevSecretsChecksums = map[string]utils.Checksum{}


Oooh.... I'm glad @szh asked this question!!! I don't think we should be using a global variable for prevSecretsChecksums here, or for the Push-to-File provider either. The issue that we might face with using global variables is that if our UT test cases are ever run in parallel, then one test case could potentially stomp on another test case's values in the global prevSecretsChecksums.

I believe the prevSecretsChecksums should be moved to the K8sProvider struct. Each UT test case should be creating its own K8sProvider to test with, so each one should be created with its own prevSecretsChecksums. Thinking about this a bit more, this would also help in the future if we should ever need to run multiple K8sProviders in parallel.

We need to fix this for the Push-to-File provider as well. I don't know if you want to tackle that here or separately.

diverdane · 2022-03-21T23:15:32Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

@@ -372,6 +375,7 @@ func TestProvide(t *testing.T) {
 		for secretName, secretData := range tc.k8sSecrets {


While we're changing these unit tests, would you mind modifying the way that we're invoking test cases so that each is called via t.Run(tc.desc ...). What the iterations would look like is:

for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { // Set up test case mocks := newTestMocks() . . .

The advantage to doing this is that if/when UT failures occur, the tc.desc is displayed prominently in the failure summary, and it makes it very obvious which test case(s) have failed.

Also, I believe that we would need to run the test cases via t.Run(...) if we should ever want to run the test cases in parallel (reference: https://eleni.blog/2019/05/11/parallel-test-execution-in-go/).

diverdane · 2022-03-21T23:19:06Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

@@ -383,3 +387,91 @@ func TestProvide(t *testing.T) {
 		}
 	}
 }
+
+func Test_secrets_contentChanges(t *testing.T) {


Ruh-Roh, the name of this test function uses snake_case. (Function name should be TestSecretContentChanges).

I fear we've been doing too many context switches between Bash and Go.

diverdane · 2022-03-21T23:28:56Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

+		},
+		expectedMissingValues{} ) (t, mocks, err, desc)
+
+	// Call Provide again, verify is doesn't try to update the secret


Typo, should be "verify it doesn't ...".

diverdane · 2022-03-21T23:33:05Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

+
+	// Call Provide again, verify is doesn't try to update the secret
+	// as there should be an error if it tried to write the secrets
+	denyK8sUpdate = true


That's a clever way to detect that it's not trying to update! :)

diverdane · 2022-03-21T23:40:10Z

pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go

+		expectedK8sSecrets{
+			"k8s-secret1": {"secret1": "secret-value1"},
+		},
+		expectedMissingValues{} ) (t, mocks, err, desc)


Something that might be worth considering here... this test involves a whole bunch of operations, and if when there's a test failure, it won't be immediately obvious which assert in this test is failing (and where in the sequence of operations it's failing), without having to match up line numbers in the source code. What you could do is to modify the desc string as you work your way through the sequence. That way, the failure summary for the assert that fails will provide us with details on where exactly it's failing.

szh

Looks great. I left a couple docs comments.

szh · 2022-03-23T19:13:21Z

ROTATION.md

@@ -71,7 +71,13 @@ There are two new annotations introduced and one annotation is updated.
 Prerequisites:

 Requires secrets-provider-for-k8s v1.4.0 or later.
+
+<details><summary>For Push to File mode</summary>


I think there needs to be an empty line after the <summary> block. I think this is causing the hyperlinks to not render as links.

See here for example.

Good catch!

szh · 2022-03-23T19:17:10Z

CHANGELOG.md

+- Kubernetes Secrets are only updated when there are changes when secrets rotation is enabled.
+  [cyberark/secrets-provider-for-k8s#448](https://github.com/cyberark/secrets-provider-for-k8s/pull/448)


Looking at the changelog entries for the previous release, notably for for #432, and given that K8s secrets wasn't previously supported, I wonder if we should replace this with "Secrets rotation is now supported for Kubernetes Secrets"

How about "Adds support for secrets rotation with Kubernetes Secrets."

szh

LGTM!

codeclimate · 2022-03-23T21:09:18Z

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

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

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

View more on Code Climate.

rpothier self-assigned this Mar 15, 2022

rpothier force-pushed the 17475-detect-k8s-secrets branch 5 times, most recently from 10675f1 to 054f17c Compare March 17, 2022 13:47

rpothier marked this pull request as ready for review March 17, 2022 13:47

rpothier requested a review from a team as a code owner March 17, 2022 13:47

rpothier force-pushed the 17475-detect-k8s-secrets branch 8 times, most recently from cdc1563 to 0e2ffdf Compare March 18, 2022 17:41

szh self-requested a review March 21, 2022 15:47

szh suggested changes Mar 21, 2022

View reviewed changes

szh reviewed Mar 21, 2022

View reviewed changes

diverdane reviewed Mar 21, 2022

View reviewed changes

rpothier force-pushed the 17475-detect-k8s-secrets branch from 0e2ffdf to 70f52e0 Compare March 22, 2022 18:59

rpothier marked this pull request as draft March 22, 2022 18:59

rpothier force-pushed the 17475-detect-k8s-secrets branch from 70f52e0 to 3454846 Compare March 22, 2022 20:15

rpothier marked this pull request as ready for review March 23, 2022 13:47

rpothier force-pushed the 17475-detect-k8s-secrets branch 2 times, most recently from 3379346 to 6317dc6 Compare March 23, 2022 14:40

szh reviewed Mar 23, 2022

View reviewed changes

rpothier force-pushed the 17475-detect-k8s-secrets branch 2 times, most recently from 98414de to 90a1fae Compare March 23, 2022 19:52

Implement detection of changes in K8s Secret content with UT

e7bd95e

rpothier force-pushed the 17475-detect-k8s-secrets branch from 90a1fae to e7bd95e Compare March 23, 2022 19:53

szh approved these changes Mar 23, 2022

View reviewed changes

rpothier merged commit 9a0e95c into main Mar 23, 2022

rpothier deleted the 17475-detect-k8s-secrets branch March 23, 2022 22:43

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

Implement detection of changes in K8s Secret content with UT #448

Implement detection of changes in K8s Secret content with UT #448

rpothier commented Mar 15, 2022 •

edited by szh

Loading

szh Mar 21, 2022

rpothier Mar 21, 2022

diverdane Mar 21, 2022

szh Mar 21, 2022

diverdane Mar 21, 2022

diverdane Mar 21, 2022

diverdane Mar 21, 2022

diverdane Mar 21, 2022

diverdane Mar 21, 2022

diverdane Mar 21, 2022

diverdane Mar 21, 2022

szh left a comment

szh Mar 23, 2022

rpothier Mar 23, 2022

szh Mar 23, 2022

rpothier Mar 23, 2022

szh left a comment

codeclimate bot commented Mar 23, 2022

		@@ -372,6 +375,7 @@ func TestProvide(t *testing.T) {
		for secretName, secretData := range tc.k8sSecrets {

		- Kubernetes Secrets are only updated when there are changes when secrets rotation is enabled.
		[cyberark/secrets-provider-for-k8s#448](https://github.com/cyberark/secrets-provider-for-k8s/pull/448)

Implement detection of changes in K8s Secret content with UT #448

Implement detection of changes in K8s Secret content with UT #448

Conversation

rpothier commented Mar 15, 2022 • edited by szh Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

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

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

szh left a comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 23, 2022

rpothier commented Mar 15, 2022 •

edited by szh

Loading