From 70b60f2a94e958df78a5cfe4f2e514701cfc1bca Mon Sep 17 00:00:00 2001 From: Alexander Brovikov Date: Thu, 18 Feb 2021 20:58:38 +0500 Subject: [PATCH] spread "fix: (helm) - do not add owner references to resources that contain the Helm keep resource-policy annotation #4389" to ansible-operator Signed-off-by: Alexander Brovikov --- internal/ansible/proxy/inject_owner.go | 4 +- internal/helm/client/client.go | 15 +---- internal/helm/client/client_test.go | 79 -------------------------- internal/util/k8sutil/k8sutil.go | 13 +++++ internal/util/k8sutil/k8sutil_test.go | 58 +++++++++++++++++++ 5 files changed, 75 insertions(+), 94 deletions(-) delete mode 100644 internal/helm/client/client_test.go diff --git a/internal/ansible/proxy/inject_owner.go b/internal/ansible/proxy/inject_owner.go index 867f9682e0..5960856532 100644 --- a/internal/ansible/proxy/inject_owner.go +++ b/internal/ansible/proxy/inject_owner.go @@ -143,7 +143,9 @@ func (i *injectOwnerReferenceHandler) ServeHTTP(w http.ResponseWriter, req *http http.Error(w, m, http.StatusBadRequest) return } - if addOwnerRef { + // If the resource contains the Helm resource-policy keep annotation, then do not add + // the owner reference. So when the CR is deleted, Kubernetes won't GCs the resource. + if addOwnerRef && !k8sutil.ContainsResourcePolicyKeep(data.GetAnnotations()) { data.SetOwnerReferences(append(data.GetOwnerReferences(), owner.OwnerReference)) } else { err := handler.SetOwnerAnnotations(data, ownerObject) diff --git a/internal/helm/client/client.go b/internal/helm/client/client.go index 615ad882ad..acb7d59d76 100644 --- a/internal/helm/client/client.go +++ b/internal/helm/client/client.go @@ -17,7 +17,6 @@ package client import ( "errors" "io" - "strings" "github.com/operator-framework/operator-lib/handler" "github.com/operator-framework/operator-sdk/internal/util/k8sutil" @@ -147,7 +146,7 @@ func (c *ownerRefInjectingClient) Build(reader io.Reader, validate bool) (kube.R // If the resource contains the Helm resource-policy keep annotation, then do not add // the owner reference. So when the CR is deleted, Kubernetes won't GCs the resource. - if useOwnerRef && !containsResourcePolicyKeep(u.GetAnnotations()) { + if useOwnerRef && !k8sutil.ContainsResourcePolicyKeep(u.GetAnnotations()) { ownerRef := metav1.NewControllerRef(c.owner, c.owner.GroupVersionKind()) u.SetOwnerReferences([]metav1.OwnerReference{*ownerRef}) } else { @@ -163,15 +162,3 @@ func (c *ownerRefInjectingClient) Build(reader io.Reader, validate bool) (kube.R } return resourceList, nil } - -func containsResourcePolicyKeep(annotations map[string]string) bool { - if annotations == nil { - return false - } - resourcePolicyType, ok := annotations[kube.ResourcePolicyAnno] - if !ok { - return false - } - resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType)) - return resourcePolicyType == kube.KeepPolicy -} diff --git a/internal/helm/client/client_test.go b/internal/helm/client/client_test.go deleted file mode 100644 index c223da3339..0000000000 --- a/internal/helm/client/client_test.go +++ /dev/null @@ -1,79 +0,0 @@ -// Copyright 2021 The Operator-SDK 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 client - -import ( - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "helm.sh/helm/v3/pkg/kube" -) - -func TestContainsResourcePolicyKeep(t *testing.T) { - tests := []struct { - input map[string]string - expectedVal bool - expectedOut string - name string - }{ - { - input: map[string]string{ - kube.ResourcePolicyAnno: kube.KeepPolicy, - }, - expectedVal: true, - name: "base case true", - }, - { - input: map[string]string{ - "not-" + kube.ResourcePolicyAnno: kube.KeepPolicy, - }, - expectedVal: false, - name: "base case annotation false", - }, - { - input: map[string]string{ - kube.ResourcePolicyAnno: "not-" + kube.KeepPolicy, - }, - expectedVal: false, - name: "base case value false", - }, - { - input: map[string]string{ - kube.ResourcePolicyAnno: strings.ToUpper(kube.KeepPolicy), - }, - expectedVal: true, - name: "true with upper case", - }, - { - input: map[string]string{ - kube.ResourcePolicyAnno: " " + kube.KeepPolicy + " ", - }, - expectedVal: true, - name: "true with spaces", - }, - { - input: map[string]string{ - kube.ResourcePolicyAnno: " " + strings.ToUpper(kube.KeepPolicy) + " ", - }, - expectedVal: true, - name: "true with upper case and spaces", - }, - } - - for _, test := range tests { - assert.Equal(t, test.expectedVal, containsResourcePolicyKeep(test.input), test.name) - } -} diff --git a/internal/util/k8sutil/k8sutil.go b/internal/util/k8sutil/k8sutil.go index 297fffa80a..4c259a3f0f 100644 --- a/internal/util/k8sutil/k8sutil.go +++ b/internal/util/k8sutil/k8sutil.go @@ -23,6 +23,7 @@ import ( "strings" "unicode" + "helm.sh/helm/v3/pkg/kube" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -156,3 +157,15 @@ func SupportsOwnerReference(restMapper meta.RESTMapper, owner, dependent runtime // Both owner and dependent are namespace-scoped and in the same namespace. return true, nil } + +func ContainsResourcePolicyKeep(annotations map[string]string) bool { + if annotations == nil { + return false + } + resourcePolicyType, ok := annotations[kube.ResourcePolicyAnno] + if !ok { + return false + } + resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType)) + return resourcePolicyType == kube.KeepPolicy +} diff --git a/internal/util/k8sutil/k8sutil_test.go b/internal/util/k8sutil/k8sutil_test.go index 21b6690885..726a6e6fb1 100644 --- a/internal/util/k8sutil/k8sutil_test.go +++ b/internal/util/k8sutil/k8sutil_test.go @@ -15,9 +15,11 @@ package k8sutil import ( + "strings" "testing" "github.com/stretchr/testify/assert" + "helm.sh/helm/v3/pkg/kube" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -332,3 +334,59 @@ func TestFormatOperatorNameDNS1123(t *testing.T) { }) } } + +func TestContainsResourcePolicyKeep(t *testing.T) { + tests := []struct { + input map[string]string + expectedVal bool + expectedOut string + name string + }{ + { + input: map[string]string{ + kube.ResourcePolicyAnno: kube.KeepPolicy, + }, + expectedVal: true, + name: "base case true", + }, + { + input: map[string]string{ + "not-" + kube.ResourcePolicyAnno: kube.KeepPolicy, + }, + expectedVal: false, + name: "base case annotation false", + }, + { + input: map[string]string{ + kube.ResourcePolicyAnno: "not-" + kube.KeepPolicy, + }, + expectedVal: false, + name: "base case value false", + }, + { + input: map[string]string{ + kube.ResourcePolicyAnno: strings.ToUpper(kube.KeepPolicy), + }, + expectedVal: true, + name: "true with upper case", + }, + { + input: map[string]string{ + kube.ResourcePolicyAnno: " " + kube.KeepPolicy + " ", + }, + expectedVal: true, + name: "true with spaces", + }, + { + input: map[string]string{ + kube.ResourcePolicyAnno: " " + strings.ToUpper(kube.KeepPolicy) + " ", + }, + expectedVal: true, + name: "true with upper case and spaces", + }, + } + + for _, test := range tests { + assert.Equal(t, test.expectedVal, ContainsResourcePolicyKeep(test.input), test.name) + } +}