From 92880e04072f88d814ea405a77629d5aa3ab6bd6 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 24 Jul 2024 14:55:46 +0200 Subject: [PATCH 01/17] feat(plan): add plan section support --- internals/plan/extensions_test.go | 689 ++++++++++++++++++++++++++++++ internals/plan/plan.go | 222 +++++++++- internals/plan/plan_test.go | 27 +- 3 files changed, 925 insertions(+), 13 deletions(-) create mode 100644 internals/plan/extensions_test.go diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go new file mode 100644 index 00000000..d71fe1e4 --- /dev/null +++ b/internals/plan/extensions_test.go @@ -0,0 +1,689 @@ +// Copyright (c) 2024 Canonical Ltd +// +// 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 plan_test + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "strings" + + . "gopkg.in/check.v1" + "gopkg.in/yaml.v3" + + "github.com/canonical/pebble/internals/plan" +) + +type inputLayer struct { + order int + label string + yaml string +} + +// PlanResult represents the final content of a combined plan. Since this +// package exclusively focuses extensions, all built-in sections are empty +// and ignored in the test results. +type planResult struct { + x *xSection + y *ySection +} + +var extensionTests = []struct { + extensions map[string]plan.LayerSectionExtension + layers []*inputLayer + errorString string + combinedPlanResult *planResult + combinedPlanResultYaml string +}{ + // Index 0: No Sections + { + combinedPlanResultYaml: string(reindent(` + {}`)), + }, + // Index 1: Sections with empty YAML + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + combinedPlanResult: &planResult{ + x: &xSection{}, + y: &ySection{}, + }, + combinedPlanResultYaml: string(reindent(` + {}`)), + }, + // Index 2: Load file layers invalid section + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-xy", + yaml: ` + summary: xy + description: desc + invalid: + `, + }, + }, + errorString: "cannot parse layer .*: unknown section .*", + }, + // Index 3: Load file layers not unique order + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-1", + yaml: ` + summary: xy + description: desc + `, + }, + &inputLayer{ + order: 1, + label: "layer-2", + yaml: ` + summary: xy + description: desc + `, + }, + }, + errorString: "invalid layer filename: .* not unique .*", + }, + // Index 4: Load file layers not unique label + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-xy", + yaml: ` + summary: xy + description: desc + `, + }, + &inputLayer{ + order: 2, + label: "layer-xy", + yaml: ` + summary: xy + description: desc + `, + }, + }, + errorString: "invalid layer filename: .* not unique .*", + }, + // Index 5: Load file layers with empty section + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + `, + }, + &inputLayer{ + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y + `, + }, + }, + combinedPlanResult: &planResult{ + x: &xSection{}, + y: &ySection{}, + }, + combinedPlanResultYaml: string("{}\n"), + }, + // Index 6: Load file layers with section validation failure #1 + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + z1: + override: replace + a: a + b: b + `, + }, + }, + errorString: "cannot validate layer section .* cannot accept entry not starting .*", + }, + // Index 7: Load file layers with section validation failure #2 + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + x1: + `, + }, + }, + errorString: "cannot validate layer section .* cannot have nil entry .*", + }, + // Index 8: Load file layers failed plan validation + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y2 + `, + }, + &inputLayer{ + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y + y-field: + y1: + override: replace + a: a + b: b + `, + }, + }, + errorString: "cannot validate plan section .* cannot find .* as required by .*", + }, + // Index 9: Check empty section omits entry + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + `, + }, + &inputLayer{ + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y + y-field: + `, + }, + }, + combinedPlanResult: &planResult{ + x: &xSection{}, + y: &ySection{}, + }, + combinedPlanResultYaml: string(reindent(` + {}`)), + }, + // Index 10: Load file layers + { + extensions: map[string]plan.LayerSectionExtension{ + "x-field": &xExtension{}, + "y-field": &yExtension{}, + }, + layers: []*inputLayer{ + &inputLayer{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y1 + `, + }, + &inputLayer{ + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y + y-field: + y1: + override: replace + a: a + b: b + `, + }, + }, + combinedPlanResult: &planResult{ + x: &xSection{ + Entries: map[string]*X{ + "x1": &X{ + Name: "x1", + Override: plan.ReplaceOverride, + A: "a", + B: "b", + Y: []string{ + "y1", + }, + }, + }, + }, + y: &ySection{ + Entries: map[string]*Y{ + "y1": &Y{ + Name: "y1", + Override: plan.ReplaceOverride, + A: "a", + B: "b", + }, + }, + }, + }, + combinedPlanResultYaml: string(reindent(` + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y1 + y-field: + y1: + override: replace + a: a + b: b`)), + }, +} + +func (s *S) TestPlanExtensions(c *C) { + for testIndex, planTest := range extensionTests { + c.Logf("Running TestPlanExtensions with test data index %v", testIndex) + + // Write layers to test directory. + layersDir := filepath.Join(c.MkDir(), "layers") + s.writeLayerFiles(c, layersDir, planTest.layers) + var p *plan.Plan + + // Register test extensions. + for field, extension := range planTest.extensions { + plan.RegisterExtension(field, extension) + } + + // Load the plan layer from disk (parse, combine and validate). + p, err := plan.ReadDir(layersDir) + if planTest.errorString != "" || err != nil { + // Expected error. + c.Assert(err, ErrorMatches, planTest.errorString) + } else { + if _, ok := planTest.extensions[xField]; ok { + // Verify "x-field" data. + var x *xSection + err = p.Section(xField, &x) + c.Assert(err, IsNil) + c.Assert(x.Entries, DeepEquals, planTest.combinedPlanResult.x.Entries) + } + + if _, ok := planTest.extensions[yField]; ok { + // Verify "y-field" data. + var y *ySection + err = p.Section(yField, &y) + c.Assert(err, IsNil) + c.Assert(y.Entries, DeepEquals, planTest.combinedPlanResult.y.Entries) + } + + // Verify combined plan YAML. + planYAML, err := yaml.Marshal(p) + c.Assert(err, IsNil) + c.Assert(string(planYAML), Equals, planTest.combinedPlanResultYaml) + } + + // Unregister test extensions. + for field, _ := range planTest.extensions { + plan.UnregisterExtension(field) + } + } +} + +// writeLayerFiles writes layer files of a test to disk. +func (s *S) writeLayerFiles(c *C, layersDir string, inputs []*inputLayer) { + err := os.MkdirAll(layersDir, 0755) + c.Assert(err, IsNil) + + for _, input := range inputs { + err := ioutil.WriteFile(filepath.Join(layersDir, fmt.Sprintf("%03d-%s.yaml", input.order, input.label)), reindent(input.yaml), 0644) + c.Assert(err, IsNil) + } +} + +const xField string = "x-field" + +// xExtension implements the LayerSectionExtension interface. +type xExtension struct{} + +func (x xExtension) ParseSection(data yaml.Node) (plan.LayerSection, error) { + xs := &xSection{} + err := data.Decode(xs) + if err != nil { + return nil, err + } + // Propagate the name. + for name, entry := range xs.Entries { + if entry != nil { + xs.Entries[name].Name = name + } + } + return xs, nil +} + +func (x xExtension) CombineSections(sections ...plan.LayerSection) (plan.LayerSection, error) { + xs := &xSection{} + for _, section := range sections { + err := xs.Combine(section) + if err != nil { + return nil, err + } + } + return xs, nil +} + +func (x xExtension) ValidatePlan(p *plan.Plan) error { + var xs *xSection + err := p.Section(xField, &xs) + if err != nil { + return err + } + if xs != nil { + var ys *ySection + err = p.Section(yField, &ys) + if err != nil { + return err + } + if ys == nil { + return fmt.Errorf("cannot validate %v field without %v field", xField, yField) + } + + // Test dependency: Make sure every Y field in X refer to an existing Y entry. + for xEntryField, xEntryValue := range xs.Entries { + for _, yReference := range xEntryValue.Y { + found := false + for yEntryField, _ := range ys.Entries { + if yReference == yEntryField { + found = true + break + } + } + if !found { + return fmt.Errorf("cannot find ySection entry %v as required by xSection entry %v ", yReference, xEntryField) + } + } + } + } + return nil +} + +// xSection is the backing type for xExtension. +type xSection struct { + Entries map[string]*X `yaml:",inline,omitempty"` +} + +func (xs *xSection) Validate() error { + for field, entry := range xs.Entries { + if entry == nil { + return fmt.Errorf("cannot have nil entry for %q", field) + } + // Fictitious test requirement: entry names must start with x + if !strings.HasPrefix(field, "x") { + return fmt.Errorf("cannot accept entry not starting with letter 'x'") + } + } + return nil +} + +func (xs *xSection) IsZero() bool { + return xs.Entries == nil +} + +func (xs *xSection) Combine(other plan.LayerSection) error { + otherxSection, ok := other.(*xSection) + if !ok { + return fmt.Errorf("cannot combine incompatible section type") + } + + for field, entry := range otherxSection.Entries { + xs.Entries = makeMapIfNil(xs.Entries) + switch entry.Override { + case plan.MergeOverride: + if old, ok := xs.Entries[field]; ok { + copied := old.Copy() + copied.Merge(entry) + xs.Entries[field] = copied + break + } + fallthrough + case plan.ReplaceOverride: + xs.Entries[field] = entry.Copy() + case plan.UnknownOverride: + return &plan.FormatError{ + Message: fmt.Sprintf(`invalid "override" value for entry %q`, field), + } + default: + return &plan.FormatError{ + Message: fmt.Sprintf(`unknown "override" value for entry %q`, field), + } + } + } + return nil +} + +type X struct { + Name string `yaml:"-"` + Override plan.Override `yaml:"override,omitempty"` + A string `yaml:"a,omitempty"` + B string `yaml:"b,omitempty"` + C string `yaml:"c,omitempty"` + Y []string `yaml:"y-field,omitempty"` +} + +func (x *X) Copy() *X { + copied := *x + copied.Y = append([]string(nil), x.Y...) + return &copied +} + +func (x *X) Merge(other *X) { + if other.A != "" { + x.A = other.A + } + if other.B != "" { + x.B = other.B + } + if other.C != "" { + x.C = other.C + } + x.Y = append(x.Y, other.Y...) +} + +const yField string = "y-field" + +// yExtension implements the LayerSectionExtension interface. +type yExtension struct{} + +func (y yExtension) ParseSection(data yaml.Node) (plan.LayerSection, error) { + ys := &ySection{} + err := data.Decode(ys) + if err != nil { + return nil, err + } + // Propagate the name. + for name, entry := range ys.Entries { + if entry != nil { + ys.Entries[name].Name = name + } + } + return ys, nil +} + +func (y yExtension) CombineSections(sections ...plan.LayerSection) (plan.LayerSection, error) { + ys := &ySection{} + for _, section := range sections { + err := ys.Combine(section) + if err != nil { + return nil, err + } + } + return ys, nil +} + +func (y yExtension) ValidatePlan(p *plan.Plan) error { + // This extension has no dependencies on the Plan to validate. + return nil +} + +// ySection is the backing type for yExtension. +type ySection struct { + Entries map[string]*Y `yaml:",inline,omitempty"` +} + +func (ys *ySection) Validate() error { + for field, entry := range ys.Entries { + if entry == nil { + return fmt.Errorf("cannot have nil entry for %q", field) + } + // Fictitious test requirement: entry names must start with y + if !strings.HasPrefix(field, "y") { + return fmt.Errorf("cannot accept entry not starting with letter 'y'") + } + } + return nil +} + +func (ys *ySection) IsZero() bool { + return ys.Entries == nil +} + +func (ys *ySection) Combine(other plan.LayerSection) error { + otherySection, ok := other.(*ySection) + if !ok { + return fmt.Errorf("cannot combine incompatible section type") + } + + for field, entry := range otherySection.Entries { + ys.Entries = makeMapIfNil(ys.Entries) + switch entry.Override { + case plan.MergeOverride: + if old, ok := ys.Entries[field]; ok { + copied := old.Copy() + copied.Merge(entry) + ys.Entries[field] = copied + break + } + fallthrough + case plan.ReplaceOverride: + ys.Entries[field] = entry.Copy() + case plan.UnknownOverride: + return &plan.FormatError{ + Message: fmt.Sprintf(`invalid "override" value for entry %q`, field), + } + default: + return &plan.FormatError{ + Message: fmt.Sprintf(`unknown "override" value for entry %q`, field), + } + } + } + return nil +} + +type Y struct { + Name string `yaml:"-"` + Override plan.Override `yaml:"override,omitempty"` + A string `yaml:"a,omitempty"` + B string `yaml:"b,omitempty"` + C string `yaml:"c,omitempty"` +} + +func (y *Y) Copy() *Y { + copied := *y + return &copied +} + +func (y *Y) Merge(other *Y) { + if other.A != "" { + y.A = other.A + } + if other.B != "" { + y.B = other.B + } + if other.C != "" { + y.C = other.C + } +} + +func makeMapIfNil[K comparable, V any](m map[K]V) map[K]V { + if m == nil { + m = make(map[K]V) + } + return m +} diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 2b669e63..e35d88ea 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -32,6 +32,31 @@ import ( "github.com/canonical/pebble/internals/osutil" ) +// LayerSectionExtension allows the plan layer schema to be extended without +// adding centralised schema knowledge to the plan library. +type LayerSectionExtension interface { + // ParseSection returns a newly allocated concrete type containing the + // unmarshalled section content. + ParseSection(data yaml.Node) (LayerSection, error) + + // CombineSections returns a newly allocated concrete type containing the + // result of combining the supplied sections in order. + CombineSections(sections ...LayerSection) (LayerSection, error) + + // ValidatePlan takes the complete plan as input, and allows the + // extension to validate the plan. This can be used for cross section + // dependency validation. + ValidatePlan(plan *Plan) error +} + +type LayerSection interface { + // Ask the section to validate itself. + Validate() error + + // Returns true if the section is empty. + IsZero() bool +} + const ( defaultBackoffDelay = 500 * time.Millisecond defaultBackoffFactor = 2.0 @@ -42,11 +67,80 @@ const ( defaultCheckThreshold = 3 ) +// layerExtensions keeps a map of registered extensions. +var layerExtensions = map[string]LayerSectionExtension{} + +// RegisterExtension adds a plan schema extension. All registrations must be +// done before the plan library is used. +func RegisterExtension(field string, ext LayerSectionExtension) error { + if _, ok := layerExtensions[field]; ok { + return fmt.Errorf("internal error: extension %q already registered", field) + } + layerExtensions[field] = ext + return nil +} + +// UnregisterExtension removes a plan schema extension. This is only +// intended for use by tests during cleanup. +func UnregisterExtension(field string) { + delete(layerExtensions, field) +} + type Plan struct { Layers []*Layer `yaml:"-"` Services map[string]*Service `yaml:"services,omitempty"` Checks map[string]*Check `yaml:"checks,omitempty"` LogTargets map[string]*LogTarget `yaml:"log-targets,omitempty"` + + Sections map[string]LayerSection `yaml:",inline"` +} + +// Section retrieves a section from the plan. +func (p *Plan) Section(field string, out interface{}) error { + if _, found := layerExtensions[field]; !found { + return fmt.Errorf("cannot find registered extension for field %q", field) + } + + outVal := reflect.ValueOf(out) + if outVal.Kind() != reflect.Ptr || outVal.IsNil() { + return fmt.Errorf("cannot read non pointer to section type %q", outVal.Kind()) + } + + section, exists := p.Sections[field] + if !exists { + return fmt.Errorf("internal error: section %q is nil", field) + } + + sectionVal := reflect.ValueOf(section) + sectionType := sectionVal.Type() + outValPtrType := outVal.Elem().Type() + if !sectionType.AssignableTo(outValPtrType) { + return fmt.Errorf("cannot assign value of type %s to out argument of type %s", sectionType, outValPtrType) + } + outVal.Elem().Set(sectionVal) + return nil +} + +// MarshalYAML implements an override for top level omitempty tags handling. +// This is required since Sections are based on an inlined map, for which +// omitempty and inline together is not currently supported. +func (p *Plan) MarshalYAML() (interface{}, error) { + marshalData := make(map[string]interface{}) + if len(p.Services) != 0 { + marshalData["services"] = p.Services + } + if len(p.LogTargets) != 0 { + marshalData["log-targets"] = p.LogTargets + } + if len(p.Checks) != 0 { + marshalData["checks"] = p.Checks + } + for field, section := range p.Sections { + if !section.IsZero() { + marshalData[field] = section + } + } + return marshalData, nil } type Layer struct { @@ -57,6 +151,8 @@ type Layer struct { Services map[string]*Service `yaml:"services,omitempty"` Checks map[string]*Check `yaml:"checks,omitempty"` LogTargets map[string]*LogTarget `yaml:"log-targets,omitempty"` + + Sections map[string]LayerSection `yaml:",inline"` } type Service struct { @@ -559,7 +655,29 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { Services: make(map[string]*Service), Checks: make(map[string]*Check), LogTargets: make(map[string]*LogTarget), + Sections: make(map[string]LayerSection), + } + + // Combine the same sections from each layer. Note that we do this before + // the layers length check because we need the extension to provide us with + // a zero value section, even if no layers are supplied (similar to the + // allocations taking place above for the built-in types). + for field, extension := range layerExtensions { + var sections []LayerSection + for _, layer := range layers { + if section := layer.Sections[field]; section != nil { + sections = append(sections, section) + } + } + var err error + combined.Sections[field], err = extension.CombineSections(sections...) + if err != nil { + return nil, &FormatError{ + Message: fmt.Sprintf(`cannot combine section %q: %v`, field, err), + } + } } + if len(layers) == 0 { return combined, nil } @@ -825,11 +943,18 @@ func (layer *Layer) Validate() error { } } + for field, section := range layer.Sections { + err := section.Validate() + if err != nil { + return fmt.Errorf("cannot validate layer section %q: %w", field, err) + } + } + return nil } -// Validate checks that the combined layers form a valid plan. -// See also Layer.Validate, which checks that the individual layers are valid. +// Validate checks that the combined layers form a valid plan. See also +// Layer.Validate, which checks that the individual layers are valid. func (p *Plan) Validate() error { for name, service := range p.Services { if service.Command == "" { @@ -917,6 +1042,15 @@ func (p *Plan) Validate() error { if err != nil { return err } + + // Each section extension must validate the combined plan. + for field, extension := range layerExtensions { + err = extension.ValidatePlan(p) + if err != nil { + return fmt.Errorf("cannot validate plan section %q: %w", field, err) + } + } + return nil } @@ -1085,19 +1219,84 @@ func (p *Plan) checkCycles() error { } func ParseLayer(order int, label string, data []byte) (*Layer, error) { - layer := Layer{ - Services: map[string]*Service{}, - Checks: map[string]*Check{}, - LogTargets: map[string]*LogTarget{}, - } - dec := yaml.NewDecoder(bytes.NewBuffer(data)) - dec.KnownFields(true) - err := dec.Decode(&layer) + layer := &Layer{ + Services: make(map[string]*Service), + Checks: make(map[string]*Check), + LogTargets: make(map[string]*LogTarget), + Sections: make(map[string]LayerSection), + } + + // The following manual approach is required because: + // + // 1. Extended sections are YAML inlined, and also do not have a + // concrete type at this level, we cannot simply unmarshal the layer + // in one step. + // + // 2. We honor KnownFields = true behaviour for non extended schema + // sections, and at the top field level, which includes Section field + // names. + builtinSections := map[string]interface{}{ + "summary": &layer.Summary, + "description": &layer.Description, + "services": &layer.Services, + "checks": &layer.Checks, + "log-targets": &layer.LogTargets, + } + + layerSections := make(map[string]yaml.Node) + // Deliberately pre-allocate at least an empty yaml.Node for every + // extension section. Extension sections that have unmarshalled + // will update the respective node, while non-existing sections + // will at least have an empty node. This means we can consistently + // let the extension allocate and decode the yaml node for all sections, + // and in the case where it is zero, we get an empty backing type instance. + for field, _ := range layerExtensions { + layerSections[field] = yaml.Node{} + } + err := yaml.Unmarshal(data, &layerSections) if err != nil { return nil, &FormatError{ Message: fmt.Sprintf("cannot parse layer %q: %v", label, err), } } + + for field, section := range layerSections { + if _, builtin := builtinSections[field]; builtin { + // The following issue prevents us from using the yaml.Node decoder + // with KnownFields = true behaviour. Once one of the proposals get + // merged, we can remove the intermediate Marshal step. + // https://github.com/go-yaml/yaml/issues/460 + data, err := yaml.Marshal(§ion) + if err != nil { + return nil, fmt.Errorf("internal error: cannot marshal %v section: %w", field, err) + } + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + if err = dec.Decode(builtinSections[field]); err != nil { + return nil, &FormatError{ + Message: fmt.Sprintf("cannot parse layer %q section %q: %v", label, field, err), + } + } + } else { + if extension, ok := layerExtensions[field]; ok { + // Section unmarshal rules are defined by the extension itself. + layer.Sections[field], err = extension.ParseSection(section) + if err != nil { + return nil, &FormatError{ + Message: fmt.Sprintf("cannot parse layer %q section %q: %v", label, field, err), + } + } + } else { + // At the top level we do not ignore keys we do not understand. + // This preserves the current Pebble behaviour of decoding with + // KnownFields = true. + return nil, &FormatError{ + Message: fmt.Sprintf("cannot parse layer %q: unknown section %q", label, field), + } + } + } + } + layer.Order = order layer.Label = label @@ -1125,7 +1324,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { return nil, err } - return &layer, err + return layer, err } func validServiceAction(action ServiceAction, additionalValid ...ServiceAction) bool { @@ -1228,6 +1427,7 @@ func ReadDir(layersDir string) (*Plan, error) { Services: combined.Services, Checks: combined.Checks, LogTargets: combined.LogTargets, + Sections: combined.Sections, } err = plan.Validate() if err != nil { diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 4d740c28..fbc1fb96 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -203,6 +203,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, { Order: 1, Label: "layer-1", @@ -253,6 +254,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }}, result: &plan.Layer{ Summary: "Simple override layer.", @@ -332,6 +334,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, start: map[string][]string{ "srv1": {"srv2", "srv1", "srv3"}, @@ -394,6 +397,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }}, }, { summary: "Unknown keys are not accepted", @@ -477,7 +481,7 @@ var planTests = []planTest{{ `}, }, { summary: `Invalid backoff-delay duration`, - error: `cannot parse layer "layer-0": invalid duration "foo"`, + error: `cannot parse layer "layer-0" section "services": invalid duration "foo"`, input: []string{` services: "svc1": @@ -507,7 +511,7 @@ var planTests = []planTest{{ `}, }, { summary: `Invalid backoff-factor`, - error: `cannot parse layer "layer-0": invalid floating-point number "foo"`, + error: `cannot parse layer "layer-0" section "services": invalid floating-point number "foo"`, input: []string{` services: "svc1": @@ -544,6 +548,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }}, }, { summary: `Invalid service command: cannot have any arguments after [ ... ] group`, @@ -652,6 +657,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Checks override replace works correctly", @@ -729,6 +735,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Checks override merge works correctly", @@ -812,6 +819,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Timeout is capped at period", @@ -841,6 +849,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Unset timeout is capped at period", @@ -869,6 +878,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "One of http, tcp, or exec must be present for check", @@ -989,6 +999,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Overriding log targets", @@ -1085,6 +1096,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, + Sections: map[string]plan.LayerSection{}, }, { Label: "layer-1", Order: 1, @@ -1123,6 +1135,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, + Sections: map[string]plan.LayerSection{}, }}, result: &plan.Layer{ Services: map[string]*plan.Service{ @@ -1168,6 +1181,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Log target requires type field", @@ -1277,6 +1291,7 @@ var planTests = []planTest{{ }, }, }, + Sections: map[string]plan.LayerSection{}, }, { Order: 1, Label: "layer-1", @@ -1302,6 +1317,7 @@ var planTests = []planTest{{ }, }, }, + Sections: map[string]plan.LayerSection{}, }}, result: &plan.Layer{ Services: map[string]*plan.Service{}, @@ -1329,6 +1345,7 @@ var planTests = []planTest{{ }, }, }, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Reserved log target labels", @@ -1379,6 +1396,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, + Sections: map[string]plan.LayerSection{}, }, }, { summary: "Three layers missing command", @@ -1452,6 +1470,7 @@ func (s *S) TestParseLayer(c *C) { Services: result.Services, Checks: result.Checks, LogTargets: result.LogTargets, + Sections: result.Sections, } err = p.Validate() } @@ -1494,6 +1513,7 @@ services: Services: combined.Services, Checks: combined.Checks, LogTargets: combined.LogTargets, + Sections: combined.Sections, } err = p.Validate() c.Assert(err, ErrorMatches, `services in before/after loop: .*`) @@ -1534,6 +1554,7 @@ services: Services: combined.Services, Checks: combined.Checks, LogTargets: combined.LogTargets, + Sections: combined.Sections, } err = p.Validate() c.Check(err, ErrorMatches, `plan must define "command" for service "srv1"`) @@ -1885,6 +1906,8 @@ func (s *S) TestMergeServiceContextNoContext(c *C) { Group: "grp", WorkingDir: "/working/dir", } + // This test ensures an empty service name results in no lookup, and + // simply leaves the provided context unchanged. merged, err := plan.MergeServiceContext(nil, "", overrides) c.Assert(err, IsNil) c.Check(merged, DeepEquals, overrides) From ff7df27d36e653ed2ed40a93406c45226adc6761 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Mon, 19 Aug 2024 23:44:10 +0200 Subject: [PATCH 02/17] Make plan.Section() simpler - no reflection --- internals/plan/extensions_test.go | 17 ++++------------- internals/plan/plan.go | 29 +++++------------------------ 2 files changed, 9 insertions(+), 37 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index d71fe1e4..1c02132f 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -381,7 +381,7 @@ func (s *S) TestPlanExtensions(c *C) { if _, ok := planTest.extensions[xField]; ok { // Verify "x-field" data. var x *xSection - err = p.Section(xField, &x) + x = p.Section(xField).(*xSection) c.Assert(err, IsNil) c.Assert(x.Entries, DeepEquals, planTest.combinedPlanResult.x.Entries) } @@ -389,7 +389,7 @@ func (s *S) TestPlanExtensions(c *C) { if _, ok := planTest.extensions[yField]; ok { // Verify "y-field" data. var y *ySection - err = p.Section(yField, &y) + y = p.Section(yField).(*ySection) c.Assert(err, IsNil) c.Assert(y.Entries, DeepEquals, planTest.combinedPlanResult.y.Entries) } @@ -451,19 +451,10 @@ func (x xExtension) CombineSections(sections ...plan.LayerSection) (plan.LayerSe func (x xExtension) ValidatePlan(p *plan.Plan) error { var xs *xSection - err := p.Section(xField, &xs) - if err != nil { - return err - } + xs = p.Section(xField).(*xSection) if xs != nil { var ys *ySection - err = p.Section(yField, &ys) - if err != nil { - return err - } - if ys == nil { - return fmt.Errorf("cannot validate %v field without %v field", xField, yField) - } + ys = p.Section(yField).(*ySection) // Test dependency: Make sure every Y field in X refer to an existing Y entry. for xEntryField, xEntryValue := range xs.Entries { diff --git a/internals/plan/plan.go b/internals/plan/plan.go index e35d88ea..fae05339 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -95,30 +95,11 @@ type Plan struct { Sections map[string]LayerSection `yaml:",inline"` } -// Section retrieves a section from the plan. -func (p *Plan) Section(field string, out interface{}) error { - if _, found := layerExtensions[field]; !found { - return fmt.Errorf("cannot find registered extension for field %q", field) - } - - outVal := reflect.ValueOf(out) - if outVal.Kind() != reflect.Ptr || outVal.IsNil() { - return fmt.Errorf("cannot read non pointer to section type %q", outVal.Kind()) - } - - section, exists := p.Sections[field] - if !exists { - return fmt.Errorf("internal error: section %q is nil", field) - } - - sectionVal := reflect.ValueOf(section) - sectionType := sectionVal.Type() - outValPtrType := outVal.Elem().Type() - if !sectionType.AssignableTo(outValPtrType) { - return fmt.Errorf("cannot assign value of type %s to out argument of type %s", sectionType, outValPtrType) - } - outVal.Elem().Set(sectionVal) - return nil +// Section retrieves a section from the plan. If Section is called +// before the plan is loaded, or with an unregistered field, this method +// will return nil. +func (p *Plan) Section(field string) LayerSection { + return p.Sections[field] } // MarshalYAML implements an override for top level omitempty tags handling. From bb7936b0ad98afb465125c1de4eef9cec4cc6bce Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Fri, 23 Aug 2024 11:51:13 +0200 Subject: [PATCH 03/17] Review feedback group 1 --- internals/plan/extensions_test.go | 83 +++++++++++++++---------------- internals/plan/plan.go | 42 ++++++++-------- 2 files changed, 61 insertions(+), 64 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index 1c02132f..bf7cbd54 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -42,16 +42,15 @@ type planResult struct { } var extensionTests = []struct { - extensions map[string]plan.LayerSectionExtension - layers []*inputLayer - errorString string - combinedPlanResult *planResult - combinedPlanResultYaml string + extensions map[string]plan.LayerSectionExtension + layers []*inputLayer + error string + result *planResult + resultYaml string }{ // Index 0: No Sections { - combinedPlanResultYaml: string(reindent(` - {}`)), + resultYaml: "{}\n", }, // Index 1: Sections with empty YAML { @@ -59,12 +58,11 @@ var extensionTests = []struct { "x-field": &xExtension{}, "y-field": &yExtension{}, }, - combinedPlanResult: &planResult{ + result: &planResult{ x: &xSection{}, y: &ySection{}, }, - combinedPlanResultYaml: string(reindent(` - {}`)), + resultYaml: "{}\n", }, // Index 2: Load file layers invalid section { @@ -73,7 +71,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-xy", yaml: ` @@ -83,7 +81,7 @@ var extensionTests = []struct { `, }, }, - errorString: "cannot parse layer .*: unknown section .*", + error: "cannot parse layer .*: unknown section .*", }, // Index 3: Load file layers not unique order { @@ -92,7 +90,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-1", yaml: ` @@ -100,7 +98,7 @@ var extensionTests = []struct { description: desc `, }, - &inputLayer{ + { order: 1, label: "layer-2", yaml: ` @@ -109,7 +107,7 @@ var extensionTests = []struct { `, }, }, - errorString: "invalid layer filename: .* not unique .*", + error: "invalid layer filename: .* not unique .*", }, // Index 4: Load file layers not unique label { @@ -118,7 +116,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-xy", yaml: ` @@ -126,7 +124,7 @@ var extensionTests = []struct { description: desc `, }, - &inputLayer{ + { order: 2, label: "layer-xy", yaml: ` @@ -135,7 +133,7 @@ var extensionTests = []struct { `, }, }, - errorString: "invalid layer filename: .* not unique .*", + error: "invalid layer filename: .* not unique .*", }, // Index 5: Load file layers with empty section { @@ -144,7 +142,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-x", yaml: ` @@ -152,7 +150,7 @@ var extensionTests = []struct { description: desc-x `, }, - &inputLayer{ + { order: 2, label: "layer-y", yaml: ` @@ -161,11 +159,11 @@ var extensionTests = []struct { `, }, }, - combinedPlanResult: &planResult{ + result: &planResult{ x: &xSection{}, y: &ySection{}, }, - combinedPlanResultYaml: string("{}\n"), + resultYaml: "{}\n", }, // Index 6: Load file layers with section validation failure #1 { @@ -174,7 +172,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-x", yaml: ` @@ -188,7 +186,7 @@ var extensionTests = []struct { `, }, }, - errorString: "cannot validate layer section .* cannot accept entry not starting .*", + error: "cannot validate layer section .* cannot accept entry not starting .*", }, // Index 7: Load file layers with section validation failure #2 { @@ -197,7 +195,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-x", yaml: ` @@ -208,7 +206,7 @@ var extensionTests = []struct { `, }, }, - errorString: "cannot validate layer section .* cannot have nil entry .*", + error: "cannot validate layer section .* cannot have nil entry .*", }, // Index 8: Load file layers failed plan validation { @@ -217,7 +215,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-x", yaml: ` @@ -232,7 +230,7 @@ var extensionTests = []struct { - y2 `, }, - &inputLayer{ + { order: 2, label: "layer-y", yaml: ` @@ -246,7 +244,7 @@ var extensionTests = []struct { `, }, }, - errorString: "cannot validate plan section .* cannot find .* as required by .*", + error: "cannot validate plan section .* cannot find .* as required by .*", }, // Index 9: Check empty section omits entry { @@ -255,7 +253,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-x", yaml: ` @@ -264,7 +262,7 @@ var extensionTests = []struct { x-field: `, }, - &inputLayer{ + { order: 2, label: "layer-y", yaml: ` @@ -274,12 +272,11 @@ var extensionTests = []struct { `, }, }, - combinedPlanResult: &planResult{ + result: &planResult{ x: &xSection{}, y: &ySection{}, }, - combinedPlanResultYaml: string(reindent(` - {}`)), + resultYaml: "{}\n", }, // Index 10: Load file layers { @@ -288,7 +285,7 @@ var extensionTests = []struct { "y-field": &yExtension{}, }, layers: []*inputLayer{ - &inputLayer{ + { order: 1, label: "layer-x", yaml: ` @@ -303,7 +300,7 @@ var extensionTests = []struct { - y1 `, }, - &inputLayer{ + { order: 2, label: "layer-y", yaml: ` @@ -317,7 +314,7 @@ var extensionTests = []struct { `, }, }, - combinedPlanResult: &planResult{ + result: &planResult{ x: &xSection{ Entries: map[string]*X{ "x1": &X{ @@ -342,7 +339,7 @@ var extensionTests = []struct { }, }, }, - combinedPlanResultYaml: string(reindent(` + resultYaml: string(reindent(` x-field: x1: override: replace @@ -374,16 +371,16 @@ func (s *S) TestPlanExtensions(c *C) { // Load the plan layer from disk (parse, combine and validate). p, err := plan.ReadDir(layersDir) - if planTest.errorString != "" || err != nil { + if planTest.error != "" || err != nil { // Expected error. - c.Assert(err, ErrorMatches, planTest.errorString) + c.Assert(err, ErrorMatches, planTest.error) } else { if _, ok := planTest.extensions[xField]; ok { // Verify "x-field" data. var x *xSection x = p.Section(xField).(*xSection) c.Assert(err, IsNil) - c.Assert(x.Entries, DeepEquals, planTest.combinedPlanResult.x.Entries) + c.Assert(x.Entries, DeepEquals, planTest.result.x.Entries) } if _, ok := planTest.extensions[yField]; ok { @@ -391,13 +388,13 @@ func (s *S) TestPlanExtensions(c *C) { var y *ySection y = p.Section(yField).(*ySection) c.Assert(err, IsNil) - c.Assert(y.Entries, DeepEquals, planTest.combinedPlanResult.y.Entries) + c.Assert(y.Entries, DeepEquals, planTest.result.y.Entries) } // Verify combined plan YAML. planYAML, err := yaml.Marshal(p) c.Assert(err, IsNil) - c.Assert(string(planYAML), Equals, planTest.combinedPlanResultYaml) + c.Assert(string(planYAML), Equals, planTest.resultYaml) } // Unregister test extensions. diff --git a/internals/plan/plan.go b/internals/plan/plan.go index fae05339..b6ddc12a 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -50,10 +50,10 @@ type LayerSectionExtension interface { } type LayerSection interface { - // Ask the section to validate itself. + // Validate checks whether the section is valid, returning an error if not. Validate() error - // Returns true if the section is empty. + // IsZero reports whether the section is empty. IsZero() bool } @@ -72,12 +72,11 @@ var layerExtensions = map[string]LayerSectionExtension{} // RegisterExtension adds a plan schema extension. All registrations must be // done before the plan library is used. -func RegisterExtension(field string, ext LayerSectionExtension) error { +func RegisterExtension(field string, ext LayerSectionExtension) { if _, ok := layerExtensions[field]; ok { - return fmt.Errorf("internal error: extension %q already registered", field) + panic(fmt.Sprintf("internal error: extension %q already registered", field)) } layerExtensions[field] = ext - return nil } // UnregisterExtension removes a plan schema extension. This is only @@ -106,22 +105,22 @@ func (p *Plan) Section(field string) LayerSection { // This is required since Sections are based on an inlined map, for which // omitempty and inline together is not currently supported. func (p *Plan) MarshalYAML() (interface{}, error) { - marshalData := make(map[string]interface{}) + data := make(map[string]interface{}) if len(p.Services) != 0 { - marshalData["services"] = p.Services + data["services"] = p.Services } if len(p.LogTargets) != 0 { - marshalData["log-targets"] = p.LogTargets + data["log-targets"] = p.LogTargets } if len(p.Checks) != 0 { - marshalData["checks"] = p.Checks + data["checks"] = p.Checks } for field, section := range p.Sections { if !section.IsZero() { - marshalData[field] = section + data[field] = section } } - return marshalData, nil + return data, nil } type Layer struct { @@ -654,7 +653,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { combined.Sections[field], err = extension.CombineSections(sections...) if err != nil { return nil, &FormatError{ - Message: fmt.Sprintf(`cannot combine section %q: %v`, field, err), + Message: fmt.Sprintf("cannot combine section %q: %v", field, err), } } } @@ -1259,15 +1258,8 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { } } } else { - if extension, ok := layerExtensions[field]; ok { - // Section unmarshal rules are defined by the extension itself. - layer.Sections[field], err = extension.ParseSection(section) - if err != nil { - return nil, &FormatError{ - Message: fmt.Sprintf("cannot parse layer %q section %q: %v", label, field, err), - } - } - } else { + extension, ok := layerExtensions[field] + if !ok { // At the top level we do not ignore keys we do not understand. // This preserves the current Pebble behaviour of decoding with // KnownFields = true. @@ -1275,6 +1267,14 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { Message: fmt.Sprintf("cannot parse layer %q: unknown section %q", label, field), } } + + // Section unmarshal rules are defined by the extension itself. + layer.Sections[field], err = extension.ParseSection(section) + if err != nil { + return nil, &FormatError{ + Message: fmt.Sprintf("cannot parse layer %q section %q: %v", label, field, err), + } + } } } From 23f054330c10fb5a47b4af356cdda19a7809708d Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Mon, 26 Aug 2024 09:36:54 +0200 Subject: [PATCH 04/17] Review feedback group 2 --- internals/plan/extensions_test.go | 98 +++++++++++++++++-------------- internals/plan/plan.go | 4 ++ 2 files changed, 58 insertions(+), 44 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index bf7cbd54..1455913c 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -42,18 +42,24 @@ type planResult struct { } var extensionTests = []struct { + summary string extensions map[string]plan.LayerSectionExtension layers []*inputLayer error string result *planResult resultYaml string }{ - // Index 0: No Sections { + summary: "No Sections", resultYaml: "{}\n", - }, - // Index 1: Sections with empty YAML - { + }, { + summary: "Section using built-in name", + extensions: map[string]plan.LayerSectionExtension{ + "summary": &xExtension{}, + }, + error: ".*already used as built-in field.*", + }, { + summary: "Sections with empty YAML", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -63,9 +69,8 @@ var extensionTests = []struct { y: &ySection{}, }, resultYaml: "{}\n", - }, - // Index 2: Load file layers invalid section - { + }, { + summary: "Load file layers invalid section", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -82,9 +87,8 @@ var extensionTests = []struct { }, }, error: "cannot parse layer .*: unknown section .*", - }, - // Index 3: Load file layers not unique order - { + }, { + summary: "Load file layers not unique order", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -108,9 +112,8 @@ var extensionTests = []struct { }, }, error: "invalid layer filename: .* not unique .*", - }, - // Index 4: Load file layers not unique label - { + }, { + summary: "Load file layers not unique label", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -134,9 +137,8 @@ var extensionTests = []struct { }, }, error: "invalid layer filename: .* not unique .*", - }, - // Index 5: Load file layers with empty section - { + }, { + summary: "Load file layers with empty section", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -164,9 +166,8 @@ var extensionTests = []struct { y: &ySection{}, }, resultYaml: "{}\n", - }, - // Index 6: Load file layers with section validation failure #1 - { + }, { + summary: "Load file layers with section validation failure #1", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -187,9 +188,8 @@ var extensionTests = []struct { }, }, error: "cannot validate layer section .* cannot accept entry not starting .*", - }, - // Index 7: Load file layers with section validation failure #2 - { + }, { + summary: "Load file layers with section validation failure #2", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -207,9 +207,8 @@ var extensionTests = []struct { }, }, error: "cannot validate layer section .* cannot have nil entry .*", - }, - // Index 8: Load file layers failed plan validation - { + }, { + summary: "Load file layers failed plan validation", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -245,9 +244,8 @@ var extensionTests = []struct { }, }, error: "cannot validate plan section .* cannot find .* as required by .*", - }, - // Index 9: Check empty section omits entry - { + }, { + summary: "Check empty section omits entry", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -277,9 +275,8 @@ var extensionTests = []struct { y: &ySection{}, }, resultYaml: "{}\n", - }, - // Index 10: Load file layers - { + }, { + summary: "Load file layers", extensions: map[string]plan.LayerSectionExtension{ "x-field": &xExtension{}, "y-field": &yExtension{}, @@ -356,49 +353,62 @@ var extensionTests = []struct { } func (s *S) TestPlanExtensions(c *C) { - for testIndex, planTest := range extensionTests { - c.Logf("Running TestPlanExtensions with test data index %v", testIndex) +nexttest: + for testIndex, testData := range extensionTests { + c.Logf("TestPlanExtensions :: %s (data index %v)", testData.summary, testIndex) // Write layers to test directory. layersDir := filepath.Join(c.MkDir(), "layers") - s.writeLayerFiles(c, layersDir, planTest.layers) + s.writeLayerFiles(c, layersDir, testData.layers) var p *plan.Plan // Register test extensions. - for field, extension := range planTest.extensions { - plan.RegisterExtension(field, extension) + for field, extension := range testData.extensions { + err := func() (err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("%v", r) + } + }() + plan.RegisterExtension(field, extension) + return nil + }() + if err != nil { + c.Assert(err, ErrorMatches, testData.error) + continue nexttest + } } // Load the plan layer from disk (parse, combine and validate). p, err := plan.ReadDir(layersDir) - if planTest.error != "" || err != nil { + if testData.error != "" || err != nil { // Expected error. - c.Assert(err, ErrorMatches, planTest.error) + c.Assert(err, ErrorMatches, testData.error) } else { - if _, ok := planTest.extensions[xField]; ok { + if _, ok := testData.extensions[xField]; ok { // Verify "x-field" data. var x *xSection x = p.Section(xField).(*xSection) c.Assert(err, IsNil) - c.Assert(x.Entries, DeepEquals, planTest.result.x.Entries) + c.Assert(x.Entries, DeepEquals, testData.result.x.Entries) } - if _, ok := planTest.extensions[yField]; ok { + if _, ok := testData.extensions[yField]; ok { // Verify "y-field" data. var y *ySection y = p.Section(yField).(*ySection) c.Assert(err, IsNil) - c.Assert(y.Entries, DeepEquals, planTest.result.y.Entries) + c.Assert(y.Entries, DeepEquals, testData.result.y.Entries) } // Verify combined plan YAML. planYAML, err := yaml.Marshal(p) c.Assert(err, IsNil) - c.Assert(string(planYAML), Equals, planTest.resultYaml) + c.Assert(string(planYAML), Equals, testData.resultYaml) } // Unregister test extensions. - for field, _ := range planTest.extensions { + for field, _ := range testData.extensions { plan.UnregisterExtension(field) } } diff --git a/internals/plan/plan.go b/internals/plan/plan.go index b6ddc12a..ddb90fdf 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -73,6 +73,10 @@ var layerExtensions = map[string]LayerSectionExtension{} // RegisterExtension adds a plan schema extension. All registrations must be // done before the plan library is used. func RegisterExtension(field string, ext LayerSectionExtension) { + switch field { + case "summary", "description", "services", "checks", "log-targets": + panic(fmt.Sprintf("internal error: extension %q already used as built-in field", field)) + } if _, ok := layerExtensions[field]; ok { panic(fmt.Sprintf("internal error: extension %q already registered", field)) } From 80579e26e81fec92b1b28fc5e5efa0107d01dad2 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Mon, 26 Aug 2024 14:11:46 +0200 Subject: [PATCH 05/17] add test for layers content --- internals/plan/export_test.go | 17 +++++++++++++++ internals/plan/plan.go | 30 ++++++++++++++++++++++--- internals/plan/plan_test.go | 41 +++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 internals/plan/export_test.go diff --git a/internals/plan/export_test.go b/internals/plan/export_test.go new file mode 100644 index 00000000..9da16930 --- /dev/null +++ b/internals/plan/export_test.go @@ -0,0 +1,17 @@ +// Copyright (c) 2024 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package plan + +var LayerBuiltins = layerBuiltins diff --git a/internals/plan/plan.go b/internals/plan/plan.go index ddb90fdf..1787b1eb 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -21,6 +21,7 @@ import ( "path/filepath" "reflect" "regexp" + "slices" "strconv" "strings" "time" @@ -70,11 +71,15 @@ const ( // layerExtensions keeps a map of registered extensions. var layerExtensions = map[string]LayerSectionExtension{} +// layerBuiltins represents all the built-in layer sections. This list is used +// for identifying built-in fields in this package. It is unit tested to match +// the YAML fields exposed in the Layer type, to catch inconsistencies. +var layerBuiltins = []string{"summary", "description", "services", "checks", "log-targets"} + // RegisterExtension adds a plan schema extension. All registrations must be // done before the plan library is used. func RegisterExtension(field string, ext LayerSectionExtension) { - switch field { - case "summary", "description", "services", "checks", "log-targets": + if slices.Contains(layerBuiltins, field) { panic(fmt.Sprintf("internal error: extension %q already used as built-in field", field)) } if _, ok := layerExtensions[field]; ok { @@ -1226,6 +1231,11 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { "checks": &layer.Checks, "log-targets": &layer.LogTargets, } + // Make sure builtinSections contains the exact same fields as expected + // in the Layer type. + if !mapHasKeys(builtinSections, layerBuiltins) { + panic("internal error: parsed fields and layer fields differ") + } layerSections := make(map[string]yaml.Node) // Deliberately pre-allocate at least an empty yaml.Node for every @@ -1245,7 +1255,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { } for field, section := range layerSections { - if _, builtin := builtinSections[field]; builtin { + if slices.Contains(layerBuiltins, field) { // The following issue prevents us from using the yaml.Node decoder // with KnownFields = true behaviour. Once one of the proposals get // merged, we can remove the intermediate Marshal step. @@ -1312,6 +1322,20 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { return layer, err } +// mapHasKeys returns true if the key list supplied is an exact match of the +// keys in the map (ordering is ignored). +func mapHasKeys[M ~map[K]V, K comparable, V any](inMap M, keyList []K) bool { + if len(inMap) != len(keyList) { + return false + } + for _, key := range keyList { + if _, ok := inMap[key]; !ok { + return false + } + } + return true +} + func validServiceAction(action ServiceAction, additionalValid ...ServiceAction) bool { for _, v := range additionalValid { if action == v { diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index fbc1fb96..4e119e2c 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -19,8 +19,11 @@ import ( "fmt" "os" "path/filepath" + "reflect" + "slices" "strings" "time" + "unicode" . "gopkg.in/check.v1" "gopkg.in/yaml.v3" @@ -2068,3 +2071,41 @@ func (s *S) TestStartStopOrderMultipleLanes(c *C) { c.Assert(lanes[1], DeepEquals, []string{"srv2"}) c.Assert(lanes[2], DeepEquals, []string{"srv3"}) } + +// TestLayerBuiltinCompatible ensures layerBuiltins used in the plan package +// reflects the same YAML fields as exposed in the Layer type. +func (s *S) TestLayerBuiltinCompatible(c *C) { + fields := structYamlFields(plan.Layer{}) + c.Assert(len(fields), Equals, len(plan.LayerBuiltins)) + for _, field := range structYamlFields(plan.Layer{}) { + c.Assert(slices.Contains(plan.LayerBuiltins, field), Equals, true) + } +} + +// structYamlFields extracts the YAML fields from a struct. If the YAML tag +// is omitted, the field name with the first letter lower case will be used. +func structYamlFields(inStruct any) []string { + var fields []string + inStructType := reflect.TypeOf(inStruct) + for i := range inStructType.NumField() { + fieldType := inStructType.Field(i) + yamlTag := fieldType.Tag.Get("yaml") + if fieldType.IsExported() && yamlTag != "-" && !strings.Contains(yamlTag, ",inline") { + tag, _, _ := strings.Cut(fieldType.Tag.Get("yaml"), ",") + if tag == "" { + tag = firstLetterToLower(fieldType.Name) + } + fields = append(fields, tag) + } + } + return fields +} + +func firstLetterToLower(s string) string { + if len(s) == 0 { + return s + } + r := []rune(s) + r[0] = unicode.ToLower(r[0]) + return string(r) +} From 5abfb2e7e3f04661df3d9a142f484516c9edb8c1 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 12:34:01 +0200 Subject: [PATCH 06/17] enforce section marshal ordering --- internals/plan/extensions_test.go | 79 +++++++++++++++++++++++++++++++ internals/plan/plan.go | 67 ++++++++++++++++++-------- internals/plan/plan_test.go | 45 ++++++++++++++++++ 3 files changed, 172 insertions(+), 19 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index 1455913c..1fc09b84 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -414,6 +414,85 @@ nexttest: } } +// TestSectionOrderExt ensures built-in and extension section ordering +// does not change. Extensions are ordered according to the order of +// registration. +func (s *S) TestSectionOrderExt(c *C) { + plan.RegisterExtension("x-field", &xExtension{}) + plan.RegisterExtension("y-field", &yExtension{}) + defer func() { + plan.UnregisterExtension("x-field") + plan.UnregisterExtension("y-field") + }() + + layer, err := plan.ParseLayer(1, "label", reindent(` + y-field: + y1: + override: replace + a: a + b: b + checks: + chk1: + override: replace + exec: + command: ping 8.8.8.8 + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y1 + log-targets: + lt1: + override: replace + type: loki + location: http://192.168.1.2:3100/loki/api/v1/push + services: + srv1: + override: replace + command: cmd`)) + c.Assert(err, IsNil) + combined, err := plan.CombineLayers(layer) + c.Assert(err, IsNil) + plan := plan.Plan{ + Services: combined.Services, + Checks: combined.Checks, + LogTargets: combined.LogTargets, + Sections: combined.Sections, + } + data, err := yaml.Marshal(plan) + c.Assert(string(data), Equals, string(reindent(` + services: + srv1: + override: replace + command: cmd + checks: + chk1: + override: replace + threshold: 3 + exec: + command: ping 8.8.8.8 + log-targets: + lt1: + type: loki + location: http://192.168.1.2:3100/loki/api/v1/push + services: [] + override: replace + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y1 + y-field: + y1: + override: replace + a: a + b: b`))) +} + // writeLayerFiles writes layer files of a test to disk. func (s *S) writeLayerFiles(c *C, layersDir string, inputs []*inputLayer) { err := os.MkdirAll(layersDir, 0755) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 1787b1eb..9c10e87a 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -68,8 +68,13 @@ const ( defaultCheckThreshold = 3 ) -// layerExtensions keeps a map of registered extensions. -var layerExtensions = map[string]LayerSectionExtension{} +var ( + // layerExtensions keeps a map of registered extensions. + layerExtensions = map[string]LayerSectionExtension{} + + // layerExtensionsOrder records the order in which the extensions were registered. + layerExtensionsOrder = []string{} +) // layerBuiltins represents all the built-in layer sections. This list is used // for identifying built-in fields in this package. It is unit tested to match @@ -77,7 +82,9 @@ var layerExtensions = map[string]LayerSectionExtension{} var layerBuiltins = []string{"summary", "description", "services", "checks", "log-targets"} // RegisterExtension adds a plan schema extension. All registrations must be -// done before the plan library is used. +// done before the plan library is used. The order in which extensions are +// registered determines the order in which the sections are marshalled. +// Extension sections are marshalled after the built-in sections. func RegisterExtension(field string, ext LayerSectionExtension) { if slices.Contains(layerBuiltins, field) { panic(fmt.Sprintf("internal error: extension %q already used as built-in field", field)) @@ -86,12 +93,16 @@ func RegisterExtension(field string, ext LayerSectionExtension) { panic(fmt.Sprintf("internal error: extension %q already registered", field)) } layerExtensions[field] = ext + layerExtensionsOrder = append(layerExtensionsOrder, field) } // UnregisterExtension removes a plan schema extension. This is only // intended for use by tests during cleanup. func UnregisterExtension(field string) { delete(layerExtensions, field) + layerExtensionsOrder = slices.DeleteFunc(layerExtensionsOrder, func(n string) bool { + return n == field + }) } type Plan struct { @@ -114,22 +125,40 @@ func (p *Plan) Section(field string) LayerSection { // This is required since Sections are based on an inlined map, for which // omitempty and inline together is not currently supported. func (p *Plan) MarshalYAML() (interface{}, error) { - data := make(map[string]interface{}) - if len(p.Services) != 0 { - data["services"] = p.Services - } - if len(p.LogTargets) != 0 { - data["log-targets"] = p.LogTargets - } - if len(p.Checks) != 0 { - data["checks"] = p.Checks - } - for field, section := range p.Sections { - if !section.IsZero() { - data[field] = section - } - } - return data, nil + // Define the content inside a structure so we can control the ordering + // of top level sections. + ordered := []reflect.StructField{{ + Name: "Services", + Type: reflect.TypeOf(p.Services), + Tag: `yaml:"services,omitempty"`, + }, { + Name: "Checks", + Type: reflect.TypeOf(p.Checks), + Tag: `yaml:"checks,omitempty"`, + }, { + Name: "LogTargets", + Type: reflect.TypeOf(p.LogTargets), + Tag: `yaml:"log-targets,omitempty"`, + }} + for i, field := range layerExtensionsOrder { + section := p.Sections[field] + ordered = append(ordered, reflect.StructField{ + Name: fmt.Sprintf("Dummy%v", i), + Type: reflect.TypeOf(section), + Tag: reflect.StructTag(fmt.Sprintf("yaml:\"%s,omitempty\"", field)), + }) + } + typ := reflect.StructOf(ordered) + // Assign the plan data to the structure layout we created. + v := reflect.New(typ).Elem() + v.Field(0).Set(reflect.ValueOf(p.Services)) + v.Field(1).Set(reflect.ValueOf(p.Checks)) + v.Field(2).Set(reflect.ValueOf(p.LogTargets)) + for i, field := range layerExtensionsOrder { + v.Field(3 + i).Set(reflect.ValueOf(p.Sections[field])) + } + plan := v.Addr().Interface() + return plan, nil } type Layer struct { diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 4e119e2c..3829301f 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -2109,3 +2109,48 @@ func firstLetterToLower(s string) string { r[0] = unicode.ToLower(r[0]) return string(r) } + +// TestSectionOrder ensures built-in section order is maintained. +func (s *S) TestSectionOrder(c *C) { + layer, err := plan.ParseLayer(1, "label", reindent(` + checks: + chk1: + override: replace + exec: + command: ping 8.8.8.8 + log-targets: + lt1: + override: replace + type: loki + location: http://192.168.1.2:3100/loki/api/v1/push + services: + srv1: + override: replace + command: cmd`)) + c.Assert(err, IsNil) + combined, err := plan.CombineLayers(layer) + c.Assert(err, IsNil) + plan := plan.Plan{ + Services: combined.Services, + Checks: combined.Checks, + LogTargets: combined.LogTargets, + } + data, err := yaml.Marshal(plan) + c.Assert(string(data), Equals, string(reindent(` + services: + srv1: + override: replace + command: cmd + checks: + chk1: + override: replace + threshold: 3 + exec: + command: ping 8.8.8.8 + log-targets: + lt1: + type: loki + location: http://192.168.1.2:3100/loki/api/v1/push + services: [] + override: replace`))) +} From feae89afabf019c9519f275b383226041db78d75 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 13:47:20 +0200 Subject: [PATCH 07/17] Review feedback group 3 --- internals/plan/extensions_test.go | 682 +++++++++++++++--------------- 1 file changed, 341 insertions(+), 341 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index 1fc09b84..ea6a2136 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -19,6 +19,7 @@ import ( "io/ioutil" "os" "path/filepath" + "slices" "strings" . "gopkg.in/check.v1" @@ -41,316 +42,311 @@ type planResult struct { y *ySection } +type extension struct { + field string + ext plan.LayerSectionExtension +} + var extensionTests = []struct { summary string - extensions map[string]plan.LayerSectionExtension + extensions []extension layers []*inputLayer error string result *planResult resultYaml string -}{ - { - summary: "No Sections", - resultYaml: "{}\n", +}{{ + summary: "No Sections", + resultYaml: "{}\n", +}, { + summary: "Section using built-in name", + extensions: []extension{{ + field: "summary", + ext: &xExtension{}, + }}, + error: ".*already used as built-in field.*", +}, { + summary: "Sections with empty YAML", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Section using built-in name", - extensions: map[string]plan.LayerSectionExtension{ - "summary": &xExtension{}, - }, - error: ".*already used as built-in field.*", + field: "y-field", + ext: &yExtension{}, + }}, + result: &planResult{ + x: &xSection{}, + y: &ySection{}, + }, + resultYaml: "{}\n", +}, { + summary: "Load file layers invalid section", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Sections with empty YAML", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - result: &planResult{ - x: &xSection{}, - y: &ySection{}, - }, - resultYaml: "{}\n", + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-xy", + yaml: ` + summary: xy + description: desc + invalid:`, + }}, + error: "cannot parse layer .*: unknown section .*", +}, { + summary: "Load file layers not unique order", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Load file layers invalid section", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-xy", - yaml: ` - summary: xy - description: desc - invalid: - `, - }, - }, - error: "cannot parse layer .*: unknown section .*", + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-1", + yaml: ` + summary: xy + description: desc`, }, { - summary: "Load file layers not unique order", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-1", - yaml: ` - summary: xy - description: desc - `, - }, - { - order: 1, - label: "layer-2", - yaml: ` - summary: xy - description: desc - `, - }, - }, - error: "invalid layer filename: .* not unique .*", + order: 1, + label: "layer-2", + yaml: ` + summary: xy + description: desc`, + }}, + error: "invalid layer filename: .* not unique .*", +}, { + summary: "Load file layers not unique label", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Load file layers not unique label", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-xy", - yaml: ` - summary: xy - description: desc - `, - }, - { - order: 2, - label: "layer-xy", - yaml: ` - summary: xy - description: desc - `, - }, - }, - error: "invalid layer filename: .* not unique .*", + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-xy", + yaml: ` + summary: xy + description: desc`, }, { - summary: "Load file layers with empty section", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-x", - yaml: ` - summary: x - description: desc-x - `, - }, - { - order: 2, - label: "layer-y", - yaml: ` - summary: y - description: desc-y - `, - }, - }, - result: &planResult{ - x: &xSection{}, - y: &ySection{}, - }, - resultYaml: "{}\n", + order: 2, + label: "layer-xy", + yaml: ` + summary: xy + description: desc`, + }}, + error: "invalid layer filename: .* not unique .*", +}, { + summary: "Load file layers with empty section", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Load file layers with section validation failure #1", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-x", - yaml: ` - summary: x - description: desc-x - x-field: - z1: - override: replace - a: a - b: b - `, - }, - }, - error: "cannot validate layer section .* cannot accept entry not starting .*", + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x`, }, { - summary: "Load file layers with section validation failure #2", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-x", - yaml: ` - summary: x - description: desc-x - x-field: - x1: - `, - }, - }, - error: "cannot validate layer section .* cannot have nil entry .*", + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y`, + }}, + result: &planResult{ + x: &xSection{}, + y: &ySection{}, + }, + resultYaml: "{}\n", +}, { + summary: "Load file layers with section validation failure #1", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Load file layers failed plan validation", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-x", - yaml: ` - summary: x - description: desc-x - x-field: - x1: - override: replace - a: a - b: b - y-field: - - y2 - `, - }, - { - order: 2, - label: "layer-y", - yaml: ` - summary: y - description: desc-y - y-field: - y1: - override: replace - a: a - b: b - `, - }, - }, - error: "cannot validate plan section .* cannot find .* as required by .*", + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + z1: + override: replace + a: a + b: b`, + }}, + error: "cannot validate layer section .* cannot accept entry not starting .*", +}, { + summary: "Load file layers with section validation failure #2", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Check empty section omits entry", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-x", - yaml: ` - summary: x - description: desc-x - x-field: - `, - }, - { - order: 2, - label: "layer-y", - yaml: ` - summary: y - description: desc-y - y-field: - `, - }, - }, - result: &planResult{ - x: &xSection{}, - y: &ySection{}, - }, - resultYaml: "{}\n", + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + x1:`, + }}, + error: "cannot validate layer section .* cannot have nil entry .*", +}, { + summary: "Load file layers failed plan validation", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, }, { - summary: "Load file layers", - extensions: map[string]plan.LayerSectionExtension{ - "x-field": &xExtension{}, - "y-field": &yExtension{}, - }, - layers: []*inputLayer{ - { - order: 1, - label: "layer-x", - yaml: ` - summary: x - description: desc-x - x-field: - x1: - override: replace - a: a - b: b - y-field: - - y1 - `, - }, - { - order: 2, - label: "layer-y", - yaml: ` - summary: y - description: desc-y + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field: + x1: + override: replace + a: a + b: b y-field: - y1: - override: replace - a: a - b: b - `, - }, - }, - result: &planResult{ - x: &xSection{ - Entries: map[string]*X{ - "x1": &X{ - Name: "x1", - Override: plan.ReplaceOverride, - A: "a", - B: "b", - Y: []string{ - "y1", - }, - }, - }, - }, - y: &ySection{ - Entries: map[string]*Y{ - "y1": &Y{ - Name: "y1", - Override: plan.ReplaceOverride, - A: "a", - B: "b", - }, - }, - }, - }, - resultYaml: string(reindent(` + - y2`, + }, { + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y + y-field: + y1: + override: replace + a: a + b: b`, + }}, + error: "cannot validate plan section .* cannot find .* as required by .*", +}, { + summary: "Check empty section omits entry", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, + }, { + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x + x-field:`, + }, { + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y + y-field:`, + }}, + result: &planResult{ + x: &xSection{}, + y: &ySection{}, + }, + resultYaml: "{}\n", +}, { + summary: "Load file layers", + extensions: []extension{{ + field: "x-field", + ext: &xExtension{}, + }, { + field: "y-field", + ext: &yExtension{}, + }}, + layers: []*inputLayer{{ + order: 1, + label: "layer-x", + yaml: ` + summary: x + description: desc-x x-field: x1: override: replace a: a b: b y-field: - - y1 + - y1`, + }, { + order: 2, + label: "layer-y", + yaml: ` + summary: y + description: desc-y y-field: y1: override: replace a: a - b: b`)), + b: b`, + }}, + result: &planResult{ + x: &xSection{ + Entries: map[string]*X{ + "x1": &X{ + Name: "x1", + Override: plan.ReplaceOverride, + A: "a", + B: "b", + Y: []string{ + "y1", + }, + }, + }, + }, + y: &ySection{ + Entries: map[string]*Y{ + "y1": &Y{ + Name: "y1", + Override: plan.ReplaceOverride, + A: "a", + B: "b", + }, + }, + }, }, -} + resultYaml: string(reindent(` + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y1 + y-field: + y1: + override: replace + a: a + b: b`)), +}} func (s *S) TestPlanExtensions(c *C) { nexttest: @@ -363,14 +359,14 @@ nexttest: var p *plan.Plan // Register test extensions. - for field, extension := range testData.extensions { + for _, e := range testData.extensions { err := func() (err error) { defer func() { if r := recover(); r != nil { err = fmt.Errorf("%v", r) } }() - plan.RegisterExtension(field, extension) + plan.RegisterExtension(e.field, e.ext) return nil }() if err != nil { @@ -385,7 +381,9 @@ nexttest: // Expected error. c.Assert(err, ErrorMatches, testData.error) } else { - if _, ok := testData.extensions[xField]; ok { + if slices.ContainsFunc(testData.extensions, func(n extension) bool { + return n.field == xField + }) { // Verify "x-field" data. var x *xSection x = p.Section(xField).(*xSection) @@ -393,7 +391,9 @@ nexttest: c.Assert(x.Entries, DeepEquals, testData.result.x.Entries) } - if _, ok := testData.extensions[yField]; ok { + if slices.ContainsFunc(testData.extensions, func(n extension) bool { + return n.field == yField + }) { // Verify "y-field" data. var y *ySection y = p.Section(yField).(*ySection) @@ -408,8 +408,8 @@ nexttest: } // Unregister test extensions. - for field, _ := range testData.extensions { - plan.UnregisterExtension(field) + for _, e := range testData.extensions { + plan.UnregisterExtension(e.field) } } } @@ -426,32 +426,32 @@ func (s *S) TestSectionOrderExt(c *C) { }() layer, err := plan.ParseLayer(1, "label", reindent(` - y-field: - y1: - override: replace - a: a - b: b - checks: - chk1: - override: replace - exec: - command: ping 8.8.8.8 - x-field: - x1: - override: replace - a: a - b: b - y-field: - - y1 - log-targets: - lt1: - override: replace - type: loki - location: http://192.168.1.2:3100/loki/api/v1/push - services: - srv1: - override: replace - command: cmd`)) + y-field: + y1: + override: replace + a: a + b: b + checks: + chk1: + override: replace + exec: + command: ping 8.8.8.8 + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y1 + log-targets: + lt1: + override: replace + type: loki + location: http://192.168.1.2:3100/loki/api/v1/push + services: + srv1: + override: replace + command: cmd`)) c.Assert(err, IsNil) combined, err := plan.CombineLayers(layer) c.Assert(err, IsNil) @@ -463,34 +463,34 @@ func (s *S) TestSectionOrderExt(c *C) { } data, err := yaml.Marshal(plan) c.Assert(string(data), Equals, string(reindent(` - services: - srv1: - override: replace - command: cmd - checks: - chk1: - override: replace - threshold: 3 - exec: - command: ping 8.8.8.8 - log-targets: - lt1: - type: loki - location: http://192.168.1.2:3100/loki/api/v1/push - services: [] - override: replace - x-field: - x1: - override: replace - a: a - b: b - y-field: - - y1 - y-field: - y1: - override: replace - a: a - b: b`))) + services: + srv1: + override: replace + command: cmd + checks: + chk1: + override: replace + threshold: 3 + exec: + command: ping 8.8.8.8 + log-targets: + lt1: + type: loki + location: http://192.168.1.2:3100/loki/api/v1/push + services: [] + override: replace + x-field: + x1: + override: replace + a: a + b: b + y-field: + - y1 + y-field: + y1: + override: replace + a: a + b: b`))) } // writeLayerFiles writes layer files of a test to disk. From d1879179509770bc850fa83244f9f7c8c4e5ece6 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 14:17:11 +0200 Subject: [PATCH 08/17] Review feedback group 4 --- internals/plan/extensions_test.go | 87 +++++++++++++++++-------------- internals/plan/plan.go | 31 +++++------ internals/plan/plan_test.go | 36 ++++++------- 3 files changed, 79 insertions(+), 75 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index ea6a2136..2cbd8b86 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -44,7 +44,7 @@ type planResult struct { type extension struct { field string - ext plan.LayerSectionExtension + ext plan.SectionExtension } var extensionTests = []struct { @@ -349,16 +349,30 @@ var extensionTests = []struct { }} func (s *S) TestPlanExtensions(c *C) { + registeredExtensions := []string{} + defer func() { + // Remove remaining registered extensions. + for _, field := range registeredExtensions { + plan.UnregisterExtension(field) + } + }() + nexttest: for testIndex, testData := range extensionTests { c.Logf("TestPlanExtensions :: %s (data index %v)", testData.summary, testIndex) + // Unregister extensions from previous test iteraton. + for _, field := range registeredExtensions { + plan.UnregisterExtension(field) + } + registeredExtensions = []string{} + // Write layers to test directory. layersDir := filepath.Join(c.MkDir(), "layers") s.writeLayerFiles(c, layersDir, testData.layers) var p *plan.Plan - // Register test extensions. + // Register extensions for this test iteration. for _, e := range testData.extensions { err := func() (err error) { defer func() { @@ -367,6 +381,7 @@ nexttest: } }() plan.RegisterExtension(e.field, e.ext) + registeredExtensions = append(registeredExtensions, e.field) return nil }() if err != nil { @@ -380,37 +395,33 @@ nexttest: if testData.error != "" || err != nil { // Expected error. c.Assert(err, ErrorMatches, testData.error) - } else { - if slices.ContainsFunc(testData.extensions, func(n extension) bool { - return n.field == xField - }) { - // Verify "x-field" data. - var x *xSection - x = p.Section(xField).(*xSection) - c.Assert(err, IsNil) - c.Assert(x.Entries, DeepEquals, testData.result.x.Entries) - } - - if slices.ContainsFunc(testData.extensions, func(n extension) bool { - return n.field == yField - }) { - // Verify "y-field" data. - var y *ySection - y = p.Section(yField).(*ySection) - c.Assert(err, IsNil) - c.Assert(y.Entries, DeepEquals, testData.result.y.Entries) - } + continue nexttest + } - // Verify combined plan YAML. - planYAML, err := yaml.Marshal(p) + if slices.ContainsFunc(testData.extensions, func(n extension) bool { + return n.field == xField + }) { + // Verify "x-field" data. + var x *xSection + x = p.Sections[xField].(*xSection) c.Assert(err, IsNil) - c.Assert(string(planYAML), Equals, testData.resultYaml) + c.Assert(x.Entries, DeepEquals, testData.result.x.Entries) } - // Unregister test extensions. - for _, e := range testData.extensions { - plan.UnregisterExtension(e.field) + if slices.ContainsFunc(testData.extensions, func(n extension) bool { + return n.field == yField + }) { + // Verify "y-field" data. + var y *ySection + y = p.Sections[yField].(*ySection) + c.Assert(err, IsNil) + c.Assert(y.Entries, DeepEquals, testData.result.y.Entries) } + + // Verify combined plan YAML. + planYAML, err := yaml.Marshal(p) + c.Assert(err, IsNil) + c.Assert(string(planYAML), Equals, testData.resultYaml) } } @@ -506,10 +517,10 @@ func (s *S) writeLayerFiles(c *C, layersDir string, inputs []*inputLayer) { const xField string = "x-field" -// xExtension implements the LayerSectionExtension interface. +// xExtension implements the SectionExtension interface. type xExtension struct{} -func (x xExtension) ParseSection(data yaml.Node) (plan.LayerSection, error) { +func (x xExtension) ParseSection(data yaml.Node) (plan.Section, error) { xs := &xSection{} err := data.Decode(xs) if err != nil { @@ -524,7 +535,7 @@ func (x xExtension) ParseSection(data yaml.Node) (plan.LayerSection, error) { return xs, nil } -func (x xExtension) CombineSections(sections ...plan.LayerSection) (plan.LayerSection, error) { +func (x xExtension) CombineSections(sections ...plan.Section) (plan.Section, error) { xs := &xSection{} for _, section := range sections { err := xs.Combine(section) @@ -537,10 +548,10 @@ func (x xExtension) CombineSections(sections ...plan.LayerSection) (plan.LayerSe func (x xExtension) ValidatePlan(p *plan.Plan) error { var xs *xSection - xs = p.Section(xField).(*xSection) + xs = p.Sections[xField].(*xSection) if xs != nil { var ys *ySection - ys = p.Section(yField).(*ySection) + ys = p.Sections[yField].(*ySection) // Test dependency: Make sure every Y field in X refer to an existing Y entry. for xEntryField, xEntryValue := range xs.Entries { @@ -583,7 +594,7 @@ func (xs *xSection) IsZero() bool { return xs.Entries == nil } -func (xs *xSection) Combine(other plan.LayerSection) error { +func (xs *xSection) Combine(other plan.Section) error { otherxSection, ok := other.(*xSection) if !ok { return fmt.Errorf("cannot combine incompatible section type") @@ -645,10 +656,10 @@ func (x *X) Merge(other *X) { const yField string = "y-field" -// yExtension implements the LayerSectionExtension interface. +// yExtension implements the SectionExtension interface. type yExtension struct{} -func (y yExtension) ParseSection(data yaml.Node) (plan.LayerSection, error) { +func (y yExtension) ParseSection(data yaml.Node) (plan.Section, error) { ys := &ySection{} err := data.Decode(ys) if err != nil { @@ -663,7 +674,7 @@ func (y yExtension) ParseSection(data yaml.Node) (plan.LayerSection, error) { return ys, nil } -func (y yExtension) CombineSections(sections ...plan.LayerSection) (plan.LayerSection, error) { +func (y yExtension) CombineSections(sections ...plan.Section) (plan.Section, error) { ys := &ySection{} for _, section := range sections { err := ys.Combine(section) @@ -701,7 +712,7 @@ func (ys *ySection) IsZero() bool { return ys.Entries == nil } -func (ys *ySection) Combine(other plan.LayerSection) error { +func (ys *ySection) Combine(other plan.Section) error { otherySection, ok := other.(*ySection) if !ok { return fmt.Errorf("cannot combine incompatible section type") diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 9c10e87a..107f1d45 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -33,16 +33,16 @@ import ( "github.com/canonical/pebble/internals/osutil" ) -// LayerSectionExtension allows the plan layer schema to be extended without +// SectionExtension allows the plan layer schema to be extended without // adding centralised schema knowledge to the plan library. -type LayerSectionExtension interface { +type SectionExtension interface { // ParseSection returns a newly allocated concrete type containing the // unmarshalled section content. - ParseSection(data yaml.Node) (LayerSection, error) + ParseSection(data yaml.Node) (Section, error) // CombineSections returns a newly allocated concrete type containing the // result of combining the supplied sections in order. - CombineSections(sections ...LayerSection) (LayerSection, error) + CombineSections(sections ...Section) (Section, error) // ValidatePlan takes the complete plan as input, and allows the // extension to validate the plan. This can be used for cross section @@ -50,7 +50,7 @@ type LayerSectionExtension interface { ValidatePlan(plan *Plan) error } -type LayerSection interface { +type Section interface { // Validate checks whether the section is valid, returning an error if not. Validate() error @@ -70,7 +70,7 @@ const ( var ( // layerExtensions keeps a map of registered extensions. - layerExtensions = map[string]LayerSectionExtension{} + layerExtensions = map[string]SectionExtension{} // layerExtensionsOrder records the order in which the extensions were registered. layerExtensionsOrder = []string{} @@ -85,7 +85,7 @@ var layerBuiltins = []string{"summary", "description", "services", "checks", "lo // done before the plan library is used. The order in which extensions are // registered determines the order in which the sections are marshalled. // Extension sections are marshalled after the built-in sections. -func RegisterExtension(field string, ext LayerSectionExtension) { +func RegisterExtension(field string, ext SectionExtension) { if slices.Contains(layerBuiltins, field) { panic(fmt.Sprintf("internal error: extension %q already used as built-in field", field)) } @@ -111,14 +111,7 @@ type Plan struct { Checks map[string]*Check `yaml:"checks,omitempty"` LogTargets map[string]*LogTarget `yaml:"log-targets,omitempty"` - Sections map[string]LayerSection `yaml:",inline"` -} - -// Section retrieves a section from the plan. If Section is called -// before the plan is loaded, or with an unregistered field, this method -// will return nil. -func (p *Plan) Section(field string) LayerSection { - return p.Sections[field] + Sections map[string]Section `yaml:",inline"` } // MarshalYAML implements an override for top level omitempty tags handling. @@ -170,7 +163,7 @@ type Layer struct { Checks map[string]*Check `yaml:"checks,omitempty"` LogTargets map[string]*LogTarget `yaml:"log-targets,omitempty"` - Sections map[string]LayerSection `yaml:",inline"` + Sections map[string]Section `yaml:",inline"` } type Service struct { @@ -673,7 +666,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { Services: make(map[string]*Service), Checks: make(map[string]*Check), LogTargets: make(map[string]*LogTarget), - Sections: make(map[string]LayerSection), + Sections: make(map[string]Section), } // Combine the same sections from each layer. Note that we do this before @@ -681,7 +674,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { // a zero value section, even if no layers are supplied (similar to the // allocations taking place above for the built-in types). for field, extension := range layerExtensions { - var sections []LayerSection + var sections []Section for _, layer := range layers { if section := layer.Sections[field]; section != nil { sections = append(sections, section) @@ -1241,7 +1234,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { Services: make(map[string]*Service), Checks: make(map[string]*Check), LogTargets: make(map[string]*LogTarget), - Sections: make(map[string]LayerSection), + Sections: make(map[string]Section), } // The following manual approach is required because: diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 3829301f..34692c93 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -206,7 +206,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, { Order: 1, Label: "layer-1", @@ -257,7 +257,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }}, result: &plan.Layer{ Summary: "Simple override layer.", @@ -337,7 +337,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, start: map[string][]string{ "srv1": {"srv2", "srv1", "srv3"}, @@ -400,7 +400,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }}, }, { summary: "Unknown keys are not accepted", @@ -551,7 +551,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }}, }, { summary: `Invalid service command: cannot have any arguments after [ ... ] group`, @@ -660,7 +660,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Checks override replace works correctly", @@ -738,7 +738,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Checks override merge works correctly", @@ -822,7 +822,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Timeout is capped at period", @@ -852,7 +852,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Unset timeout is capped at period", @@ -881,7 +881,7 @@ var planTests = []planTest{{ }, }, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "One of http, tcp, or exec must be present for check", @@ -1002,7 +1002,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Overriding log targets", @@ -1099,7 +1099,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, { Label: "layer-1", Order: 1, @@ -1138,7 +1138,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }}, result: &plan.Layer{ Services: map[string]*plan.Service{ @@ -1184,7 +1184,7 @@ var planTests = []planTest{{ Override: plan.MergeOverride, }, }, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Log target requires type field", @@ -1294,7 +1294,7 @@ var planTests = []planTest{{ }, }, }, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, { Order: 1, Label: "layer-1", @@ -1320,7 +1320,7 @@ var planTests = []planTest{{ }, }, }, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }}, result: &plan.Layer{ Services: map[string]*plan.Service{}, @@ -1348,7 +1348,7 @@ var planTests = []planTest{{ }, }, }, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Reserved log target labels", @@ -1399,7 +1399,7 @@ var planTests = []planTest{{ }, Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{}, - Sections: map[string]plan.LayerSection{}, + Sections: map[string]plan.Section{}, }, }, { summary: "Three layers missing command", From e0c4384d8a7f74b1a9e9859231e77d314c49f48c Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 16:05:01 +0200 Subject: [PATCH 09/17] Review feedback group 5 --- internals/plan/plan.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 107f1d45..6adfbccd 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -1255,7 +1255,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { } // Make sure builtinSections contains the exact same fields as expected // in the Layer type. - if !mapHasKeys(builtinSections, layerBuiltins) { + if !mapMatchKeys(builtinSections, layerBuiltins) { panic("internal error: parsed fields and layer fields differ") } @@ -1344,14 +1344,14 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { return layer, err } -// mapHasKeys returns true if the key list supplied is an exact match of the +// mapMatchKeys returns true if the key list supplied is an exact match of the // keys in the map (ordering is ignored). -func mapHasKeys[M ~map[K]V, K comparable, V any](inMap M, keyList []K) bool { +func mapMatchKeys[M ~map[K]V, K comparable, V any](inMap M, keyList []K) bool { if len(inMap) != len(keyList) { return false } - for _, key := range keyList { - if _, ok := inMap[key]; !ok { + for key, _ := range inMap { + if !slices.Contains(keyList, key) { return false } } From 9a51cfed310808b233dc2ba46acb0ab2aa1a3168 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 16:34:49 +0200 Subject: [PATCH 10/17] Review feedback group 6 --- internals/plan/export_test.go | 2 +- internals/plan/plan.go | 52 +++++++++++++++++------------------ internals/plan/plan_test.go | 4 +-- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/internals/plan/export_test.go b/internals/plan/export_test.go index 9da16930..1078d199 100644 --- a/internals/plan/export_test.go +++ b/internals/plan/export_test.go @@ -14,4 +14,4 @@ package plan -var LayerBuiltins = layerBuiltins +var BuiltinSections = builtinSections diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 6adfbccd..df5a54af 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -69,38 +69,38 @@ const ( ) var ( - // layerExtensions keeps a map of registered extensions. - layerExtensions = map[string]SectionExtension{} + // sectionExtensions keeps a map of registered extensions. + sectionExtensions = map[string]SectionExtension{} - // layerExtensionsOrder records the order in which the extensions were registered. - layerExtensionsOrder = []string{} + // sectionExtensionsOrder records the order in which the extensions were registered. + sectionExtensionsOrder = []string{} ) -// layerBuiltins represents all the built-in layer sections. This list is used +// builtinSections represents all the built-in layer sections. This list is used // for identifying built-in fields in this package. It is unit tested to match // the YAML fields exposed in the Layer type, to catch inconsistencies. -var layerBuiltins = []string{"summary", "description", "services", "checks", "log-targets"} +var builtinSections = []string{"summary", "description", "services", "checks", "log-targets"} // RegisterExtension adds a plan schema extension. All registrations must be // done before the plan library is used. The order in which extensions are // registered determines the order in which the sections are marshalled. // Extension sections are marshalled after the built-in sections. func RegisterExtension(field string, ext SectionExtension) { - if slices.Contains(layerBuiltins, field) { + if slices.Contains(builtinSections, field) { panic(fmt.Sprintf("internal error: extension %q already used as built-in field", field)) } - if _, ok := layerExtensions[field]; ok { + if _, ok := sectionExtensions[field]; ok { panic(fmt.Sprintf("internal error: extension %q already registered", field)) } - layerExtensions[field] = ext - layerExtensionsOrder = append(layerExtensionsOrder, field) + sectionExtensions[field] = ext + sectionExtensionsOrder = append(sectionExtensionsOrder, field) } // UnregisterExtension removes a plan schema extension. This is only // intended for use by tests during cleanup. func UnregisterExtension(field string) { - delete(layerExtensions, field) - layerExtensionsOrder = slices.DeleteFunc(layerExtensionsOrder, func(n string) bool { + delete(sectionExtensions, field) + sectionExtensionsOrder = slices.DeleteFunc(sectionExtensionsOrder, func(n string) bool { return n == field }) } @@ -133,7 +133,7 @@ func (p *Plan) MarshalYAML() (interface{}, error) { Type: reflect.TypeOf(p.LogTargets), Tag: `yaml:"log-targets,omitempty"`, }} - for i, field := range layerExtensionsOrder { + for i, field := range sectionExtensionsOrder { section := p.Sections[field] ordered = append(ordered, reflect.StructField{ Name: fmt.Sprintf("Dummy%v", i), @@ -147,7 +147,7 @@ func (p *Plan) MarshalYAML() (interface{}, error) { v.Field(0).Set(reflect.ValueOf(p.Services)) v.Field(1).Set(reflect.ValueOf(p.Checks)) v.Field(2).Set(reflect.ValueOf(p.LogTargets)) - for i, field := range layerExtensionsOrder { + for i, field := range sectionExtensionsOrder { v.Field(3 + i).Set(reflect.ValueOf(p.Sections[field])) } plan := v.Addr().Interface() @@ -673,7 +673,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { // the layers length check because we need the extension to provide us with // a zero value section, even if no layers are supplied (similar to the // allocations taking place above for the built-in types). - for field, extension := range layerExtensions { + for field, extension := range sectionExtensions { var sections []Section for _, layer := range layers { if section := layer.Sections[field]; section != nil { @@ -1055,7 +1055,7 @@ func (p *Plan) Validate() error { } // Each section extension must validate the combined plan. - for field, extension := range layerExtensions { + for field, extension := range sectionExtensions { err = extension.ValidatePlan(p) if err != nil { return fmt.Errorf("cannot validate plan section %q: %w", field, err) @@ -1246,7 +1246,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { // 2. We honor KnownFields = true behaviour for non extended schema // sections, and at the top field level, which includes Section field // names. - builtinSections := map[string]interface{}{ + builtins := map[string]interface{}{ "summary": &layer.Summary, "description": &layer.Description, "services": &layer.Services, @@ -1255,29 +1255,29 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { } // Make sure builtinSections contains the exact same fields as expected // in the Layer type. - if !mapMatchKeys(builtinSections, layerBuiltins) { + if !mapMatchKeys(builtins, builtinSections) { panic("internal error: parsed fields and layer fields differ") } - layerSections := make(map[string]yaml.Node) + sections := make(map[string]yaml.Node) // Deliberately pre-allocate at least an empty yaml.Node for every // extension section. Extension sections that have unmarshalled // will update the respective node, while non-existing sections // will at least have an empty node. This means we can consistently // let the extension allocate and decode the yaml node for all sections, // and in the case where it is zero, we get an empty backing type instance. - for field, _ := range layerExtensions { - layerSections[field] = yaml.Node{} + for field, _ := range sectionExtensions { + sections[field] = yaml.Node{} } - err := yaml.Unmarshal(data, &layerSections) + err := yaml.Unmarshal(data, §ions) if err != nil { return nil, &FormatError{ Message: fmt.Sprintf("cannot parse layer %q: %v", label, err), } } - for field, section := range layerSections { - if slices.Contains(layerBuiltins, field) { + for field, section := range sections { + if slices.Contains(builtinSections, field) { // The following issue prevents us from using the yaml.Node decoder // with KnownFields = true behaviour. Once one of the proposals get // merged, we can remove the intermediate Marshal step. @@ -1288,13 +1288,13 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { } dec := yaml.NewDecoder(bytes.NewReader(data)) dec.KnownFields(true) - if err = dec.Decode(builtinSections[field]); err != nil { + if err = dec.Decode(builtins[field]); err != nil { return nil, &FormatError{ Message: fmt.Sprintf("cannot parse layer %q section %q: %v", label, field, err), } } } else { - extension, ok := layerExtensions[field] + extension, ok := sectionExtensions[field] if !ok { // At the top level we do not ignore keys we do not understand. // This preserves the current Pebble behaviour of decoding with diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 34692c93..e1f7f002 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -2076,9 +2076,9 @@ func (s *S) TestStartStopOrderMultipleLanes(c *C) { // reflects the same YAML fields as exposed in the Layer type. func (s *S) TestLayerBuiltinCompatible(c *C) { fields := structYamlFields(plan.Layer{}) - c.Assert(len(fields), Equals, len(plan.LayerBuiltins)) + c.Assert(len(fields), Equals, len(plan.BuiltinSections)) for _, field := range structYamlFields(plan.Layer{}) { - c.Assert(slices.Contains(plan.LayerBuiltins, field), Equals, true) + c.Assert(slices.Contains(plan.BuiltinSections, field), Equals, true) } } From bd65e5239cef27767cae50262f1dca4d2dda76fd Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 17:00:23 +0200 Subject: [PATCH 11/17] Review feedback group 7 --- internals/plan/extensions_test.go | 6 +++--- internals/plan/plan.go | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index 2cbd8b86..dd56b6af 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -190,7 +190,7 @@ var extensionTests = []struct { a: a b: b`, }}, - error: "cannot validate layer section .* cannot accept entry not starting .*", + error: ".*cannot accept entry not starting.*", }, { summary: "Load file layers with section validation failure #2", extensions: []extension{{ @@ -209,7 +209,7 @@ var extensionTests = []struct { x-field: x1:`, }}, - error: "cannot validate layer section .* cannot have nil entry .*", + error: ".*cannot have nil entry.*", }, { summary: "Load file layers failed plan validation", extensions: []extension{{ @@ -244,7 +244,7 @@ var extensionTests = []struct { a: a b: b`, }}, - error: "cannot validate plan section .* cannot find .* as required by .*", + error: ".*cannot find.*", }, { summary: "Check empty section omits entry", extensions: []extension{{ diff --git a/internals/plan/plan.go b/internals/plan/plan.go index df5a54af..b625eee4 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -954,10 +954,10 @@ func (layer *Layer) Validate() error { } } - for field, section := range layer.Sections { + for _, section := range layer.Sections { err := section.Validate() if err != nil { - return fmt.Errorf("cannot validate layer section %q: %w", field, err) + return err } } @@ -1055,10 +1055,10 @@ func (p *Plan) Validate() error { } // Each section extension must validate the combined plan. - for field, extension := range sectionExtensions { + for _, extension := range sectionExtensions { err = extension.ValidatePlan(p) if err != nil { - return fmt.Errorf("cannot validate plan section %q: %w", field, err) + return err } } From a7ae9ba49de451f173b2cf431789e088f4990584 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 17:07:13 +0200 Subject: [PATCH 12/17] tweak --- internals/plan/plan.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index b625eee4..2a95d7e7 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -1253,7 +1253,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { "checks": &layer.Checks, "log-targets": &layer.LogTargets, } - // Make sure builtinSections contains the exact same fields as expected + // Make sure builtins contains the exact same fields as expected // in the Layer type. if !mapMatchKeys(builtins, builtinSections) { panic("internal error: parsed fields and layer fields differ") From 22040e0045ab3ede1bf94e9860c951a0adb87665 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 20:57:48 +0200 Subject: [PATCH 13/17] extension errors must provide full context --- internals/plan/plan.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 2a95d7e7..99e5dee2 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -683,9 +683,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { var err error combined.Sections[field], err = extension.CombineSections(sections...) if err != nil { - return nil, &FormatError{ - Message: fmt.Sprintf("cannot combine section %q: %v", field, err), - } + return nil, err } } @@ -1307,9 +1305,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { // Section unmarshal rules are defined by the extension itself. layer.Sections[field], err = extension.ParseSection(section) if err != nil { - return nil, &FormatError{ - Message: fmt.Sprintf("cannot parse layer %q section %q: %v", label, field, err), - } + return nil, err } } } From 518bbc340383d80133a4d09733674ed3acd023d2 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 21:05:46 +0200 Subject: [PATCH 14/17] remove redundant query --- internals/plan/plan_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index e1f7f002..1286254e 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -2077,7 +2077,7 @@ func (s *S) TestStartStopOrderMultipleLanes(c *C) { func (s *S) TestLayerBuiltinCompatible(c *C) { fields := structYamlFields(plan.Layer{}) c.Assert(len(fields), Equals, len(plan.BuiltinSections)) - for _, field := range structYamlFields(plan.Layer{}) { + for _, field := range fields { c.Assert(slices.Contains(plan.BuiltinSections, field), Equals, true) } } From b49e261b3d771e5a7d0b1fab1fd24cb8411b23e9 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 21:09:34 +0200 Subject: [PATCH 15/17] tweak --- internals/plan/extensions_test.go | 5 +++-- internals/plan/plan_test.go | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index dd56b6af..8633af90 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -426,8 +426,9 @@ nexttest: } // TestSectionOrderExt ensures built-in and extension section ordering -// does not change. Extensions are ordered according to the order of -// registration. +// rules are maintained. Extensions are ordered according to the order of +// registration and follows the built-in sections which are ordered +// the same way they are defined in the Plan struct. func (s *S) TestSectionOrderExt(c *C) { plan.RegisterExtension("x-field", &xExtension{}) plan.RegisterExtension("y-field", &yExtension{}) diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 1286254e..4e4f22c2 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -2110,7 +2110,8 @@ func firstLetterToLower(s string) string { return string(r) } -// TestSectionOrder ensures built-in section order is maintained. +// TestSectionOrder ensures built-in section order is maintained +// during Plan marshal operations. func (s *S) TestSectionOrder(c *C) { layer, err := plan.ParseLayer(1, "label", reindent(` checks: From f665085c5aabf32509f65987b10f003a4c4cd21f Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Thu, 29 Aug 2024 10:03:15 +0200 Subject: [PATCH 16/17] simplify YAML field changes detection --- internals/plan/extensions_test.go | 4 ++-- internals/plan/plan.go | 19 ------------------- internals/plan/plan_test.go | 21 +++++++++++---------- 3 files changed, 13 insertions(+), 31 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index 8633af90..9885bf20 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -35,8 +35,8 @@ type inputLayer struct { } // PlanResult represents the final content of a combined plan. Since this -// package exclusively focuses extensions, all built-in sections are empty -// and ignored in the test results. +// test file exclusively focuses on extensions, all built-in sections are +// empty and ignored in the test results. type planResult struct { x *xSection y *ySection diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 99e5dee2..3d8dc663 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -1251,11 +1251,6 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { "checks": &layer.Checks, "log-targets": &layer.LogTargets, } - // Make sure builtins contains the exact same fields as expected - // in the Layer type. - if !mapMatchKeys(builtins, builtinSections) { - panic("internal error: parsed fields and layer fields differ") - } sections := make(map[string]yaml.Node) // Deliberately pre-allocate at least an empty yaml.Node for every @@ -1340,20 +1335,6 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { return layer, err } -// mapMatchKeys returns true if the key list supplied is an exact match of the -// keys in the map (ordering is ignored). -func mapMatchKeys[M ~map[K]V, K comparable, V any](inMap M, keyList []K) bool { - if len(inMap) != len(keyList) { - return false - } - for key, _ := range inMap { - if !slices.Contains(keyList, key) { - return false - } - } - return true -} - func validServiceAction(action ServiceAction, additionalValid ...ServiceAction) bool { for _, v := range additionalValid { if action == v { diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 4e4f22c2..99b617a8 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -20,7 +20,6 @@ import ( "os" "path/filepath" "reflect" - "slices" "strings" "time" "unicode" @@ -29,6 +28,7 @@ import ( "gopkg.in/yaml.v3" "github.com/canonical/pebble/internals/plan" + "github.com/canonical/pebble/internals/testutil" ) const ( @@ -2072,14 +2072,15 @@ func (s *S) TestStartStopOrderMultipleLanes(c *C) { c.Assert(lanes[2], DeepEquals, []string{"srv3"}) } -// TestLayerBuiltinCompatible ensures layerBuiltins used in the plan package -// reflects the same YAML fields as exposed in the Layer type. -func (s *S) TestLayerBuiltinCompatible(c *C) { - fields := structYamlFields(plan.Layer{}) - c.Assert(len(fields), Equals, len(plan.BuiltinSections)) - for _, field := range fields { - c.Assert(slices.Contains(plan.BuiltinSections, field), Equals, true) - } +// TestSectionFieldStability detects changes in plan.Layer and plan.Plan +// YAML fields, and fails on any change that could break hardcoded section +// fields in the code. On failure, please carefully inspect and update +// the plan library where required. +func (s *S) TestSectionFieldStability(c *C) { + layerFields := structYamlFields(plan.Layer{}) + c.Assert(layerFields, testutil.DeepUnsortedMatches, []string{"summary", "description", "services", "checks", "log-targets", "sections"}) + planFields := structYamlFields(plan.Plan{}) + c.Assert(planFields, testutil.DeepUnsortedMatches, []string{"services", "checks", "log-targets", "sections"}) } // structYamlFields extracts the YAML fields from a struct. If the YAML tag @@ -2090,7 +2091,7 @@ func structYamlFields(inStruct any) []string { for i := range inStructType.NumField() { fieldType := inStructType.Field(i) yamlTag := fieldType.Tag.Get("yaml") - if fieldType.IsExported() && yamlTag != "-" && !strings.Contains(yamlTag, ",inline") { + if fieldType.IsExported() && yamlTag != "-" { tag, _, _ := strings.Cut(fieldType.Tag.Get("yaml"), ",") if tag == "" { tag = firstLetterToLower(fieldType.Name) From ede4849aaba7d3a8a9f26671ca40fb9b212e37a7 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Fri, 30 Aug 2024 08:46:17 +0200 Subject: [PATCH 17/17] naming tweak --- internals/plan/extensions_test.go | 14 +++++++------- internals/plan/plan.go | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/internals/plan/extensions_test.go b/internals/plan/extensions_test.go index 9885bf20..0be9d051 100644 --- a/internals/plan/extensions_test.go +++ b/internals/plan/extensions_test.go @@ -353,7 +353,7 @@ func (s *S) TestPlanExtensions(c *C) { defer func() { // Remove remaining registered extensions. for _, field := range registeredExtensions { - plan.UnregisterExtension(field) + plan.UnregisterSectionExtension(field) } }() @@ -363,7 +363,7 @@ nexttest: // Unregister extensions from previous test iteraton. for _, field := range registeredExtensions { - plan.UnregisterExtension(field) + plan.UnregisterSectionExtension(field) } registeredExtensions = []string{} @@ -380,7 +380,7 @@ nexttest: err = fmt.Errorf("%v", r) } }() - plan.RegisterExtension(e.field, e.ext) + plan.RegisterSectionExtension(e.field, e.ext) registeredExtensions = append(registeredExtensions, e.field) return nil }() @@ -430,11 +430,11 @@ nexttest: // registration and follows the built-in sections which are ordered // the same way they are defined in the Plan struct. func (s *S) TestSectionOrderExt(c *C) { - plan.RegisterExtension("x-field", &xExtension{}) - plan.RegisterExtension("y-field", &yExtension{}) + plan.RegisterSectionExtension("x-field", &xExtension{}) + plan.RegisterSectionExtension("y-field", &yExtension{}) defer func() { - plan.UnregisterExtension("x-field") - plan.UnregisterExtension("y-field") + plan.UnregisterSectionExtension("x-field") + plan.UnregisterSectionExtension("y-field") }() layer, err := plan.ParseLayer(1, "label", reindent(` diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 3d8dc663..d75146b7 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -81,11 +81,11 @@ var ( // the YAML fields exposed in the Layer type, to catch inconsistencies. var builtinSections = []string{"summary", "description", "services", "checks", "log-targets"} -// RegisterExtension adds a plan schema extension. All registrations must be +// RegisterSectionExtension adds a plan schema extension. All registrations must be // done before the plan library is used. The order in which extensions are // registered determines the order in which the sections are marshalled. // Extension sections are marshalled after the built-in sections. -func RegisterExtension(field string, ext SectionExtension) { +func RegisterSectionExtension(field string, ext SectionExtension) { if slices.Contains(builtinSections, field) { panic(fmt.Sprintf("internal error: extension %q already used as built-in field", field)) } @@ -96,9 +96,9 @@ func RegisterExtension(field string, ext SectionExtension) { sectionExtensionsOrder = append(sectionExtensionsOrder, field) } -// UnregisterExtension removes a plan schema extension. This is only +// UnregisterSectionExtension removes a plan schema extension. This is only // intended for use by tests during cleanup. -func UnregisterExtension(field string) { +func UnregisterSectionExtension(field string) { delete(sectionExtensions, field) sectionExtensionsOrder = slices.DeleteFunc(sectionExtensionsOrder, func(n string) bool { return n == field