diff --git a/CHANGELOG.md b/CHANGELOG.md index 1895b38..224aa64 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 9eb596e..f6eefd7 100644 --- a/README.md +++ b/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. diff --git a/scripts/check_hiera_data.rb b/scripts/check_hiera_data.rb index 23fac3c..a6f8de9 100755 --- a/scripts/check_hiera_data.rb +++ b/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,6 +229,48 @@ 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 @@ -228,28 +278,69 @@ def advisories 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| diff --git a/spec/fixtures/data_check/README.md b/spec/fixtures/data_check/README.md index c826c3c..f486f1e 100644 --- a/spec/fixtures/data_check/README.md +++ b/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` | diff --git a/spec/fixtures/data_check/nested_default_sources/data/common.yaml b/spec/fixtures/data_check/nested_default_sources/data/common.yaml new file mode 100644 index 0000000..f2d5fe0 --- /dev/null +++ b/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). diff --git a/spec/fixtures/data_check/nested_default_sources/data/nodes/ci-runner.yaml b/spec/fixtures/data_check/nested_default_sources/data/nodes/ci-runner.yaml new file mode 100644 index 0000000..79daed6 --- /dev/null +++ b/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' diff --git a/spec/fixtures/data_check/nested_default_sources/hiera.yaml b/spec/fixtures/data_check/nested_default_sources/hiera.yaml new file mode 100644 index 0000000..0139fc4 --- /dev/null +++ b/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' diff --git a/spec/fixtures/data_check/nested_self_update/data/common.yaml b/spec/fixtures/data_check/nested_self_update/data/common.yaml new file mode 100644 index 0000000..dbbd99d --- /dev/null +++ b/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). diff --git a/spec/fixtures/data_check/nested_self_update/data/nodes/ci-runner.yaml b/spec/fixtures/data_check/nested_self_update/data/nodes/ci-runner.yaml new file mode 100644 index 0000000..d4f32b7 --- /dev/null +++ b/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' diff --git a/spec/fixtures/data_check/nested_self_update/hiera.yaml b/spec/fixtures/data_check/nested_self_update/hiera.yaml new file mode 100644 index 0000000..9cf773d --- /dev/null +++ b/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' diff --git a/spec/unit/check_hiera_data_spec.rb b/spec/unit/check_hiera_data_spec.rb index eeeddd3..c1bc330 100644 --- a/spec/unit/check_hiera_data_spec.rb +++ b/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