From fb8abd2a8b281bc138361dad41858dfef7d1b0d7 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 28 Aug 2024 12:34:01 +0200 Subject: [PATCH] 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`))) +}