apt_key type/provider #212

daenney · 2014-01-17T08:24:05Z

Started working on a type and provider for apt_key to replace the current apt::key.

It's the first time I'm trying this. Basically read through the Types and Providers book yesterday and then came up with this. I think I got all the params right and that there are no properties but, not sure. I'm also not entirely convinced about my autorequire but I think it makes sense.

I purposefully decided to not use the same attribute names as apt::key as I find key_id, key_server etc fairly redundant on a type called *_key.

igalic · 2014-01-17T08:34:17Z

Where exactly does apt_key improve upon, other than be written in Ruby? Does it require less checks? etc…

daenney · 2014-01-17T08:45:18Z

The idea is that this would be less troublesome than apt::key because we can stop calling execs and wgets to get the work done. Validation is easier and the commands in a provider is less tricky to get right because escaping and a few other things are taken care of.

Parsing the output of the different commands is also easier and less tricky to get right in Ruby instead of in an exec.

There's also a few nice other things that @apenney pointed out, 'native' types can be used by mcollective contrary to defined types which is nice for inventory.

I'm also going to add a few read-only attributes that show when the key was added and when it will expire which would be useful for inventory too.

igalic · 2014-01-17T11:47:46Z

That sounds pretty cool. Thanks for pointing those out. I think I need to re-read the Types & Provider book and get hacking…

daenney · 2014-01-17T13:19:03Z

There's one feature of the current apt::key that I have no idea how to emulate with a native type/provider. It's the thing where it does magic so that multiple apt::sources can have the same key without getting duplicate resources all over the place.

igalic · 2014-01-17T14:03:13Z

They way they do it now is by declaring a unique title each time:

    apt::key { "Add key: ${key} from Apt::Source ${title}":

Another way to do it, would be to create a virtual resource here and then collect it somewhere:

  if $key {
    @apt_key { $key:
      source => $source,
    }
  }

# elsewhere
Apt_key <| |>

daenney · 2014-01-17T15:16:26Z

I need some feedback on how I did things:

type: Does the namevar make sense?
provider: In the self.instances I set both :name and :id to the key ID, is this okay?
type and provider: A bunch of things like when the key was added is now marked as a parameter and is set in the property hash. But, because it's a parameter it doesn't show up when you do puppet resource apt_key. I would like those to show as it's useful. Should I make those into properties instead and only implement the getter or is there a better way? It would also be useful if people could filter based on those in for example mcollective allowing them to ask for all expired apt_key's on a host etc.

nanliu · 2014-01-17T15:34:22Z

The type should be mostly properties not parameters, otherwise the provider would not be able to update the config after initial creation.

daenney · 2014-01-17T15:36:11Z

@nanliu Can you elaborate a bit? Things like added etc can never be updated by the provider, they can only be discovered by apt-key list for example, there's no way (and there shouldn't be a way) to set that yourself.

igalic · 2014-01-17T15:41:59Z

Yes, but you're implementing it as parameter, rather than as property

daenney · 2014-01-17T16:08:38Z

I think something in the book is tripping me up:

Figuring out if an attribute should be a property is one of the most important design decisions for a resource type. In general, you can decide if an attribute should be a property by asking the following questions:

Can I discover the state of this attribute?

Can I update the state of this attribute?

If the answer to both of those questions is yes, then that attribute should be implemented as a property. In general, if the answer to one or both of these questions is no, then the characteristic should not be a property.

And later on:

Parameters supply additional information to providers, which is used to manage its properties. In contrast with properties, parameters are not discovered from the system and cannot be created or updated.

The first paragraph seems to suggest these things shouldn't be properties and the second seems to suggest they shouldn't be parameters either.

Thinking about it it would make sense to have most of the things I now have as a param actually be a property and simply not implement the setter on the provider.

nanliu · 2014-01-17T16:38:59Z

Can I discover the state of this attribute?
Can I update the state of this attribute?

If the answer to both of those questions is yes, then that attribute should be implemented as a property. In general, if the answer to one or both of these questions is no, then the characteristic should not be a property.

That is true. In this case the question is whether there's any value fetching settings that are implicit and cannot be configured? You aren't really using puppet as a tool to fetch the values, but to configure them.

For example: newparam(:expired), since we can't change the key expiration, is it necessary to fetch the value? (or if the key expiration doesn't match, does that mean we need to replace they current apt-key?)

daenney · 2014-01-17T17:14:11Z

That is true. In this case the question is whether there's any value fetching settings that are implicit and cannot be configured? You aren't really using puppet as a tool to fetch the values, but to configure them.

I think it really depends on the case. There is a certain value to showing information especially coupled with mcollective or the puppet resource type command. Knowing that a key exists on the system is useful but only up to a certain point. I'd be very interested from an inventory point of view to know if there are expired keys on the system or older/weaker keys.

It's for the same reason I think it's also very valuable to know when the key was added to the system and since apt-key list is already providing that information I don't see a good reason not to show it.

Consider the difference between this:

puppet resource apt_key '46925553'
apt_key { '46925553':
  ensure => 'present',
}

And this:

puppet resource apt_key '46925553'
apt_key { '46925553':
  ensure  => 'present',
  added   => '2012-04-27',
  expired => 'false',
  expiry  => '2020-04-25',
  size    => '4096',
  type    => 'rsa',
}

Personally, even though I can't change most of these options I do find them very useful to know.

apenney · 2014-01-17T17:32:33Z

lib/puppet/provider/apt_key/apt_key.rb

+
+  def self.instances
+    keys_list = apt_key('list')
+    key_array = keys_list.split("\n").collect do |line|


So fussy a nitpick but we might as well just do keys_array = apt_key('list').split, saving a variable from a single line of life

apenney · 2014-01-17T18:07:43Z

I think based on the comments I lean on the side of having the extra information - but that's because I routinely use puppet resource x as a kind of inventory system for figuring out what's on a server. I've relied on mco to do this in the past network wide and I think it's an awesome thing you effectively get for free.

Overall I don't have any major comments, the provider/type looks fine to me. I wish we didn't have to regex the hell out of the world in providers but.. it is what it is. :)

On the name/id being the same, I think that's reasonable considering it's really the only unique thing we have to name them off!

apenney · 2014-01-17T21:50:04Z

lib/puppet/provider/apt_key/apt_key.rb

+    @property_hash[:ensure] == :present
+  end
+
+  def added


If you strip out the getters and put the mk_resource stuff back in before you do the def x=, it'll all magically work

apenney · 2014-01-17T22:12:44Z

type tests look good, the provider ones are always trickier :)

apenney · 2014-01-21T17:02:07Z

As mentioned on IRC the only real "what?" that I see currently is the mixing of the temp file and the command pushing in creates. You're pushing things into an array to build up the command but there's that side effect of building the file at the same time and it just feels messy to me. I feel like splitting that somehow to be more obviously two steps (build file, push file) would be preferable.

apenney · 2014-01-24T20:12:49Z

Something wierd is going on with travis, failing to find stuff on 1.8. :/

daenney · 2014-01-25T11:50:25Z

As far as I can see it's specifically ~~1.8.7 with~~ Puppet 3.0.0. Dominic commented on IRC they had similar issues with Foreman and just removed 3.0.0 from the test matrix.

apenney · 2014-01-25T14:23:52Z

I would be down with just stealing https://github.com/puppetlabs/puppetlabs-apache/blob/master/.travis.yml and slapping our secure bit back into it. We talked about just testing the latest 3.x anyway, seems silly to have to test 3.0, 3.1, 3.2, etc.

igalic · 2014-01-25T21:18:35Z

Well, we definitely need 3.2. I don't know how exactly the current support for 2.7 (the previous PE) looks like, but depending on that, we may need to drag that along some more..

janschumann · 2014-02-04T13:19:42Z

I just opened #217. Could someone please look at that issue? Is it worth fixing it as a part of this change?

daenney · 2014-02-16T15:55:26Z

@apenney Right, I think my implementation of type and provider are now complete and in my limited testing all the use cases seem to work, including the 0x issue that @janschumann had run into with the current implementation.

Could you have a last look at it before I write the beaker specs?

apenney · 2014-02-16T16:52:42Z

+1 from me, can't think of anything else critical at this point. Bet the beaker tests will throw up some surprises however! :)

igalic · 2014-02-16T17:49:10Z

Other than squashing it and fixing the commit message.

daenney · 2014-02-18T12:58:01Z

@igalic @apenney: Do your worst!

igalic · 2014-02-18T15:27:51Z

lib/puppet/provider/apt_key/apt_key.rb

+      command.push('add', source_to_file(resource[:source]))
+    # In case we really screwed up, better safe than sorry.
+    else
+      fail('an unexpected condition occurred in the apt_key provider')


fail('apt_key: an unexpected condition occurred when trying to create a key')

igalic · 2014-02-18T15:46:53Z

General comments: HULK SQUASH! - Also, remove the (WIP) please.

igalic · 2014-02-18T15:48:53Z

Perhaps, instead of squash, you could open a new PR to keep the history for future generations ;)

This commits introduces: * The apt_key type; * The apt_key provider; * Unit tests for the type; * Beaker/acceptance tests for the type/provider. The idea behind apt_key is that apt::key will simply become a wrapper that uses apt_key. Being a native type/provider apt_key is a lot less error prone than the current exec behaviour of apt::key and adds a few nice bonuses like inventory capabilities for mcollective users.

3.0 is just broken in tests, something is wrong with the module loading rendering tests completely useless. Start testing on Puppet 3.3 and 3.4.

apt_key type/provider

igalic · 2014-02-19T15:35:01Z

\o/

We welcome our new ruby overlords.

apenney reviewed Jan 17, 2014
View reviewed changes

daenney mentioned this pull request Feb 4, 2014

Apt::Key upper cases the key #217

Closed

igalic reviewed Feb 18, 2014
View reviewed changes

Daniele Sluijters added 2 commits February 18, 2014 22:51

Travis: Remove 3.0, add newer 3.x releases.

9ade658

3.0 is just broken in tests, something is wrong with the module loading rendering tests completely useless. Start testing on Puppet 3.3 and 3.4.

apenney pushed a commit that referenced this pull request Feb 19, 2014

Merge pull request #212 from daenney/apt-key

c9443f9

apt_key type/provider

apenney merged commit c9443f9 into puppetlabs:master Feb 19, 2014

daenney deleted the apt-key branch February 19, 2014 15:12

Ramesh7 added feature bugfix labels Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apt_key type/provider #212

apt_key type/provider #212

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

nanliu commented Jan 17, 2014

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

nanliu commented Jan 17, 2014

daenney commented Jan 17, 2014

apenney Jan 17, 2014

apenney commented Jan 17, 2014

apenney Jan 17, 2014

apenney commented Jan 17, 2014

apenney commented Jan 21, 2014

apenney commented Jan 24, 2014

daenney commented Jan 25, 2014

apenney commented Jan 25, 2014

igalic commented Jan 25, 2014

janschumann commented Feb 4, 2014

daenney commented Feb 16, 2014

apenney commented Feb 16, 2014

igalic commented Feb 16, 2014

daenney commented Feb 18, 2014

igalic Feb 18, 2014

igalic commented Feb 18, 2014

igalic commented Feb 18, 2014

igalic commented Feb 19, 2014

apt_key type/provider #212

apt_key type/provider #212

Conversation

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

nanliu commented Jan 17, 2014

daenney commented Jan 17, 2014

igalic commented Jan 17, 2014

daenney commented Jan 17, 2014

nanliu commented Jan 17, 2014

daenney commented Jan 17, 2014

apenney Jan 17, 2014

Choose a reason for hiding this comment

apenney commented Jan 17, 2014

apenney Jan 17, 2014

Choose a reason for hiding this comment

apenney commented Jan 17, 2014

apenney commented Jan 21, 2014

apenney commented Jan 24, 2014

daenney commented Jan 25, 2014

apenney commented Jan 25, 2014

igalic commented Jan 25, 2014

janschumann commented Feb 4, 2014

daenney commented Feb 16, 2014

apenney commented Feb 16, 2014

igalic commented Feb 16, 2014

daenney commented Feb 18, 2014

igalic Feb 18, 2014

Choose a reason for hiding this comment

igalic commented Feb 18, 2014

igalic commented Feb 18, 2014

igalic commented Feb 19, 2014