(CAT-1731) improve handling of pinned nodes

jonathannewman commented 2 months ago

Within the Puppet Enterprise UI console the concept of pinned nodes allows more complex rules to be expressed while including pinned nodes.

Pinned nodes are formed by having a top level "or" clause, with expressions in the form

['=', 'name', 'hostname']

Prior to this change, the code would not properly maintain pinned nodes, nor correctly handle merging expressions like:

['and',
  ['~',
    ['fact', 'pe_server_version'],
    '.+']]

with

['or', ['~',
    ['trusted', 'extensions', '1.3.6.1.4.1.34380.1.1.9812'],
    '^puppet/']]

Incorrectly combining them into an and clause when logically they should be an "or" clause.

With this change, pinned nodes are separated our from the other rules and then recombined later (if they are present) with a top-level "or" clause.

Test are added to demonstrate the behaviors.

Summary

Provide a detailed description of all the changes present in this pull request.

Additional Context

Add any additional context about the problem here.

[x] Root cause and the steps to reproduce. (If applicable)
[x] Thought process behind the implementation.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

[x] 🟢 Spec tests.
[ ] 🟢 Acceptance tests.
[ ] Manually verified. (For example puppet apply)

jonathannewman commented 2 months ago

@timidri I believe this is correct and complete. I put it up against your branch as I think we would like to merge the whole thing as one package?

jonathannewman commented 2 months ago

node_groups
  input validation
    is expected to run node_groups("", "", [], "", "extra") and raise an ArgumentError with the message matching /Function accepts a single String/i
    is expected to run node_groups([]) and raise an ArgumentError with the message matching /Function accepts a single String/i
  without an argument
    is expected to run node_groups() and return {"All Nodes"=>{"classes"=>{}, "environment"=>"production", "environment_trumps"=>false, "id"=>"00000000-0000-4000-8000-000000000000", "name"=>"All Nodes", "parent"=>"00000000-0000-4000-8000-000000000000", "rule"=>["and", ["~", "name", ".*"]], "variables"=>{}}, "Production environment"=>{"classes"=>{}, "environment"=>"production", "environment_trumps"=>false, "id"=>"7233f964-951e-4a7f-88ea-72676ed3104d", "name"=>"Production environment", "parent"=>"00000000-0000-4000-8000-000000000000", "rule"=>["and", ["~", "name", ".*"]], "variables"=>{}}}
  with 1 String argument
    is expected to run node_groups("All Nodes") and return {"All Nodes"=>{"classes"=>{}, "environment"=>"production", "environment_trumps"=>false, "id"=>"00000000-0000-4000-8000-000000000000", "name"=>"All Nodes", "parent"=>"00000000-0000-4000-8000-000000000000", "rule"=>["and", ["~", "name", ".*"]], "variables"=>{}}}

Puppet::Type::Node_group::ProviderHttps
  #instances
WARN: Unresolved or ambiguous specs during Gem::Specification.reset:
      rake (~> 13.0)
      Available/installed versions of this gem:
      - 13.2.1
      - 13.1.0
      - 13.0.6
WARN: Clearing out unresolved specs. Try 'gem cleanup <gem>'
Please report a bug if this causes problems.
    returns each node group
  .create
    creates the node group
  .destroy
    deletes the node group
  .flush
    updates the node group

Puppet::Type::Node_group::ProviderHttps
  .add_nulls
    if we change one value from true to false in a hash of multiple keys
      is expected to return a hash with a false value
    if we set a new key to true
      is expected to return a hash with a true value
    if we set a new key to false
      is expected to return a hash with a false value
    if we set a new key to zero
      is expected to return a hash with a zero
    if we set a new key to a string
      is expected to return a hash with a true value
    if we set a new key to a non-zero number
      is expected to return a hash the same non-zero number
    if we change a value from true to false
      is expected to return a hash with a false value
    if we change a value from nil to false
      is expected to return a hash with a false value
    if we change a value from 0 to false
      is expected to return a hash with a false value
    if we change a value from a string to false
      is expected to return a hash with a false value
    if we change a value from false to true
      is expected to return a hash with a true value
    if we change a value from nil to true
      is expected to return a hash with a true value
    if we change a value from true to true
      is expected to return a hash with a true value
    if we change a value from one string to another string
      is expected to return a hash with a true value

Puppet::Type::Node_group
  allows agent-specified environment
  allows environment name with an underscore
  allows environment name with a number
  allows environment name without an underscore
  allows environment name 'agent-specified'
  does not allow environment name with a dash
  allows name with symbols, numbers, and whitespace
  accepts a description parameter
  purge_behavior
    group_with_rule
      replaces by default
      merges when purge behaviour set to :none
      replaces rule when purge behaviour set to :all
      updates rule when purge behaviour set to :rule
    group with pinned nodes
      matches rule exactly by default
      merges and rule correctly when purge behaviour set to :none
      merges or rule correctly when purge behaviour set to :none
      merges pinned node correctly when purge behaviour set to :none
      replaces rule when purge behaviour set to :all
      replaces rule when purge behaviour set to :rule
    group with top-level and clause
      replaces rule exactly by default
start
end
      merges and rule correctly when purge behaviour set to :none
      merges or rule correctly when purge behaviour set to :none
      merges pinned node correctly when purge behaviour set to :none
      replaces rule when purge behaviour set to :all
      replaces rule when purge behaviour set to :rule
    resource hash
      matches classes and data exactly by default
      merges in classes and data when set to :none
      merges in classes and match data exactly when set to :data
      merges in data and match classes exactly when set to :classes
  .insync? for data, classes
    is insync when `is` and `should` are identical
    is insync when `is` and `should` are identical but have different ordering
    is not insync when `is` is only a subset of `should`

Finished in 1.2 seconds (files took 8.8 seconds to load)
53 examples, 0 failures

timidri commented 2 months ago

@timidri I believe this is correct and complete. I put it up against your branch as I think we would like to merge the whole thing as one package?

@jonathannewman Yes, thank you, that seems the easiest way forward to me. The logic is looking good to me.

jonathannewman commented 2 months ago

Going to merge into Dimitri's branch and open a PR from that branch.

puppetlabs / puppetlabs-node_manager