refactors to fix provider options overwrite bug

Signed-off-by: chaosinthecrd <[email protected]>
ChaosInTheCRD · Feb 13, 2024 · 0fcfa8d · 0fcfa8d
1 parent 1fd8181
commit 0fcfa8d
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 84 deletions.
diff --git a/cmd/keyloader.go b/cmd/keyloader.go
@@ -60,8 +60,7 @@ func loadSigners(ctx context.Context, so options.SignerOptions, ko options.KMSSi
 		// NOTE: We want to initialze the KMS provider specific options if a KMS signer has been invoked
 		if ksp, ok := sp.(*kms.KMSSignerProvider); ok {
 			for _, opt := range ksp.Options {
-				pn := opt.ProviderName()
-				for _, setter := range ko[pn] {
+				for _, setter := range ko[opt.ProviderName()] {
 					sp, err = setter(ksp)
 					if err != nil {
 						continue

diff --git a/options/options.go b/options/options.go
@@ -28,65 +28,71 @@ type Interface interface {
 	AddFlags(cmd *cobra.Command)
 }
 
-func addFlagsFromRegistry[T any](prefix string, registrationEntries []registry.Entry[T], cmd *cobra.Command) map[string][]func(T) (T, error) {
-	optSettersByName := make(map[string][]func(T) (T, error))
-
-	for _, registration := range registrationEntries {
-		for _, opt := range registration.Options {
-			name := fmt.Sprintf("%s-%s-%s", prefix, registration.Name, opt.Name())
-			switch optT := opt.(type) {
-			case *registry.ConfigOption[T, int]:
-				{
-					val := cmd.Flags().Int(name, optT.DefaultVal(), opt.Description())
-					optSettersByName[registration.Name] = append(optSettersByName[registration.Name], func(a T) (T, error) {
-						return optT.Setter()(a, *val)
-					})
-				}
-
-			case *registry.ConfigOption[T, string]:
-				{
-					// this is kind of a hacky solution to maintain backward compatibility with the old "-k" flag
-					var val *string
-					if name == "signer-file-key-path" {
-						val = cmd.Flags().StringP(name, "k", optT.DefaultVal(), optT.Description())
-					} else {
-						val = cmd.Flags().String(name, optT.DefaultVal(), opt.Description())
-					}
+func addFlags[T any](prefix string, regName string, options []registry.Configurer, optSettersMap map[string][]func(T) (T, error), cmd *cobra.Command) map[string][]func(T) (T, error) {
+	for _, opt := range options {
+		name := fmt.Sprintf("%s-%s-%s", prefix, regName, opt.Name())
+		switch optT := opt.(type) {
+		case *registry.ConfigOption[T, int]:
+			{
+				val := cmd.Flags().Int(name, optT.DefaultVal(), opt.Description())
+				optSettersMap[regName] = append(optSettersMap[regName], func(a T) (T, error) {
+					return optT.Setter()(a, *val)
+				})
+			}
 
-					optSettersByName[registration.Name] = append(optSettersByName[registration.Name], func(a T) (T, error) {
-						return optT.Setter()(a, *val)
-					})
+		case *registry.ConfigOption[T, string]:
+			{
+				// this is kind of a hacky solution to maintain backward compatibility with the old "-k" flag
+				var val *string
+				if name == "signer-file-key-path" {
+					val = cmd.Flags().StringP(name, "k", optT.DefaultVal(), optT.Description())
+				} else {
+					val = cmd.Flags().String(name, optT.DefaultVal(), opt.Description())
 				}
 
-			case *registry.ConfigOption[T, []string]:
-				{
-					val := cmd.Flags().StringSlice(name, optT.DefaultVal(), opt.Description())
-					optSettersByName[registration.Name] = append(optSettersByName[registration.Name], func(a T) (T, error) {
-						return optT.Setter()(a, *val)
-					})
-				}
+				optSettersMap[regName] = append(optSettersMap[regName], func(a T) (T, error) {
+					return optT.Setter()(a, *val)
+				})
+			}
 
-			case *registry.ConfigOption[T, bool]:
-				{
-					val := cmd.Flags().Bool(name, optT.DefaultVal(), opt.Description())
-					optSettersByName[registration.Name] = append(optSettersByName[registration.Name], func(a T) (T, error) {
-						return optT.Setter()(a, *val)
-					})
-				}
+		case *registry.ConfigOption[T, []string]:
+			{
+				val := cmd.Flags().StringSlice(name, optT.DefaultVal(), opt.Description())
+				optSettersMap[regName] = append(optSettersMap[regName], func(a T) (T, error) {
+					return optT.Setter()(a, *val)
+				})
+			}
 
-			case *registry.ConfigOption[T, time.Duration]:
-				{
-					val := cmd.Flags().Duration(name, optT.DefaultVal(), opt.Description())
-					optSettersByName[registration.Name] = append(optSettersByName[registration.Name], func(a T) (T, error) {
-						return optT.Setter()(a, *val)
-					})
-				}
+		case *registry.ConfigOption[T, bool]:
+			{
+				val := cmd.Flags().Bool(name, optT.DefaultVal(), opt.Description())
+				optSettersMap[regName] = append(optSettersMap[regName], func(a T) (T, error) {
+					return optT.Setter()(a, *val)
+				})
+			}
 
-			default:
-				log.Debugf("unrecognized attestor option type: %T", optT)
+		case *registry.ConfigOption[T, time.Duration]:
+			{
+				val := cmd.Flags().Duration(name, optT.DefaultVal(), opt.Description())
+				optSettersMap[regName] = append(optSettersMap[regName], func(a T) (T, error) {
+					return optT.Setter()(a, *val)
+				})
 			}
+
+		default:
+			log.Debugf("unrecognized attestor option type: %T", optT)
 		}
 	}
 
+	return optSettersMap
+}
+
+func addFlagsFromRegistry[T any](prefix string, registrationEntries []registry.Entry[T], cmd *cobra.Command) map[string][]func(T) (T, error) {
+	optSettersByName := make(map[string][]func(T) (T, error))
+
+	for _, registration := range registrationEntries {
+		addFlags(prefix, registration.Name, registration.Options, optSettersByName, cmd)
+	}
+
 	return optSettersByName
 }
diff --git a/options/signers.go b/options/signers.go
@@ -15,10 +15,6 @@
 package options
 
 import (
-	"fmt"
-	"strings"
-
-	"github.com/in-toto/go-witness/registry"
 	"github.com/in-toto/go-witness/signer"
 	"github.com/in-toto/go-witness/signer/kms"
 	"github.com/spf13/cobra"
@@ -35,18 +31,9 @@ type KMSSignerProviderOptions map[string][]func(signer.SignerProvider) (signer.S
 
 func (ko *KMSSignerProviderOptions) AddFlags(cmd *cobra.Command) {
 	kmsProviderOpts := kms.ProviderOptions()
-	for k, v := range kmsProviderOpts {
-		if v != nil {
-			opts := v.Init()
-			name := fmt.Sprintf("kms-%s", strings.TrimSuffix(k, "kms://"))
-			// NOTE: this strikes me as a bad idea since it isn't a registry entry. however we wish to piggy back on the add flags logic, and when splitting it out I got errors
-			entry := []registry.Entry[signer.SignerProvider]{
-				{
-					Name:    name,
-					Options: opts,
-				},
-			}
-			*ko = addFlagsFromRegistry[signer.SignerProvider]("signer", entry, cmd)
+	for k := range kmsProviderOpts {
+		if kmsProviderOpts[k] != nil {
+			*ko = addFlags("signer", kmsProviderOpts[k].ProviderName(), kmsProviderOpts[k].Init(), *ko, cmd)
 		}
 	}
 }
diff --git a/options/verifiers.go b/options/verifiers.go
@@ -15,10 +15,6 @@
 package options
 
 import (
-	"fmt"
-	"strings"
-
-	"github.com/in-toto/go-witness/registry"
 	"github.com/in-toto/go-witness/signer"
 	"github.com/in-toto/go-witness/signer/kms"
 	"github.com/spf13/cobra"
@@ -35,18 +31,9 @@ type KMSVerifierProviderOptions map[string][]func(signer.SignerProvider) (signer
 
 func (ko *KMSVerifierProviderOptions) AddFlags(cmd *cobra.Command) {
 	kmsProviderOpts := kms.ProviderOptions()
-	for k, v := range kmsProviderOpts {
-		if v != nil {
-			opts := v.Init()
-			name := fmt.Sprintf("kms-%s", strings.TrimSuffix(k, "kms://"))
-			// NOTE: this strikes me as a bad idea since it isn't a registry entry. however we wish to piggy back on the add flags logic, and when splitting it out I got errors
-			entry := []registry.Entry[signer.SignerProvider]{
-				{
-					Name:    name,
-					Options: opts,
-				},
-			}
-			*ko = addFlagsFromRegistry[signer.SignerProvider]("signer", entry, cmd)
+	for k := range kmsProviderOpts {
+		if kmsProviderOpts[k] != nil {
+			*ko = addFlags("verifier", kmsProviderOpts[k].ProviderName(), kmsProviderOpts[k].Init(), *ko, cmd)
 		}
 	}
 }