From 30e40ebac4385c6b7d9e292b7ef9de2d72b8b3c0 Mon Sep 17 00:00:00 2001 From: Dave Revell Date: Tue, 5 Sep 2023 15:23:04 -0700 Subject: [PATCH] Add YAML model structs for manifest files Part of #191. These structs will represent the contents of the new "manifest" file, which contains all the information we'll need in the future to cleanly upgrade a template output. This PR contains custom unmarshaling but not custom marshaling. That will be a separate PR. Example manifest file: ``` api_version: 'cli.abcxyz.dev/v1alpha1' template_location: 'github.com/abcxyz/abc.git//t/rest_server' template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' inputs: - name: 'my_input_1' value: 'my_value_1' - name: 'my_input_2' value: 'my_value_2' output_hashes: - file: 'a/b/c.txt' hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c' - file: 'd/e/f.txt' hash: 'h1:7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730' ``` Some of the field names have been changed since the design doc because the old names seemed unclear. --- templates/commands/render/render_test.go | 2 +- templates/model/manifest/manifest.go | 127 +++++++++++++ templates/model/manifest/manifest_test.go | 220 ++++++++++++++++++++++ templates/model/pos.go | 6 +- templates/model/spec/spec.go | 17 +- templates/model/spec/spec_test.go | 8 +- templates/model/test/test.go | 14 +- templates/model/unmarshal.go | 16 ++ templates/model/validate.go | 19 +- 9 files changed, 399 insertions(+), 30 deletions(-) create mode 100644 templates/model/manifest/manifest.go create mode 100644 templates/model/manifest/manifest_test.go diff --git a/templates/commands/render/render_test.go b/templates/commands/render/render_test.go index 5634f04f6..15357efec 100644 --- a/templates/commands/render/render_test.go +++ b/templates/commands/render/render_test.go @@ -307,7 +307,7 @@ steps: wantTemplateContents: map[string]string{ "spec.yaml": "this is an unparseable YAML file *&^#%$", }, - wantErr: "error parsing YAML spec file", + wantErr: "error parsing spec", }, { name: "existing_dest_file_with_overwrite_flag_should_succeed", diff --git a/templates/model/manifest/manifest.go b/templates/model/manifest/manifest.go new file mode 100644 index 000000000..2d9132202 --- /dev/null +++ b/templates/model/manifest/manifest.go @@ -0,0 +1,127 @@ +// Copyright 2023 The Authors (see AUTHORS file) +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package manifest + +import ( + "errors" + "io" + + "github.com/abcxyz/abc/templates/model" + "gopkg.in/yaml.v3" +) + +// Decode unmarshals the YAML Spec from r. This function exists so we can +// validate the Spec model before providing it to the caller; we don't want the +// caller to forget, and thereby introduce bugs. +// +// If the Spec parses successfully but then fails validation, the spec will be +// returned along with the validation error. +func Decode(r io.Reader) (*Manifest, error) { + out := &Manifest{} + if err := model.DecodeAndValidate(r, "manifest", out); err != nil { + return nil, err //nolint:wrapcheck + } + return out, nil +} + +// Manifest represents the contents of a manifest file. A manifest file is the +// set of all information that is needed to cleanly upgrade to a new template +// version in the future. +type Manifest struct { + Pos model.ConfigPos `yaml:"-"` + + APIVersion model.String `yaml:"api_version"` + + // The template source address as passed to `abc templates render` + TemplateLocation model.String `yaml:"template_location"` + + // The dirhash (https://pkg.go.dev/golang.org/x/mod/sumdb/dirhash) of the + // template source tree (not the output). This shows exactly what version of + // the template was installed. + TemplateDirhash model.String `yaml:"template_dirhash"` + + // The input values that were supplied by the user when rendering the template. + Inputs []*Input `yaml:"inputs"` + + // The hash of each output file created by the template. + OutputHashes []*OutputHash `yaml:"output_hashes"` +} + +// UnmarshalYAML implements yaml.Unmarshaler. +func (m *Manifest) UnmarshalYAML(n *yaml.Node) error { + return model.UnmarshalPlain(n, m, &m.Pos) //nolint:wrapcheck +} + +// Validate() implements model.Validator. +func (m *Manifest) Validate() error { + // Inputs and OutputHashes can legally be empty, since a template + // doesn't necessarily have these. + return errors.Join( + model.IsKnownSchemaVersion(&m.Pos, m.APIVersion, "api_version"), + model.NotZeroModel(&m.Pos, m.TemplateLocation, "template_location"), + model.NotZeroModel(&m.Pos, m.TemplateDirhash, "template_dirhash"), + model.ValidateEach(m.Inputs), + model.ValidateEach(m.OutputHashes), + ) +} + +// Input is a YAML object representing an input value that was provided to the +// template when it was rendered. +type Input struct { + Pos model.ConfigPos + + // The name of the template input, e.g. "my_service_account" + Name model.String `yaml:"name"` + // The value of the template input, e.g. "foo@iam.gserviceaccount.com". + Value model.String `yaml:"value"` +} + +// UnmarshalYAML implements yaml.Unmarshaler. +func (i *Input) UnmarshalYAML(n *yaml.Node) error { + return model.UnmarshalPlain(n, i, &i.Pos) //nolint:wrapcheck +} + +// Validate() implements model.Validator. +func (i *Input) Validate() error { + return errors.Join( + model.NotZeroModel(&i.Pos, i.Name, "name"), + model.NotZeroModel(&i.Pos, i.Value, "value"), + ) +} + +// OutputHash records a checksum of a single file as it was created during +// template rendering. +type OutputHash struct { + Pos model.ConfigPos + + // The path, relative to the destination directory, of this file. + File model.String `yaml:"file"` + // The dirhash-style hash (see https://pkg.go.dev/golang.org/x/mod/sumdb/dirhash) + // of this file. The format looks like "h1:0a1b2c3d...". + Hash model.String `yaml:"hash"` +} + +// UnmarshalYAML implements yaml.Unmarshaler. +func (f *OutputHash) UnmarshalYAML(n *yaml.Node) error { + return model.UnmarshalPlain(n, f, &f.Pos) //nolint:wrapcheck +} + +// Validate() implements model.Validator. +func (f *OutputHash) Validate() error { + return errors.Join( + model.NotZeroModel(&f.Pos, f.File, "file"), + model.NotZeroModel(&f.Pos, f.Hash, "hash"), + ) +} diff --git a/templates/model/manifest/manifest_test.go b/templates/model/manifest/manifest_test.go new file mode 100644 index 000000000..8e40ca236 --- /dev/null +++ b/templates/model/manifest/manifest_test.go @@ -0,0 +1,220 @@ +// Copyright 2023 The Authors (see AUTHORS file) +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package manifest + +import ( + "strings" + "testing" + + "github.com/abcxyz/abc/templates/model" + "github.com/abcxyz/pkg/testutil" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "gopkg.in/yaml.v3" +) + +func TestDecode(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + in string + want *Manifest + wantUnmarshalErr string + wantValidateErr string + }{ + { + name: "simple-success", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' + - name: 'my_input_2' + value: 'my_value_2' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c' + - file: 'd/e/f.txt' + hash: 'h1:7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730'`, + want: &Manifest{ + APIVersion: model.String{Val: "cli.abcxyz.dev/v1alpha1"}, + TemplateLocation: model.String{Val: "github.com/abcxyz/abc.git//t/rest_server"}, + TemplateDirhash: model.String{Val: "h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"}, + Inputs: []*Input{ + { + Name: model.String{Val: "my_input_1"}, + Value: model.String{Val: "my_value_1"}, + }, + { + Name: model.String{Val: "my_input_2"}, + Value: model.String{Val: "my_value_2"}, + }, + }, + OutputHashes: []*OutputHash{ + { + File: model.String{Val: "a/b/c.txt"}, + Hash: model.String{Val: "h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c"}, + }, + { + File: model.String{Val: "d/e/f.txt"}, + Hash: model.String{Val: "h1:7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730"}, + }, + }, + }, + }, + { + name: "fields-missing", + in: `api_version: "foo"`, + wantValidateErr: `at line 1 column 14: field "api_version" value must be one of [cli.abcxyz.dev/v1alpha1] +at line 1 column 1: field "template_location" is required +at line 1 column 1: field "template_dirhash" is required`, + }, + { + name: "input-missing-name", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - value: 'my_value_1' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + wantValidateErr: `at line 6 column 5: field "name" is required`, + }, + { + name: "input-missing-value", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + wantValidateErr: `at line 6 column 5: field "value" is required`, + }, + { + name: "output-hash-missing-file", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' +output_hashes: + - hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + wantValidateErr: `at line 9 column 5: field "file" is required`, + }, + { + name: "output-hash-missing-file", + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' +output_hashes: + - file: 'a/b/c.txt'`, + wantValidateErr: `at line 9 column 5: field "hash" is required`, + }, + { + name: "no-hashes", // It's rare but legal for a template to have no output files + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +inputs: + - name: 'my_input_1' + value: 'my_value_1' +`, + want: &Manifest{ + APIVersion: model.String{Val: "cli.abcxyz.dev/v1alpha1"}, + TemplateLocation: model.String{Val: "github.com/abcxyz/abc.git//t/rest_server"}, + TemplateDirhash: model.String{Val: "h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"}, + Inputs: []*Input{ + { + Name: model.String{Val: "my_input_1"}, + Value: model.String{Val: "my_value_1"}, + }, + }, + }, + }, + { + name: "no-inputs", // It's legal for a template to have no inputs + in: ` +api_version: 'cli.abcxyz.dev/v1alpha1' +template_location: 'github.com/abcxyz/abc.git//t/rest_server' +template_dirhash: 'h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03' +output_hashes: + - file: 'a/b/c.txt' + hash: 'h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'`, + want: &Manifest{ + APIVersion: model.String{Val: "cli.abcxyz.dev/v1alpha1"}, + TemplateLocation: model.String{Val: "github.com/abcxyz/abc.git//t/rest_server"}, + TemplateDirhash: model.String{Val: "h1:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03"}, + OutputHashes: []*OutputHash{ + { + File: model.String{Val: "a/b/c.txt"}, + Hash: model.String{Val: "h1:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c"}, + }, + }, + }, + }, + { + name: "bad-yaml-syntax", + in: `[[[[[[[`, + wantUnmarshalErr: "did not find expected node content", + }, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := &Manifest{} + dec := yaml.NewDecoder(strings.NewReader(tc.in)) + err := dec.Decode(got) + + if diff := testutil.DiffErrString(err, tc.wantUnmarshalErr); diff != "" { + t.Fatal(diff) + } + if err != nil { + return + } + + err = got.Validate() + if diff := testutil.DiffErrString(err, tc.wantValidateErr); diff != "" { + t.Fatal(diff) + } + if err != nil { + return + } + + opt := cmpopts.IgnoreTypes(&model.ConfigPos{}, model.ConfigPos{}) // don't force test authors to assert the line and column numbers + if diff := cmp.Diff(got, tc.want, opt); diff != "" { + t.Errorf("unmarshaling didn't yield expected struct. Diff (-got +want): %s", diff) + } + }) + } +} diff --git a/templates/model/pos.go b/templates/model/pos.go index 7da8eb4b7..a5056ce81 100644 --- a/templates/model/pos.go +++ b/templates/model/pos.go @@ -33,6 +33,10 @@ type ConfigPos struct { Column int } +func (c ConfigPos) IsZero() bool { + return c == ConfigPos{} +} + // YAMLPos constructs a position struct based on a YAML parse cursor. func YAMLPos(n *yaml.Node) *ConfigPos { return &ConfigPos{ @@ -51,7 +55,7 @@ func YAMLPos(n *yaml.Node) *ConfigPos { // Creating an error: c.Errorf("something went wrong doing action %s", action) func (c *ConfigPos) Errorf(fmtStr string, args ...any) error { err := fmt.Errorf(fmtStr, args...) - if c == nil || *c == (ConfigPos{}) { + if c == nil || c.IsZero() { return err } diff --git a/templates/model/spec/spec.go b/templates/model/spec/spec.go index e3b602714..a9c38cce8 100644 --- a/templates/model/spec/spec.go +++ b/templates/model/spec/spec.go @@ -19,7 +19,6 @@ package spec import ( "errors" - "fmt" "io" "strings" @@ -35,19 +34,11 @@ import ( // If the Spec parses successfully but then fails validation, the spec will be // returned along with the validation error. func Decode(r io.Reader) (*Spec, error) { - dec := newDecoder(r) - var spec Spec - if err := dec.Decode(&spec); err != nil { - return nil, fmt.Errorf("error parsing YAML spec file: %w", err) + out := &Spec{} + if err := model.DecodeAndValidate(r, "spec", out); err != nil { + return nil, err } - return &spec, spec.Validate() -} - -// newDecoder returns a yaml Decoder with the desired options. -func newDecoder(r io.Reader) *yaml.Decoder { - dec := yaml.NewDecoder(r) - dec.KnownFields(true) // Fail if any unexpected fields are seen. Often doesn't work: https://github.com/go-yaml/yaml/issues/460 - return dec + return out, nil } // Spec represents a parsed spec.yaml file describing a template. diff --git a/templates/model/spec/spec_test.go b/templates/model/spec/spec_test.go index 4b4ab35c1..95f05eec2 100644 --- a/templates/model/spec/spec_test.go +++ b/templates/model/spec/spec_test.go @@ -22,6 +22,7 @@ import ( "github.com/abcxyz/pkg/testutil" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "gopkg.in/yaml.v3" ) func TestSpecUnmarshal(t *testing.T) { @@ -178,7 +179,8 @@ steps: t.Run(tc.name, func(t *testing.T) { t.Parallel() got := &Spec{} - dec := newDecoder(strings.NewReader(tc.in)) + + dec := yaml.NewDecoder(strings.NewReader(tc.in)) err := dec.Decode(got) if diff := testutil.DiffErrString(err, tc.wantUnmarshalErr); diff != "" { t.Fatal(diff) @@ -317,7 +319,7 @@ rules: t.Run(tc.name, func(t *testing.T) { t.Parallel() got := &Input{} - dec := newDecoder(strings.NewReader(tc.in)) + dec := yaml.NewDecoder(strings.NewReader(tc.in)) err := dec.Decode(got) if diff := testutil.DiffErrString(err, tc.wantUnmarshalErr); diff != "" { t.Fatal(diff) @@ -1129,7 +1131,7 @@ params: t.Run(tc.name, func(t *testing.T) { t.Parallel() got := &Step{} - dec := newDecoder(strings.NewReader(tc.in)) + dec := yaml.NewDecoder(strings.NewReader(tc.in)) err := dec.Decode(got) if diff := testutil.DiffErrString(err, tc.wantUnmarshalErr); diff != "" { t.Fatal(diff) diff --git a/templates/model/test/test.go b/templates/model/test/test.go index 24f810c45..7ab6734fd 100644 --- a/templates/model/test/test.go +++ b/templates/model/test/test.go @@ -16,7 +16,6 @@ package test import ( "errors" - "fmt" "io" "github.com/abcxyz/abc/templates/model" @@ -58,7 +57,7 @@ type Test struct { // Validate implements model.Validator. func (t *Test) Validate() error { return errors.Join( - model.OneOf(&t.Pos, t.APIVersion, []string{"cli.abcxyz.dev/v1alpha1"}, "api_version"), + model.IsKnownSchemaVersion(&t.Pos, t.APIVersion, "api_version"), model.ValidateEach(t.Inputs), ) } @@ -70,16 +69,9 @@ func (t *Test) UnmarshalYAML(n *yaml.Node) error { // DecodeTest unmarshals the YAML Spec from r. func DecodeTest(r io.Reader) (*Test, error) { - dec := yaml.NewDecoder(r) - dec.KnownFields(true) - var test Test - if err := dec.Decode(&test); err != nil { - return nil, fmt.Errorf("error parsing test YAML file: %w", err) - } - - if err := test.Validate(); err != nil { - return nil, err + if err := model.DecodeAndValidate(r, "test", &test); err != nil { + return nil, err //nolint:wrapcheck } return &test, nil } diff --git a/templates/model/unmarshal.go b/templates/model/unmarshal.go index c88ef751b..771efed71 100644 --- a/templates/model/unmarshal.go +++ b/templates/model/unmarshal.go @@ -26,6 +26,8 @@ package model // based on the value of the "action" field. import ( + "fmt" + "io" "reflect" "strings" @@ -86,3 +88,17 @@ func UnmarshalPlain(n *yaml.Node, outPtr any, outPos *ConfigPos, extraYAMLFields *outPos = *YAMLPos(n) return nil } + +// DecodeAndValidate unmarshals the YAML text in the given Reader into the given +// pointer-to-struct, and calls Validate() on it. Returns any unmarshaling error +// or validation error. +func DecodeAndValidate(r io.Reader, filename string, outPtr Validator) error { + dec := yaml.NewDecoder(r) + if err := dec.Decode(outPtr); err != nil { + return fmt.Errorf("error parsing %s YAML file: %w", filename, err) + } + if err := outPtr.Validate(); err != nil { + return fmt.Errorf("validation failed in %s YAML file: %w", filename, err) + } + return nil +} diff --git a/templates/model/validate.go b/templates/model/validate.go index aeb35d92b..ae2a9093c 100644 --- a/templates/model/validate.go +++ b/templates/model/validate.go @@ -70,13 +70,30 @@ func IsValidRegexGroupName(s String, fieldName string) error { } // OneOf returns error if x.Val is not one of the given allowed choices. -func OneOf[T comparable](pos *ConfigPos, x valWithPos[T], allowed []T, fieldName string) error { +func OneOf[T comparable](parentPos *ConfigPos, x valWithPos[T], allowed []T, fieldName string) error { if slices.Contains(allowed, x.Val) { return nil } + pos := x.Pos + if pos == nil || pos.IsZero() { + pos = parentPos + } + return pos.Errorf("field %q value must be one of %v", fieldName, allowed) } +var schemaVersions = []string{"cli.abcxyz.dev/v1alpha1"} + +// IsKnownSchemaVersion returns error if the given string is not one of the +// accepted abc schema versions. +// +// parentPos is the position of the yaml object that contains the api_version +// field. We need this because if the api_version field is missing from the +// YAML, then we won't have any position information for it. +func IsKnownSchemaVersion(parentPos *ConfigPos, apiVersion String, fieldName string) error { + return OneOf(parentPos, apiVersion, schemaVersions, fieldName) +} + // extrafields returns error if any unexpected fields are seen. The input must // be a mapping/object; anything else will result in an error. This is a // workaround for a bug in KnownFields in the upstream yaml lib