Onyx-11974 write to file for push to file #367

doodlesbykumbi · 2021-10-04T19:37:25Z

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?

Resolves #[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

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/push_to_file/secret_group.go

pkg/secrets/push_to_file/secret_spec.go

diverdane

Here are my comments so far, still have LOTS to get through.

diverdane · 2021-10-14T00:44:07Z

pkg/secrets/push_to_file/push_to_writer.go

@@ -0,0 +1,81 @@
+package push_to_file


I see you're changing the package name (and the directory that it's in) to "push_to_file". The Effective Go guide section for package names says that, "By convention, packages are given lower case, single-word names; there should be no need for underscores or mixedCaps.".

In this case, push to file doesn't exactly fit the "single-word" form, but I'm thinking that we should stick with the all lower-case, no underscores convention.

Where this could make a difference is if/when this package is imported from another package... If we use "push-to-file" for the package name, then any references to exported functions in the package would be e.g. push_to_file.<ExportedFunctionName>, which wouldn't match Go CamelCase convention.

diverdane · 2021-10-14T00:50:38Z

cmd/push-to-file/main.go

+	}
+
+	// Fetch secrets MOCK!
+	// TODO: Should we make sure any secret id is only fetched once ?


The comments on this line (Line #43) and Line #45 can be deleted given @rpothier 's latest changes?

pkg/secrets/annotations/annotation_parser.go

diverdane · 2021-10-14T01:21:58Z

pkg/secrets/push_to_file/push_to_writer.go

+// templateData describes the form in which data is presented to push-to-file templates
+type templateData struct {
+	SecretsArray []*Secret
+	SecretsMap   map[string]*Secret


Do we need both an array of secret pointers and a map to secret pointers?
I see the SecretsMap is used for the secret function. The SecretsArray is used in some of the built-in templates, using a Go template range command, but couldn't we use range on a map?

It looks like we do. We should discuss this but in trying to use the templates I realised that there are simple things you can't do without being able to have both a map or slice version of secrets

A comma separated list like JSON requires the slice because you need to know which index you're on

To be able to refer to a secret by alias you need a map

pkg/secrets/push_to_file/secret_group.go

pkg/secrets/push_to_file/push_to_writer.go

pkg/secrets/push_to_file/secret_spec.go

diverdane

More comments/questions. Still reviewing the UT test cases.

diverdane · 2021-10-14T12:22:51Z

pkg/secrets/push_to_file/push_to_writer.go

+) (io.WriteCloser, error)
+
+// openFileToWriterCloser opens a file to write-to with some permissions.
+func openFileToWriterCloser(path string, permissions os.FileMode) (io.WriteCloser, error) {


I think I'm starting to understand the function naming convention that you're using. The ToWriterCloser portion of this function name means that the function is returning an io.WriteCloser (after opening the file). Is there any way that we can drop some of the details from the function names and have names that describe what it's doing from a higher level? If this would make things too vague, I can get used to this level of specificity now that I'm understanding your naming convention.

diverdane · 2021-10-14T12:41:16Z

pkg/secrets/push_to_file/push_to_writer.go

+		return err
+	}
+
+	return t.Execute(writer, templateData{


The assumption here is that the call to Execute will never display the SecretsMap contents as part of error messages, right?

diverdane · 2021-10-14T13:03:03Z

pkg/secrets/push_to_file/secret_group.go

+	}
+
+	// fileTemplate is only modified when
+	// 1. fileTemplate is not set. fileTemplate takes precedence


Maybe this is just me, but I experienced a bit of cognitive dissonance when I read the comments: fileTemplate is only modified when . . . fileTemplate is not set (i.e. how can it be modified if it's not set :) ). Maybe it could be fileTemplate is generated only when ... fileTemplate is not already defined?

diverdane · 2021-10-14T13:16:44Z

pkg/secrets/push_to_file/secret-group_utils_test.go

+	groupSecrets []*Secret,
+) error {
+	spy._calls++
+	if spy._calls > 1 {


Is it even possible that the writerPusher could be called more than once? Looks like this would be called as part of pushToFileWithDeps(), and that function should only call the toWriterPusher that is passed to it once and only once.

This is to ensure the spy is only ever used once. I'll add a comment

pkg/secrets/push_to_file/push_to_writer.go

diverdane · 2021-10-15T15:26:33Z

pkg/secrets/push_to_file/push_to_writer.go

+// pushToWriter takes a (group's) path, template and secrets, and processes the template
+// to generate text content that is pushed to a writer. push-to-file wraps around this.
+func pushToWriter(
+	writer io.Writer,


You'll probably hate this idea, but let me put it out there for consideration. I see this function is passed both an io.Writer and a group template, and it seems that the reason these are required is that the <template>.Execute() method needs a writer, and the <template>.Parse() and <template>.Execute() are done in the same operation.

But let's suppose we modularize by separating the template processing (parsing and execution) from the file writing. What I mean is that we can perform the <template>.Execute() using a plain text buffer, and then perform the file write by writing that buffer to an actual opened file.

This would add an extra step (the file write), but it might:

Simplify UT for the template processing??? (Not sure of this, though)

Allow us to do other things in the future with the parsed/executed template content besides writing a file.

I thought I had responded to this, apologies. I agree to separate these steps. Having Parse take place at the time of consuming annotations would certainly allow SP to fail earlier.

diverdane

This looks really good!
Just a few more very minor comments/questions, but otherwise I'm ready to approve.

diverdane · 2021-10-20T12:56:04Z

cmd/push-to-file/testdata/generate.go

+	annotationsFilePath, _ := filepath.Rel(workingdir, filepath.Join(basepath, "./annotations.txt"))
+	fmt.Println("Generating " + annotationsFilePath)
+	_ = ioutil.WriteFile(annotationsFilePath, []byte(annotations), 0644)
+}


Nit, if you're doing any updates... last line of this file needs a newline.

cmd/secrets-provider/main.go

pkg/secrets/annotations/annotation_parser.go

pkg/secrets/pushtofile/push_to_writer.go

pkg/secrets/pushtofile/secret_spec.go

pkg/secrets/pushtofile/standard_templates_test.go

pkg/secrets/pushtofile/secret_spec_test.go

pkg/secrets/pushtofile/secret_group.go

pkg/secrets/pushtofile/secret_group_test.go

pkg/secrets/clients/conjur/conjur_secrets_retriever.go

cmd/push-to-file/DEMO.md

pkg/secrets/pushtofile/README.md

cmd/push-to-file/DEMO.md

pkg/secrets/pushtofile/secret_group.go

diverdane

LGTM!!!
Although I think @rpothier has some minor comments left.

The push to file secret fetcher work is reconciled with the latest work on the write to file implementation. The push to file secret fetcher interface is updated to not rely on the existence of implementation details such as access tokens

The batch retrieval endpoint returns Conjur secrets using their full id <account>:variable:<variable_id>. Before this change this detail was spilling over into the rest of the code. The function to retrieve secrets should simply return secrets grouped by their input variable id.

diverdane

LGTM!!!

codeclimate · 2021-10-26T11:46:36Z

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

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

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

View more on Code Climate.

codeclimate bot reviewed Oct 4, 2021

View reviewed changes

pkg/secrets/push_to_file/secret_group.go Outdated Show resolved Hide resolved

pkg/secrets/push_to_file/secret_spec.go Outdated Show resolved Hide resolved

doodlesbykumbi changed the title ~~Onyx 11974~~ Onyx-11974 write to file for push to file Oct 12, 2021

doodlesbykumbi marked this pull request as ready for review October 13, 2021 14:05

doodlesbykumbi requested review from a team as code owners October 13, 2021 14:05

doodlesbykumbi force-pushed the ONYX-11974 branch 3 times, most recently from 22354df to da22960 Compare October 13, 2021 17:46

diverdane reviewed Oct 14, 2021

View reviewed changes

diverdane pushed a commit that referenced this pull request Oct 14, 2021

Add unmerged changes from PR #367

bb06c08

diverdane reviewed Oct 15, 2021

View reviewed changes

doodlesbykumbi force-pushed the ONYX-11974 branch 2 times, most recently from f3f6692 to c322e27 Compare October 19, 2021 15:00

diverdane reviewed Oct 20, 2021

View reviewed changes

rpothier reviewed Oct 20, 2021

View reviewed changes

cmd/push-to-file/DEMO.md Outdated Show resolved Hide resolved

pkg/secrets/pushtofile/README.md Show resolved Hide resolved

rpothier reviewed Oct 20, 2021

View reviewed changes

cmd/push-to-file/DEMO.md Show resolved Hide resolved

rpothier reviewed Oct 25, 2021

View reviewed changes

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

diverdane approved these changes Oct 25, 2021

View reviewed changes

doodlesbykumbi added 11 commits October 26, 2021 11:45

Implements the push in Push to file

eba8a16

Improvements

af02458

Define fetchSecretsForGroups interface

474ccef

Fix CC issues + Cleanup

c152a20

StandardFileTemplate validation

25a1c4f

Refactor standard templates

36c001a

Fix: optional use of annotations file

337d8a7

Address comments on PR

f9deb32

Reconcile push to file secret fetcher

30060ba

The push to file secret fetcher work is reconciled with the latest work on the write to file implementation. The push to file secret fetcher interface is updated to not rely on the existence of implementation details such as access tokens

Clean up

f4337e6

doodlesbykumbi added 2 commits October 26, 2021 11:45

Clean up + default to yaml format

5a2037b

Add alias to undefined secret template error

d6719a8

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

diverdane approved these changes Oct 26, 2021

View reviewed changes

doodlesbykumbi merged commit a176eb2 into main Oct 26, 2021

doodlesbykumbi deleted the ONYX-11974 branch October 26, 2021 14:17

john-odonnell mentioned this pull request Oct 26, 2021

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

Merged

6 tasks

doodlesbykumbi mentioned this pull request Oct 29, 2021

Incorporate secrets base path into secret group filepath #381

Merged

13 tasks

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

Onyx-11974 write to file for push to file #367

Onyx-11974 write to file for push to file #367

doodlesbykumbi commented Oct 4, 2021

diverdane left a comment

diverdane Oct 14, 2021

diverdane Oct 14, 2021

diverdane Oct 14, 2021

doodlesbykumbi Oct 19, 2021

diverdane left a comment

diverdane Oct 14, 2021

diverdane Oct 14, 2021

diverdane Oct 14, 2021

diverdane Oct 14, 2021

doodlesbykumbi Oct 19, 2021

diverdane Oct 15, 2021

doodlesbykumbi Oct 26, 2021

diverdane left a comment

diverdane Oct 20, 2021

diverdane left a comment

diverdane left a comment

codeclimate bot commented Oct 26, 2021

Onyx-11974 write to file for push to file #367

Onyx-11974 write to file for push to file #367

Conversation

doodlesbykumbi commented Oct 4, 2021

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

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

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

diverdane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diverdane left a comment

Choose a reason for hiding this comment

diverdane left a comment

Choose a reason for hiding this comment

codeclimate bot commented Oct 26, 2021