Add Annotation parsing and Config from Annotations functions #359

john-odonnell · 2021-09-02T18:47:16Z

What does this PR do?

This PR allows for configuring Secrets Provider with Pod annotations. Annotation settings are prioritized over environment variable settings, but envvars can still be used to configure SP to maintain backwards compatibility.

Adds `annotations_parser.go` and `annotation_parser_test.go` to package `secrets/annotations`, including:

AnnotationsFromFile(path string) (map[string]string, error)
ParseAnnotations(annotationsFile io.Reader) (map[string]string, error)
Test Cases
- A valid annotations file is parsed successfully
- Malformed annotations file lines result in an error indicating the line number and desired format

Adds functions to `secrets/config` package:

ValidateAnnotations(annotations map[string]string) ([]error, []error)
- Confirms annotation keys & value types, and confirms value for those annotations with enumerated value options
- Returns lists of Error-level and Info-level logs
- Test Cases
  - Properly formatted annotations return empty arrays
  - Confirm Info log if for unrecognized annotations with and without conjur.org/ prefix
  - Confirm Error log for annotations given improper value type
  - Confirm Error log for annotations with explicit input ranges given invalid values
GatherSecretsProviderSettings(annotations map[string]string) map[string]string
- Returns an map containing annotations and envvars required from Secrets Provider config
- Test Cases
  - If the annotations map is empty, the resulting map contains envvars used in SP config
  - If envvars are not set, the resulting map contains contains the annotations used in SP config
  - If both are present, the resulting map is all envvars and annotations used in SP config
  - Annotations and envvars not used in SP config are excluded
ValidateSecretsProviderSettings(envAndAnnots map[string]string) ([]error, []error)
- Confirm that the provided annotations and envvars lend to a valid SP config
- Input validation on annotation keys and values
- Returns Error and Info level log messages. An empty Error array means validation has succeeded.
- Test Cases
  - Given a valid configuration of annotations, no errors are returned
  - Given a valid configuration of envvars, no errors are returned
  - Confirm that MY_POD_NAMESPACE is required in either case
  - Confirm Error log if StoreType is not set
  - Confirm Error log is Push-to-File is being configured with envvars
  - Confirm Error log if K8s Secrets mode is missing required K8s secrets
  - Confirm Info log if values are provided to equivalent annotation and envvar
NewConfig(settings map[string]string) *Config
- Create a new SP config from a validated map of annotations and SP Config envvars
- Test Cases
  - A valid map of annotation-based settings results in a valid Config
  - A valid map of envvar-based settings results in a valid Config
  - A valid map of both annotation and envvar settings defers to annotation values

New functions are used in SP main function to create a new SP Config

Acceptance Criteria Review

An annotation parser exists for extracting arbitrary annotations from a downward API annotation file. This should include a test case to validate this behavior.
Annotations can be used instead of environment variables. This should include a test case to validate this behavior.
Annotations take precedence over environment variables. This should include a test case to validate this behavior.
Unknown Conjur annotation causes INFO level log
Multiline annotation values are parsed correctly

What ticket does this PR close?

ONYX-11827
Closes #330

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/annotations/annotation_parser.go

Includes Annotation Parser, custom error and info messages, and unit tests.

- Convert tests from Convey to vanilla Go unit testing, for both Annotations and Config packages - Refactor ParseAnnotations function to remove unit test dependency on file system operations

- Move annotation key and value validation to config package. The annotations package should only parse - validation is the responsibility of the consuming entity. - Consolidate NewFromEnv, NewFromAnnotations, and MergeConfig into one function, NewConfig. Helps solve problems that arise from handling Configs compiled from different sources that are dependent on each other. - Update unit tests to reflect these changes.

Validate annotation key-values and Secrets Provider config separately

diverdane

Really nice job weaving all of the different pieces of the configuration together (SP env variables, authn-k8s env variables, Annotations)! The resulting flow is pretty easy to follow, and it looks like you've covered all of the edge cases.

Nice work on the table-driven test cases and the use of closure for the assertion funcs!

I just have minor comments and some just-for-grins color commentary. Otherwise looks good-to-go.

pkg/secrets/annotations/annotation_parser.go

cmd/secrets-provider/main.go

diverdane · 2021-09-24T10:14:07Z

cmd/secrets-provider/main.go

+		printErrorAndExit(messages.CSPFK049E)
+	}
+
+	secretsProviderSettings := secretsConfigProvider.GatherSecretsProviderSettings(annotationsMap)


Nice work on fitting together all of the config puzzle pieces here.

diverdane · 2021-09-24T10:27:27Z

pkg/secrets/annotations/annotation_parser.go

+	"github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages"
+)
+
+func AnnotationsFromFile(path string) (map[string]string, error) {


Nit: Exported names should have a preceding doc comment that begins with the function name and briefly describes what it does, e.g.:

// AnnotationsFromFile reads and parses an annotations file that has been created by Kubernetes // via the Kubernetes Downward API, based on Pod Annotations that are defined in a deployment // manifest.

I surprised we're not seeing errors for missing doc comments for exported names.... maybe we need to add runs of go lint in our tests? I think my version of Vim automatically flags this.

Noted, and gave all exported functions proper comments.
Is there a go-to linter for Go projects? I looked at golint, but it looks like it's been deprecated for a while.

Looks like revive is pretty standard, and is comparable to what golint was.
I installed and ran revive manually with:

go install github.com/mgechev/revive@latest revive -formatter friendly

When I removed the doc comment that you just added and ran revive, I see:

$ revive -formatter friendly ⚠ https://revive.run/r#exported exported function AnnotationsFromFile should have comment or be unexported annotation_parser.go:14:1 ⚠ https://revive.run/r#exported func name will be used as annotations.AnnotationsFromFile by other packages, and that stutters; consider calling this FromFile annotation_parser.go:14:6 ⚠ 2 problems (0 errors, 2 warnings) Warnings: 2 exported $

I'll have to see if I can add this to my Go-Vim config (and/or Goland).

Ran revive on the files I changed, and had to update some constants in config.go that propagated to a couple other files. Had to convert from THIS_CASE to ThisCase.

diverdane · 2021-09-24T10:28:12Z

pkg/secrets/annotations/annotation_parser.go

+	return ParseAnnotations(annotationsFile)
+}
+
+// List and multi-line annotations are formatted as a single string in the annotations file,


This comment really helps!

pkg/secrets/config/config.go

diverdane · 2021-09-24T11:41:40Z

pkg/secrets/config/config.go

+			return fmt.Errorf(messages.CSPFK042E, key, value, "Integer")
+		}
+	case "bool":
+		if value != "true" && value != "false" {


Can a boolean Annotation value be set to all-caps "TRUE" or "FALSE"?

No, not with this implementation. If we want, I can use strconv.ParseBool to determine if a boolean setting was acceptable, which would allow "TRUE" and "FALSE", and some others:

ParseBool returns the boolean value represented by the string. It accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False. Any other value returns an error.

pkg/secrets/config/config_test.go

diverdane · 2021-09-24T12:07:50Z

pkg/secrets/config/config_test.go

+}
+
+func TestValidateAnnotations(t *testing.T) {
+	for _, tc := range validateAnnotationsTestCases {


This is COMPLETELY a personal preference sort of thing, but in the past, I've created table-driven tests where all of these are defined in one test function:

The test case struct (this struct can be anonymous, if it's included in an instantiation of this struct, i.e. the test list)

The test case list (e.g. validateAnnotationsTestCases)

The iteration/running through all of the test cases

The benefit to this is that all definitions required for the test are gathered into one spot (one function). The downside is that the test function ends up with a LOT of lines (less visual breaks).

codeclimate · 2021-09-24T22:24:50Z

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

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

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

View more on Code Climate.

diverdane

LGTM!!!

john-odonnell changed the title ~~Add Annotation Parser to utils package~~ Add Annotation Parser package Sep 9, 2021

codeclimate bot reviewed Sep 9, 2021

View reviewed changes

pkg/annotations/annotation_parser.go Outdated Show resolved Hide resolved

codeclimate bot reviewed Sep 9, 2021

View reviewed changes

pkg/annotations/annotation_parser.go Outdated Show resolved Hide resolved

john-odonnell force-pushed the ONYX-11827 branch 3 times, most recently from efbcf5a to 6a3a84c Compare September 13, 2021 16:40

john-odonnell changed the title ~~Add Annotation Parser package~~ Add Annotation parsing and Config from Annotations functions Sep 14, 2021

john-odonnell force-pushed the ONYX-11827 branch from 6a3a84c to c0d76ca Compare September 14, 2021 13:50

john-odonnell marked this pull request as ready for review September 14, 2021 15:12

john-odonnell requested review from a team as code owners September 14, 2021 15:12

john-odonnell added 6 commits September 22, 2021 02:17

Add Annotations package

d879d71

Includes Annotation Parser, custom error and info messages, and unit tests.

Add NewFromAnnotations function to config package

f85c0f1

Add MergeConfig function to config package

52c8b84

Move Annotations package to pkg/secrets/annotations

e6094ff

Refactor unit tests

3c3e3e0

- Convert tests from Convey to vanilla Go unit testing, for both Annotations and Config packages - Refactor ParseAnnotations function to remove unit test dependency on file system operations

john-odonnell force-pushed the ONYX-11827 branch 2 times, most recently from 859523a to 379477f Compare September 23, 2021 19:44

Refactor Secrets Provider Config functions

bc04b63

Validate annotation key-values and Secrets Provider config separately

john-odonnell force-pushed the ONYX-11827 branch from 379477f to 6f45dce Compare September 23, 2021 20:46

diverdane reviewed Sep 24, 2021

View reviewed changes

john-odonnell force-pushed the ONYX-11827 branch from c6000e1 to e2099da Compare September 24, 2021 17:57

john-odonnell added 3 commits September 24, 2021 16:33

Update integration test cases

37bb1b6

PR comment fixes

6557a48

Linting fixes

1026bf3

john-odonnell force-pushed the ONYX-11827 branch 2 times, most recently from a1c298c to 1026bf3 Compare September 24, 2021 22:20

diverdane approved these changes Sep 27, 2021

View reviewed changes

john-odonnell merged commit a664958 into main Sep 27, 2021

john-odonnell deleted the ONYX-11827 branch September 27, 2021 16:49

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 Annotation parsing and Config from Annotations functions #359

Add Annotation parsing and Config from Annotations functions #359

john-odonnell commented Sep 2, 2021 •

edited

Loading

diverdane left a comment

diverdane Sep 24, 2021

diverdane Sep 24, 2021

john-odonnell Sep 24, 2021

diverdane Sep 24, 2021

john-odonnell Sep 24, 2021

diverdane Sep 24, 2021

diverdane Sep 24, 2021

john-odonnell Sep 24, 2021

diverdane Sep 24, 2021

codeclimate bot commented Sep 24, 2021

diverdane left a comment

Add Annotation parsing and Config from Annotations functions #359

Add Annotation parsing and Config from Annotations functions #359

Conversation

john-odonnell commented Sep 2, 2021 • edited Loading

What does this PR do?

Adds annotations_parser.go and annotation_parser_test.go to package secrets/annotations, including:

Adds functions to secrets/config package:

New functions are used in SP main function to create a new SP Config

Acceptance Criteria Review

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

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 Sep 24, 2021

diverdane left a comment

Choose a reason for hiding this comment

john-odonnell commented Sep 2, 2021 •

edited

Loading

Adds `annotations_parser.go` and `annotation_parser_test.go` to package `secrets/annotations`, including:

Adds functions to `secrets/config` package: