Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ParsedFile provider work with composite namevars #9130

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Oct 23, 2023

Previously it was assumed that the name was the key. This changes the implementation to instead match on the key attributes. This allows it to work with composite namevars. While there was previously code for multiple target support, this makes it possible to use the same key in multiple files.

It also gets rid of some code that wasn't needed.

To achieve this, it also needs to use resource titles as identifiers in preloading

When using composite namevars for resources then resource.name always nil. Prior to this it was used as a key in a hash when prefetching resources. By using the title it should work, since that's guaranteed to be set and unique.

This can break providers where the name is different from the title.

Right now there are no tests, but I tested it locally with puppetlabs-postgresql.

When using composite namevars for resources then `resource.name` always
`nil`. Prior to this it was used as a key in a hash when prefetching
resources. By using the title it should work, since that's guaranteed to
be set and unique.

This can break providers where the name is different from the title.
Previously it was assumed that the name was the key. This changes the
implementation to instead match on the key attributes. This allows it to
work with composite namevars. While there was previously code for
multiple target support, this makes it possible to use the same key in
multiple files.

It also gets rid of some code that wasn't needed.
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@@ -449,7 +440,7 @@ def flush
# If the target isn't set, then this is our first modification, so
# mark it for flushing.
unless @property_hash[:target]
@property_hash[:target] = @resource.should(:target) || self.class.default_target
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may be because I started to use target as a parameter instead of a property.

@@ -429,17 +422,15 @@ def create
end
end
mark_target_modified
(@resource.class.name.to_s + "_created").intern
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK create is supposed to not return anything.

end

def destroy
# We use the method here so it marks the target as modified.
self.ensure = :absent
(@resource.class.name.to_s + "_deleted").intern
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK destroy it not supposed to return anything

@@ -465,13 +456,7 @@ def initialize(record)
# The 'record' could be a resource or a record, depending on how the provider
# is initialized. If we got an empty property hash (probably because the resource
# is just being initialized), then we want to set up some defaults.
@property_hash = self.class.record?(resource[:name]) || {:record_type => self.class.name, :ensure => :absent} if @property_hash.empty?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing it's not needed to set the properties when a Puppet::Resource is passed in, but it is needed to set the record_type to skip things. Perhaps skip_record? isn't needed at all anymore and can be dropped. Then this line can also be dropped.

end

attr_accessor :property_hash
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why it needs to be an accessor. I think it can only break things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in principal, but it's referenced by subclasses of ParsedFile like https://github.com/puppetlabs/puppetlabs-sshkeys_core/blob/fbd0c6dc9ca7314a567b6873cb66e2adf52af30b/lib/puppet/provider/sshkey/parsed.rb#L32C8-L32C21 and is therefore part of the parsed file API.

@SimonHoenscheid
Copy link

@ekohl I have just a question out of curiosity: I tried to patch the postgresql_conf provider to work with multiple files and was told that the cron type, which is also based on ParsedFile is able to do this. But I never found the code which does it. Is there a working patch in the cron type/provider?

@ekohl
Copy link
Contributor Author

ekohl commented Oct 23, 2023

I did not think about that, but just to be explicit: you're talking about https://github.com/puppetlabs/puppetlabs-cron_core? I see it does override some methods, like resource_for_record, match and targets. It also implements prefetch_hook. Those do appear to be the broken methods and it may be possible to apply the same in puppetlabs-postgresql.

It would also be an interesting test to see how much of that can be dropped with this patch.

@SimonHoenscheid
Copy link

Yes, I was referring to https://github.com/puppetlabs/puppetlabs-cron_core

@@ -361,7 +361,7 @@ def resources_by_provider(type_name, provider_name)
@catalog.vertices.each do |resource|
if resource.class.attrclass(:provider)
prov = resource.provider && resource.provider.class.name
@resources_by_provider[resource.type][prov][resource.name] = resource
@resources_by_provider[resource.type][prov][resource.title] = resource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since resource title doesn't always equal name, I'm pretty sure this will be break something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like puppet originally prefetched using title (see commit c35d07b)

                prefetchers[provider.class][resource.title] = resource

Later there was a bug reported in the mount provider redmine #2013 And puppet was changed to use name in 9577d3a as "ParsedFile types seem to be the main one that suffers from (mismatch between type and title)"

-                prefetchers[provider.class][resource.title] = resource
+                prefetchers[provider.class][resource.name] = resource

I'm not convinced that changing the transaction to use name was the right fix (as opposed to a bug in the mount provider), but that ship sailed awhile ago. We'll need a different way for parsed file providers to opt-into the new behavior.

return nil unless @records
@records.find { |r| r[:name] == name }
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called in

@property_hash = self.class.record?(resource[:name]) || {:record_type => self.class.name, :ensure => :absent} if @property_hash.empty?

@joshcooper joshcooper added the enhancement New feature or request label Nov 15, 2023
@joshcooper
Copy link
Contributor

I just ran into this issue while investigating composite name var support for host_core to enable managing both ipv4 and v6 entries. I'll take a closer look.

Copy link
Contributor Author

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshcooper This originally came up in puppetlabs-postgresql, but I don't have time to finish this so as with my other PRs you should feel free to take over. I'll happily review.

Comment on lines +384 to 389
resources&.each do |name, resource|
value = resource[:target]
if value
targets << value
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be optimized by relying on Ruby 2.7+

targets += resources.filter_map { |_name, resource| resource[:target] } if resources

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants