Use kubeyaml for YAML updates

Makefile

@@ -37,7 +37,7 @@ realclean: clean
 	rm -rf ./cache
 
 test:
-	go test ${TEST_FLAGS} $(shell go list ./... | grep -v "^github.com/weaveworks/flux/vendor" | sort -u)
+	PATH=${PATH}:${PWD}/bin go test ${TEST_FLAGS} $(shell go list ./... | grep -v "^github.com/weaveworks/flux/vendor" | sort -u)
 
 build/.%.done: docker/Dockerfile.%
 	mkdir -p ./build/docker/$*

bin/kubeyaml

@@ -0,0 +1,2 @@
+#!/bin/sh
+docker run --rm -i squaremo/kubeyaml:latest "$@"

cluster/kubernetes/policies.go

@@ -153,3 +153,7 @@ func (m *Manifests) ServicesWithPolicies(root string) (policy.ResourceMap, error
 	}
 	return result, nil
 }
+
+func multilineRE(lines ...string) *regexp.Regexp {
+	return regexp.MustCompile(`(?m:^` + strings.Join(lines, "\n") + `$)`)
+}

cluster/kubernetes/update.go

@@ -2,14 +2,13 @@ package kubernetes
 
 import (
 	"bytes"
-	"fmt"
-	"io"
-	"regexp"
+	"os/exec"
 	"strings"
 
+	"github.com/pkg/errors"
+
 	"github.com/weaveworks/flux"
 	"github.com/weaveworks/flux/image"
-	"github.com/weaveworks/flux/resource"
 )
 
 // updatePodController takes the body of a resource definition
@@ -20,165 +19,26 @@ import (
 // where all references to the old image have been replaced with the
 // new one.
 //
-// This function has some requirements of the YAML structure. Read the
-// source and comments below to learn about them.
-func updatePodController(def []byte, resourceID flux.ResourceID, container string, newImageID image.Ref) ([]byte, error) {
-	// Sanity check
-	obj, err := parseObj(def)
-	if err != nil {
-		return nil, err
-	}
-
-	if _, ok := resourceKinds[strings.ToLower(obj.Kind)]; !ok {
-		return nil, UpdateNotSupportedError(obj.Kind)
-	}
-
-	var buf bytes.Buffer
-	err = tryUpdate(def, resourceID, container, newImageID, &buf)
-	return buf.Bytes(), err
-}
-
-// Attempt to update an RC or Deployment config. This makes several assumptions
-// that are justified only with the phrase "because that's how we do it",
-// including:
-//
-//  * the file is a replication controller or deployment
-//  * the update is from one tag of an image to another tag of the
-//    same image; e.g., "weaveworks/helloworld:a00001" to
-//    "weaveworks/helloworld:a00002"
-//  * the container spec to update is the (first) one that uses the
-//    same image name (e.g., weaveworks/helloworld)
-//  * the name of the controller is updated to reflect the new tag
-//  * there's a label which must be updated in both the pod spec and the selector
-//  * the file uses canonical YAML syntax, that is, one line per item
-//  * ... other assumptions as encoded in the regular expressions used
-//
-// Here's an example of the assumed structure:
-//
-// ```
-// apiVersion: v1
-// kind: Deployment # not presently checked
-// metadata:                         # )
-//   ...                             # ) any number of equally-indented lines
-//   name: helloworld-master-a000001 # ) can precede the name
-// spec:
-//   replicas: 2
-//   selector:                 # )
-//     name: helloworld        # ) this use of labels is assumed
-//     version: master-a000001 # )
-//   template:
-//     metadata:
-//       labels:                   # )
-//         name: helloworld        # ) this structure is assumed, as for the selector
-//         version: master-a000001 # )
-//     spec:
-//       containers:
-//       # extra container specs are allowed here ...
-//       - name: helloworld                                    # )
-//         image: quay.io/weaveworks/helloworld:master-a000001 # ) these must be together
-//         args:
-//         - -msg=Ahoy
-//         ports:
-//         - containerPort: 80
-// ```
-func tryUpdate(def []byte, id flux.ResourceID, container string, newImage image.Ref, out io.Writer) error {
-	containers, err := extractContainers(def, id)
-
-	matchingContainers := map[int]resource.Container{}
-	for i, c := range containers {
-		if c.Name != container {
-			continue
-		}
-		currentImage := c.Image
-		if err != nil {
-			return fmt.Errorf("could not parse image %s", c.Image)
-		}
-		if currentImage.CanonicalName() == newImage.CanonicalName() {
-			matchingContainers[i] = c
-		}
-	}
-
-	if len(matchingContainers) == 0 {
-		return fmt.Errorf("could not find container using image: %s", newImage.Repository())
-	}
-
-	// Detect how indented the "containers" block is.
-	// TODO: delete all regular expressions which are used to modify YAML.
-	// See #1019. Modifying this is not recommended.
-	newDef := string(def)
-	matches := regexp.MustCompile(`( +)containers:.*`).FindStringSubmatch(newDef)
-	if len(matches) != 2 {
-		return fmt.Errorf("could not find container specs")
-	}
-	indent := matches[1]
-
-	// TODO: delete all regular expressions which are used to modify YAML.
-	// See #1019. Modifying this is not recommended.
-	optq := `["']?` // An optional single or double quote
-	// Replace the container images
-	// Parse out all the container blocks
-	containersRE := regexp.MustCompile(`(?m:^` + indent + `containers:\s*(?:#.*)*$(?:\n(?:` + indent + `[-\s#].*)?)*)`)
-	// Parse out an individual container blog
-	containerRE := regexp.MustCompile(`(?m:` + indent + `-.*(?:\n(?:` + indent + `\s+.*)?)*)`)
-	// Parse out the image ID
-	imageRE := regexp.MustCompile(`(` + indent + `[-\s]\s*` + optq + `image` + optq + `:\s*)` + optq + `(?:[\w\.\-/:]+\s*?)*` + optq + `([\t\f #]+.*)?`)
-	imageReplacement := fmt.Sprintf("${1}%s${2}", maybeQuote(newImage.String()))
-	// Find the block of container specs
-	newDef = containersRE.ReplaceAllStringFunc(newDef, func(containers string) string {
-		i := 0
-		// Find each container spec
-		return containerRE.ReplaceAllStringFunc(containers, func(spec string) string {
-			if _, ok := matchingContainers[i]; ok {
-				// container matches, let's replace the image
-				spec = imageRE.ReplaceAllString(spec, imageReplacement)
-				delete(matchingContainers, i)
-			}
-			i++
-			return spec
-		})
-	})
-
-	if len(matchingContainers) > 0 {
-		missed := []string{}
-		for _, c := range matchingContainers {
-			missed = append(missed, c.Name)
-		}
-		return fmt.Errorf("did not update expected containers: %s", strings.Join(missed, ", "))
+// This function has many additional requirements that are likely in flux. Read
+// the source to learn about them.
+func updatePodController(file []byte, resource flux.ResourceID, container string, newImageID image.Ref) ([]byte, error) {
+	namespace, kind, name := resource.Components()
+	if _, ok := resourceKinds[strings.ToLower(kind)]; !ok {
+		return nil, UpdateNotSupportedError(kind)
 	}
 
-	// TODO: delete all regular expressions which are used to modify YAML.
-	// See #1019. Modifying this is not recommended.
-	// Replacing labels: these are in two places, the container template and the selector
-	// TODO: This doesn't handle # comments
-	// TODO: This encodes an expectation of map keys being ordered (i.e. name *then* version)
-	// TODO: This assumes that these are indented by exactly 2 spaces (which may not be true)
-	replaceLabelsRE := multilineRE(
-		`((?:  selector|      labels):.*)`,
-		`((?:  ){2,4}name:.*)`,
-		`((?:  ){2,4}version:\s*) (?:`+optq+`[-\w]+`+optq+`)(\s.*)`,
-	)
-	replaceLabels := fmt.Sprintf("$1\n$2\n$3 %s$4", maybeQuote(newImage.Tag))
-	newDef = replaceLabelsRE.ReplaceAllString(newDef, replaceLabels)
-
-	fmt.Fprint(out, newDef)
-	return nil
-}
-
-func multilineRE(lines ...string) *regexp.Regexp {
-	return regexp.MustCompile(`(?m:^` + strings.Join(lines, "\n") + `$)`)
-}
-
-// TODO: delete all regular expressions which are used to modify YAML.
-// See #1019. Modifying this is not recommended.
-var looksLikeNumber *regexp.Regexp = regexp.MustCompile("^(" + strings.Join([]string{
-	`(-?[1-9](\.[0-9]*[1-9])?(e[-+][1-9][0-9]*)?)`,
-	`(-?(0|[1-9][0-9]*))`,
-	`(0|(\.inf)|(-\.inf)|(\.nan))`},
-	"|") + ")$")
-
-func maybeQuote(scalar string) string {
-	if looksLikeNumber.MatchString(scalar) {
-		return `"` + scalar + `"`
+	args := []string{"--namespace", namespace, "--kind", kind, "--name", name}
+	args = append(args, "--container", container, "--image", newImageID.String())
+
+	println("TRACE:", "kubeyaml", strings.Join(args, " "))
+	cmd := exec.Command("kubeyaml", args...)
+	out := &bytes.Buffer{}
+	errOut := &bytes.Buffer{}
+	cmd.Stdin = bytes.NewBuffer(file)
+	cmd.Stdout = out
+	cmd.Stderr = errOut
+	if err := cmd.Run(); err != nil {
+		return nil, errors.Wrap(err, errOut.String())
 	}
-	return scalar
+	return out.Bytes(), nil
 }

cluster/kubernetes/update_test.go

@@ -70,27 +70,27 @@ spec:
       imagePullSecrets:
       - name: quay-secret
       containers:
-        - name: pr-assigner
-          image: quay.io/weaveworks/pr-assigner:master-6f5e816
-          imagePullPolicy: IfNotPresent
-          args:
-            - --conf_path=/config/pr-assigner.json
-          env:
-            - name: GITHUB_TOKEN
-              valueFrom:
-                secretKeyRef:
-                  name: pr-assigner
-                  key: githubtoken
-          volumeMounts:
-            - name: config-volume
-              mountPath: /config
-      volumes:
+      - name: pr-assigner
+        image: quay.io/weaveworks/pr-assigner:master-6f5e816
+        imagePullPolicy: IfNotPresent
+        args:
+        - --conf_path=/config/pr-assigner.json
+        env:
+        - name: GITHUB_TOKEN
+          valueFrom:
+            secretKeyRef:
+              name: pr-assigner
+              key: githubtoken
+        volumeMounts:
         - name: config-volume
-          configMap:
-            name: pr-assigner
-            items:
-              - key: conffile
-                path: pr-assigner.json
+          mountPath: /config
+      volumes:
+      - name: config-volume
+        configMap:
+          name: pr-assigner
+          items:
+          - key: conffile
+            path: pr-assigner.json
 `
 
 const case1resource = "extra:deployment/pr-assigner"
@@ -114,27 +114,27 @@ spec:
       imagePullSecrets:
       - name: quay-secret
       containers:
-        - name: pr-assigner
-          image: quay.io/weaveworks/pr-assigner:master-1234567
-          imagePullPolicy: IfNotPresent
-          args:
-            - --conf_path=/config/pr-assigner.json
-          env:
-            - name: GITHUB_TOKEN
-              valueFrom:
-                secretKeyRef:
-                  name: pr-assigner
-                  key: githubtoken
-          volumeMounts:
-            - name: config-volume
-              mountPath: /config
-      volumes:
+      - name: pr-assigner
+        image: quay.io/weaveworks/pr-assigner:master-1234567
+        imagePullPolicy: IfNotPresent
+        args:
+        - --conf_path=/config/pr-assigner.json
+        env:
+        - name: GITHUB_TOKEN
+          valueFrom:
+            secretKeyRef:
+              name: pr-assigner
+              key: githubtoken
+        volumeMounts:
         - name: config-volume
-          configMap:
-            name: pr-assigner
-            items:
-              - key: conffile
-                path: pr-assigner.json
+          mountPath: /config
+      volumes:
+      - name: config-volume
+        configMap:
+          name: pr-assigner
+          items:
+          - key: conffile
+            path: pr-assigner.json
 `
 
 // Version looks like a number
@@ -149,7 +149,6 @@ spec:
     metadata:
       labels:
         name: fluxy
-        version: master-a000001
     spec:
       volumes:
       - name: key
@@ -194,7 +193,6 @@ spec:
     metadata:
       labels:
         name: fluxy
-        version: "1234567"
     spec:
       volumes:
       - name: key
@@ -230,7 +228,7 @@ apiVersion: extensions/v1beta1
 kind: Deployment
 metadata:
  namespace: monitoring
- name: grafana # comment, and only one space
+ name: grafana # comment, and only one space indent
 spec:
   replicas: 1
   template:
@@ -262,8 +260,8 @@ const case3out = `---
 apiVersion: extensions/v1beta1
 kind: Deployment
 metadata:
- namespace: monitoring
- name: grafana # comment, and only one space
+  namespace: monitoring
+  name: grafana # comment, and only one space indent
 spec:
   replicas: 1
   template:
@@ -313,7 +311,7 @@ spec:
           runAsUser: 10001
           capabilities:
             drop:
-              - all
+            - all
           readOnlyRootFilesystem: true
 `
 
@@ -349,7 +347,7 @@ spec:
           runAsUser: 10001
           capabilities:
             drop:
-              - all
+            - all
           readOnlyRootFilesystem: true
 `
 
@@ -439,7 +437,7 @@ spec:
         - containerPort: 80
         image: nginx:1.10-alpine
         name: nginx
-      - image: nginx:1.10-alpine # testing comments, and this image is on the first line.
+      - image: nginx:1.10-alpine    # testing comments, and this image is on the first line.
         name: nginx2
 `
 
@@ -515,7 +513,7 @@ spec:
       labels:
         name: authfe
       annotations:
-        prometheus.io.port: "8080"
+        prometheus.io.port: '8080'
     spec:
       # blank comment spacers in the following
       containers:

http/daemon/upstream.go

@@ -175,7 +175,6 @@ func (a *Upstream) setConnectionDuration(duration float64) {
 }
 
 func (a *Upstream) LogEvent(event event.Event) error {
-	// Instance ID is set via token here, so we can leave it blank.
 	return a.apiClient.LogEvent(context.TODO(), event)
 }