Skip to content

fix: (helm) - do not add owner references to resources that contain the Helm keep resource-policy annotation#4389

Merged
joelanford merged 3 commits intooperator-framework:masterfrom
mikeshng:fix-helm-keep
Jan 13, 2021
Merged

fix: (helm) - do not add owner references to resources that contain the Helm keep resource-policy annotation#4389
joelanford merged 3 commits intooperator-framework:masterfrom
mikeshng:fix-helm-keep

Conversation

@mikeshng
Copy link
Copy Markdown
Contributor

Signed-off-by: Mike Ng ming@redhat.com

Description of the change:
Helm operator: updated the ownerRefInjectingClient to not inject the owner references if it contains the annotation:

  annotations:
    "helm.sh/resource-policy": keep

see https://helm.sh/docs/howto/charts_tips_and_tricks/ for more details about the keep annotation.

Motivation for the change:
For the Helm operator delete CR, namespace scope resources with the Helm keep annotation are being GC'ed by Kubernetes.
This change is to make sure those resources are not owned by the CR so they won't get GC'ed.

Closes: #4378

After this change, I can no longer reproduces the issue described in #4378

Checklist

If the pull request includes user-facing changes, extra documentation is required:

…he Helm keep resource-policy annotation

Signed-off-by: Mike Ng <ming@redhat.com>
@mikeshng
Copy link
Copy Markdown
Contributor Author

I followed the helm repo on how to check if the keep annotation exists.
https://github.com/helm/helm/blob/v3.4.2/pkg/action/resource_policy.go#L33-L40

Comment thread internal/helm/client/client.go Outdated
Comment on lines +151 to +160
containsKeepAnno := false
if u.GetAnnotations() != nil {
resourcePolicyType, ok := u.GetAnnotations()[kube.ResourcePolicyAnno]
if ok {
resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType))
containsKeepAnno = resourcePolicyType == kube.KeepPolicy
}
}

if useOwnerRef && !containsKeepAnno {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you refactor this into a helper

Suggested change
containsKeepAnno := false
if u.GetAnnotations() != nil {
resourcePolicyType, ok := u.GetAnnotations()[kube.ResourcePolicyAnno]
if ok {
resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType))
containsKeepAnno = resourcePolicyType == kube.KeepPolicy
}
}
if useOwnerRef && !containsKeepAnno {
if useOwnerRef && !containsResourcePolicyKeep(u.GetAnnotations()) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as suggested and added unit tests.

Comment thread internal/helm/client/client.go Outdated
Comment on lines +168 to +176
if annotations != nil {
resourcePolicyType, ok := annotations[kube.ResourcePolicyAnno]
if ok {
resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType))
return resourcePolicyType == kube.KeepPolicy
}
}

return false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, but in order to minimize indentation, could you refactor to this?

Suggested change
if annotations != nil {
resourcePolicyType, ok := annotations[kube.ResourcePolicyAnno]
if ok {
resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType))
return resourcePolicyType == kube.KeepPolicy
}
}
return false
if annotations == nil {
return false
}
resourcePolicyType, ok := annotations[kube.ResourcePolicyAnno]
if !ok {
return false
}
resourcePolicyType = strings.ToLower(strings.TrimSpace(resourcePolicyType))
return resourcePolicyType == kube.KeepPolicy

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as suggested. Locally tested again and it seems fine.

Signed-off-by: Mike Ng <ming@redhat.com>
Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021
@joelanford
Copy link
Copy Markdown
Member

/lgtm

@joelanford joelanford merged commit ca390f1 into operator-framework:master Jan 13, 2021
@mikeshng mikeshng deleted the fix-helm-keep branch January 13, 2021 19:02
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 4, 2021
…he Helm keep resource-policy annotation (operator-framework#4389)

Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…he Helm keep resource-policy annotation (operator-framework#4389)

Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…he Helm keep resource-policy annotation (operator-framework#4389)

Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…he Helm keep resource-policy annotation (operator-framework#4389)

Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helm operator: namespace scope resources with Helm keep annotations are being deleted by garbage collection

4 participants