Plugins should not be able to override core functionality.
|Affected Puppet version:||Branch:|
If you take core functionality like a provider, say puppet/providers/package/apt.rb and copy it to a module libdir, and then have pluginsync on… then anyone consuming that modulepath, regardless of whether they consume this particular module, will have this plugin override the core functionality.
I think this is a problem. It really doesn’t seem like a good idea from a security point of view for plugins to be able to override an equivalent core library.
I understand that some people find this useful for patching functionality with providers. This doesn’t make it right.
They should instead be inheriting from the desired provider and specifying the provider they want.
#4 Updated by donavan m about 3 years ago
Is there a distinction being made between modules/custom/lib/puppet/ and modules/foo/lib/puppet/, or was this obviated with the .25 change? I can see the reasoning for modules/custom/lib/puppet/providers/package/apt.rb always overriding core functionality. Having modules/foo/lib/puppet/providers/package/apt.rb override the core provider seems like a bad plan.
#8 Updated by eric sorenson almost 3 years ago
eric sorenson wrote:
A data point for what it’s worth: the current functionality is highly useful for sites like mine where rolling new packages sitewide is a slow-cycle process. This saved my bacon tonight to roll a hotfix to mount.rb for #6027.
I take it back, this is evil and a Bad Idea[tm].
#10 Updated by eric sorenson almost 3 years ago
- makes upgrading awful
- appears to trigger a bug if you add core-override plugins and then later (perhaps in an attempt to upgrade) try to remove them
Feb 17 03:36:25db005 puppetd: (/File[/var/lib/puppet/lib/puppet/type/mount.rb]/ensure) change from absent to file failed: Could not find any content at puppet://puppet/pl ugins/puppet/type/mount.rb Feb 17 03:36:26 db005 puppetd: Could not retrieve catalog from remote server: Error 400 on SERVER: Failed to parse template puppet-client/puppet.conf.erb: Could not find value for ‘operatingsystem’ at /etc/puppetlabs/puppet-test/modules/puppet-client/manifests/config.pp:5 on node db005.mydomain.com
#14 Updated by Jacob Helwig over 2 years ago
- Status changed from Rejected to Accepted
Nigel Kersten wrote:
I’d actually forgotten I’d filed this a while ago before I joined Puppet Labs.
We’re reversing position on this one given we’d like to be able to pluginsync more and more of the code base.
I’m going to reject it.
This shouldn’t interfere with the plan to pluginsync more and more types and providers, and probably becomes more important to be careful about what things plugins can override as we increase the frequency with which pluginsync is used to deliver things.
It’s also something that we were planning on tackling in the near future on the Open Source Team. Re-opening this, since it’s relatively high on our current backlog, and we still feel that it’s something that should be done as part of the larger plan of cleaning up the autoloader behavior in Puppet.
[Ed (Nigel)] – We all got on the same page in meat space before this update in case it looks like we’re churning without direction.
#17 Updated by Daniel Pittman almost 2 years ago
- Status changed from Needs Decision to Rejected
Ultimately, Patrick and I have looked at this and are rejecting it.
I think that some part of this would be valuable, but it is not clear what the reasonable scope of that would be – and I think it would be much smaller than expected, given we want to move toward a world where more and more functionality can be supplied in modules and, consequently, on a per-environment basis.
That isn’t the heart of the rejection, though. The heart of that is that we cannot stop people futzing with “core” functionality in Ruby. It is literally impossible; any code we load can result in code being injected that overrides core functionality, and without an AI complete analysis phase we can’t tell if a seemingly innocent “type” or “provider” extension is secretly reaching out and overriding something in a core module.
It doesn’t feel worth the bother trying to stop people accidentally breaking things in that regard, at least in a way that can be expressed a as a general “protect the core” mandate. Specific parts of functionality might end up having specific protections, but we will look to that when we hit them, not as part of this general move.