Custom Manifests via Hooks

The present implementation of hooks only perform for docker exec, which isn't that flexible. This PR permits the user to greater customize systemd units on the instances

- cleaned up the manifest code, added tests and permit setting a section raw
- added the ability to filter hooks via master and node roles
- updated the documentation to reflect the changes
- cleaned up some of the vetting issues

docs/cluster_spec.md

@@ -196,13 +196,29 @@ Hooks allow the execution of a container before the installation of Kubneretes o
 spec:
   # many sections removed
   hooks:
-  - execContainer:
-      documentation: http://some_url
-      before:
-      - some_service.service
-      requires:
-      - docker.service
+  - before:
+    - some_service.service
+    requires:
+    - docker.service
+    execContainer:
       image: kopeio/nvidia-bootstrap:1.6
+
+  # or a raw systemd unit
+  hooks:
+  - name: iptable-restore.service
+    masterOnly: true|false # only run this on masters
+    nodeOnly: true|false   # only run this on compute nodes
+    before:
+    - kubelet.service
+    manifest: |
+      [Service]
+      EnvironmentFile=/etc/enviroment
+      # do some stuff
+
+  # or disable a systemd unit
+  hooks:
+  - name: update-engine.service
+    disable: true
 ```
 
 Install Ceph

nodeup/pkg/model/hooks.go

@@ -17,14 +17,16 @@ limitations under the License.
 package model
 
 import (
-	"strconv"
+	"errors"
+	"fmt"
 	"strings"
 
-	"github.com/golang/glog"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/systemd"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/nodeup/nodetasks"
+
+	"github.com/golang/glog"
 )
 
 // HookBuilder configures the hooks
@@ -34,37 +36,98 @@ type HookBuilder struct {
 
 var _ fi.ModelBuilder = &HookBuilder{}
 
-func (b *HookBuilder) Build(c *fi.ModelBuilderContext) error {
-	for i := range b.Cluster.Spec.Hooks {
-		hook := &b.Cluster.Spec.Hooks[i]
+// Build is responsible for implementing the cluster hook
+func (h *HookBuilder) Build(c *fi.ModelBuilderContext) error {
+	// iterate the hooks and render the systemd units
+	for i := range h.Cluster.Spec.Hooks {
+		hook := &h.Cluster.Spec.Hooks[i]
 
-		// TODO: Allow (alphanumeric?) names
-		name := strconv.Itoa(i + 1)
+		// filter out on master and node flags if required
+		if (hook.MasterOnly && !h.IsMaster) || (hook.NodeOnly && h.IsMaster) {
+			continue
+		}
 
-		service, err := b.buildSystemdService(name, hook)
+		// are we disabling the service?
+		if hook.Disabled {
+			enabled := false
+			managed := true
+			c.AddTask(&nodetasks.Service{
+				Name:        hook.Name,
+				ManageState: &managed,
+				Enabled:     &enabled,
+				Running:     &enabled,
+			})
+			continue
+		}
+
+		// use the default naming convention - kops-hook-<index>
+		name := fmt.Sprintf("kops-hook-%d", i)
+		if hook.Name != "" {
+			name = hook.Name
+		}
+
+		// generate the systemd service
+		service, err := h.buildSystemdService(name, hook)
 		if err != nil {
 			return err
 		}
-		c.AddTask(service)
+
+		if service != nil {
+			c.AddTask(service)
+		}
 	}
 
 	return nil
 }
 
-func (b *HookBuilder) buildSystemdService(name string, hook *kops.HookSpec) (*nodetasks.Service, error) {
-	// We could give the container a kubeconfig, but we would probably do better to have a real pod / daemonset / job at that point
-	execContainer := hook.ExecContainer
-	if execContainer == nil {
-		glog.Warningf("No ExecContainer found for hook: %v", hook)
+// buildSystemdService is responsible for generating the service
+func (h *HookBuilder) buildSystemdService(name string, hook *kops.HookSpec) (*nodetasks.Service, error) {
+	// perform some basic validation
+	if hook.ExecContainer == nil && hook.Manifest == "" {
+		glog.Warningf("hook: %s has neither a raw unit or exec image configured", name)
 		return nil, nil
 	}
+	if hook.ExecContainer != nil {
+		if err := isValidExecContainerAction(hook.ExecContainer); err != nil {
+			glog.Warningf("invalid hook action, name: %s, error: %v", name, err)
+			return nil, nil
+		}
+	}
+	// build the base unit file
+	unit := &systemd.Manifest{}
+	unit.Set("Unit", "Description", "Kops Hook "+name)
 
-	image := strings.TrimSpace(execContainer.Image)
-	if image == "" {
-		glog.Warningf("No Image found for hook: %v", hook)
-		return nil, nil
+	// add any service dependencies to the unit
+	for _, x := range hook.Requires {
+		unit.Set("Unit", "Requires", x)
+	}
+	for _, x := range hook.Before {
+		unit.Set("Unit", "Before", x)
 	}
 
+	// are we a raw unit file or a docker exec?
+	switch hook.ExecContainer {
+	case nil:
+		unit.SetSection("Service", hook.Manifest)
+	default:
+		if err := h.buildDockerService(unit, hook); err != nil {
+			return nil, err
+		}
+	}
+
+	service := &nodetasks.Service{
+		Name:       name,
+		Definition: s(unit.Render()),
+	}
+
+	service.InitDefaults()
+
+	return service, nil
+}
+
+// buildDockerService is responsible for generating a docker exec unit file
+func (h *HookBuilder) buildDockerService(unit *systemd.Manifest, hook *kops.HookSpec) error {
+	// the docker command line
 	dockerArgs := []string{
 		"/usr/bin/docker",
 		"run",
@@ -75,38 +138,26 @@ func (b *HookBuilder) buildSystemdService(name string, hook *kops.HookSpec) (*no
 		"--privileged",
 		hook.ExecContainer.Image,
 	}
-
-	dockerArgs = append(dockerArgs, execContainer.Command...)
+	dockerArgs = append(dockerArgs, hook.ExecContainer.Command...)
 
 	dockerRunCommand := systemd.EscapeCommand(dockerArgs)
 	dockerPullCommand := systemd.EscapeCommand([]string{"/usr/bin/docker", "pull", hook.ExecContainer.Image})
 
-	manifest := &systemd.Manifest{}
-	manifest.Set("Unit", "Description", "Kops Hook "+name)
-	if hook.Documentation != "" {
-		manifest.Set("Unit", "Documentation", hook.Documentation)
-	}
-	for _, x := range hook.Requires {
-		manifest.Set("Unit", "Requires", x)
-	}
-	for _, x := range hook.Before {
-		manifest.Set("Unit", "Before", x)
-	}
+	unit.Set("Service", "ExecStartPre", dockerPullCommand)
+	unit.Set("Service", "ExecStart", dockerRunCommand)
+	unit.Set("Service", "Type", "oneshot")
+	unit.Set("Install", "WantedBy", "multi-user.target")
 
-	manifest.Set("Service", "ExecStartPre", dockerPullCommand)
-	manifest.Set("Service", "ExecStart", dockerRunCommand)
-	manifest.Set("Service", "Type", "oneshot")
-	manifest.Set("Install", "WantedBy", "multi-user.target")
-
-	manifestString := manifest.Render()
-	glog.V(8).Infof("Built systemd manifest %q\n%s", name, manifestString)
-
-	service := &nodetasks.Service{
-		Name:       "kops-hook-" + name + ".service",
-		Definition: s(manifestString),
-	}
-
-	service.InitDefaults()
-
-	return service, nil
+	return nil
+}
+
+// isValidExecContainerAction checks the validatity of the execContainer - personally i think this validation
+// should be done high up the chain, but
+func isValidExecContainerAction(action *kops.ExecContainerAction) error {
+	action.Image = strings.TrimSpace(action.Image)
+	if action.Image == "" {
+		return errors.New("the image for the hook exec action not set")
+	}
+
+	return nil
 }

pkg/apis/kops/cluster.go

@@ -152,14 +152,22 @@ type Assets struct {
 
 // HookSpec is a definition hook
 type HookSpec struct {
-	// Documentation is a link the documentation
-	Documentation string `json:"documentation,omitempty"`
+	// Name is an optional name for the hook, otherwise the name is kops-hook-<index>
+	Name string `json:"name,omitempty"`
+	// Disabled indicates if you want the unit switched off
+	Disabled bool `json:"disabled,omitempty"`
+	// MasterOnly indicates this hooks should only run on a master
+	MasterOnly bool `json:"masterOnly,omitempty"`
+	// NodeOnly indicates this hooks should only run on a compute node
+	NodeOnly bool `json:"nodeOnly,omitempty"`
 	// Requires is a series of systemd units the action requires
 	Requires []string `json:"requires,omitempty"`
 	// Before is a series of systemd units which this hook must run before
 	Before []string `json:"before,omitempty"`
 	// ExecContainer is the image itself
 	ExecContainer *ExecContainerAction `json:"execContainer,omitempty"`
+	// Manifest is a raw systemd unit file
+	Manifest string `json:"manifest,omitempty"`
 }
 
 // ExecContainerAction defines an hood action

pkg/apis/kops/v1alpha1/cluster.go

@@ -258,14 +258,22 @@ type Assets struct {
 
 // HookSpec is a definition hook
 type HookSpec struct {
-	// Documentation is a link the documentation
-	Documentation string `json:"documentation,omitempty"`
+	// Name is an optional name for the hook, otherwise the name is kops-hook-<index>
+	Name string `json:"name,omitempty"`
+	// Disabled indicates if you want the unit switched off
+	Disabled bool `json:"disabled,omitempty"`
+	// MasterOnly indicates this hooks should only run on a master
+	MasterOnly bool `json:"masterOnly,omitempty"`
+	// NodeOnly indicates this hooks should only run on a compute node
+	NodeOnly bool `json:"nodeOnly,omitempty"`
 	// Requires is a series of systemd units the action requires
 	Requires []string `json:"requires,omitempty"`
 	// Before is a series of systemd units which this hook must run before
 	Before []string `json:"before,omitempty"`
 	// ExecContainer is the image itself
 	ExecContainer *ExecContainerAction `json:"execContainer,omitempty"`
+	// Manifest is a raw systemd unit file
+	Manifest string `json:"manifest,omitempty"`
 }
 
 // ExecContainerAction defines an hood action

pkg/apis/kops/v1alpha1/zz_generated.conversion.go

@@ -1233,7 +1233,10 @@ func Convert_kops_HTTPProxy_To_v1alpha1_HTTPProxy(in *kops.HTTPProxy, out *HTTPP
 }
 
 func autoConvert_v1alpha1_HookSpec_To_kops_HookSpec(in *HookSpec, out *kops.HookSpec, s conversion.Scope) error {
-	out.Documentation = in.Documentation
+	out.Name = in.Name
+	out.Disabled = in.Disabled
+	out.MasterOnly = in.MasterOnly
+	out.NodeOnly = in.NodeOnly
 	out.Requires = in.Requires
 	out.Before = in.Before
 	if in.ExecContainer != nil {
@@ -1245,6 +1248,7 @@ func autoConvert_v1alpha1_HookSpec_To_kops_HookSpec(in *HookSpec, out *kops.Hook
 	} else {
 		out.ExecContainer = nil
 	}
+	out.Manifest = in.Manifest
 	return nil
 }
 
@@ -1254,7 +1258,10 @@ func Convert_v1alpha1_HookSpec_To_kops_HookSpec(in *HookSpec, out *kops.HookSpec
 }
 
 func autoConvert_kops_HookSpec_To_v1alpha1_HookSpec(in *kops.HookSpec, out *HookSpec, s conversion.Scope) error {
-	out.Documentation = in.Documentation
+	out.Name = in.Name
+	out.Disabled = in.Disabled
+	out.MasterOnly = in.MasterOnly
+	out.NodeOnly = in.NodeOnly
 	out.Requires = in.Requires
 	out.Before = in.Before
 	if in.ExecContainer != nil {
@@ -1266,6 +1273,7 @@ func autoConvert_kops_HookSpec_To_v1alpha1_HookSpec(in *kops.HookSpec, out *Hook
 	} else {
 		out.ExecContainer = nil
 	}
+	out.Manifest = in.Manifest
 	return nil
 }
 

pkg/apis/kops/v1alpha2/cluster.go

@@ -184,14 +184,22 @@ type Assets struct {
 
 // HookSpec is a definition hook
 type HookSpec struct {
-	// Documentation is a link the documentation
-	Documentation string `json:"documentation,omitempty"`
+	// Name is an optional name for the hook, otherwise the name is kops-hook-<index>
+	Name string `json:"name,omitempty"`
+	// Disabled indicates if you want the unit switched off
+	Disabled bool `json:"disabled,omitempty"`
+	// MasterOnly indicates this hooks should only run on a master
+	MasterOnly bool `json:"masterOnly,omitempty"`
+	// NodeOnly indicates this hooks should only run on a compute node
+	NodeOnly bool `json:"nodeOnly,omitempty"`
 	// Requires is a series of systemd units the action requires
 	Requires []string `json:"requires,omitempty"`
 	// Before is a series of systemd units which this hook must run before
 	Before []string `json:"before,omitempty"`
 	// ExecContainer is the image itself
 	ExecContainer *ExecContainerAction `json:"execContainer,omitempty"`
+	// Manifest is a raw systemd unit file
+	Manifest string `json:"manifest,omitempty"`
 }
 
 // ExecContainerAction defines an hood action

pkg/apis/kops/v1alpha2/zz_generated.conversion.go

@@ -1331,7 +1331,10 @@ func Convert_kops_HTTPProxy_To_v1alpha2_HTTPProxy(in *kops.HTTPProxy, out *HTTPP
 }
 
 func autoConvert_v1alpha2_HookSpec_To_kops_HookSpec(in *HookSpec, out *kops.HookSpec, s conversion.Scope) error {
-	out.Documentation = in.Documentation
+	out.Name = in.Name
+	out.Disabled = in.Disabled
+	out.MasterOnly = in.MasterOnly
+	out.NodeOnly = in.NodeOnly
 	out.Requires = in.Requires
 	out.Before = in.Before
 	if in.ExecContainer != nil {
@@ -1343,6 +1346,7 @@ func autoConvert_v1alpha2_HookSpec_To_kops_HookSpec(in *HookSpec, out *kops.Hook
 	} else {
 		out.ExecContainer = nil
 	}
+	out.Manifest = in.Manifest
 	return nil
 }
 
@@ -1352,7 +1356,10 @@ func Convert_v1alpha2_HookSpec_To_kops_HookSpec(in *HookSpec, out *kops.HookSpec
 }
 
 func autoConvert_kops_HookSpec_To_v1alpha2_HookSpec(in *kops.HookSpec, out *HookSpec, s conversion.Scope) error {
-	out.Documentation = in.Documentation
+	out.Name = in.Name
+	out.Disabled = in.Disabled
+	out.MasterOnly = in.MasterOnly
+	out.NodeOnly = in.NodeOnly
 	out.Requires = in.Requires
 	out.Before = in.Before
 	if in.ExecContainer != nil {
@@ -1364,6 +1371,7 @@ func autoConvert_kops_HookSpec_To_v1alpha2_HookSpec(in *kops.HookSpec, out *Hook
 	} else {
 		out.ExecContainer = nil
 	}
+	out.Manifest = in.Manifest
 	return nil
 }
 

pkg/apis/kops/validation/validation.go

@@ -132,16 +132,21 @@ func validateSubnet(subnet *kops.ClusterSubnetSpec, fieldPath *field.Path) field
 	return allErrs
 }
 
-func validateHook(v *kops.HookSpec, fldPath *field.Path) field.ErrorList {
+func validateHook(v *kops.HookSpec, fieldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	if v.ExecContainer == nil {
-		allErrs = append(allErrs, field.Required(fldPath, "An action is required"))
+	if !v.Disabled && v.MasterOnly && v.NodeOnly {
+		allErrs = append(allErrs, field.Invalid(fieldPath, v.MasterOnly, "you cannot have both masterOnly and nodeOnly set true"))
 	}
 
-	if v.ExecContainer != nil {
-		allErrs = append(allErrs, validateExecContainerAction(v.ExecContainer, fldPath.Child("ExecContainer"))...)
+	if !v.Disabled && v.ExecContainer == nil && v.Manifest == "" {
+		allErrs = append(allErrs, field.Required(fieldPath, "you must set either manifest or execContainer for a hook"))
 	}
+
+	if !v.Disabled && v.ExecContainer != nil {
+		allErrs = append(allErrs, validateExecContainerAction(v.ExecContainer, fieldPath.Child("ExecContainer"))...)
+	}
+
 	return allErrs
 }
 

pkg/systemd/escaping.go

@@ -19,10 +19,12 @@ package systemd
 import (
 	"bytes"
 	"encoding/hex"
-	"github.com/golang/glog"
 	"strings"
+
+	"github.com/golang/glog"
 )
 
+// EscapeCommand is used to escape a command
 func EscapeCommand(argv []string) string {
 	var escaped []string
 	for _, arg := range argv {
@@ -69,7 +71,7 @@ func escapeArg(s string) string {
 
 	if needQuotes {
 		return "\"" + b.String() + "\""
-	} else {
-		return b.String()
 	}
+
+	return b.String()
 }

pkg/systemd/manifest.go

@@ -16,74 +16,64 @@ limitations under the License.
 
 package systemd
 
-import "bytes"
+import (
+	"bytes"
+	"fmt"
+)
 
+// Manifest defined a systemd unit
 type Manifest struct {
-	Sections []*ManifestSection
+	sections []*section
 }
 
-type ManifestSection struct {
-	Key     string
-	Entries []*ManifestEntry
+// section defines a section of the unit i.e. Unit, Service etc,
+type section struct {
+	key     string
+	content string
+	entries []string
 }
 
-type ManifestEntry struct {
-	Key   string
-	Value string
+// Set adds a key/pair to the a section in the systemd manifest
+func (m *Manifest) Set(name, key, value string) {
+	s := m.getSection(name)
+	s.entries = append(s.entries, fmt.Sprintf("%s=%s\n", key, value))
 }
 
-func (s *Manifest) Set(sectionKey string, key string, value string) {
-	section := s.getOrCreateSection(sectionKey)
-	section.Set(key, value)
+// SetSection sets the raw content of a section
+func (m *Manifest) SetSection(name, content string) {
+	m.getSection(name).content = content
 }
 
-func (s *Manifest) getOrCreateSection(key string) *ManifestSection {
-	for _, section := range s.Sections {
-		if section.Key == key {
-			return section
+// getSection checks if a section already exists
+func (m *Manifest) getSection(key string) *section {
+	for _, s := range m.sections {
+		if s.key == key {
+			return s
 		}
 	}
-	section := &ManifestSection{
-		Key: key,
-	}
-	s.Sections = append(s.Sections, section)
-	return section
+	// create a new section for this manifest
+	s := &section{key: key, entries: make([]string, 0)}
+	m.sections = append(m.sections, s)
+
+	return s
 }
 
-func (s *Manifest) Render() string {
+// Render is responsible for generating the final unit
+func (m *Manifest) Render() string {
 	var b bytes.Buffer
+	size := len(m.sections) - 1
 
-	for i, section := range s.Sections {
-		if i != 0 {
+	for i, section := range m.sections {
+		b.WriteString(fmt.Sprintf("[%s]\n", section.key))
+		if section.content != "" {
+			b.WriteString(section.content)
+		}
+		for _, x := range section.entries {
+			b.WriteString(x)
+		}
+		if i < size {
 			b.WriteString("\n")
 		}
-		b.WriteString(section.Render())
-	}
-
-	return b.String()
-}
-
-func (s *ManifestSection) Set(key string, value string) {
-	for _, entry := range s.Entries {
-		if entry.Key == key {
-			entry.Value = value
-			return
-		}
-	}
-
-	entry := &ManifestEntry{
-		Key:   key,
-		Value: value,
-	}
-	s.Entries = append(s.Entries, entry)
-}
-
-func (s *ManifestSection) Render() string {
-	var b bytes.Buffer
-
-	b.WriteString("[" + s.Key + "]\n")
-	for _, entry := range s.Entries {
-		b.WriteString(entry.Key + "=" + entry.Value + "\n")
 	}
 
 	return b.String()

pkg/systemd/manifest_test.go

@@ -0,0 +1,92 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package systemd
+
+import (
+	"testing"
+)
+
+func TestRawManifest(t *testing.T) {
+	expected := `[Unit]
+Description=test
+Requires=docker.service
+
+[Service]
+run the command
+in this manifest`
+	m := &Manifest{}
+	m.Set("Unit", "Description", "test")
+	m.Set("Unit", "Requires", "docker.service")
+	m.SetSection("Service", `run the command
+in this manifest`)
+
+	rendered := m.Render()
+	if rendered != expected {
+		t.Errorf("the rendered manifest is not as expected: '%v', got: '%v'", expected, rendered)
+	}
+}
+
+func TestRawMixedManifest(t *testing.T) {
+	expected := `[Unit]
+Description=test
+Requires=docker.service
+
+[Service]
+run the command
+in this manifest
+key=pair
+another=pair
+`
+	m := &Manifest{}
+	m.Set("Unit", "Description", "test")
+	m.Set("Unit", "Requires", "docker.service")
+	m.SetSection("Service", `run the command
+in this manifest
+`)
+	m.Set("Service", "key", "pair")
+	m.Set("Service", "another", "pair")
+
+	rendered := m.Render()
+	if rendered != expected {
+		t.Errorf("the rendered manifest is not as expected: '%v', got: '%v'", expected, rendered)
+	}
+}
+
+func TestKeyPairOnlyManifest(t *testing.T) {
+	expected := `[Unit]
+Description=test
+Requires=docker.service
+
+[Service]
+EnvironmentFile=/etc/somefile
+EnvironmentFile=/etc/another_file
+StartExecPre=some_command
+Start=command
+`
+	m := &Manifest{}
+	m.Set("Unit", "Description", "test")
+	m.Set("Unit", "Requires", "docker.service")
+	m.Set("Service", "EnvironmentFile", "/etc/somefile")
+	m.Set("Service", "EnvironmentFile", "/etc/another_file")
+	m.Set("Service", "StartExecPre", "some_command")
+	m.Set("Service", "Start", "command")
+
+	rendered := m.Render()
+	if rendered != expected {
+		t.Errorf("the rendered manifest is not as expected: '%v'\n, got: '%v'\n", expected, rendered)
+	}
+}

upup/pkg/fi/nodeup/nodetasks/service.go

@@ -26,11 +26,12 @@ import (
 	"strings"
 	"time"
 
-	"github.com/golang/glog"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/nodeup/cloudinit"
 	"k8s.io/kops/upup/pkg/fi/nodeup/local"
 	"k8s.io/kops/upup/pkg/fi/nodeup/tags"
+
+	"github.com/golang/glog"
 )
 
 const (