From c322e271cbdd64badce779e25a1494af9c966539 Mon Sep 17 00:00:00 2001 From: Kumbirai Tanekha Date: Tue, 19 Oct 2021 15:42:08 +0100 Subject: [PATCH] Address comments on PR --- cmd/push-to-file/main.go | 46 +++++++++---------- cmd/secrets-provider/main.go | 2 + pkg/secrets/annotations/annotation_parser.go | 8 +++- .../{push_to_file => pushtofile}/README.md | 0 .../push_to_writer.go | 27 ++++------- .../push_to_writer_test.go | 2 +- .../secret-group_utils_test.go | 28 +++++------ .../secret_group.go | 15 +++--- .../secret_group_test.go | 32 ++++++------- .../secret_spec.go | 7 +-- .../secret_spec_test.go | 4 +- .../standard_templates.go | 2 +- .../standard_templates_definitions.go | 2 +- .../standard_templates_test.go | 2 +- 14 files changed, 88 insertions(+), 89 deletions(-) rename pkg/secrets/{push_to_file => pushtofile}/README.md (100%) rename pkg/secrets/{push_to_file => pushtofile}/push_to_writer.go (66%) rename pkg/secrets/{push_to_file => pushtofile}/push_to_writer_test.go (98%) rename pkg/secrets/{push_to_file => pushtofile}/secret-group_utils_test.go (61%) rename pkg/secrets/{push_to_file => pushtofile}/secret_group.go (93%) rename pkg/secrets/{push_to_file => pushtofile}/secret_group_test.go (92%) rename pkg/secrets/{push_to_file => pushtofile}/secret_spec.go (93%) rename pkg/secrets/{push_to_file => pushtofile}/secret_spec_test.go (96%) rename pkg/secrets/{push_to_file => pushtofile}/standard_templates.go (98%) rename pkg/secrets/{push_to_file => pushtofile}/standard_templates_definitions.go (97%) rename pkg/secrets/{push_to_file => pushtofile}/standard_templates_test.go (98%) diff --git a/cmd/push-to-file/main.go b/cmd/push-to-file/main.go index 9b43c15cf..5a48d7c22 100644 --- a/cmd/push-to-file/main.go +++ b/cmd/push-to-file/main.go @@ -4,18 +4,15 @@ import ( "flag" "os" - "github.com/cyberark/conjur-authn-k8s-client/pkg/access_token" "github.com/cyberark/conjur-authn-k8s-client/pkg/log" "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/annotations" - "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/clients/conjur" - "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/k8s_secrets_storage/mocks" - "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/push_to_file" + "github.com/cyberark/secrets-provider-for-k8s/pkg/secrets/pushtofile" ) func main() { var annotationFilePath string - flag.StringVar(&annotationFilePath, "f", "", "path to annotation file") + flag.StringVar(&annotationFilePath, "f", "./annotations", "path to annotation file") flag.Parse() @@ -30,7 +27,7 @@ func main() { // Generate secret Groups log.Info("Generate secret Groups") - secretGroups, errs := push_to_file.NewSecretGroups(annotations) + secretGroups, errs := pushtofile.NewSecretGroups(annotations) if len(errs) > 0 { for _, err := range errs { log.Error(err.Error()) @@ -45,9 +42,16 @@ func main() { // TODO: secret fetching should be concurrent and where possible parallel log.Info("Fetching secrets") - atoken := mocks.MockAccessToken{} secretsByGroup, err := fetchSecretsForGroups( - atoken, nil, secretGroups) + func(variableIDs []string) (map[string][]byte, error) { + var res = map[string][]byte{} + for _, id := range variableIDs { + log.Info("Processing secret %s", id) + res[id] = []byte("value-" + id) + } + + return res, nil + }, secretGroups) if err != nil { log.Error(err.Error()) os.Exit(1) @@ -69,23 +73,23 @@ func main() { } func fetchSecretsForGroups( - accessToken access_token.AccessToken, - retrieveConjurSecrets conjur.RetrieveConjurSecretsFunc, - secretGroups []*push_to_file.SecretGroup, -) (map[string][]*push_to_file.Secret, error) { + retrieveConjurSecrets func(variableIDs []string) (map[string][]byte, error), + secretGroups []*pushtofile.SecretGroup, +) (map[string][]*pushtofile.Secret, error) { // map[group name] => group secret vales // Gather secret paths var secretPaths []string var uniqueSecretPaths = map[string]struct{}{} for _, group := range secretGroups { + log.Info("Processing group", group) specs := group.ResolvedSecretSpecs() for _, spec := range specs { - if _, ok := uniqueSecretPaths[spec.Path]; !ok { - uniqueSecretPaths[spec.Path] = struct{}{} + if _, ok := uniqueSecretPaths[spec.Path]; ok { continue } + uniqueSecretPaths[spec.Path] = struct{}{} secretPaths = append(secretPaths, spec.Path) } } @@ -95,27 +99,21 @@ func fetchSecretsForGroups( // TODO: create better abstraction that hides authenticator and retry logic from the // rest of the code // - // TODO: provide a single interface that this method takes as input - // the interface should hide access tokens etc. - accessTokenData, err := accessToken.Read() - if err != nil { - return nil, err - } - secretValueByPath, err := retrieveConjurSecrets(accessTokenData, secretPaths) + secretValueByPath, err := retrieveConjurSecrets(secretPaths) if err != nil { return nil, err } // Gather secret values - var secretsByGroup = map[string][]*push_to_file.Secret{} + var secretsByGroup = map[string][]*pushtofile.Secret{} for _, group := range secretGroups { specs := group.ResolvedSecretSpecs() - groupSecrets := make([]*push_to_file.Secret, len(specs)) + groupSecrets := make([]*pushtofile.Secret, len(specs)) for i, spec := range specs { secretValue := secretValueByPath[spec.Path] - groupSecrets[i] = &push_to_file.Secret{ + groupSecrets[i] = &pushtofile.Secret{ Alias: spec.Alias, Value: string(secretValue), } diff --git a/cmd/secrets-provider/main.go b/cmd/secrets-provider/main.go index ae80214e5..d30b532a9 100644 --- a/cmd/secrets-provider/main.go +++ b/cmd/secrets-provider/main.go @@ -42,6 +42,8 @@ func main() { log.Info(messages.CSPFK008I, secrets.FullVersionName) // Only attempt to populate from annotations if the annotations file exists + // TODO: Figure out strategy for dealing with explicit annotation file path + // set by user. In that case we can't just ignore that the file is missing. if _, err := os.Stat(annotationsFile); err == nil { annotationsMap, err = annotations.NewAnnotationsFromFile(annotationsFile) if err != nil { diff --git a/pkg/secrets/annotations/annotation_parser.go b/pkg/secrets/annotations/annotation_parser.go index e5b21605a..585c004f3 100644 --- a/pkg/secrets/annotations/annotation_parser.go +++ b/pkg/secrets/annotations/annotation_parser.go @@ -2,6 +2,7 @@ package annotations import ( "bufio" + "fmt" "io" "os" "strconv" @@ -30,7 +31,12 @@ func osFileOpener(name string, flag int, perm os.FileMode) (io.ReadCloser, error // are defined in a deployment manifest. func NewAnnotationsFromFile(path string) (map[string]string, error) { // Use standard OS - return newAnnotationsFromFile(osFileOpener, path) + res, err := newAnnotationsFromFile(osFileOpener, path) + if err != nil { + return nil, fmt.Errorf(messages.CSPFK041E, path, err) + } + + return res, nil } // newAnnotationsFromFile performs the work of NewAnnotationsFromFile(), and diff --git a/pkg/secrets/push_to_file/README.md b/pkg/secrets/pushtofile/README.md similarity index 100% rename from pkg/secrets/push_to_file/README.md rename to pkg/secrets/pushtofile/README.md diff --git a/pkg/secrets/push_to_file/push_to_writer.go b/pkg/secrets/pushtofile/push_to_writer.go similarity index 66% rename from pkg/secrets/push_to_file/push_to_writer.go rename to pkg/secrets/pushtofile/push_to_writer.go index ac298299c..47a821dfd 100644 --- a/pkg/secrets/push_to_file/push_to_writer.go +++ b/pkg/secrets/pushtofile/push_to_writer.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "io" @@ -12,24 +12,24 @@ type templateData struct { SecretsMap map[string]*Secret } -// toWriterPusher is the func definition for pushToWriter. It allows switching out pushToWriter +// pushToWriterFunc is the func definition for pushToWriter. It allows switching out pushToWriter // for a mock implementation -type toWriterPusher func( +type pushToWriterFunc func( writer io.Writer, groupName string, groupTemplate string, groupSecrets []*Secret, ) error -// toWriteCloserOpener is the func definition for openFileToWriterCloser. It allows switching -// out openFileToWriterCloser for a mock implementation -type toWriteCloserOpener func( +// openWriteCloserFunc is the func definition for openFileAsWriteCloser. It allows switching +// out openFileAsWriteCloser for a mock implementation +type openWriteCloserFunc func( path string, permissions os.FileMode, ) (io.WriteCloser, error) -// openFileToWriterCloser opens a file to write-to with some permissions. -func openFileToWriterCloser(path string, permissions os.FileMode) (io.WriteCloser, error) { +// openFileAsWriteCloser opens a file to write-to with some permissions. +func openFileAsWriteCloser(path string, permissions os.FileMode) (io.WriteCloser, error) { return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, permissions) } @@ -48,7 +48,7 @@ func pushToWriter( t, err := template.New(groupName).Funcs(template.FuncMap{ // secret is a custom utility function for streamlined access to secret values. - // It panics for errors not specified on the group. + // It panics for secrets aliases not specified on the group. "secret": func(label string) string { v, ok := secretsMap[label] if ok { @@ -59,15 +59,6 @@ func pushToWriter( // when the template is executed. panic("secret alias not present in specified secrets for group") }, - // toYaml marshals a given value to YAML - //"toYaml": func(value interface{}) string { - // d, err := yaml.Marshal(&value) - // if err != nil { - // panic(err) - // } - // - // return string(d) - //}, }).Parse(groupTemplate) if err != nil { return err diff --git a/pkg/secrets/push_to_file/push_to_writer_test.go b/pkg/secrets/pushtofile/push_to_writer_test.go similarity index 98% rename from pkg/secrets/push_to_file/push_to_writer_test.go rename to pkg/secrets/pushtofile/push_to_writer_test.go index 278363300..05a0ca39b 100644 --- a/pkg/secrets/push_to_file/push_to_writer_test.go +++ b/pkg/secrets/pushtofile/push_to_writer_test.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "bytes" diff --git a/pkg/secrets/push_to_file/secret-group_utils_test.go b/pkg/secrets/pushtofile/secret-group_utils_test.go similarity index 61% rename from pkg/secrets/push_to_file/secret-group_utils_test.go rename to pkg/secrets/pushtofile/secret-group_utils_test.go index 0273a95fb..8fefb2e7f 100644 --- a/pkg/secrets/push_to_file/secret-group_utils_test.go +++ b/pkg/secrets/pushtofile/secret-group_utils_test.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "bytes" @@ -13,32 +13,33 @@ type ClosableBuffer struct { func (c ClosableBuffer) Close() error { return c.CloseErr } -//// toWriterPusher -type toWriterPusherArgs struct { +//// pushToWriterFunc +type pushToWriterArgs struct { writer io.Writer groupName string groupTemplate string groupSecrets []*Secret } -type toWriterPusherSpy struct { - args toWriterPusherArgs +type pushToWriterSpy struct { + args pushToWriterArgs err error _calls int } -func (spy *toWriterPusherSpy) Call( +func (spy *pushToWriterSpy) Call( writer io.Writer, groupName string, groupTemplate string, groupSecrets []*Secret, ) error { spy._calls++ + // This is to ensure the spy is only ever used once! if spy._calls > 1 { panic("spy called more than once") } - spy.args = toWriterPusherArgs{ + spy.args = pushToWriterArgs{ writer: writer, groupName: groupName, groupTemplate: groupTemplate, @@ -48,26 +49,27 @@ func (spy *toWriterPusherSpy) Call( return spy.err } -//// toWriteCloserOpener -type toWriteCloserOpenerArgs struct { +//// openWriteCloserFunc +type openWriteCloserArgs struct { path string permissions os.FileMode } -type toWriteCloserOpenerSpy struct { - args toWriteCloserOpenerArgs +type openWriteCloserSpy struct { + args openWriteCloserArgs writeCloser io.WriteCloser err error _calls int } -func (spy *toWriteCloserOpenerSpy) Call(path string, permissions os.FileMode) (io.WriteCloser, error) { +func (spy *openWriteCloserSpy) Call(path string, permissions os.FileMode) (io.WriteCloser, error) { spy._calls++ + // This is to ensure the spy is only ever used once! if spy._calls > 1 { panic("spy called more than once") } - spy.args = toWriteCloserOpenerArgs{ + spy.args = openWriteCloserArgs{ path: path, permissions: permissions, } diff --git a/pkg/secrets/push_to_file/secret_group.go b/pkg/secrets/pushtofile/secret_group.go similarity index 93% rename from pkg/secrets/push_to_file/secret_group.go rename to pkg/secrets/pushtofile/secret_group.go index a7273f095..4fab4a20b 100644 --- a/pkg/secrets/push_to_file/secret_group.go +++ b/pkg/secrets/pushtofile/secret_group.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "fmt" @@ -51,12 +51,12 @@ func (s *SecretGroup) ResolvedSecretSpecs() []SecretSpec { // PushToFile uses the configuration on a secret group to inject secrets into a template // and write the result to a file. func (s *SecretGroup) PushToFile(secrets []*Secret) error { - return s.pushToFileWithDeps(pushToWriter, openFileToWriterCloser, secrets) + return s.pushToFileWithDeps(openFileAsWriteCloser, pushToWriter, secrets) } func (s *SecretGroup) pushToFileWithDeps( - pushToWriter toWriterPusher, - openWriteCloser toWriteCloserOpener, + openWriteCloser openWriteCloserFunc, + pushToWriter pushToWriterFunc, secrets []*Secret) error { // Make sure all the secret specs are accounted for err := validateSecretsAgainstSpecs(secrets, s.SecretSpecs) @@ -139,10 +139,9 @@ func maybeFileTemplateFromFormat( return "", fmt.Errorf("%s", `missing one of "file template" or "file format" for group`) } - // fileTemplate is only modified when - // 1. fileTemplate is not set. fileTemplate takes precedence - // 2. fileFormat is set - if len(fileTemplate) == 0 && len(fileFormat) > 0 { + // fileFormat is used to set fileTemplate when fileTemplate is not + // already set + if len(fileTemplate) == 0 { var err error fileTemplate, err = FileTemplateForFormat( diff --git a/pkg/secrets/push_to_file/secret_group_test.go b/pkg/secrets/pushtofile/secret_group_test.go similarity index 92% rename from pkg/secrets/push_to_file/secret_group_test.go rename to pkg/secrets/pushtofile/secret_group_test.go index a2e6cc057..23a0be084 100644 --- a/pkg/secrets/push_to_file/secret_group_test.go +++ b/pkg/secrets/pushtofile/secret_group_test.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "fmt" @@ -15,7 +15,7 @@ type pushToFileWithDepsTestCase struct { overrideSecrets []*Secret // Overrides secrets generated from group secret specs toWriterPusherErr error toWriteCloserOpenerErr error - assert func(t *testing.T, spyOpen toWriteCloserOpenerSpy, closableBuf *ClosableBuffer, spyPush toWriterPusherSpy, err error) + assert func(t *testing.T, spyOpen openWriteCloserSpy, closableBuf *ClosableBuffer, spyPush pushToWriterSpy, err error) } func (tc *pushToFileWithDepsTestCase) Run(t *testing.T) { @@ -25,10 +25,10 @@ func (tc *pushToFileWithDepsTestCase) Run(t *testing.T) { // Setup mocks closableBuf := new(ClosableBuffer) - spyPushToWriter := toWriterPusherSpy{ + spyPushToWriter := pushToWriterSpy{ err: tc.toWriterPusherErr, } - spyOpenWriteCloser := toWriteCloserOpenerSpy{ + spyOpenWriteCloser := openWriteCloserSpy{ writeCloser: closableBuf, err: tc.toWriteCloserOpenerErr, } @@ -49,8 +49,8 @@ func (tc *pushToFileWithDepsTestCase) Run(t *testing.T) { // Exercise err := group.pushToFileWithDeps( - spyPushToWriter.Call, spyOpenWriteCloser.Call, + spyPushToWriter.Call, secrets) tc.assert(t, spyOpenWriteCloser, closableBuf, spyPushToWriter, err) @@ -159,14 +159,14 @@ var pushToFileWithDepsTestCases = []pushToFileWithDepsTestCase{ overrideSecrets: nil, assert: func( t *testing.T, - spyOpen toWriteCloserOpenerSpy, + spyOpen openWriteCloserSpy, closableBuf *ClosableBuffer, - spyPush toWriterPusherSpy, + spyPush pushToWriterSpy, err error, ) { // Assertions assert.NoError(t, err) - // Assert on toWriterPusher + // Assert on pushToWriterFunc assert.Equal(t, "groupname", spyPush.args.groupName, ) assert.Equal(t, closableBuf, spyPush.args.writer) assert.Equal(t, "filetemplate", spyPush.args.groupTemplate) @@ -196,9 +196,9 @@ var pushToFileWithDepsTestCases = []pushToFileWithDepsTestCase{ overrideSecrets: nil, assert: func( t *testing.T, - spyOpen toWriteCloserOpenerSpy, + spyOpen openWriteCloserSpy, closableBuf *ClosableBuffer, - spyPush toWriterPusherSpy, + spyPush pushToWriterSpy, err error, ) { // Assertions @@ -212,9 +212,9 @@ var pushToFileWithDepsTestCases = []pushToFileWithDepsTestCase{ overrideSecrets: []*Secret{}, assert: func( t *testing.T, - spyOpen toWriteCloserOpenerSpy, + spyOpen openWriteCloserSpy, closableBuf *ClosableBuffer, - spyPush toWriterPusherSpy, + spyPush pushToWriterSpy, err error, ) { // Assertions @@ -235,9 +235,9 @@ var pushToFileWithDepsTestCases = []pushToFileWithDepsTestCase{ overrideSecrets: nil, assert: func( t *testing.T, - spyOpen toWriteCloserOpenerSpy, + spyOpen openWriteCloserSpy, closableBuf *ClosableBuffer, - spyPush toWriterPusherSpy, + spyPush pushToWriterSpy, err error, ) { // Assertions @@ -266,9 +266,9 @@ func TestSecretGroup_pushToFileWithDeps(t *testing.T) { overrideSecrets: nil, assert: func( t *testing.T, - spyOpen toWriteCloserOpenerSpy, + spyOpen openWriteCloserSpy, closableBuf *ClosableBuffer, - spyPush toWriterPusherSpy, + spyPush pushToWriterSpy, err error, ) { // Assertions diff --git a/pkg/secrets/push_to_file/secret_spec.go b/pkg/secrets/pushtofile/secret_spec.go similarity index 93% rename from pkg/secrets/push_to_file/secret_spec.go rename to pkg/secrets/pushtofile/secret_spec.go index 11bebedd1..40aa5efb5 100644 --- a/pkg/secrets/push_to_file/secret_spec.go +++ b/pkg/secrets/pushtofile/secret_spec.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "fmt" @@ -18,8 +18,9 @@ func (t SecretSpec) MarshalYAML() (interface{}, error) { const invalidSecretSpecErr = `expected a "string (path)" or "single entry map of string to string (alias to path)" on line %d` -// UnmarshalYAML is a custom unmarshaller for SecretSpec that allows it to unmarshal -// from different yaml types by trying each one +// UnmarshalYAML is a custom unmarshaller for SecretSpec that allows us to +// unmarshal from different YAML node representations i.e. literal string or +// map. func (t *SecretSpec) UnmarshalYAML(node *yaml.Node) error { switch node.Kind { case yaml.ScalarNode: diff --git a/pkg/secrets/push_to_file/secret_spec_test.go b/pkg/secrets/pushtofile/secret_spec_test.go similarity index 96% rename from pkg/secrets/push_to_file/secret_spec_test.go rename to pkg/secrets/pushtofile/secret_spec_test.go index cb6cdfed6..1e9701d66 100644 --- a/pkg/secrets/push_to_file/secret_spec_test.go +++ b/pkg/secrets/pushtofile/secret_spec_test.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "testing" @@ -61,7 +61,7 @@ another-password: dev/openshift/password `, assert: func(t *testing.T, result []SecretSpec, err error) { assert.Contains(t, err.Error(), "cannot unmarshal") - assert.Contains(t, err.Error(), "into []push_to_file.SecretSpec") + assert.Contains(t, err.Error(), "into []pushtofile.SecretSpec") }, }, { diff --git a/pkg/secrets/push_to_file/standard_templates.go b/pkg/secrets/pushtofile/standard_templates.go similarity index 98% rename from pkg/secrets/push_to_file/standard_templates.go rename to pkg/secrets/pushtofile/standard_templates.go index 64fd3622e..72c614dc0 100644 --- a/pkg/secrets/push_to_file/standard_templates.go +++ b/pkg/secrets/pushtofile/standard_templates.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import "fmt" diff --git a/pkg/secrets/push_to_file/standard_templates_definitions.go b/pkg/secrets/pushtofile/standard_templates_definitions.go similarity index 97% rename from pkg/secrets/push_to_file/standard_templates_definitions.go rename to pkg/secrets/pushtofile/standard_templates_definitions.go index 69dd125a7..9a5f1d252 100644 --- a/pkg/secrets/push_to_file/standard_templates_definitions.go +++ b/pkg/secrets/pushtofile/standard_templates_definitions.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile const jsonTemplate = `{ {{- range $index, $secret := .SecretsArray }} diff --git a/pkg/secrets/push_to_file/standard_templates_test.go b/pkg/secrets/pushtofile/standard_templates_test.go similarity index 98% rename from pkg/secrets/push_to_file/standard_templates_test.go rename to pkg/secrets/pushtofile/standard_templates_test.go index af85e73ad..1449ad2fd 100644 --- a/pkg/secrets/push_to_file/standard_templates_test.go +++ b/pkg/secrets/pushtofile/standard_templates_test.go @@ -1,4 +1,4 @@ -package push_to_file +package pushtofile import ( "testing"