Allow custom insync? checking for resources

[DavidS]

Use Case

For some use-cases - like specifying upper/lower bounds on values or set memberships ("for instance, I have a use-case where I want things to be treated as in-sync as long as the array specified in Puppet is a equal to or a subset of the array in the response from the API") - the Resource API type schema is not expressive enough. This leads to nasty workarounds, like munging up the desired values in canonicalize.

The current restriction is so that anyone dealing with raw resource data doesn't need to run the provider code to understand whether something is insync or not.

Describe the Solution You Would Like

???

Describe Alternatives You've Considered

• Add a comparison_operator key (with values like eq, min, max) to the attribute definition.
  See ASN.1/LDAP schemas for an example of data-defined comparisons.
• Add a feature that allows the provider to have a compare(a,b) method.
  This would violate one of the design principles of the Resource API, namely no provider code being needed to run for operations not related to the actual management of the resource.
• pass in a marker object similar to Deferred that encapsulates the comparison:
  age => LessThan($upper_limit)
  that is sent to the agent and evaluated there.

Additional Context

Originally filed as https://tickets.puppetlabs.com/browse/MODULES-9574.

[seanmil]

Not having a way of defining an insync? / compare method has been causing me a lot of headache recently and - so far - I have resorted to the nasty munging in canonicalize method. I recalled seeing a ticket on this so I came here to determine if it was something I could implement and the method I was considering implementing seems not to be in favor (adding a compare type function).

Has any more thought been given to how else this might be implemented? A comparison_operator key might be a good start for "slightly complicated" comparison cases, but I still feel that having something more flexible is ultimately required.

[DavidS]

@seanmil With the restrictions specified in the description, I'm currently having no good idea on how to implement that (which usually means I'll have to compromise on the reqs eventually). Providing a list of specific usecases would definitely help either way (finding better ideas and putting pressure on the requirements).

[seanmil]

The current use case I have in mind is something like the user type's inclusive vs. minimum option which affects how the group state is determined.

This is not (as far as I can tell) possible to currently achieve, even via abuse of canonicalize, but would be simple to achieve if a compare() method were available.

It might be possible with a sophisticated enough ASN.1/LDAP style filter/query syntax, but I'm not sure that complexity of designing and building something like that outweighs having a compare() function available and, even with some type of LDAP-style data-defined comparison I suspect there would be limits on what you could express, which would still leave some folks needing a compare() function.

Here is what I propose: add support for a compare(is, should) method that returns a true if a resource needs to be synchronized, or a false if they do not. Enable it with a feature flag, e.g. custom_compare.

This doesn't preclude adding a set of simple attribute comparisons later (e.g. min, max, subset, etc.) that might be useful for folks to use in the simple case, but allows all cases to be handled immediately.

[seanmil]

Alternatively, maybe name it something like needs_sync? to provide contextual clues as to the return value meaning (otherwise I know I'll have to be looking it up each time).

[DavidS]

Thinking about a resource-level equal?(context, is, should) method, this would need to override Type#insync?(is), which is really weirdly set up. It should be possible.

[DavidS]

Frédéric Lespez writes in trying to implement a upper boundary to a property.

[DavidS]

@seanmil , I've added another idea to the description: adding a bunch of comparator objects that get packed into the catalog like Deferred and get evaluated agent-side. This still would require some hairy coding in the Resource API to make it happen, but might be a great user experience.

What do you think?

[FredL69]

Just my two cents:

About parameters (attributes declared with this behavior):
Currently, the provider code can only use them in two places:

• the canonicalize() method (but only with the possibility to do munging)
• the set() method. But this method is only called if some properties need to be changed. No parameter can trigger a call to the set() method.

So for now, this restricts the parameters to influence how the provider behaves only :

• to munge resource during canonicalize(). See https://github.com/magicmemories/puppet-auth0 for an example
• to change the way the real state is modified. For example: a 'force' parameter to authorize the provider to overwrite existing data.

In the use cases we are discussing, the problem is how to extend the way we decide that the 'is' and 'should' states are the same. Currently, only equality of properties (ignoring parameters) is supported.

We need a way:

• either to specify how to combine properties and parameters to decide if the 'is' and 'should' states are the same. The 2nd alternative in the feature description falls down here.
• or to specify other properties requirements (like an upper limit) to be taken into account when evaluating the 'is' and 'should' states.The 1st and 3rd alternatives fall down here.

Concerning the first alternative (comparison_operator key to the attribute definition)
For example, to set an upper limit, we will need to do something like that into the definition of the 'age' attribute: comparison_operator: 'Max(:max_age)' where 'max_age' is a parameter.
If we do something simple, we will be limited again very quickly. If we do something powerful, I agree with Sean that it will add a lot of complexity.
What is more, we will end up with the need to write code inside the type definition and in another language (ASN.1 for example). The distinction between definition and logic will be blurred. Not desirable and not very readable in my opinion.

Concerning the third alternative (pass in a marker object similar to Deferred)
In a manifest, we will add a age => LessThan($upper_limit) to resource declaration. 'age', in this use case, is a read-only property. Won't it cause problems to set a read_only property in a manifest ?

In the other use case (inclusive vs. minimum for group state), how will we use it ?
By adding a group => Minimum($array) or a group => Inclusive($array) ?

This way of doing things we also have an impact Puppet wide: we will be able to apply this kind of markers on any resource handled by Puppet, won't we ?
In the end, you will need to know how the resource is implemented (Puppet Resource API vs. the classic hard way) to use it at its best. This will introduce incoherence: not desirable in my opinion.

Concerning the second alternative (Add a feature that allows the provider to have a compare(a,b) method) :
I am in favor of this one like Sean.
You can make your 'compare' method as complex as you need.
Your resource logic is confined inside the provider code.
You doesn't need to learn a new language.
Your resource usage is coherent with the usage of native Puppet resources.

Does it really violate one of the design principles of the Resource API, namely : no provider code being needed to run for operations not related to the actual management of the resource.
Is comparing the 'is' and 'should' states not related to the management of the resource ?

I think it is related: The provider is in charge of reporting the real state of resources (through the get method). It does mean that the only provider is able to describe the real state of a resource. Doesn't it also mean that only the provider is able to tell if the 'is' and 'should' states are the same ?

The other advantage is that it will extend parameter usage in a way that is coherent with how other Puppet Resources (like User) are used.

So, for me, adding a method called equal?(context, is, should) returning true if the 'is' and 'should' states are the same and false otherwise, triggering a call of the set() method would be a welcome addition.

Hope it helps.

[MAXxATTAXx]

Adding a use case:

I'm working on a provider that implements calls to an API.
Some of the resources I have defined have Array properties that need to be compared for changes.
Some of these arrays are Array[Struct]
I need a way where I can compare the status of those properties in an unordered matter.
Currently I'm getting false positive that a change needs to be executed because of out_of_sync status.

E.g.:

from: [
  { 'name' => '1.3.8.187' },
  { 'name' => '1.3.16.200' },
  { 'name' => '1.3.16.207' },
  { 'name' => '1.3.16.210' },
  { 'name' => '1.3.54.19' },
  { 'name' => '1.3.54.25' }
]
to: [
  { 'name' => '1.3.8.187' },
  { 'name' => '1.3.16.200' },
  { 'name' => '1.3.16.207' },
  { 'name' => '1.3.16.210' },
  { 'name' => '1.3.54.19' },
  { 'name' => '1.3.54.25' }
]

[DavidS]

I need a way where I can compare the status of those properties in an unordered matter.

You can do this today by sorting the arrays from your API when geting the values and sorting arrays from the manifest in canonicalize. This is transparent to the user (and under strict mode, the rsapi will check that the two implementations match up).

[alexjfisher]

Adding my use-case...

I'm trying to write provider(s) for rundeck via its API. The type I'm currently working on is for managing SCM configs.

rundeck_scm_config { 'SomeProject/export/git-export': # 3 project, integration, and 'type' namevars.
  ensure => present,
  enabled => true,
  config = {
    'somefieldicareabout' => 'value1',
    'someotherconfig'     => 'value2',
    # The API supports many other fields, but I don't care about them, or are happy with defaults, so I don't want to specify them here.
  },
}

The allowed config is based on the version of rundeck, and its scm plugin versions etc. For that reason, the type has a hash config attribute instead of individual properties.

What I'd like, is to be able to specify just some of the keys in config, and have only those managed. ie If config in my manifest is a subnet of config returned by the provider, I'd like to treat it as insync.

[michaeltlombardi]

custom_insync? for the Resource API

A strict property-by-property equivalence comparison is sometimes too strict; being able to write a custom insync? handler to a Resource API compliant provider adds flexibility to address the following (non-exclusive) use cases:

1. If a type needs to support a minimum or inclusive array
2. If a type needs to support a minimum or maximum number/version/etc
3. If a type needs to support a hash with only some keys being cared about by Puppet
4. If a type needs to support a case insensitive string

Requirements:

1. Fit into the Resource API's design language
2. A Provider can choose to trigger a state change even if no attributes have visibly changed against the desired state in the manifest
3. A Provider can choose to avoid a state change even if attributes have visibly changed against the desired state in the manifest
4. Implementation does not consume more resources for providers that do not use the feature
5. Do not effect existing Resource API types and providers
6. Resources taking advantage of this feature must opt-in
7. Do not raise complexity of type definitions
8. Enable custom comparison checking to be as complex or simple as needed for each provider
9. Do not change the behaviour of get or canonicalize, only whether set should be called
10. Allow for custom logging inside the provider
11. Be able to suppress Puppet's default change logging if no changes are actually needed
12. Only trigger downstream resources via notify/subscribe if and only if a change was executed
13. Have a way to use custom comparison for only a subset of properties
14. Keep the existing default comparison behaviour for resources that do not opt-in to the new feature
15. Custom compare method reports only minimal pass/fail information back to the Resource API

Design Considerations

1. Option should be a feature flag for the type
2. If the feature flag is specified, have a custom comparison method that checks the post-canonicalized should against the is

Implementation

1. Add support for a custom_insync feature flag on the type in the type_definition
   • This does not raise the complexity of type definitions, it re-uses existing methods for altering provider behavior
   • This does not effect existing Resource API resources
   • This makes the custom comparison behaviour opt-in
2. If custom_insync feature flag is present, call my_provider.insync? in insync? on each property (except for ensure) and return the result if implemented, otherwise rely on the existing property insync? behavior
   • This implementation does not change the behavior of get or canonicalize, only whether a property is considered insync by Puppet and therefore whether set should be called; thereby it also does not change existing behaviour for notify/subscribe.
   • Keeping all of the logic for custom insync validation in the provider:
     1. Does not raise complexity of type definitions
     2. Enables implementation to be as complex or simple as needed per provider
     3. Keeps implementation complexity proportional to added flexibility
3. Document custom_insync feature flag and give example of insync? implementation in docs

Minimum and Inclusive Arrays & Hashes

Placing the logic in the custom comparison method instead of in the type definition could allow for resource to have a property be treated as either minimum or inclusive depending on the manifest definition. For example:

# Resource Definitions in test.pp
resource_with_arrays { 'Minimum Array':
  the_array => ['a', 'b'],
}
resource_with_arrays { 'Inclusive Array':
  the_array => ['a', 'b'],
  # An existing pattern in Puppet resources is to specify a force flag when you want
  # to ensure that precise comparisons of arrays are used, instead of a minimal array
  force     => true,
}

# Custom comparison in the provider
def insync?(context, name, attribute_name, is_hash, should_hash)
  case attribute_name
  when :the_array
    if should_hash[:force]
      context.notice("Checking an order independent array")
      return is_hash[attribute_name].sort == should_hash[attribute_name].sort
    else
      context.notice("Checking a subset match array")
      return (is_hash[attribute_name] & should_hash[attribute_name]).sort == should_hash[attribute_name].sort
    end
  # No further statements are necessary; if the custom insync? in the provider
  # does not return true or false, the comparison for the attribute will default
  # to the normal insync check in Puppet::Property
  end
end

This behavior could also be implemented to always be minimal or inclusive, depending on the provider, and could be used for hashes as well as arrays (though hash comparisons can be trickier if there's nested hashes).

Minimum and Maximum Versions/Numbers

Placing the logic in the custom comparison method could allow for resource to properties for minimum/maximum/exact versions. For example:

# Resource Definitions in test.pp
# If version is not specified, it defaults to ''
resource_with_version_properties { 'Exact Version':
  version => '1.2.3',
}
resource_with_version_properties { 'Minimum Version':
  minimum_version => '1.2.3',
}
resource_with_version_properties { 'Maximum Version':
  maximum_version => '1.2.3',
}
resource_with_version_properties { 'Custom Version String':
  maximum_version => '>= 1.2.3',
}

# Custom comparison in the provider for multiple version properties
# For particularly complex insync checks, it can be useful to break
# the check into a separate method for testing and readability.
def version_insync?(context, is_hash, should_hash)
  # When version is not specified, it is expected you're passing either/both
  # the minimum & maximum version bounds; if neither is specified, then we
  # don't care about version for this declaration and don't trigger a change.
  if should_hash[:version] == ''
    if should_hash[:minimum_version] || should_hash[:maximum_version]
      context.notice('Checking a min/max version')
      meets_minimum_expectation = Gem::Version.new(is_hash[:version]) >= Gem::Version.new(should_hash[:minimum_version]) unless should_hash[:minimum_version].nil?
      meets_maximum_expectation = Gem::Version.new(is_hash[:version]) <= Gem::Version.new(should_hash[:maximum_version]) unless should_hash[:maximum_version].nil?
      if should_hash[:minimum_version] && should_hash[:maximum_version]
        return meets_minimum_expectation && meets_maximum_expectation
      elsif should_hash[:minimum_version]
        return meets_minimum_expectation
      else
        return meets_maximum_expectation
      end
    else
      return true
    end
  # If a version is specified and matches a dependency string, use
  # Gem::Dependency to determine if it's insync. Otherwise, default
  # to normal insync comparison.
  elsif !should_hash[:version].nil? && should_hash[:version].match?(%r{^\D\s+})
    context.notice("Checking a custom version string")
    return Gem::Dependency.new('', should_hash[:version]).match?('', is_hash[:version])
  end
end
# Custom comparison in the provider called for each property
def insync?(context, name, attribute_name, is_hash, should_hash)
  case attribute_name
  when :version
    return version_insync?(context, is_hash, should_hash)
  end
end

Case Insensitive String Comparisons

Placing the logic in the custom comparison method could allow for resource to have a property for which does not care about the casing of a string. For example:

# Resource Definition in test.pp
resource_with_strings { 'example':
  case_sensitive_string   => 'FooBar',
  case_insensitive_string => 'foobar',
}

# Custom comparison in the provider
def insync?(context, name, attribute_name, is_hash, should_hash)
    case attribute_name
    when :case_insensitive_string
      return is_hash[attribute_name].downcase == should_hash[attribute_name].downcase
  end

DSC Specific Notes

1. We can have insync? call Invoke-DscResource -Method Test ... if use_dsc_test_method is true in the manifest definition for a resource and otherwise do the normal comparison checks.
   • For DSC in particular, we'd actually ignore the is and run Invoke-DscResource -Action Test with the should hash, but that's an outlier implementation
2. This should allow users to choose at manifest time whether they want to do the full comparison check which respects the strict interpretation of the DSC Resource's API surface or not. Some DSC Resources will likely always want to use the Test method for the same reasons that other Puppet resources do (ie a strict comparison of should and is states is inaccurate for whether the resource is actually in the desired state).
3. Use of this flag should be documented in the auto-generated readme for the modules and added to the concept documentation for the PowerShell module.

Summary

This implementation should be scoped to new/updated resources which specify a feature flag and implement a custom method in their provider - while not effecting current behavior or simple providers. Furthermore, it should enable as much complexity of comparison as a type requires without being overly opinionated or adding too much maintenance burden to the Resource API itself; only types which utilize the feature should have an increased maintenance cost (commensurate with the benefit of being able to accurately manage the resource).

I do not have strong opinions on the name of the flag or method, I used custom_insync for the flag and insync? for the method because that idiom is familiar to me. I believe custom_comparison and compare would be equally valid (and possibly more natural English as compared to insync).

Are there other thoughts or feedback on this? Implementing a solution is becoming a priority for the DSC work as numerous critical resources cannot be effectively managed with a strict property value comparison check. That being said, whatever is implemented here must work for more generalized use cases as mentioned up-thread and in the requirements.

[michaeltlombardi]

Folks watching the thread: I went back with some input from @DavidS and updated the comment to be a bit more clear, separate out requirements, considerations, and implementation more usefully, and provide some examples for how insync? in the provider might look for a couple different use cases.

I'm doing some investigation for the next couple days and will update that comment again a few times and drop a new comment downthread when it's "finalized."

[michaeltlombardi]

\o It's been a while, but for those of you still watching the thread, we have a functional draft PR up which implements custom insync for Resource API types that specify the custom_insync flag.

If you have any thoughts or feedback, I'd love to hear it, but the way things worked out, from the perspective of a developer using the Resource API to define a resource that needs custom insync you would need to:

1. Specify the custom_insync flag on your type
2. Specify an insync? method in your provider which takes the parameters:
   • context - so you can do custom logging if needed
   • name - the rsapi title of the resource)
   • attribute_name - the attribute currently being evaluated by Puppet in the overall state validation
   • is_hash - the current state of the resource, including all values, not just the one under test
   • should_hash - the canonicalized should hash of the resource, including all values so that parameters etc can be accessed in the custom insync logic
3. Use a case statement on attribute_name and add your custom handling for the property whose insync behavior you want to change.
   • In each of these cases, you will want to return true or false if the insync status can be known, and otherwise continue execution (barring an error).
   • If the insync? method in the provider returns nil for that attribute - in other words, if not custom logic is triggered which is able to determine the insync status of the attribute - then the run will fall back on the default comparison behavior in Puppet::Property - this means you don't need to add handling for every attribute, only the ones whose behavior you want to change.

Our hope is that by requiring minimal extra handling in the provider's insync? method you can get a flexible but not onerous way to override default insync comparisons. Note that insync? is only called for properties, not parameters or read only properties, and once for each property.

[FredL69]

@michaeltlombardi Thanks for your PR.

I am currently testing the new custom_insync feature.
So far, it is really good and I easily managed to implement an upper boundary to a property.

I found a little bug:
If insync? returns false with a string to change the default report message, the custom message is not displayed when you run puppet agent with --noop (the default message is displayed). The custom message is correctly displayed when running the agent without --noop.