Added unless_uid option to User Resource management. #628

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
10 participants
Contributor

barttenbrinke commented Apr 5, 2012

Currently there are only two options for user management: purge => true or purge => false.
Purge true works great if all your apps are deployed via puppet, but when you have a mix of puppet system uids and application uids, you are forced to turn purge off.
This has the undesired side-effect that users that are managed through puppet will never revoked for the servers that have purge disabled.

At our company Devops deploys the servers & frameworks using Puppet so that Devs can take over and deploy applications on their own.
Each of these application runs under its own uid. For this reason we have to run with purge => false, which is something we don't want.

The only option available to alter this behavior is "unless_system_user => true", but this just protects users with a UID < 500 and that is not where I want to have my application UIDs.
In order to fix this properly, I've added an extra option to puppet: unless_uid.

You van specify specific Uids here or even Ranges of UIDS. Uids that match these ranges will not be purged, even though purge => true.

This way Puppet still automatically revokes access to the servers for Devs and Devops that are removed, while allowing Devs to deploy their own application application.

Example:

class users::resources {
  resources { 'user':
  purge              => true,
  unless_system_user => true,
  unless_uid => [10_000..20_000];
  }
}

Unless_uid accepts Integers, Ranges or Arrays with both.
I also added specs for both the old and the new check_user behavior, as the old behaviour was not specced.

Member

daenney commented Apr 5, 2012

+1

olafz commented Apr 6, 2012

+1

Contributor

jeffweiss commented Apr 10, 2012

@barttenbrinke Thank you for the contribution. Before we can merge it, you must sign the Contributor License Agreement, available here: https://projects.puppetlabs.com/contributor_licenses/sign

Contributor

barttenbrinke commented Apr 11, 2012

Done. Username is the same as github.

Contributor

barttenbrinke commented Apr 23, 2012

@jeffweiss When will this be pulled? We are kind of waiting on it :).

@hunner hunner commented on an outdated diff May 15, 2012

lib/puppet/type/resources.rb
+ newparam(:unless_uid) do
+ desc "This keeps specific uids or ranges of uids from being purged when purge is true.
+ Accepts ranges, integers and (mixed) arrays of both."
+
+ munge do |value|
+ case value
+ when /^\d+/
+ [Integer(value)]
+ when Integer
+ [value]
+ when Range
+ [value]
+ when Array
+ value
+ when /^\[\d+/
+ values.split(',').collect{|x| x.include?('..') ? Integer(x.split('..')[0])..Integer(x.split('..')[1]) : Integer(x) }
@hunner

hunner May 15, 2012

Member

s/values/value/

Contributor

barttenbrinke commented May 15, 2012

Fixed, specced & rebased.

Contributor

jeffweiss commented May 15, 2012

@barttenbrinke We've had a lot of internal discussion about your contribution. We haven't forgotten about it. We all agree that there's a clear use case, and that while we may not love the implementation (not a value judgement against you or your code, but rather what the codebase itself enables), we don't necessarily see a better near-term solution. A "better" implementation would require a fair amount of investment to make resource collections first-class puppet entities.

To reiterate, we appreciate the contribution, we see value, we're debating whether we should defer until we have a better collections syntax. We're going to solicit further input from the community.

Contributor

barttenbrinke commented May 15, 2012

Thanks, @hunner has just moved this to a custom_resource.rb, so that we can use it in production, but I think more people have this use case. It changes very little of the existing code, but as the existing code was unspecced and is somewhat ugly I understand the discussion. My patch at least increases spec coverage of the original code (which was 0).

@jeffweiss Did we get more input from the community, do we have a verdict?

Contributor

jeffweiss commented Jul 17, 2012

@kelseyhightower I did a quick recap of the thread. In general, the consensus seemed to be "yeah, I have a couple of philosophical issues with it, but I can't offer anything better." There was also a side track of how awesome a mini-boolean language would be for some of these things.

I suggest taking the crazy action of merging actual, working (though perhaps suboptimal) code instead of waiting indefinitely for code for the perfect solution to magically appear.

Contributor

barttenbrinke commented Aug 10, 2012

Guys, did the the perfect solution to magically appear yet?

CLA Signed by barttenbrinke on 2012-04-10 21:00:00 -0700

Contributor

jeffmccune commented Oct 3, 2013

@barttenbrinke Sorry this has sat so long. I'm actually taking a look at this and trying to get it merged in, but I still have a couple of comments on the code. Could you review them and let me know how you'd like us to proceed?

@jeffmccune jeffmccune commented on the diff Oct 3, 2013

lib/puppet/type/resources.rb
@@ -64,6 +64,28 @@
}
end
+ newparam(:unless_uid) do
+ desc "This keeps specific uids or ranges of uids from being purged when purge is true.
+ Accepts ranges, integers and (mixed) arrays of both."
+
+ munge do |value|
+ case value
+ when /^\d+/
+ [Integer(value)]
+ when Integer
+ [value]
+ when Range
+ [value]
@jeffmccune

jeffmccune Oct 3, 2013

Contributor

Is it ever possible in Puppet to have a value that's a type of class Range? I'm just curious where this case statement came from.

@jeffmccune jeffmccune commented on the diff Oct 3, 2013

lib/puppet/type/resources.rb
@@ -64,6 +64,28 @@
}
end
+ newparam(:unless_uid) do
+ desc "This keeps specific uids or ranges of uids from being purged when purge is true.
+ Accepts ranges, integers and (mixed) arrays of both."
+
+ munge do |value|
+ case value
+ when /^\d+/
+ [Integer(value)]
+ when Integer
+ [value]
+ when Range
+ [value]
+ when Array
+ value
+ when /^\[\d+/
@jeffmccune

jeffmccune Oct 3, 2013

Contributor

Is this parameter expected to take a string that looks like [1,2,3]. If so, that seems really confusing since it's a string as far as the Puppet DSL is concerned, but it's presented just like an Array would be presented in the DSL.

Is there another notation we could use?

@jeffmccune jeffmccune commented on the diff Oct 3, 2013

lib/puppet/type/resources.rb
@@ -64,6 +64,28 @@
}
end
+ newparam(:unless_uid) do
+ desc "This keeps specific uids or ranges of uids from being purged when purge is true.
+ Accepts ranges, integers and (mixed) arrays of both."
@jeffmccune

jeffmccune Oct 3, 2013

Contributor

What are some concrete examples? It looks like unless_uid => '[1,2,3]' is valid, but it's not clear from the description that the [ and ] are part of a special new syntax for these ranges.

@jeffmccune jeffmccune commented on the diff Oct 3, 2013

spec/unit/type/resources_spec.rb
+ end
+
+ it "should purge system users over 500 if unless_system_user => 600" do
+ res = Puppet::Type.type(:resources).new :name => :user, :purge => true, :unless_system_user => 600
+ res.catalog = Puppet::Resource::Catalog.new
+ user_hash = {:name => 'system_user', :uid => 525, :system => true}
+ user = Puppet::Type.type(:user).new(user_hash)
+ user.stubs(:retrieve_resource).returns Puppet::Resource.new("user", user_hash[:name], :parameters => user_hash)
+ res.user_check(user).should be_false
+ end
+ end
+
+ describe "with unless_uid" do
+ describe "with a uid range" do
+ before do
+ @res = Puppet::Type.type(:resources).new :name => :user, :purge => true, :unless_uid => 10_000..20_000
@jeffmccune

jeffmccune Oct 3, 2013

Contributor

In the example you're using 10_000..20_000 which is a ruby instance of class Range. As far as I can tell, the Puppet DSL will never produce a value that's of type Range, so I don't think we should explicitly support this in the parameter values.

@jeffmccune jeffmccune commented on the diff Oct 3, 2013

spec/unit/type/resources_spec.rb
+ user_hash = {:name => 'special_user', :uid => 25_000}
+ user = Puppet::Type.type(:user).new(user_hash)
+ user.stubs(:retrieve_resource).returns Puppet::Resource.new("user", user_hash[:name], :parameters => user_hash)
+ @res.user_check(user).should be_true
+ end
+
+ it "should not purge uids that are in a specified range array" do
+ user_hash = {:name => 'special_user', :uid => 15_000}
+ user = Puppet::Type.type(:user).new(user_hash)
+ user.stubs(:retrieve_resource).returns Puppet::Resource.new("user", user_hash[:name], :parameters => user_hash)
+ @res.user_check(user).should be_false
+ end
+
+ end
+
+ describe "with a uid array" do
@jeffmccune

jeffmccune Oct 3, 2013

Contributor

In the implementation, there's a check against the regular expression /^\[\d+/ This seems like the implementation has some expected behavior for the scenario where the user writes some Puppet DSL like unless_uid => "[0,1]"

I don't see any examples of this behavior... It seems like since the value can be an Array, then we shouldn't implement the special behavior of parsing a string that looks like an array. Could you please remove the /^\[\d+/ case in the implementation or provide some clear examples of why the behavior is necessary and distinct from the Array case?

@jeffmccune jeffmccune commented on the diff Oct 3, 2013

lib/puppet/type/resources.rb
@@ -64,6 +64,28 @@
}
end
+ newparam(:unless_uid) do
+ desc "This keeps specific uids or ranges of uids from being purged when purge is true.
+ Accepts ranges, integers and (mixed) arrays of both."
+
+ munge do |value|
+ case value
+ when /^\d+/
+ [Integer(value)]
+ when Integer
+ [value]
+ when Range
+ [value]
+ when Array
@jeffmccune

jeffmccune Oct 3, 2013

Contributor

Since Puppet doesn't support a Range type, the Array and String cases should be updated to support ranges that are simple strings with a hypen as the range separator. e.g. unless_uid => ['0-99',101].

@jeffmccune jeffmccune commented on the diff Oct 3, 2013

lib/puppet/type/resources.rb
@@ -64,6 +64,28 @@
}
end
+ newparam(:unless_uid) do
+ desc "This keeps specific uids or ranges of uids from being purged when purge is true.
+ Accepts ranges, integers and (mixed) arrays of both."
+
+ munge do |value|
+ case value
+ when /^\d+/
+ [Integer(value)]
@jeffmccune

jeffmccune Oct 3, 2013

Contributor

If the user declares a resource using unless_uid => '10..99' then this case will pass and raise an ArgumentError. This needs to be addressed, please see my comment about parsing ranges below since the Puppet DSL doesn't actually support a range type.

irb(main):008:0> value = '10..99'
=> "10..99"
irb(main):009:0> /^\d+/ === value
=> true
irb(main):010:0> [Integer(value)]
ArgumentError: invalid value for Integer(): "10..99"
    from (irb):10:in `Integer'
    from (irb):10
    from /opt/pvm/versions/ruby/1.9.3-p448/bin/irb:12:in `<main>'
Contributor

jeffmccune commented Oct 3, 2013

@barttenbrinke OK, I think all of my comments on the patch have made it in.

In addition, this just needs a rebase since it's been so long. Sorry again about that, I definitely encourage you to take this back up if you're willing, I'm fairly certain we'll get this merged quickly if we can get these concerns addressed.

There's really only two main concerns:

  1. Puppet doesn't have a Range type in the DSL, so the parameter value shouldn't try and deal with this Ruby class.
  2. The current implementation makes it confusing for the end user in that both unless_uid => "[1,2,3]" and unless_uid => [1,2,3] appear valid. The implementation should not parse a string as if it's an Array, it should simply take an Array and clearly express an example of the expected behavior in the specs.

The patch rebases cleanly with git rebase master, so please rebase since it's been so long.

Thanks for the contribution, and again I'm really sorry this has sat for so long.

-Jeff

Contributor

barttenbrinke commented Oct 4, 2013

Hi jeffmccune,
I honestly forgot all about this as we have a custom mixin that does this, so it is workedaround in our puppet setup.

About your concerns:

  1. It is correct that the current Puppet catalog checking does not support Range types in the DSL, however, Puppet DOES support it, and I've seen other people taking "advantage" of this.
  2. This is a workaround needed because of the Puppet checking system.

So it pretty much boils down to the use of Range. The nicest solution IMHO is to build in Range support into Puppet or think of a different DSL.

Contributor

barttenbrinke commented Oct 4, 2013

Rebased, but somebody forced pushed master :X?!

Contributor

jeffmccune commented Oct 4, 2013

Rebased, but somebody forced pushed master :X?!

I don't think anyone force-pushed, I was able to rebase without a problem...

-Jeff

Contributor

ferventcoder commented Jan 17, 2014

Rebased on a recent master, squashed, and tested specs locally - #2267

Contributor

ferventcoder commented Jan 17, 2014

Merged into master at e9b8f48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment