diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index df48f7e78..17ae69f2f 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -562,6 +562,97 @@ func initialize(cmd *cobra.Command, out, errOut io.Writer) error { return nil } +// scalarString renders one decoded config value as the text a flag would have +// been given on the command line, and refuses a value that is itself a +// structure. A flag holds values, so a nested sequence or mapping has nowhere to +// go; rendering it would produce a plausible-looking string rather than an +// error. +func scalarString(value any) (string, error) { + switch value.(type) { + case []interface{}: + return "", fmt.Errorf("expects values, but one of them is a list") + case map[string]interface{}: + return "", fmt.Errorf("expects values, but one of them is a set of key/value pairs") + default: + return fmt.Sprintf("%v", value), nil + } +} + +// scalarStrings renders every element of a decoded sequence, refusing the whole +// sequence if any element is itself a structure. +func scalarStrings(values []interface{}) ([]string, error) { + rendered := make([]string, len(values)) + for i, value := range values { + element, err := scalarString(value) + if err != nil { + return nil, err + } + rendered[i] = element + } + return rendered, nil +} + +// applyConfigValue writes a value taken from the config file or the environment +// onto its flag, and refuses one whose shape the flag cannot hold. +// +// A decoded YAML value is one of three things: a sequence, a mapping, or a +// scalar. A flag holds values, not structures, so only two of those shapes have +// anywhere to go - a sequence onto a multi-value flag, a mapping onto a +// key=value flag - and only when their contents are scalars. +// +// Anything else is refused rather than rendered. Rendering is what makes this +// worth stating: %v turns any shape into a plausible-looking string, so a list +// of mappings would become the attestation name "map[name:coverage]" instead of +// an error. The shapes someone can write are unbounded, so the check is on what +// fits rather than on a list of known mistakes: the four ways to arrive here +// without a home are a sequence on a flag that holds one value, a sequence whose +// elements are not scalars, a mapping on a flag that is not key=value, and a +// mapping whose values are not scalars. +func applyConfigValue(flags *pflag.FlagSet, flag *pflag.Flag, value any) error { + switch shaped := value.(type) { + case []interface{}: + sliceValue, isSlice := flag.Value.(pflag.SliceValue) + if !isSlice { + return fmt.Errorf("expects a single value, but a list was given") + } + elements, err := scalarStrings(shaped) + if err != nil { + return err + } + if err := sliceValue.Replace(elements); err != nil { + return err + } + + case map[string]interface{}: + // The pairs are applied one at a time, in the form the flag accepts, + // exactly as repeating the flag on the command line would. The first Set + // replaces whatever the flag held and later ones merge, so the random + // order of a Go map range does not affect the result. + if flag.Value.Type() != "stringToString" { + return fmt.Errorf("expects a single value, but a set of key/value pairs was given") + } + for key, item := range shaped { + if _, err := scalarString(item); err != nil { + return err + } + if err := flags.Set(flag.Name, fmt.Sprintf("%v=%v", key, item)); err != nil { + return err + } + } + return nil + + default: + return flags.Set(flag.Name, fmt.Sprintf("%v", value)) + } + + // Replace writes the value directly, bypassing FlagSet.Set, so the flag must + // be marked as set here. Required-flag validation and every Changed() check + // read this field. + flag.Changed = true + + return nil +} + // configValueSource names where a flag's value came from, so that a failure to // apply it points at the thing the user has to edit. viper reads a value from // the environment when the bound variable holds one, and from the config file @@ -638,7 +729,7 @@ func bindFlags(cmd *cobra.Command, v *viper.Viper) error { } } - if err := cmd.Flags().Set(f.Name, fmt.Sprintf("%v", val)); err != nil { + if err := applyConfigValue(cmd.Flags(), f, val); err != nil { bindErr = errors.Join(bindErr, fmt.Errorf("failed to set flag '--%s' from %s: %v", f.Name, configValueSource(f.Name), err)) } } diff --git a/cmd/kosli/root_test.go b/cmd/kosli/root_test.go index 0555b6820..ecfd711ee 100644 --- a/cmd/kosli/root_test.go +++ b/cmd/kosli/root_test.go @@ -93,6 +93,95 @@ func (suite *RootCommandTestSuite) TestConfigProcessing() { runTestCmd(suite.T(), tests) } +// TestConfigFileYamlListReachesSliceFlagAsElements pins that a list written the +// YAML way reaches a multi-value flag as separate elements, which is the +// spelling anyone writing a YAML config file reaches for first. +// +// attach-policy is used because it reads --environment without mutating it, so +// the assertion sees only what parsing produced. +func (suite *RootCommandTestSuite) TestConfigFileYamlListReachesSliceFlagAsElements() { + c, _, _, _, err := executeCommandC( + "attach-policy mypolicy --config-file testdata/config/yaml-list-value.yaml --dry-run") + suite.Require().NoError(err) + + environments, err := c.Flags().GetStringSlice("environment") + suite.Require().NoError(err) + suite.Equal([]string{"prod", "staging"}, environments) +} + +// TestConfigFileYamlMapReachesMapFlagAsPairs pins that a mapping written the +// YAML way reaches a map-valued flag as its pairs, the counterpart of the list +// case and the other spelling a YAML config file invites. +func (suite *RootCommandTestSuite) TestConfigFileYamlMapReachesMapFlagAsPairs() { + c, _, _, _, err := executeCommandC( + "attest generic --config-file testdata/config/yaml-map-value.yaml " + + "--fingerprint 0000000000000000000000000000000000000000000000000000000000000001 " + + "--name foo --flow f --trail t --dry-run") + suite.Require().NoError(err) + + externalURLs, err := c.Flags().GetStringToString("external-url") + suite.Require().NoError(err) + suite.Equal(map[string]string{"docs": "https://example.com/docs"}, externalURLs) +} + +// TestConfigFileYamlListWithEmptyElementIsRejected pins that reading a YAML list +// does not become a way past the refusal of empty elements. The flag types that +// refuse an empty element do so in Set and in Replace, and a config file list +// arrives through Replace, so the two have to meet here rather than each being +// correct alone. +func (suite *RootCommandTestSuite) TestConfigFileYamlListWithEmptyElementIsRejected() { + _, _, _, _, err := executeCommandC( + "attest generic --config-file testdata/config/yaml-list-with-empty-element.yaml " + + "--fingerprint 0000000000000000000000000000000000000000000000000000000000000001 " + + "--name foo --flow f --trail t --dry-run") + + suite.Require().Error(err) + suite.ErrorContains(err, "attachments") + suite.ErrorContains(err, "empty values are not allowed") +} + +// TestConfigFileValueThatDoesNotFitItsFlagIsRejected is the counterpart to the +// two tests above: the shapes a flag has nowhere to put, and the flag each +// message must name. +// +// A flag holds values, not structures, so a value fits only when it is a scalar, +// a list of scalars on a multi-value flag, or a mapping of scalars on a +// key=value flag. Everything else is refused rather than rendered, which is the +// point: rendering turns any shape into a plausible-looking string, so a list of +// mappings would become the attestation name "map[name:coverage]" and a list on +// --description would become the literal "[a b]" - both the kind of thing +// noticed long after the run that produced it. +// +// The shapes someone can write are unbounded, so the rule is what fits rather +// than a list of known mistakes. New shapes belong here as rows. +func (suite *RootCommandTestSuite) TestConfigFileValueThatDoesNotFitItsFlagIsRejected() { + cases := []struct { + name string + file string + wantFlag string + }{ + { + name: "list on a flag that holds one value", + file: "yaml-list-on-scalar-flag.yaml", + wantFlag: "description", + }, + { + name: "list whose elements are mappings", + file: "yaml-list-of-mappings.yaml", + wantFlag: "template", + }, + } + for _, tc := range cases { + suite.Run(tc.name, func() { + _, _, _, _, err := executeCommandC( + "create flow myflow --config-file testdata/config/" + tc.file + " --dry-run") + + suite.Require().Error(err) + suite.ErrorContains(err, tc.wantFlag) + }) + } +} + // TestEmptyApiTokenEnvVarStillDecryptsConfigToken pins that KOSLI_API_TOKEN set // to an empty string does not suppress decryption of a config-file token. // bindFlags decides the token came from the environment with os.LookupEnv, where diff --git a/cmd/kosli/testdata/config/invalid-flag-value.yaml b/cmd/kosli/testdata/config/invalid-flag-value.yaml index 7e95853ab..702e08122 100644 --- a/cmd/kosli/testdata/config/invalid-flag-value.yaml +++ b/cmd/kosli/testdata/config/invalid-flag-value.yaml @@ -1,4 +1,3 @@ org: demo api-token: DRY_RUN -link: - docs: https://example.com/docs +link: notkeyvalue diff --git a/cmd/kosli/testdata/config/two-invalid-flag-values.yaml b/cmd/kosli/testdata/config/two-invalid-flag-values.yaml index fcc2a546f..70bd08f5d 100644 --- a/cmd/kosli/testdata/config/two-invalid-flag-values.yaml +++ b/cmd/kosli/testdata/config/two-invalid-flag-values.yaml @@ -1,5 +1,4 @@ org: demo api-token: DRY_RUN -link: - docs: https://example.com/docs +link: notkeyvalue max-api-retries: not-a-number diff --git a/cmd/kosli/testdata/config/yaml-list-of-mappings.yaml b/cmd/kosli/testdata/config/yaml-list-of-mappings.yaml new file mode 100644 index 000000000..8e32adfca --- /dev/null +++ b/cmd/kosli/testdata/config/yaml-list-of-mappings.yaml @@ -0,0 +1,5 @@ +org: demo +api-token: DRY_RUN +template: + - name: coverage + - name: unit-test diff --git a/cmd/kosli/testdata/config/yaml-list-on-scalar-flag.yaml b/cmd/kosli/testdata/config/yaml-list-on-scalar-flag.yaml new file mode 100644 index 000000000..67a2075a0 --- /dev/null +++ b/cmd/kosli/testdata/config/yaml-list-on-scalar-flag.yaml @@ -0,0 +1,5 @@ +org: demo +api-token: DRY_RUN +description: + - a + - b diff --git a/cmd/kosli/testdata/config/yaml-list-value.yaml b/cmd/kosli/testdata/config/yaml-list-value.yaml new file mode 100644 index 000000000..ec797ac65 --- /dev/null +++ b/cmd/kosli/testdata/config/yaml-list-value.yaml @@ -0,0 +1,5 @@ +org: demo +api-token: DRY_RUN +environment: + - prod + - staging diff --git a/cmd/kosli/testdata/config/yaml-list-with-empty-element.yaml b/cmd/kosli/testdata/config/yaml-list-with-empty-element.yaml new file mode 100644 index 000000000..b30c9dc96 --- /dev/null +++ b/cmd/kosli/testdata/config/yaml-list-with-empty-element.yaml @@ -0,0 +1,5 @@ +org: demo +api-token: DRY_RUN +attachments: + - "" + - testdata/file1 diff --git a/cmd/kosli/testdata/config/yaml-map-value.yaml b/cmd/kosli/testdata/config/yaml-map-value.yaml new file mode 100644 index 000000000..ecb0ab4e7 --- /dev/null +++ b/cmd/kosli/testdata/config/yaml-map-value.yaml @@ -0,0 +1,4 @@ +org: demo +api-token: DRY_RUN +external-url: + docs: https://example.com/docs