Skip to content

(PUP-2086) Allow purging of groups with filters#2484

Closed
ChrisPortman wants to merge 1 commit intopuppetlabs:masterfrom
ChrisPortman:resources_unless_gid
Closed

(PUP-2086) Allow purging of groups with filters#2484
ChrisPortman wants to merge 1 commit intopuppetlabs:masterfrom
ChrisPortman:resources_unless_gid

Conversation

@ChrisPortman
Copy link

Implementing unless_gid and unless_system_group as parameters of the resources type to allow filtering when purging groups

@ChrisPortman
Copy link
Author

Added tests

Implementing unless_gid and unless_system_group as parameters of the resources type to allow filtering when purging groups
The behaviour when a combination of only_gid, unless_gid and unless_system_group is as follows:

   only_gid && unless_gid raises error:  Theses to parameters conflict and it is an error to use them both
   only_gid && unless_system_group:  If the only_gid option contains a GID that is lower than the GID specified by unless_system_group (default 500) then an error is raised because it is a direct conflict. If there is no overlap, then no error is raised nor is the unless_system_group evaluated as the system GIDs are outside the only_gid selection anyway ant thus would not be purged.
   unless_gid && unless_system_group: These are complimentary as they both exclude GIDs from being purged.  Unless_gid is first evaluated and then if that doesnt disqualify the GID from purging, unless_system_group is evaluated, if that still does not disqualify the GID from purging, the GID is purged.
@ffrank
Copy link
Contributor

ffrank commented Apr 6, 2014

To be honest, I'm not a fan of all those purge filters. Are there use cases that make unless_gid or only_gid unavoidable?

The general idea of purging is to make puppet get rid of unmanaged resources. To keep resources from getting purged, you generally just want to start managing them. Note that you need not even manage the ensure property for a given resource, so just enumerating them suffices for protection:

group { [ "dont", "purge", "us" ]: }

If we do go this route after all, I don't see why they are conflicting. As a user, I would expect unless_gid and the system_gid state to take precedence over only_gid, so that this would be a thing:

resources { "group": purge => true, only_gid => '[1000..2000]', unless_gid => 1150 }

I.e., purge in a given range, excempting the mentioned IDs. This would need adequate documentation, of course.

One final remark: It's fine for all of these new parameters to be merged from one topic branch, but they should be added in the form of at least one distinct commit each.

@ChrisPortman
Copy link
Author

Hi @ffrank,

Thanks for the feedback.

Basically its about being able to nominate a range of UIDs/GIDs that should be under complete control by Puppet.

There are situations where installing packages will pollute the users and groups outside of the system groups range. But as the package creates the users, and potentially the puppet module only tracks the package and not the users, groups or whatever else it creates, the users and groups are not in the catalogue and then get purged.

It also does allow portions of the system to by manually maintained. In our environment which is still a bit old school with regards to ITIL and all that, whereby our team is responsible for the OS SOE (including user accounts provisioning) other teams are responsible to different applications and those teams may not be ready to puppetise their applications. This will allow those teams to manually maintain their application users without fear of being purged, so long as they stay outside of the nominated range.

@ChrisPortman
Copy link
Author

Also, yes I agree regarding the unless_uid having precedence over only_uid. I can make that change.

@ffrank
Copy link
Contributor

ffrank commented Apr 6, 2014

Hmm, I see. Is it a sound conclusion that this feature will enhance Puppet's ability to work with systems that are partially managed?

In all honesty, I'm not sure that such features will gain the appropriate political traction to get merged. (Some of us are even questioning the final decision in favor of unless_uid by now.)

Is it feasible to implement this functionality in a module instead?

@ChrisPortman
Copy link
Author

Hi @ffrank,

Yes thats the crux of it.  Basically my use case is to manage a range of uids and gids (in absolute terms) for the adding and removal of accounts for real people. Without having to bother with application/system users.

With users, its not too bad, the unless_system_user excludes about 99% of "other" users, but occasionly a package will add a user with a wierd uid and I think debian puts the nobody user at uid 65536.

Groups has no similar attribute so I had to get round it orriginally by creating a custom fact that listed all the "system" users and the create resources for them. But this is problematic because I had to basically ensure_resource() them and make sure it occured as close to the end of the parse order as possible.

But all that seems hackish in the absense of being able to just define that "this range(s) is completely under puppet control".

Thanks
Chris

@ChrisPortman
Copy link
Author

H @ffrank

Just in regards to:
resources { "group": purge => true, only_gid => '[1000..2000]', unless_gid => 1150 }

Unless_gid currently has the behaviour of purging everything that's not specified. So in the above example, given a possible UID range of 1 - 65536 (or whatever), the current intentions behind only_gid and unless_gid is, 1000 - 2000 get purged as per only_gid, and 1 - 1149,1151 - 65536 get purged as per unless_gid. 1150 is in conflict between the 2.

If the users intention is to purge anything not managed by Puppet from 1000 - 2000, but not 1150 regardless of whether puppet declares the resource or not, then possibly better to just have the user go only_uid => 1000..1149,1151..2000

Regarding the use of A..Z as a value to be translated into a Ruby range, which is probably a lot better than using the range(A,Z) from stdlib and having ruby create an array of possibly 1000's of elements. Should that not be defined as part of the Puppet DSL rather than a bespoke syntax accommodated specifically by the resources resource type?

Thanks,
Chris

@ffrank
Copy link
Contributor

ffrank commented Apr 14, 2014

Hi Chris,

so as we discussed on IRC: It would be great if you could create a Puppet module with one or more new types akin to resources that would implement this enhanced purging functionality. I'd recommend to keep the feature set simple initially.

You would need to copy parts of the resources type code to your new type(s) (e.g., the generate method). This is undesirable, so it would be helpful to move all such shared functionality into a base provider, that both resources and your new type(s) can inherit. I'd be grateful to a pull request to that effect, whereas this on would be closed in favor of a future module release by you.

Alternatively, there may be a module already in which this would fit, you can just request them to merge a change to the effect of including the new purging type(s). Ideally, this would be preceded by the proposed change to the type code layout, but that could also be done afterwards.

If that is all right with you, we'd close this PR, along with #2487. The fixes to unless_uid are likely fit for merging, although I'd hope that this functionality would move to your module in time, too.

Best regards,
Felix

@adrienthebo
Copy link
Contributor

Per the discussions on GH-2484 and GH-2487, we've decided that we're not going to go ahead with these pull requests. Adding parameters for filtering specific types of resources to purge isn't a tenable long term solution, and not something we want to have in core.

An alternate approach that we've discussed is to have real providers for the resources type, and push specific functionality into the providers themselves. This will make it easier to purge multiple types of resources while keeping things modular. In addition this code might be best suited to be implemented as a module. There definitely is a need for this functionality but we can't justify merging it into core, but putting it into a module will still make that functionality available for users that need this. If there's enough interest shown to justify bringing this into core we can revisit this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants