Resolve nested and module-default manage toggles in the Hiera data check

CHANGELOG.md

@@ -10,7 +10,7 @@ module follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 ### Added
 
 - An externally provisioned runner account whose primary group is named differently from the account now converges on the first apply: the new optional `runner_account.group` names the account's primary group and feeds every group ownership the module manages. It defaults to the account name, so existing hosts are unchanged.
-- A Hiera data check ships with the module: it fails on any data key that no deployed class declares, instead of Hiera silently ignoring it, and flags subkeys set under a disabled `manage` toggle with a non-failing advisory.
+- A Hiera data check ships with the module: it fails on any data key that no deployed class declares, instead of Hiera silently ignoring it, and flags subkeys the module will not read under a disabled `manage` toggle with a non-failing advisory. The advisory resolves the toggle wherever it sits, including nested toggles and toggles left at their module default, and spares a subkey that merely restates the default.
 
 ### Changed
 

README.md

@@ -475,16 +475,22 @@ ruby puppet/modules/rootless_gitlab_runner/scripts/check_hiera_data.rb --data-di
 pipeline (parser validation, YAML lint, this check) for a standalone control repository; the
 same command also works as a pre-commit hook for local feedback. Two behaviors to know:
 
-- **Advisory, non-failing:** subkeys set under a hash parameter whose effective `manage`
-  toggle resolves to `false` are recognized but not enforced as resources (the module is
-  hands-off that concern, though it may still read some of them as shared inputs; see
-  the `manage`-key rule above). The check reports them as an advisory rather than a
-  failure — declaring the state of an externally owned concern can be intentional — and a
-  human judges intent.
-- **Stated limits:** the check validates key names, not values (types are the compiler's job,
-  enforced at compile time), and it cannot see data layers outside the repository, such as the
-  off-repository secret store. Hierarchy levels addressed by `glob`/`globs` or `mapped_paths`
-  are not modeled; advisory resolution covers `path`/`paths` levels only.
+- **Advisory, non-failing:** a subkey set under a hash parameter whose effective
+  `manage` toggle resolves to `false` is recognized but not enforced as a resource
+  (the module is hands-off that concern, though it may still read some subkeys as
+  shared inputs; see the `manage`-key rule above). The toggle is resolved wherever
+  it sits, at the top level or nested in a sub-hash (`packages.sources.manage`,
+  `standalone.self_update.manage`), and over the module's own defaults, so a toggle
+  left at its default is still caught. A value that differs from the module default
+  is reported; a mere restatement of the default is treated as inert. The check
+  reports these as an advisory rather than a failure (declaring the state of an
+  externally owned concern can be intentional), and a human judges intent.
+- **Stated limits:** the check validates key names, not values (types are the
+  compiler's job, enforced at compile time), and it cannot see consumer data layers
+  outside the repository, such as the off-repository secret store. The module
+  defaults are read from static `path`/`paths` levels only. Hierarchy levels
+  addressed by `glob`/`globs` or `mapped_paths` are not modeled; advisory resolution
+  covers `path`/`paths` levels only.
 
 The module's own [`examples/data/`](examples/data/) is held to the same rule by the unit
 suite, so the shipped examples cannot drift from the parameter surface.

scripts/check_hiera_data.rb

@@ -30,26 +30,34 @@
 # without a `::` are not automatic-parameter-lookup keys and are ignored.
 #
 # Advisory (non-failing): a declared hash-valued parameter can carry a
-# `manage` toggle ("false means hands-off"). A `manage` that resolves to
-# false means the module does not create or enforce that concern's resources,
-# so subkeys set under it are not enforced — though a module may still read
-# some of them as shared inputs (identity keys other concerns derive from).
-# Setting them can be legitimate declared-state documentation of an
-# externally owned concern, so the check reports an advisory and a human
-# judges intent. The
-# effective `manage` is resolved from the repository's own data layers: the
-# hierarchy levels of the given hiera.yaml that resolve inside the data
-# directory, in hierarchy order, highest-priority `manage` subkey wins (the
-# first-found value, matching both Hiera first-found and deep-merge semantics
-# for a scalar subkey). Node contexts are derived from the data file names
-# matched by interpolated hierarchy paths. When no in-repository layer sets
-# `manage`, the check stays silent — the module-side default is a data layer
-# it cannot see.
+# `manage` toggle ("false means hands-off"), at the top level or nested inside
+# a sub-hash (for example `packages.sources.manage`,
+# `standalone.self_update.manage`). Each subkey is judged against the nearest
+# enclosing `manage` on its own path: a `manage` that resolves to false means
+# the module does not create or enforce that concern's resources, so subkeys
+# set under it are not enforced — though a module may still read some of them
+# as shared inputs (identity keys other concerns derive from). Setting them can
+# be legitimate declared-state documentation of an externally owned concern, so
+# the check reports an advisory and a human judges intent.
+#
+# The effective `manage` is resolved from the consumer's own data layers over
+# the deployed module's module data layer (its `hiera.yaml` and data directory,
+# the lowest-priority source), in hierarchy order, highest-priority `manage`
+# subkey wins (first-found, matching Hiera's first-found and deep-merge
+# semantics for a scalar subkey). Reading the module defaults resolves a toggle
+# left at its default (silent before) and lets the advisory separate a consumer
+# value that differs from the module default (flagged) from a mere restatement
+# of the default (inert, suppressed). Node contexts are derived from the data
+# file names matched by interpolated hierarchy paths.
 #
 # Stated limits: the check validates key names, not values (types are the
-# compiler's job), and it cannot see data layers outside the repository (for
-# example an off-repository secret store; hierarchy levels whose datadir
-# resolves outside the checked data directory are skipped). Hierarchy levels
+# compiler's job), and it cannot see consumer data layers outside the checked
+# directory (for example an off-repository secret store; hierarchy levels whose
+# datadir resolves outside the checked data directory are skipped). The module
+# data layer is read for its defaults over static `path`/`paths` levels only; a
+# module with no data layer, or whose defaults sit behind interpolated,
+# `glob`/`globs`, or `mapped_paths` levels, contributes no default and its
+# toggles fall back to unresolved. On the consumer side too, hierarchy levels
 # addressed by `glob`/`globs` or `mapped_paths` are not modeled: advisory
 # resolution covers `path`/`paths` levels only. The `manage` resolution is
 # subkey-level, i.e. deep-merge semantics (what the module's `lookup_options`
@@ -221,35 +229,118 @@ def files_for(context)
     end
   end
 
+  # class::param => deep-merged default value across every deployed module's
+  # own module data layer (its hiera.yaml + data dir): the lowest-priority
+  # source for the manage resolution. Static path/paths levels only; a module
+  # without a data layer, or whose data sits behind interpolated/glob/
+  # mapped_paths levels, contributes nothing (its toggles stay unresolved).
+  def module_defaults
+    @module_defaults ||= module_dirs.each_with_object({}) do |dir, acc|
+      module_layer(dir).each { |key, value| acc[key] = value }
+    end
+  end
+
+  def module_layer(dir)
+    hiera_config = File.join(dir, 'hiera.yaml')
+    return {} unless File.exist?(hiera_config)
+
+    cfg = YAML.safe_load_file(hiera_config)
+    base = File.expand_path(dir)
+    default_datadir = cfg.to_h.dig('defaults', 'datadir') || 'data'
+    files = cfg.to_h['hierarchy'].to_a.flat_map do |level|
+      datadir = File.expand_path(level['datadir'] || default_datadir, base)
+      Array(level['paths'] || level['path']).reject { |p| p.include?('%{') }
+                                            .map { |p| File.join(datadir, p) }
+    end.select { |f| File.file?(f) }
+    merge_layers(files.filter_map do |file|
+      content = YAML.safe_load_file(file, aliases: true, permitted_classes: [Date, Time, Symbol])
+      content if content.is_a?(Hash)
+    end)
+  end
+
+  # Deep-merge a list of hash layers given highest-priority first.
+  def merge_layers(values)
+    values.reverse.reduce({}) { |acc, value| deep_merge(value, acc) }
+  end
+
+  # Merge high over low: hashes merge recursively; for any non-hash the
+  # higher-priority value wins (array-union across layers is not modeled).
+  def deep_merge(high, low)
+    return high unless high.is_a?(Hash) && low.is_a?(Hash)
+
+    low.merge(high) { |_key, low_val, high_val| deep_merge(high_val, low_val) }
+  end
+
   def advisories
     @advisories ||= contexts.flat_map { |context| context_advisories(context) }.uniq
   end
 
   def context_advisories(context)
     files = files_for(context)
     offending_keys = offenders.map { |_, key, _| key }.to_set
-    candidate_keys(files).filter_map do |key|
-      next if offending_keys.include?(key)
-
-      # Highest-priority manage subkey wins; unresolved in-repository => silent.
-      managed = files.filter_map { |f| parsed_data[f][key] }
-                     .find { |v| v.is_a?(Hash) && v.key?('manage') }
-      next unless managed && managed['manage'] == false
-
-      unenforced = files.filter_map do |file|
-        value = parsed_data[file][key]
-        subkeys = value.is_a?(Hash) ? value.keys - ['manage'] : []
-        "#{subkeys.sort.join(', ')} (#{file})" unless subkeys.empty?
+    candidate_keys(files).flat_map do |key|
+      next [] if offending_keys.include?(key)
+
+      repo_values = files.filter_map { |f| parsed_data[f][key] }
+      default = module_defaults[key]
+      consumer = merge_layers(repo_values)
+      effective = merge_layers(repo_values + [default].compact)
+
+      # One line per governing toggle, so subkeys under different nested
+      # toggles of the same key are each named against the toggle that disables
+      # them (a top-level toggle renders as 'manage').
+      leaves = unenforced_leaves(consumer, effective, default, nil, [], [])
+      leaves.group_by { |_, _, toggle| toggle }.sort.map do |toggle, entries|
+        grouped = entries.group_by { |dotted, value, _| source_file(files, key, dotted, value) }
+                         .map { |file, subkeys| "#{subkeys.map(&:first).sort.join(', ')} (#{file})" }
+        "'#{key}': effective '#{toggle}' is false, so the module does not enforce " \
+          "resources from these subkeys: #{grouped.sort.join('; ')} — the module " \
+          'may still read some of them as shared inputs; legitimate as declared ' \
+          'state of an externally owned concern; a human judges intent'
       end
-      next if unenforced.empty?
+    end
+  end
 
-      "'#{key}': effective 'manage' is false, so the module does not " \
-        "enforce resources from these subkeys: #{unenforced.join('; ')} — " \
-        'the module may still read some of them as shared inputs; legitimate ' \
-        'as declared state of an externally owned concern; a human judges intent'
+  # Consumer-set leaves ([dotted path, value, toggle]) governed by an effective
+  # manage:false whose value differs from the module default (a restatement of
+  # the default is inert and suppressed). `enclosing` is the nearest manage on
+  # the path, resolved from the effective (consumer-over-module) value, and
+  # `enclosing_path` is where it sits, so the toggle can be named (`manage`,
+  # `sources.manage`, `self_update.manage`).
+  def unenforced_leaves(consumer, effective, default, enclosing, enclosing_path, path)
+    return [] unless consumer.is_a?(Hash)
+
+    if effective.is_a?(Hash) && effective.key?('manage')
+      enclosing = effective['manage']
+      enclosing_path = path
+    end
+    consumer.flat_map do |key, value|
+      next [] if key == 'manage'
+
+      child_path = path + [key]
+      eff_child = effective.is_a?(Hash) ? effective[key] : nil
+      def_child = default.is_a?(Hash) ? default[key] : nil
+      if value.is_a?(Hash)
+        unenforced_leaves(value, eff_child, def_child, enclosing, enclosing_path, child_path)
+      elsif enclosing == false && value != def_child
+        [[child_path.join('.'), value, (enclosing_path + ['manage']).join('.')]]
+      else
+        []
+      end
     end
   end
 
+  # The highest-priority data file supplying this leaf: the merged value hides
+  # which layer won under first-found, so re-derive it for the advisory.
+  def source_file(files, key, dotted_path, value)
+    keys = dotted_path.split('.')
+    files.find { |f| dig_path(parsed_data[f][key], keys) == value } || files.first
+  end
+
+  def dig_path(node, keys)
+    keys.reduce(node) { |n, k| n.is_a?(Hash) ? n[k] : nil }
+  end
+
   # Hash-valued class::param keys present in the given files.
   def candidate_keys(files)
     files.flat_map do |file|

spec/fixtures/data_check/README.md

@@ -13,3 +13,5 @@ because the hierarchy resolves them via `%{facts.networking.hostname}`.
 | `advisory` | Struct subkeys set under `manage: false` in the common layer | The non-failing advisory fires |
 | `advisory_on` | The same, with a higher-priority node layer setting `manage: true` | Hierarchy priority suppresses the advisory |
 | `demo_modules/demo` | A minimal module whose class has a struct parameter with a `manage` subkey | The surface the advisory fixtures resolve against, kept module-independent |
+| `nested_default_sources` | `packages.sources.*` subkeys set while `sources.manage` stays at the module default | Module-default toggle resolution, nested paths, and restated-default suppression |
+| `nested_self_update` | `standalone.self_update.*` subkeys set with `standalone.manage: true` but the loop toggle at its module default | A nested toggle governs its own subkeys independently of the enclosing `manage` |

spec/fixtures/data_check/nested_default_sources/data/common.yaml

@@ -0,0 +1,3 @@
+---
+# No packages.sources.manage is set anywhere in this repository: the toggle
+# resolves from the deployed module's own data layer (its default, false).

spec/fixtures/data_check/nested_default_sources/data/nodes/ci-runner.yaml

@@ -0,0 +1,11 @@
+---
+# packages.sources.manage stays at the module default (false), so its subkeys
+# are not enforced. The docker location is a mirror that differs from the
+# default and is flagged; the gitlab-runner location merely restates the
+# module default and is inert, so the advisory names only the docker mirror.
+rootless_gitlab_runner::packages:
+  sources:
+    docker:
+      location: 'https://mirror.example.org/docker'
+    gitlab_runner:
+      location: 'https://packages.gitlab.com/runner/gitlab-runner/ubuntu'

spec/fixtures/data_check/nested_default_sources/hiera.yaml

@@ -0,0 +1,13 @@
+---
+# Fixture hierarchy for the nested module-default advisory: node over common.
+version: 5
+
+defaults:
+  datadir: data
+  data_hash: yaml_data
+
+hierarchy:
+  - name: 'Per-node'
+    path: 'nodes/%{facts.networking.hostname}.yaml'
+  - name: 'Common defaults'
+    path: 'common.yaml'

spec/fixtures/data_check/nested_self_update/data/common.yaml

@@ -0,0 +1,3 @@
+---
+# self_update.manage is never set here: the nested loop toggle resolves from
+# the deployed module's own data layer (its default, false).

spec/fixtures/data_check/nested_self_update/data/nodes/ci-runner.yaml

@@ -0,0 +1,10 @@
+---
+# The standalone topology is managed, but the nested self-update loop toggle
+# stays at the module default (false): the loop's subkeys are not enforced.
+# apply_interval differs from the default and is flagged; apply_timeout
+# restates the default and is inert, so the advisory names only apply_interval.
+rootless_gitlab_runner::standalone:
+  manage: true
+  self_update:
+    apply_interval: '2min'
+    apply_timeout: '15min'

spec/fixtures/data_check/nested_self_update/hiera.yaml

@@ -0,0 +1,13 @@
+---
+# Fixture hierarchy for the nested self-update advisory: node over common.
+version: 5
+
+defaults:
+  datadir: data
+  data_hash: yaml_data
+
+hierarchy:
+  - name: 'Per-node'
+    path: 'nodes/%{facts.networking.hostname}.yaml'
+  - name: 'Common defaults'
+    path: 'common.yaml'

spec/unit/check_hiera_data_spec.rb

@@ -85,4 +85,22 @@ def run_check(fixture_dir, modulepath, data_dir: File.join(fixture_dir, 'data'))
       expect(output).not_to match(/advisory \(non-failing\)/)
     end
   end
+
+  context 'against the real module surface, resolving nested and module-default toggles' do
+    it 'flags a nested subkey whose toggle sits at the module default, sparing a restated default' do
+      output, status = run_check(File.join(CHECK_FIXTURES, 'nested_default_sources'), REAL_MODULE_PATH)
+      expect(status.exitstatus).to eq(0)
+      expect(output).to match(%r{advisory \(non-failing\): 'rootless_gitlab_runner::packages': effective 'sources\.manage' is false.*sources\.docker\.location .*nodes/ci-runner\.yaml})
+      # The gitlab-runner location merely restates the module default: inert, not flagged.
+      expect(output).not_to match(/sources\.gitlab_runner\.location/)
+    end
+
+    it 'judges a nested loop toggle at the module default independently of the enclosing manage' do
+      output, status = run_check(File.join(CHECK_FIXTURES, 'nested_self_update'), REAL_MODULE_PATH)
+      expect(status.exitstatus).to eq(0)
+      expect(output).to match(%r{advisory \(non-failing\): 'rootless_gitlab_runner::standalone': effective 'self_update\.manage' is false.*self_update\.apply_interval .*nodes/ci-runner\.yaml})
+      # apply_timeout restates the module default; standalone.manage is true but does not lift the inner toggle.
+      expect(output).not_to match(/apply_timeout/)
+    end
+  end
 end