From 55fbed95d586db554b33cfa4dceff1475d2d4d41 Mon Sep 17 00:00:00 2001 From: Jacob LeGrone Date: Mon, 4 Mar 2019 10:09:05 -0500 Subject: [PATCH 1/3] feat(resource-policy): delete manifests when policy value is delete Signed-off-by: Jacob LeGrone --- pkg/kube/client.go | 8 +++++--- pkg/kube/resource_policy.go | 23 +++++++++++++++++++---- pkg/tiller/resource_policy.go | 12 +++--------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index eccf888e8..955c75ab1 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -23,12 +23,13 @@ import ( goerrors "errors" "fmt" "io" - "k8s.io/apimachinery/pkg/api/meta" "log" "sort" "strings" "time" + "k8s.io/apimachinery/pkg/api/meta" + "github.com/evanphx/json-patch" appsv1 "k8s.io/api/apps/v1" appsv1beta1 "k8s.io/api/apps/v1beta1" @@ -362,8 +363,9 @@ func (c *Client) Update(namespace string, originalReader, targetReader io.Reader if err != nil { c.Log("Unable to get annotations on %q, err: %s", info.Name, err) } - if annotations != nil && annotations[ResourcePolicyAnno] == KeepPolicy { - c.Log("Skipping delete of %q due to annotation [%s=%s]", info.Name, ResourcePolicyAnno, KeepPolicy) + if ResourcePolicyIsKeep(annotations) { + policy := annotations[ResourcePolicyAnno] + c.Log("Skipping delete of %q due to annotation [%s=%s]", info.Name, ResourcePolicyAnno, policy) continue } diff --git a/pkg/kube/resource_policy.go b/pkg/kube/resource_policy.go index 45cebcba8..9bce63a7c 100644 --- a/pkg/kube/resource_policy.go +++ b/pkg/kube/resource_policy.go @@ -19,8 +19,23 @@ package kube // ResourcePolicyAnno is the annotation name for a resource policy const ResourcePolicyAnno = "helm.sh/resource-policy" -// KeepPolicy is the resource policy type for keep +// deletePolicy is the resource policy type for delete // -// This resource policy type allows resources to skip being deleted -// during an uninstallRelease action. -const KeepPolicy = "keep" +// This resource policy type allows explicitly opting in to the default +// resource deletion behavior, for example when overriding a chart's +// default annotations. Any other value allows resources to skip being +// deleted during an uninstallRelease action. +const deletePolicy = "delete" + +// ResourcePolicyIsKeep accepts a map of Kubernetes resource annotations and +// returns true if the resource should be kept, otherwise false if it is safe +// for Helm to delete. +func ResourcePolicyIsKeep(annotations map[string]string) bool { + if annotations != nil { + resourcePolicyType, ok := annotations[ResourcePolicyAnno] + if ok && resourcePolicyType != deletePolicy { + return true + } + } + return false +} diff --git a/pkg/tiller/resource_policy.go b/pkg/tiller/resource_policy.go index aa9c5d2bd..c97621fcd 100644 --- a/pkg/tiller/resource_policy.go +++ b/pkg/tiller/resource_policy.go @@ -34,17 +34,11 @@ func filterManifestsToKeep(manifests []Manifest) ([]Manifest, []Manifest) { continue } - resourcePolicyType, ok := m.Head.Metadata.Annotations[kube.ResourcePolicyAnno] - if !ok { - remaining = append(remaining, m) - continue - } - - resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType)) - if resourcePolicyType == kube.KeepPolicy { + if kube.ResourcePolicyIsKeep(m.Head.Metadata.Annotations) { keep = append(keep, m) + } else { + remaining = append(remaining, m) } - } return keep, remaining } From ce4c3f51ade97b71a6eee5068d4f529d60b1272b Mon Sep 17 00:00:00 2001 From: Jacob LeGrone Date: Mon, 4 Mar 2019 10:17:37 -0500 Subject: [PATCH 2/3] test(resource-policy): verify behavior of non-standard policy types Signed-off-by: Jacob LeGrone --- pkg/kube/client_test.go | 2 +- pkg/kube/resource_policy_test.go | 72 ++++++++++++++++++++++++++++ pkg/tiller/release_server_test.go | 27 ++++++++--- pkg/tiller/release_uninstall_test.go | 5 +- 4 files changed, 97 insertions(+), 9 deletions(-) create mode 100644 pkg/kube/resource_policy_test.go diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index f601b6536..89e630bb3 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -213,7 +213,7 @@ func TestUpdate(t *testing.T) { // Test resource policy is respected actions = nil - listA.Items[2].ObjectMeta.Annotations = map[string]string{ResourcePolicyAnno: KeepPolicy} + listA.Items[2].ObjectMeta.Annotations = map[string]string{ResourcePolicyAnno: "keep"} if err := c.Update(v1.NamespaceDefault, objBody(&listA), objBody(&listB), false, false, 0, false); err != nil { t.Fatal(err) } diff --git a/pkg/kube/resource_policy_test.go b/pkg/kube/resource_policy_test.go new file mode 100644 index 000000000..de6061b48 --- /dev/null +++ b/pkg/kube/resource_policy_test.go @@ -0,0 +1,72 @@ +/* +Copyright The Helm 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 kube + +import "testing" + +func TestResourcePolicyIsKeep(t *testing.T) { + type annotations map[string]string + type testcase struct { + annotations + keep bool + } + cases := []testcase{ + {nil, false}, + { + annotations{ + "foo": "bar", + }, + false, + }, + { + annotations{ + ResourcePolicyAnno: "keep", + }, + true, + }, + { + annotations{ + ResourcePolicyAnno: "KEEP ", + }, + true, + }, + { + annotations{ + ResourcePolicyAnno: "", + }, + true, + }, + { + annotations{ + ResourcePolicyAnno: "delete", + }, + false, + }, + { + annotations{ + ResourcePolicyAnno: "DELETE", + }, + true, + }, + } + + for _, tc := range cases { + if tc.keep != ResourcePolicyIsKeep(tc.annotations) { + t.Errorf("Expected function to return %t for annotations %v", tc.keep, tc.annotations) + } + } +} diff --git a/pkg/tiller/release_server_test.go b/pkg/tiller/release_server_test.go index a1502e8a2..4e29e4413 100644 --- a/pkg/tiller/release_server_test.go +++ b/pkg/tiller/release_server_test.go @@ -89,13 +89,22 @@ spec: var manifestWithKeep = `kind: ConfigMap metadata: - name: test-cm-keep + name: test-cm-keep-a annotations: "helm.sh/resource-policy": keep data: name: value ` +var manifestWithKeepEmpty = `kind: ConfigMap +metadata: + name: test-cm-keep-b + annotations: + "helm.sh/resource-policy": "" +data: + name: value +` + var manifestWithUpgradeHooks = `kind: ConfigMap metadata: name: test-cm @@ -449,23 +458,27 @@ func releaseWithKeepStub(rlsName string) *release.Release { Name: "bunnychart", }, Templates: []*chart.Template{ - {Name: "templates/configmap", Data: []byte(manifestWithKeep)}, + {Name: "templates/configmap-keep-a", Data: []byte(manifestWithKeep)}, + {Name: "templates/configmap-keep-b", Data: []byte(manifestWithKeepEmpty)}, }, } date := timestamp.Timestamp{Seconds: 242085845, Nanos: 0} - return &release.Release{ + rl := &release.Release{ Name: rlsName, Info: &release.Info{ FirstDeployed: &date, LastDeployed: &date, Status: &release.Status{Code: release.Status_DEPLOYED}, }, - Chart: ch, - Config: &chart.Config{Raw: `name: value`}, - Version: 1, - Manifest: manifestWithKeep, + Chart: ch, + Config: &chart.Config{Raw: `name: value`}, + Version: 1, } + + helm.RenderReleaseMock(rl, false) + + return rl } func MockEnvironment() *environment.Environment { diff --git a/pkg/tiller/release_uninstall_test.go b/pkg/tiller/release_uninstall_test.go index cb59b6bf5..d95a52c4d 100644 --- a/pkg/tiller/release_uninstall_test.go +++ b/pkg/tiller/release_uninstall_test.go @@ -150,7 +150,10 @@ func TestUninstallReleaseWithKeepPolicy(t *testing.T) { if res.Info == "" { t.Errorf("Expected response info to not be empty") } else { - if !strings.Contains(res.Info, "[ConfigMap] test-cm-keep") { + if !strings.Contains(res.Info, "[ConfigMap] test-cm-keep-a") { + t.Errorf("unexpected output: %s", res.Info) + } + if !strings.Contains(res.Info, "[ConfigMap] test-cm-keep-b") { t.Errorf("unexpected output: %s", res.Info) } } From e06c605b88d38c369a52dc86a2fa9890ea1aab66 Mon Sep 17 00:00:00 2001 From: Jacob LeGrone Date: Mon, 4 Mar 2019 10:18:26 -0500 Subject: [PATCH 3/3] docs(resource-policy): explain "delete" policy type Signed-off-by: Jacob LeGrone --- docs/charts_tips_and_tricks.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/charts_tips_and_tricks.md b/docs/charts_tips_and_tricks.md index d988184c5..fd7a08f7e 100644 --- a/docs/charts_tips_and_tricks.md +++ b/docs/charts_tips_and_tricks.md @@ -235,6 +235,9 @@ orphaned. Helm will no longer manage it in any way. This can lead to problems if using `helm install --replace` on a release that has already been deleted, but has kept resources. +To explicitly opt in to resource deletion, for example when overriding a chart's +default annotations, set the resource policy annotation value to `delete`. + ## Using "Partials" and Template Includes Sometimes you want to create some reusable parts in your chart, whether