diff --git a/parser/document.go b/parser/document.go index e86b0ac..d80508d 100644 --- a/parser/document.go +++ b/parser/document.go @@ -92,6 +92,18 @@ type Node struct { RawNode *yaml.Node } +// sharedNodeFrame accumulates the properties documented while a shared +// (non-end) node's subtree is being walked for the first time, relative to +// that node's own path. Once the walk of that subtree finishes, the frame's +// properties are cached so a later encounter of the same *yaml.Node — via a +// different alias or "<<" merge site — can replay them at its own path +// instead of either silently dropping them or re-walking the whole subtree +// again. +type sharedNodeFrame struct { + root paths.Path + properties []Property +} + func Load(filename string, includeHidden bool) (*Document, error) { file, err := os.Open(filename) if err != nil { @@ -109,10 +121,42 @@ func Load(filename string, includeHidden bool) (*Document, error) { HeadComments: parseComments(root.HeadComment), FootComment: parseComments(root.FootComment), } - // visited prevents alias cycles (CWE-674 stack overflow) and alias fan-out - // (OOM) by stopping re-descent into any non-end node already on this walk. - visited := map[*yaml.Node]bool{} - err = walk(node, func(node Node) (bool, error) { + + // memo caches the properties documented under a shared node's subtree, + // relative to the path it was first reached at, keyed by the underlying + // *yaml.Node so later encounters (via aliases or "<<" merge keys) can + // replay them rebased onto their own path. + memo := map[*yaml.Node][]Property{} + // inProgress guards against a node that, directly or transitively, + // refers to itself (CWE-674 stack overflow) — it is only ever true while + // that node's subtree is actively being walked for the first time. + inProgress := map[*yaml.Node]bool{} + // frames is a stack of shared nodes currently being walked for the first + // time, so any property found anywhere below them gets recorded + // (relative to each) for later replay. Diamond-shaped reuse (the same + // node reached from several sibling sites) is fine and expected; only a + // true cycle — re-entering a node already in this stack — is refused. + var frames []*sharedNodeFrame + + // record appends a property to the document's current section and, for + // every shared node currently being walked for the first time, also + // records it (relative to that node's own path) for replay at future + // encounters. + record := func(prop Property) { + sectionIdx := len(document.Sections) - 1 + document.Sections[sectionIdx].Properties = append(document.Sections[sectionIdx].Properties, prop) + + for _, f := range frames { + if len(prop.Path) < len(f.root) { + continue + } + rel := prop + rel.Path = append(paths.Path{}, prop.Path[len(f.root):]...) + f.properties = append(f.properties, rel) + } + } + + err = walk(node, func(node Node) (bool, func(), error) { comment := pop(&node.HeadComments) parseCommentsOntoDocument(node.Path.Parent(), &document, node.HeadComments) @@ -120,12 +164,12 @@ func Load(filename string, includeHidden bool) (*Document, error) { // If we have a comment instructing us to skip this node, obey it if comment.Tags.GetBool(TagIgnore) { - return true, nil + return true, nil, nil } // If we have a comment instructing us to hide this node, obey it if we are not including hidden nodes if comment.Tags.GetBool(TagHidden) && !includeHidden { - return true, nil + return true, nil, nil } // An end node is a node we find a property at, this is usually a scalar @@ -133,24 +177,46 @@ func Load(filename string, includeHidden bool) (*Document, error) { // +docs:property tag (or if they have no values). if !isEndNode(node, comment) { parseCommentsOntoDocument(node.Path.Parent(), &document, []Comment{comment}) - // Only guard recursion into children — end nodes are always safe to - // visit multiple times (scalars have no children to cycle through). - if visited[node.RawNode] { - return true, nil + + // We've fully documented this exact node before, via a different + // alias or "<<" merge site — replay what we found there instead + // of silently dropping it. + if cached, ok := memo[node.RawNode]; ok { + for _, p := range cached { + p.Path = append(append(paths.Path{}, node.Path...), p.Path...) + record(p) + } + return true, nil, nil + } + + // This node is already being walked further up the current call + // stack — a genuine cycle (e.g. a mapping merging itself). + // Stop here rather than recursing forever. + if inProgress[node.RawNode] { + return true, nil, nil } - visited[node.RawNode] = true - return false, nil + + inProgress[node.RawNode] = true + frame := &sharedNodeFrame{root: node.Path} + frames = append(frames, frame) + + after := func() { + memo[node.RawNode] = frame.properties + delete(inProgress, node.RawNode) + frames = frames[:len(frames)-1] + } + + return false, after, nil } - sectionIdx := len(document.Sections) - 1 - document.Sections[sectionIdx].Properties = append(document.Sections[sectionIdx].Properties, Property{ + record(Property{ Path: node.Path, Description: comment, Type: getTypeOf(node, comment), Default: getDefaultValue(node, comment), }) - return true, nil + return true, nil, nil }) return &document, err @@ -243,14 +309,24 @@ func parseCommentsOntoDocument(path paths.Path, document *Document, comments []C } } -func walk(root Node, fn func(node Node) (bool, error)) error { +// walk performs a depth-first traversal of a yaml node tree, calling fn for +// every node encountered. fn reports whether to stop descending into this +// node's children and may optionally return an "after" function, which is +// called once this node and all of its descendants have been fully visited — +// this lets the caller pair up per-node setup (e.g. entering a subtree for +// the first time) with a matching cleanup step. +func walk(root Node, fn func(node Node) (stop bool, after func(), err error)) error { // Call the function for every node, we the method can decide to stop // walking this branch as part of this call - stop, err := fn(root) + stop, after, err := fn(root) if err != nil { return err } + if after != nil { + defer after() + } + if stop { return nil } @@ -271,15 +347,12 @@ func walk(root Node, fn func(node Node) (bool, error)) error { } } case yaml.MappingNode: - for i := 0; i < len(root.RawNode.Content); i += 2 { - keyNode := root.RawNode.Content[i] - valueNode := root.RawNode.Content[i+1] - + for _, entry := range mappingEntries(root.RawNode, map[*yaml.Node]bool{}) { n := Node{ - Path: root.Path.WithProperty(keyNode.Value), - HeadComments: parseComments(keyNode.HeadComment), - FootComment: parseComments(keyNode.FootComment), - RawNode: valueNode, + Path: root.Path.WithProperty(entry.Key.Value), + HeadComments: parseComments(entry.Key.HeadComment), + FootComment: parseComments(entry.Key.FootComment), + RawNode: entry.Value, } if err := walk(n, fn); err != nil { @@ -315,6 +388,91 @@ func walk(root Node, fn func(node Node) (bool, error)) error { return nil } +// mappingEntry is a resolved key/value pair from a mapping node, after +// expanding any "<<" merge keys. +type mappingEntry struct { + Key *yaml.Node + Value *yaml.Node +} + +// isMergeKey returns true if n is a "<<" merge key, per the YAML merge key +// convention (not part of the core YAML spec, but widely supported). +func isMergeKey(n *yaml.Node) bool { + return n.Kind == yaml.ScalarNode && n.Value == "<<" && (n.Tag == "" || n.Tag == "!" || n.ShortTag() == "!!merge") +} + +// resolveMergeTargets expands a merge key's value into the mapping nodes it +// refers to. The value may be a single mapping, an alias to one, or a +// sequence of either (for merging in more than one mapping at once). +func resolveMergeTargets(n *yaml.Node) []*yaml.Node { + switch n.Kind { + case yaml.AliasNode: + return resolveMergeTargets(n.Alias) + case yaml.MappingNode: + return []*yaml.Node{n} + case yaml.SequenceNode: + var out []*yaml.Node + for _, item := range n.Content { + out = append(out, resolveMergeTargets(item)...) + } + return out + default: + // Not a valid merge target, e.g. an alias to a scalar. Ignore it + // rather than failing the whole document. + return nil + } +} + +// mappingEntries returns a mapping node's effective key/value pairs with any +// "<<" merge keys expanded in place. Explicit keys always take precedence +// over merged-in ones regardless of declaration order, and where more than +// one merge source defines the same key, the earliest one wins — both match +// the YAML merge key convention. +// +// inProgress guards against a merge target that (directly or transitively) +// merges itself, which would otherwise recurse forever. +func mappingEntries(n *yaml.Node, inProgress map[*yaml.Node]bool) []mappingEntry { + if inProgress[n] { + return nil + } + inProgress[n] = true + defer delete(inProgress, n) + + explicit := map[string]bool{} + for i := 0; i < len(n.Content); i += 2 { + if key := n.Content[i]; !isMergeKey(key) { + explicit[key.Value] = true + } + } + + seen := map[string]bool{} + var entries []mappingEntry + + for i := 0; i < len(n.Content); i += 2 { + key, value := n.Content[i], n.Content[i+1] + + if !isMergeKey(key) { + if !seen[key.Value] { + seen[key.Value] = true + entries = append(entries, mappingEntry{Key: key, Value: value}) + } + continue + } + + for _, target := range resolveMergeTargets(value) { + for _, entry := range mappingEntries(target, inProgress) { + if explicit[entry.Key.Value] || seen[entry.Key.Value] { + continue + } + seen[entry.Key.Value] = true + entries = append(entries, entry) + } + } + } + + return entries +} + // isEndNode returns true if the yaml node is considered one that should // be documented as a parameter. // diff --git a/parser/document_test.go b/parser/document_test.go index 508e79f..765d482 100644 --- a/parser/document_test.go +++ b/parser/document_test.go @@ -44,7 +44,10 @@ func TestLoad_SelfReferentialAlias(t *testing.T) { _ = err } -// Scaled-down billion-laughs must not OOM. +// Scaled-down billion-laughs must not OOM, and — since a shared node's +// properties are now memoized and replayed rather than just walked once — +// must still produce the exact (bounded, polynomial) number of properties +// implied by the fan-out, not an exponential blow-up. // Uses 5 levels (10^5 = 100 000 virtual nodes) instead of 9 to keep the test // fast while still exercising the fan-out path. func TestLoad_BillionLaughs(t *testing.T) { @@ -56,14 +59,20 @@ d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c,*c] e: [*d,*d,*d,*d,*d,*d,*d,*d,*d,*d] ` path := writeTemp(t, yaml) - _, err := Load(path, false) - _ = err + doc, err := Load(path, false) + require.NoError(t, err) + + total := 0 + for _, s := range doc.Sections { + total += len(s.Properties) + } + // a:10 + b:10*10 + c:10*100 + d:10*1000 + e:10*10000 = 111110 + assert.Equal(t, 111110, total) } -// Shared anchors must not panic. Note: the visited-set means a non-scalar -// anchor's subtree is walked only at the first alias reference, so serviceB's -// properties are not documented — that trade-off is intentional. -func TestLoad_SharedAnchorDoesNotPanic(t *testing.T) { +// A mapping merged into multiple mappings via "<<" must surface its +// properties under each merge site, not just the first. +func TestLoad_MergeKeySharedAcrossMappings(t *testing.T) { yaml := ` defaults: &defaults replicaCount: 1 @@ -81,6 +90,148 @@ serviceB: doc, err := Load(path, false) require.NoError(t, err) require.NotNil(t, doc) + + var paths []string + for _, s := range doc.Sections { + for _, p := range s.Properties { + paths = append(paths, p.Path.String()) + } + } + assert.Contains(t, paths, "serviceA.replicaCount") + assert.Contains(t, paths, "serviceA.image") + assert.Contains(t, paths, "serviceA.port") + assert.Contains(t, paths, "serviceB.replicaCount") + assert.Contains(t, paths, "serviceB.image") + assert.Contains(t, paths, "serviceB.port") + assert.NotContains(t, paths, "serviceA.<<") + assert.NotContains(t, paths, "serviceB.<<") +} + +// An explicit key must override a merged-in key of the same name regardless +// of whether it's declared before or after the "<<" merge key. +func TestLoad_MergeKeyExplicitOverridesMerged(t *testing.T) { + yaml := ` +defaults: &defaults + # -- from defaults + replicaCount: 1 + +service: + <<: *defaults + # -- overridden + replicaCount: 3 +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var found *Property + for _, s := range doc.Sections { + for i, p := range s.Properties { + if p.Path.String() == "service.replicaCount" { + found = &s.Properties[i] + } + } + } + require.NotNil(t, found, "expected service.replicaCount to be documented") + assert.Equal(t, "3", found.Default) +} + +// Merging multiple mappings via a sequence must resolve conflicting keys in +// favour of the earliest mapping in the sequence. +func TestLoad_MergeKeySequencePrecedence(t *testing.T) { + yaml := ` +a: &a + value: from-a +b: &b + value: from-b + +service: + <<: [*a, *b] +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var found *Property + for _, s := range doc.Sections { + for i, p := range s.Properties { + if p.Path.String() == "service.value" { + found = &s.Properties[i] + } + } + } + require.NotNil(t, found, "expected service.value to be documented") + assert.Equal(t, "from-a", found.Default) +} + +// A mapping that merges itself (directly or transitively) must not cause a +// stack overflow. +func TestLoad_MergeKeySelfReferential(t *testing.T) { + yaml := "a: &a\n <<: *a\n b: 1\n" + path := writeTemp(t, yaml) + _, err := Load(path, false) + // The call must return (possibly with an error) — a stack overflow is + // fatal and would kill the test process before we reach this line. + _ = err +} + +// A non-scalar value nested inside a mapping merged at more than one site +// must be documented at every site, not just the first. Before memoization +// was added, only the first site to reach the shared "nested" node got its +// properties — later sites silently got nothing, because the cycle/fan-out +// guard treated every repeat encounter as already handled. +func TestLoad_MergeKeyNestedSharedSubtreeDocumentedAtEverySite(t *testing.T) { + yaml := ` +template: &template + nested: + # -- from the shared template + value: shared-default + +svcA: + <<: *template + +svcB: + <<: *template +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var paths []string + for _, s := range doc.Sections { + for _, p := range s.Properties { + paths = append(paths, p.Path.String()) + } + } + assert.Contains(t, paths, "svcA.nested.value") + assert.Contains(t, paths, "svcB.nested.value") +} + +// The same nested-sharing fix must also apply to plain alias reuse, not just +// "<<" merge keys — a nested non-scalar value reachable from two different +// alias sites must be documented at both. +func TestLoad_AliasNestedSharedSubtreeDocumentedAtEverySite(t *testing.T) { + yaml := ` +template: &template + nested: + # -- from the shared template + value: shared-default + +svcA: *template +svcB: *template +` + path := writeTemp(t, yaml) + doc, err := Load(path, false) + require.NoError(t, err) + + var paths []string + for _, s := range doc.Sections { + for _, p := range s.Properties { + paths = append(paths, p.Path.String()) + } + } + assert.Contains(t, paths, "svcA.nested.value") + assert.Contains(t, paths, "svcB.nested.value") } // A plain acyclic values file must parse correctly and surface its properties.