Issue 122 - Puppet ENC via Hiera

run.py

Index: run.py
===================================================================
--- a/run.py
+++ b/run.py
@@ -3,6 +3,7 @@
 
 import sys
 import os
+import os.path

[Felix Dahlke, 2015/03/03 16:43:32]

Not necessary, we're already importing os. Looks like we're actually only using
os.path here so we could get rid of the `import os`, however, that's an
unrelated change that should better go to a separate (Noissue) commit.

[mathias, 2015/03/04 12:32:37]

Agreed, the explicit os.path import will be removed in RC2.

 import re
 import subprocess
 import getopt
@@ -41,78 +42,42 @@
 
   return user, hosts, ignore_errors, args
 
-def readMonitoringConfig():
-  # Use Puppet's parser to convert monitoringserver.pp into YAML
-  manifest = os.path.join(os.path.dirname(__file__), 'manifests', 'monitoringserver.pp')
-  parseScript = '''
-    require 'puppet'
-    require 'puppet/parser'
-    parser = Puppet::Parser::Parser.new(Puppet[:environment])
-    Puppet.settings[:ignoreimport] = true
-    parser.file = ARGV[0]
-    print ZAML.dump(parser.parse)
-  '''
-  data, dummy = subprocess.Popen(['ruby', '', manifest],
-                  stdin=subprocess.PIPE,
-                  stdout=subprocess.PIPE).communicate(parseScript)
-
-  # See http://stackoverflow.com/q/8357650/785541 on parsing Puppet's YAML
-  yaml.add_multi_constructor(u"!ruby/object:", lambda loader, suffix, node: loader.construct_yaml_map(node))
-  yaml.add_constructor(u"!ruby/sym", lambda loader, node: loader.construct_yaml_str(node))
-  return yaml.load(data)
 
 def getValidHosts():
-  def processNode(node, hosts=None, groups=None):
-    if hosts == None:
-      hosts = set()
-    if groups == None:
-      groups = {}
-
-    if 'context' in node and 'code' in node['context']:
-      node = node['context']['code']
-
-    if node.get('type', None) == 'nagios_hostgroup':
-      data = node['instances']['children'][0]
-      title = data['title']['value']
-      members = filter(lambda c: c['param'] == 'members', data['parameters']['children'])[0]['value']['value']
-      members = re.split(r'\s*,\s*', members)
-      groups[title] = members
-    elif node.get('type', None) == 'nagios_host':
-      data = node['instances']['children'][0]
-      title = data['title']['value']
-      hosts.add(title)
-
-    for child in node['children']:
-      processNode(child, hosts, groups)
-    return hosts, groups
-
-  monitoringConfig = readMonitoringConfig()
-  if not monitoringConfig:
-    print >>sys.stderr, "Failed to parse monitoring configuration"
-    return [[], []]
-  # Extract hosts and groups from monitoring config
-  return processNode(monitoringConfig)
+  dirname = os.path.dirname(sys.argv[0])
+  path_name = os.path.join(dirname, "hiera", "private", "hosts.yaml")
+  with open(path_name, 'rb') as handle:
+    config = yaml.load(handle)
+  servers = config.get('servers', {})
+  return servers  
 
 def resolveHostList(hosts):
-  validHosts, validGroups = getValidHosts()
-  if not validHosts:
-    print >>sys.stderr, "Warning: No valid hosts found, not validating"
-    return hosts
 
+  host_names = set(str(item) for item in hosts)

[Wladimir Palant, 2015/03/03 20:00:19]

Given that hosts is a list of string, this seems equivalent to |host_names =
set(hosts)|. However, I don't see why we should convert it to a set at all.

[mathias, 2015/03/04 12:32:37]

"Given that hosts is a list of string" <- this is an assumption; inside the
function there is no guarantee for that one. You know already that I don't like
dropping good practices without a good reason.

But I do agree that there is no need for host_names to be a set, since
uniqueness is ensured in the result set already. That also means it can remain a
generator.

[Felix Dahlke, 2015/03/04 14:27:53]

Nit: The parentheses are now not necessary anymore.

[Wladimir Palant, 2015/03/04 15:14:11]

This isn't a library function you wrote here, it is a function used by this
script alone. And the parameter passed in to the function is a list of strings,
that's not an assumption. If you like you can document the parameter type.

Note that you are now turning a list into a tuple, that makes no sense at all
(see below on the difference between lists and tuples).

[mathias, 2015/03/04 18:13:51]

Done.

[mathias, 2015/03/04 18:13:51]

Done.

   result = set()
-  for param in hosts:
-    if param in validGroups:
-      for host in validGroups[param]:
-        if host == '*':
-          result = result | validHosts
-        else:
-          result.add(host)
-    elif param in validHosts:
-      result.add(param)
-    elif '%s.adblockplus.org' % param in validHosts:
-      result.add('%s.adblockplus.org' % param)
-    else:
-      print >>sys.stderr, 'Warning: failed to recognize host or group %s' %param
+
+  try:
+    valid_hosts = getValidHosts()

[Felix Dahlke, 2015/03/03 16:43:32]

Host validation doesn't make _that_ much sense anymore now that the development
infrastructure doesn't have to mirror the production infrastructure anymore.
That'd only work if we work with the real infrastructure-private rather than
private-stub locally, which is probably not a good idea.

How about relying on us sticking to our naming scheme consistently and just
checking that there's a role for that server name? For example, we'd check that
there's a "downloadserver" role when the user wants to run something on a server
called "download\d".

[Wladimir Palant, 2015/03/03 20:00:19]

Indeed, that's something we discussed previously - so I'm rather surprised to
see parsing of the local hosts.yaml here. The idea we discussed was rather
getting the list of hosts from the puppet master. So we would update the
repositories on the puppet master first, ask it for its hosts.yaml (something
along the lines of "ssh puppetmaster cat
/etc/puppet/infrastructure/hiera/private/hosts.yaml" should do) and do host
validation based on that list.

[mathias, 2015/03/04 12:32:37]

This version does two jobs at the same time; validation and resolving (groups
and fqdns). It's the latter that makes it a bit complex, the validation "layer"
is rather thin. Under these circumstances, I take any validation support I can
get.

In addition, your suggestion sounds kind of kludgy. Why would we limit our
system and options with non-required constraints? Even if there where a
guarantee that we always run servers with single, distinct purpose only (and
there is none), why would we base our system's conditionals on something so
debatable and thus volatile like a naming convention?


On 2015/03/03 20:00:19, Wladimir Palant wrote:
I do remember that discussion, but IMHO the "idea we discussed" was just the
base where we started. We also discussed the idea to use run.py and kick.py in
all environments, not only production. This would be more clean, make the
scripts useful for anybody (and actually testable!), but of course require a
hosts.yaml to be the source of information. The plan here was to introduce
another parameter, e.g. -m (for master) or -a (authority), so that fetching the
info from another host is optional. This is still to be done, but for now this
should be enough though - especially since we are trying to keep the changes at
minimum.

(One could even avoid having the private module cloned locally by just invoking
both run.py and kick.py from the Puppet master in the meantime. This would
require us to enable AgentForwarding, though.)

[Felix Dahlke, 2015/03/04 14:27:53]

For the record: I actually prefer Wladimir's suggestion to mine.

But to be practical: What IMO really gets in the way is any kind of validation
that requires us to have the real private module checked out locally. Wladimir's
suggestion sounds like something we want to do eventually, but if we want to
keep things simple for now, I suggest we just get rid of the validation for now.

[Wladimir Palant, 2015/03/04 15:14:11]

Actually, changing the current validation code to download remote data should be
a fairly trivial change. The only problem is, using and testing it can only be
done once the puppet master has a hosts.yaml file checked out. So IMHO it is
better to leave the validation as it is for now and then change it to use remote
data as soon as the changes are pushed. While this change is implemented, not
having a private/hosts.yaml file is enough to disable validation.

[mathias, 2015/03/04 18:13:51]

@Felix
As I wrote before, this is actually intended and just requires a later extension
of run.py to integrate fetching from another resource. The code in this
patch-set is already compatible with that, and the validation is just a minor
part compared to the resolving.
@Wladimir
Yep, I think we're on the same page now. And yes the extension of run.py will
follow as one of the first improvements after this merge - I do not intend on
leaving that unfinished for too long.

[Felix Dahlke, 2015/03/09 23:14:51]

Didn't realise this, but yeah, the catch clause still calls result.update. All
good then, I can live with a warning.

+  except Warning as error:
+    print >>sys.stderr, 'Warning: failed to determine valid hosts:', error
+    result.update(host_names)
+  else:
+    for name in host_names:
+      chunk = tuple(
+          item[1].get('dns', item[0]) for item in valid_hosts.items()

[Wladimir Palant, 2015/03/03 20:00:19]

for (key, value) in valid_hosts.iteritems() please - destructure the tuple
immediately, otherwise this is unreadable.

Also, why tuple() if this really a list?

  chunk = [
    value.get('dns', key) for (key, value) in valid_hosts.iteritems()
    ...
  ]

[mathias, 2015/03/04 12:32:37]

Done. 

To answer your question: There is no need for this to be a list, a tuple has all
the features required here. I see that this may sound like over-thinking, but
types are an aspect developers preferring statically typed languages always have
in mind anyway, so this was more the result of an automatism.

[Wladimir Palant, 2015/03/04 15:14:11]

The difference between a list and a tuple isn't the functionality they offer -
they are semantically different. From the documentation:

[mathias, 2015/03/04 18:13:51]

This has already been changed in the current patch-set, aligned with your
previous comment.

+
+          if name == item[0]
+          or name == '*'
+          or name == item[1].get('dns', None)
+          or name in item[1]['ip']

[mathias, 2015/02/26 18:41:23]

While resolving IPs would be a nice feature, this line here is not enough. It
does not handle different phenotypes of the same IPv6 address, for example, and
also does not reflect that we actually allow a single IP to be given as a string
in `modules/base/manifests/init.pp`. Since the effort to integrate these facets
is way too high for now, I'll remove that line in the next patch-set (RC2).

[mathias, 2015/03/04 12:32:37]

Done.

+          or name in item[1].get('groups', ())
+      )
+
+      if len(chunk) == 0:
+        print >>sys.stderr, 'Warning: failed to recognize host or group', name
+      else:
+        result.update(chunk)
+
   return result
 
 def runCommand(user, host, command, ignore_errors=False):