Bug #10237

An empty array of principals to k5login type does not empty the .k5login.

Added by Steve Traylen 7 months ago. Updated 4 months ago.

Status:Code Insufficient Start date:10/23/2011
Priority:Normal Due date:
Assignee:Daniel Pittman % Done:

0%
Category:RAL
Target version:-
Affected Puppet version:0.24.0 Branch:https://github.com/stschulte/puppet/tree/ticket/2.7.x/10237
Keywords:
Votes: 0

Description

Running 2.6.6 on RHEL6.1 and starting with a non-empty /root/.k5login

# cat /root/.k5login 
me@CERN.CH

The the following configuration

k5login {'/root/.k5login': principals => []}

Results in an unchanged /root/.k5login still containing the principal. I believe in this case the file should be emptied.

In the null string case the file is emptied.

k5login {'/root/.k5login': principals => ''}

however in the this case nothing happens

k5login {'/root/.k5login': }

which is quite possibly correct?

Certainly the zero length array should be empty, it’s important to me since I’m returning the array from a custom function.

Apologies for not confirming with the latest greatest puppet, I’ll set one up shortly for confirming these kind of things are still present if they are.

History

#1
Updated by Stefan Schulte 7 months ago

  • Description updated (diff)

I agree with you, an empty array should remove any principals that might appear in the file.

FWIW this is due to the way insync? is implemented in puppet/property.rb:

172   def insync?(is)
173     self.devfail "#{self.class.name}'s should is not array" unless @should.is_a?(Array)
174 
175     # an empty array is analogous to no should values
176     return true if @should.empty?
177 
178     # Look for a matching value
179     return (is == @should or is == @should.collect { |v| v.to_s }) if match_all?
180 
181     @should.each { |val| return true if is == val or is == val.to_s }
182 
183     # otherwise, return false
184     false
185   end

I can see two possible solutions:

  • interchange line 176 and 179 (seems reasonable to me)
  • use Puppet::Property::List instead of Puppet::Property and don’t depend on array_matching => true at all.

#2
Updated by Stefan Schulte 7 months ago

  • Category set to RAL
  • Assignee set to Stefan Schulte

#3
Updated by Stefan Schulte 7 months ago

  • Affected Puppet version changed from 2.6.6 to 0.24.0
  • Branch set to https://github.com/stschulte/puppet/tree/ticket/2.7.x/10237

I went for the first solution and added some tests. This changes the behaviour of any type that uses array_matching => all.

https://github.com/puppetlabs/puppet/pull/174

#4
Updated by Stefan Schulte 7 months ago

  • Status changed from Unreviewed to In Topic Branch Pending Review

#5
Updated by Jacob Helwig 5 months ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient

I don’t think the change proposed in pull request 174 can/should be done in a point release, since it affects all types that have properties/parameters supporting arrays with a rather large change in behavior. I’m not convinced that it needs a full deprecation cycle (not going in until Telly+1), however.

The change may be fine as-is rebased against master, but we’ll need to be very careful about potential accidental behavior changes introduced as a result of this.

#6
Updated by Stefan Schulte 5 months ago

Hi Jacob, I agree that this a major change in behaviour. But do have any idea how to »deprecate it« in a smart way? Because assigning an empty array to a property is valid right now and will be valid in the future and I can’t tell what the user is expecting when he writes property => [].

The only way that I can think of is to throw a warning every time an empty array is assigned to a property. Like

You've assigned an empty array to principals. The current behaviour is to ignore this
property but that will change in the future: An empty array will cause puppet to
remove any principal it finds. If this is not what you want remove the property from
the resource definition or use principals => undef.

Now the problem is that I don’t know a way to get the desired behaviour (remove all principals) in the current setup. So if someone has principals => [] and wants to remove any principal they

  • have no way to achieve that (at least not that I know of)
  • they have to remove principals => [] in their manifests or they will now get my deprecation message all the time

So how am I going to tell them that they can now switch back to principals => [] once it has the desired behaviour?

#7
Updated by Stefan Schulte 4 months ago

  • Assignee changed from Stefan Schulte to Jacob Helwig

Sorry I addressed a question to you but haven’t set the assignee properly.

The main question is how can I decide if

property => []

was written because the user wants to ignore the property (they should then see a warning) or if the user wants to remove any current value of the property and is using that statment because a) they alaways thought this is the way puppet behaves (like Steve thought) b) they know that puppet will behave that way in the future.

#8
Updated by Jacob Helwig 4 months ago

  • Assignee changed from Jacob Helwig to Daniel Pittman

Assigning to Daniel, since he has taken over my previous position with Puppet Labs.

Also available in: Atom PDF