Address comments on PR

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),
 			}

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 {

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

pkg/secrets/push_to_file/push_to_writer.go → 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

pkg/secrets/push_to_file/push_to_writer_test.go → pkg/secrets/pushtofile/push_to_writer_test.go

@@ -1,4 +1,4 @@
-package push_to_file
+package pushtofile
 
 import (
 	"bytes"

pkg/secrets/push_to_file/secret-group_utils_test.go → 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,
 	}

pkg/secrets/push_to_file/secret_group.go → 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(