Fix accidental Mattermost enable on master + generic auto-vivification regression spec

What does this MR do?

Ports the master-side fix for the accidental Mattermost auto-enable bug from !9650 (merged) (which had only landed on 18-11-stable), and adds a generic regression spec so future deprecation-check helpers can't reintroduce the same class of bug.

Gitlab::Deprecations.remove_mattermost was reading config with plain []:

config = existing_config['mattermost']
return [] unless config.is_a?(Hash)
return [] unless config['enable'] == true

existing_config is Gitlab['node'].normal, a Chef::Node::VividMash, where reading a missing key creates it. Since package::config does Gitlab[:node] = node, that object is the actual live attributes of the node being converged — the read silently vivified mattermost['enable'] => {}, which is truthy and could win against the real false default, turning Mattermost on for users who never configured it.

Commit 1 — cherry-pick from !9650 (merged)

Cherry-picks eb4afadb4605547bd65788f39f0b396e99ebb622 ("Do not enable Mattermost by accident") as-is, preserving authorship. Switches both reads in remove_mattermost to dig, matching every other helper in deprecations.rb.

Commit 2 — generic regression spec

Adds a new describe block in deprecations_spec.rb that:

  • Snapshots node.normal before/after running Gitlab::Deprecations.check_config (both :removal and :deprecation) on an empty Chef::Node, and asserts it is byte-for-byte unchanged.
  • Walks every config_keys path declared in Gitlab::Deprecations.list, plus the hardcoded paths used by additional_deprecations helpers (mattermost, registry notifications, git_data_dirs, gitaly bundled-binaries/bin_path), asserting none of them get vivified.

This generalizes past the single mattermost case and will catch the same mistake in any future or existing deprecation-check helper, without needing to update the test when new deprecations are added.

I verified both new tests actually catch bugs (not just pass trivially) by running them against the unfixed remove_mattermost first — both failed with the exact vivification signature (mattermost['enable'] => {}) before the fix, and pass after.

Commit 3 - use key? check to avoid dig

Follow up item per Introduce the vivix fix for consitency on the m... (#10037 - closed) to remove the need for dib and subsequent Rubocop exclustion.

Closes Introduce the vivix fix for consitency on the m... (#10037 - closed)

Verification

bundle exec rspec spec/chef/cookbooks/package/libraries/deprecations_spec.rb
# 32 examples, 0 failures

bundle exec rubocop files/gitlab-cookbooks/package/libraries/deprecations.rb spec/chef/cookbooks/package/libraries/deprecations_spec.rb
# 2 files inspected, no offenses detected

Checklist

See Definition of done.

For anything in this list which will not be completed, please provide a reason in the MR discussion.

Required

  • MR title and description are up to date, accurate, and descriptive.
  • MR targeting the appropriate branch.
  • Latest Merge Result pipeline is green.
  • When ready for review, MR is labeled workflowready for review per the Distribution MR workflow.
  • The UBT version and corresponding checksum hash have been updated and referenced in the merge request if applicable. - n/a
    • UBT EE pipeline (Trigger:ee-package-ubt) is green

For GitLab team members

If you don't have access to this, the reviewer should trigger these jobs for you during the review process.

  • The manual Trigger:ee-package jobs have a green pipeline running against latest commit.
  • If config/software or config/patches directories are changed, make sure the build-package-on-all-os job within the Trigger:ee-package downstream pipeline succeeded.
  • If you are changing anything SSL related, then the Trigger:package:fips manual job within the Trigger:ee-package downstream pipeline must succeed. -n/a
  • If CI configuration is changed, the branch must be pushed to dev.gitlab.org to confirm regular branch builds aren't broken. - n/a

Expected (please provide an explanation if not completing)

  • Test plan indicating conditions for success has been posted and passes.
  • Documentation created/updated. -n/a
  • Tests added.
  • Integration tests added to GitLab QA. -n/a
  • Equivalent MR/issue for the GitLab Chart opened. - n/a
Edited by Nailia Iskhakova

Merge request reports

Loading
Loading