Merge pull request #2565 from fluxcd/kustomize-ignore-unlinky-patches

Improve experience with .flux.yaml files

go.mod

@@ -22,6 +22,7 @@ require (
 	github.com/pkg/errors v0.8.1
 	github.com/pkg/term v0.0.0-20190109203006-aa71e9d9e942
 	github.com/prometheus/client_golang v1.1.0
+	github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90
 	github.com/ryanuber/go-glob v1.0.0
 	github.com/shurcooL/httpfs v0.0.0-20190707220628-8d4bc4ba7749
 	github.com/shurcooL/vfsgen v0.0.0-20181202132449-6a9ea43bcacd

pkg/cluster/kubernetes/patch.go

@@ -74,7 +74,7 @@ func applyManifestPatch(originalManifests, patchManifests []byte, originalSource
 	// Make sure all patch resources have a matching resource
 	for id, patchResource := range patchResources {
 		if _, ok := originalResources[id]; !ok {
-			return nil, fmt.Errorf("missing resource (%s) for patch", resourceID(patchResource))
+			return nil, fmt.Errorf("patch refers to missing resource (%s)", resourceID(patchResource))
 		}
 	}
 
@@ -94,7 +94,7 @@ func applyManifestPatch(originalManifests, patchManifests []byte, originalSource
 			// There was a patch, apply it
 			patched, err := applyPatch(originalResource, patchedResource, scheme)
 			if err != nil {
-				return nil, fmt.Errorf("cannot obtain patch for resource %s: %s", id, err)
+				return nil, fmt.Errorf("cannot apply patch for resource %s: %s", id, err)
 			}
 			resourceBytes = patched
 		}
@@ -221,14 +221,13 @@ func applyPatch(originalManifest, patchManifest kresource.KubeManifest, scheme *
 	}
 	patched, err := jsonyaml.JSONToYAML(patchedJSON)
 	if err != nil {
-		return nil, fmt.Errorf("cannot transform patched resource (%s) to YAML: %s",
-			resourceID(originalManifest), err)
+		return nil, fmt.Errorf("cannot transform patched resource (%s) to YAML: %s", resourceID(originalManifest), err)
 	}
 	return patched, nil
 }
 
 // resourceID works like Resource.ID() but avoids <cluster> namespaces,
 // since they may be incorrect
 func resourceID(manifest kresource.KubeManifest) resource.ID {
-	return resource.MakeID(manifest.GetNamespace(), manifest.GetKind(), manifest.GetKind())
+	return resource.MakeID(manifest.GetNamespace(), manifest.GetKind(), manifest.GetName())
 }

pkg/manifests/configaware.go

@@ -1,11 +1,8 @@
 package manifests
 
 import (
-	"bytes"
 	"context"
-	"errors"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
@@ -143,131 +140,15 @@ func (ca *configAware) SetWorkloadContainerImage(ctx context.Context, resourceID
 		if err := ca.rawFiles.setManifestWorkloadContainerImage(resWithOrigin.resource, container, newImageID); err != nil {
 			return err
 		}
-	} else if err := ca.setConfigFileWorkloadContainerImage(ctx, resWithOrigin.configFile, resWithOrigin.resource, container, newImageID); err != nil {
+	} else if err := resWithOrigin.configFile.SetWorkloadContainerImage(ctx, ca.manifests, resWithOrigin.resource, container, newImageID); err != nil {
 		return err
 	}
 	// Reset resources, since we have modified one
 	ca.resetResources()
 	return nil
 }
 
-func (ca *configAware) setConfigFileWorkloadContainerImage(ctx context.Context, cf *ConfigFile, r resource.Resource,
-	container string, newImageID image.Ref) error {
-	if cf.PatchUpdated != nil {
-		return ca.updatePatchFile(ctx, cf, func(previousManifests []byte) ([]byte, error) {
-			return ca.manifests.SetWorkloadContainerImage(previousManifests, r.ResourceID(), container, newImageID)
-		})
-	}
-
-	// Command-updated
-	result := cf.ExecContainerImageUpdaters(ctx,
-		r.ResourceID(),
-		container,
-		newImageID.Name.String(), newImageID.Tag,
-	)
-	if len(result) > 0 && result[len(result)-1].Error != nil {
-		updaters := cf.CommandUpdated.Updaters
-		return fmt.Errorf("error executing image updater command %q from file %q: %s\noutput:\n%s",
-			updaters[len(result)-1].ContainerImage.Command,
-			result[len(result)-1].Error,
-			r.Source(),
-			result[len(result)-1].Output,
-		)
-	}
-	return nil
-}
-
-func (ca *configAware) updatePatchFile(ctx context.Context, cf *ConfigFile,
-	updateF func(previousManifests []byte) ([]byte, error)) error {
-
-	patchUpdated := *cf.PatchUpdated
-	generatedManifests, patchedManifests, patchFilePath, err := ca.getGeneratedAndPatchedManifests(ctx, cf, patchUpdated)
-	if err != nil {
-		relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
-		if err != nil {
-			return err
-		}
-		return fmt.Errorf("error parsing generated, patched output from file %s: %s", relConfigFilePath, err)
-	}
-	finalManifests, err := updateF(patchedManifests)
-	if err != nil {
-		return err
-	}
-	newPatch, err := ca.manifests.CreateManifestPatch(generatedManifests, finalManifests,
-		"generated manifests", "patched and updated manifests")
-	if err != nil {
-		return err
-	}
-	return ioutil.WriteFile(patchFilePath, newPatch, 0600)
-}
-
-func (ca *configAware) getGeneratedAndPatchedManifests(ctx context.Context, cf *ConfigFile, patchUpdated PatchUpdated) ([]byte, []byte, string, error) {
-	generatedManifests, err := ca.getGeneratedManifests(ctx, cf, patchUpdated.Generators)
-	if err != nil {
-		return nil, nil, "", err
-	}
-
-	// The patch file is expressed relatively to the configuration file's working directory
-	explicitPatchFilePath := patchUpdated.PatchFile
-	patchFilePath := filepath.Join(cf.WorkingDir, explicitPatchFilePath)
-
-	// Make sure that the patch file doesn't fall out of the Git repository checkout
-	_, _, err = cleanAndEnsureParentPath(ca.baseDir, patchFilePath)
-	if err != nil {
-		return nil, nil, "", err
-	}
-	patch, err := ioutil.ReadFile(patchFilePath)
-	if err != nil {
-		if !os.IsNotExist(err) {
-			return nil, nil, "", err
-		}
-		// Tolerate a missing patch file, since it may not have been created yet.
-		// However, its base path must exist.
-		patchBaseDir := filepath.Dir(patchFilePath)
-		if stat, err := os.Stat(patchBaseDir); err != nil || !stat.IsDir() {
-			err := fmt.Errorf("base directory (%q) of patchFile (%q) does not exist",
-				filepath.Dir(explicitPatchFilePath), explicitPatchFilePath)
-			return nil, nil, "", err
-		}
-		patch = nil
-	}
-	relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
-	if err != nil {
-		return nil, nil, "", err
-	}
-	patchedManifests, err := ca.manifests.ApplyManifestPatch(generatedManifests, patch, relConfigFilePath, explicitPatchFilePath)
-	if err != nil {
-		return nil, nil, "", fmt.Errorf("cannot patch generated resources: %s", err)
-	}
-	return generatedManifests, patchedManifests, patchFilePath, nil
-}
-
-func (ca *configAware) getGeneratedManifests(ctx context.Context, cf *ConfigFile, generators []Generator) ([]byte, error) {
-	buf := bytes.NewBuffer(nil)
-	for i, cmdResult := range cf.ExecGenerators(ctx, generators) {
-		relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
-		if err != nil {
-			return nil, err
-		}
-		if cmdResult.Error != nil {
-			err := fmt.Errorf("error executing generator command %q from file %q: %s\nerror output:\n%s\ngenerated output:\n%s",
-				generators[i].Command,
-				relConfigFilePath,
-				cmdResult.Error,
-				string(cmdResult.Stderr),
-				string(cmdResult.Stderr),
-			)
-			return nil, err
-		}
-		if err := ca.manifests.AppendManifestToBuffer(cmdResult.Stdout, buf); err != nil {
-			return nil, err
-		}
-	}
-	return buf.Bytes(), nil
-}
-
-func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID resource.ID,
-	update resource.PolicyUpdate) (bool, error) {
+func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID resource.ID, update resource.PolicyUpdate) (bool, error) {
 	resourcesByID, err := ca.getResourcesByID(ctx)
 	if err != nil {
 		return false, err
@@ -280,7 +161,8 @@ func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID re
 	if resWithOrigin.configFile == nil {
 		changed, err = ca.rawFiles.updateManifestWorkloadPolicies(resWithOrigin.resource, update)
 	} else {
-		changed, err = ca.updateConfigFileWorkloadPolicies(ctx, resWithOrigin.configFile, resWithOrigin.resource, update)
+		cf := resWithOrigin.configFile
+		changed, err = cf.UpdateWorkloadPolicies(ctx, ca.manifests, resWithOrigin.resource, update)
 	}
 	if err != nil {
 		return false, err
@@ -290,48 +172,6 @@ func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID re
 	return changed, nil
 }
 
-func (ca *configAware) updateConfigFileWorkloadPolicies(ctx context.Context, cf *ConfigFile, r resource.Resource,
-	update resource.PolicyUpdate) (bool, error) {
-	if cf.PatchUpdated != nil {
-		var changed bool
-		err := ca.updatePatchFile(ctx, cf, func(previousManifests []byte) ([]byte, error) {
-			updatedManifests, err := ca.manifests.UpdateWorkloadPolicies(previousManifests, r.ResourceID(), update)
-			if err == nil {
-				changed = bytes.Compare(previousManifests, updatedManifests) != 0
-			}
-			return updatedManifests, err
-		})
-		return changed, err
-	}
-
-	// Command-updated
-	workload, ok := r.(resource.Workload)
-	if !ok {
-		return false, errors.New("resource " + r.ResourceID().String() + " does not have containers")
-	}
-	changes, err := resource.ChangesForPolicyUpdate(workload, update)
-	if err != nil {
-		return false, err
-	}
-
-	for key, value := range changes {
-		result := cf.ExecPolicyUpdaters(ctx, r.ResourceID(), key, value)
-		if len(result) > 0 && result[len(result)-1].Error != nil {
-			updaters := cf.CommandUpdated.Updaters
-			err := fmt.Errorf("error executing annotation updater command %q from file %q: %s\noutput:\n%s",
-				updaters[len(result)-1].Policy.Command,
-				result[len(result)-1].Error,
-				r.Source(),
-				result[len(result)-1].Output,
-			)
-			return false, err
-		}
-	}
-	// We assume that the update changed the resource. Alternatively, we could generate the resources
-	// again and compare the output, but that's expensive.
-	return true, nil
-}
-
 func (ca *configAware) GetAllResourcesByID(ctx context.Context) (map[string]resource.Resource, error) {
 	resourcesByID, err := ca.getResourcesByID(ctx)
 	if err != nil {
@@ -364,23 +204,11 @@ func (ca *configAware) getResourcesByID(ctx context.Context) (map[string]resourc
 	}
 
 	for _, cf := range ca.configFiles {
-		var (
-			resourceManifests []byte
-			err               error
-		)
-		if cf.CommandUpdated != nil {
-			var err error
-			resourceManifests, err = ca.getGeneratedManifests(ctx, cf, cf.CommandUpdated.Generators)
-			if err != nil {
-				return nil, err
-			}
-		} else {
-			_, resourceManifests, _, err = ca.getGeneratedAndPatchedManifests(ctx, cf, *cf.PatchUpdated)
-		}
+		resourceManifests, err := cf.GenerateManifests(ctx, ca.manifests)
 		if err != nil {
 			return nil, err
 		}
-		relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
+		relConfigFilePath, err := cf.RelativeConfigPath()
 		if err != nil {
 			return nil, err
 		}

pkg/manifests/configaware_test.go

@@ -90,8 +90,20 @@ commandUpdated:
 
 	assert.Len(t, configFiles, 2)
 	// We assume config files are processed in order to simplify the checks
-	assert.Equal(t, filepath.Join(baseDir, "envs/staging"), configFiles[0].WorkingDir)
-	assert.Equal(t, filepath.Join(baseDir, "envs/production"), configFiles[1].WorkingDir)
+	assert.Equal(t, filepath.Join(baseDir, "envs/staging"), configFiles[0].workingDir)
+	assert.Equal(t, filepath.Join(baseDir, "envs/production"), configFiles[1].workingDir)
+
+	mustRelativeConfigPath := func(cf *ConfigFile) string {
+		p, err := cf.RelativeConfigPath()
+		if err != nil {
+			t.Error(err)
+		}
+		return p
+	}
+
+	assert.Equal(t, "../.flux.yaml", mustRelativeConfigPath(configFiles[0]))
+	assert.Equal(t, "../.flux.yaml", mustRelativeConfigPath(configFiles[1]))
+
 	assert.NotNil(t, configFiles[0].CommandUpdated)
 	assert.Len(t, configFiles[0].CommandUpdated.Generators, 1)
 	assert.Equal(t, "echo g1", configFiles[0].CommandUpdated.Generators[0].Command)
@@ -223,3 +235,44 @@ spec:
 	assert.NoError(t, err)
 	assert.Equal(t, expectedPatch, string(patch))
 }
+
+const mistakenConf = `
+version: 1
+commandUpdated: # <-- because this is commandUpdated, patchFile is ignored
+  generators:
+    - command: |
+       echo "apiVersion: extensions/v1beta1
+       kind: Deployment
+       metadata:
+         name: helloworld
+       spec:
+         template:
+           metadata:
+             labels:
+              name: helloworld
+           spec:
+             containers:
+             - name: greeter
+               image: quay.io/weaveworks/helloworld:master-a000001
+       ---
+       apiVersion: v1
+       kind: Namespace
+       metadata:
+         name: demo"
+  patchFile: patchfile.yaml
+`
+
+// This tests that when using a config with no update commands, and
+// update operation results in an error, rather than a silent failure
+// to make any changes.
+func TestMistakenConfigFile(t *testing.T) {
+	frs, cleanup := setup(t, mistakenConf)
+	defer cleanup()
+
+	deploymentID := resource.MustParseID("default:deployment/helloworld")
+	ref, _ := image.ParseRef("repo/image:tag")
+
+	ctx := context.Background()
+	err := frs.SetWorkloadContainerImage(ctx, deploymentID, "greeter", ref)
+	assert.Error(t, err)
+}