From 0fe0df629a92333fa00a5e3cc050d8cf85f78c10 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 4 Apr 2019 15:27:48 +0200 Subject: [PATCH] Fix `tag_all` pseudo policy on default-namespaced wokloads This fixes a regression introduced by #1442 in which applying the `tag_all` pseudo policy failed for workload manifests in which the namespace is omitted. The effective namespace of the workload wasn't set after parsing, causing a mismatch in the resource identifier (the parsed resource identifier was cluster-scoped due to the lack of explicit namespace in the manifest). --- cluster/kubernetes/manifests.go | 8 ++++---- cluster/kubernetes/policies.go | 31 ++++++++++------------------- cluster/kubernetes/policies_test.go | 19 ++++++++++++++++-- cluster/kubernetes/sync_test.go | 2 +- 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/cluster/kubernetes/manifests.go b/cluster/kubernetes/manifests.go index e7cb5602a4..9b5a757ad2 100644 --- a/cluster/kubernetes/manifests.go +++ b/cluster/kubernetes/manifests.go @@ -60,7 +60,7 @@ func getCRDScopes(manifests map[string]kresource.KubeManifest) ResourceScopes { return result } -func postProcess(manifests map[string]kresource.KubeManifest, nser namespacer) (map[string]resource.Resource, error) { +func setEffectiveNamespaces(manifests map[string]kresource.KubeManifest, nser namespacer) (map[string]resource.Resource, error) { knownScopes := getCRDScopes(manifests) result := map[string]resource.Resource{} for _, km := range manifests { @@ -76,15 +76,15 @@ func postProcess(manifests map[string]kresource.KubeManifest, nser namespacer) ( return result, nil } -func (c *Manifests) LoadManifests(base string, paths []string) (map[string]resource.Resource, error) { +func (m *Manifests) LoadManifests(base string, paths []string) (map[string]resource.Resource, error) { manifests, err := kresource.Load(base, paths) if err != nil { return nil, err } - return postProcess(manifests, c.Namespacer) + return setEffectiveNamespaces(manifests, m.Namespacer) } -func (c *Manifests) UpdateImage(def []byte, id flux.ResourceID, container string, image image.Ref) ([]byte, error) { +func (m *Manifests) UpdateImage(def []byte, id flux.ResourceID, container string, image image.Ref) ([]byte, error) { return updateWorkload(def, id, container, image) } diff --git a/cluster/kubernetes/policies.go b/cluster/kubernetes/policies.go index 79d51221ce..2dfda5d5e9 100644 --- a/cluster/kubernetes/policies.go +++ b/cluster/kubernetes/policies.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/pkg/errors" - "gopkg.in/yaml.v2" "github.com/weaveworks/flux" kresource "github.com/weaveworks/flux/cluster/kubernetes/resource" @@ -21,7 +20,7 @@ func (m *Manifests) UpdatePolicies(def []byte, id flux.ResourceID, update policy // what all the containers are. if tagAll, ok := update.Add.Get(policy.TagAll); ok { add = add.Without(policy.TagAll) - containers, err := extractContainers(def, id) + containers, err := m.extractWorkloadContainers(def, id) if err != nil { return nil, err } @@ -49,25 +48,17 @@ func (m *Manifests) UpdatePolicies(def []byte, id flux.ResourceID, update policy return (KubeYAML{}).Annotate(def, ns, kind, name, args...) } -type manifest struct { - Metadata struct { - Annotations map[string]string `yaml:"annotations"` - } `yaml:"metadata"` -} - -func extractAnnotations(def []byte) (map[string]string, error) { - var m manifest - if err := yaml.Unmarshal(def, &m); err != nil { - return nil, errors.Wrap(err, "decoding manifest for annotations") - } - if m.Metadata.Annotations == nil { - return map[string]string{}, nil +func (m *Manifests) extractWorkloadContainers(def []byte, id flux.ResourceID) ([]resource.Container, error) { + kresources, err := kresource.ParseMultidoc(def, "stdin") + if err != nil { + return nil, err } - return m.Metadata.Annotations, nil -} - -func extractContainers(def []byte, id flux.ResourceID) ([]resource.Container, error) { - resources, err := kresource.ParseMultidoc(def, "stdin") + // Note: setEffectiveNamespaces() won't work for CRD instances whose CRD is yet to be created + // (due to the CRD not being present in kresources). + // We could get out of our way to fix this (or give a better error) but: + // 1. With the exception of HelmReleases CRD instances are not workloads anyways. + // 2. The problem is eventually fixed by the first successful sync. + resources, err := setEffectiveNamespaces(kresources, m.Namespacer) if err != nil { return nil, err } diff --git a/cluster/kubernetes/policies_test.go b/cluster/kubernetes/policies_test.go index 985c49d719..78af6d9eef 100644 --- a/cluster/kubernetes/policies_test.go +++ b/cluster/kubernetes/policies_test.go @@ -8,9 +8,16 @@ import ( "github.com/stretchr/testify/assert" "github.com/weaveworks/flux" + kresource "github.com/weaveworks/flux/cluster/kubernetes/resource" "github.com/weaveworks/flux/policy" ) +type constNamespacer string + +func (ns constNamespacer) EffectiveNamespace(manifest kresource.KubeManifest, _ ResourceScopes) (string, error) { + return string(ns), nil +} + func TestUpdatePolicies(t *testing.T) { for _, c := range []struct { name string @@ -166,13 +173,21 @@ func TestUpdatePolicies(t *testing.T) { }, wantErr: true, }, + { + name: "set tag to all containers", + in: nil, + out: []string{"flux.weave.works/tag.nginx", "semver:*"}, + update: policy.Update{ + Add: policy.Set{policy.TagAll: "semver:*"}, + }, + }, } { t.Run(c.name, func(t *testing.T) { caseIn := templToString(t, annotationsTemplate, c.in) caseOut := templToString(t, annotationsTemplate, c.out) resourceID := flux.MustParseResourceID("default:deployment/nginx") - out, err := (&Manifests{}).UpdatePolicies([]byte(caseIn), resourceID, c.update) - assert.Equal(t, c.wantErr, err != nil) + out, err := (&Manifests{constNamespacer("default")}).UpdatePolicies([]byte(caseIn), resourceID, c.update) + assert.Equal(t, c.wantErr, err != nil, "unexpected error value: %s", err) if !c.wantErr { assert.Equal(t, string(out), caseOut) } diff --git a/cluster/kubernetes/sync_test.go b/cluster/kubernetes/sync_test.go index 12f042e797..66e4049fec 100644 --- a/cluster/kubernetes/sync_test.go +++ b/cluster/kubernetes/sync_test.go @@ -319,7 +319,7 @@ metadata: } // Needed to get from KubeManifest to resource.Resource - resources, err := postProcess(resources0, namespacer) + resources, err := setEffectiveNamespaces(resources0, namespacer) if err != nil { t.Fatal(err) }