Repository: helm/helm
Pull request #12581 (2023-11-20 17:34:05)

Consider full GroupVersionKind when matching resources
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/main/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:
This change shall take Group, Version and Kind from GroupVersionKind into consideration instead of the current behavior of only considering the Kind when matching resources.
This is needed as resources with different apiVersion are being considered the same.

Closes: #12578


**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains documentation
- [ ] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility


jnonino: Hi, the comment [#2976 (comment](https://github.com/helm/helm/pull/2976#issuecomment-334206483) makes sense, the issue is that when you upgrade a chart that have resources with API group and/or version changes the old resource is not removed but it is not managed by Helm anymore. 

The old resource is left orphaned in Kubernetes, we found that when we upgraded Traefik chart and they changed the API group from `traefik.containo.us` to `traefik.io`, at some point we started seeing weird errors and they were caused by orphaned IngressRoutes that were still active with the old API group.

So, I get that if the API group/version is different Helm mus recognise them as different resources, but if a resource was being managed by Helm and in an upgrade operation a resource is not there anymore because it was replaced with a new resource with new API group/version, Helm should do something with the old resource it was managing, right now it just leave it orphaned as it was never part of the chart.
MichaelMorrisEst: I'm starting to think my solution is wrong. 
Using 'kubectl apply' with a changed value for apiVersion considers it the same resource as the already deployed resource regardless of the fact the apiVersions are different.
Helm should probably be consistent with this behaviour.

Currently helm will check for an existing resource on the cluster with the same name, namespace and apiVersion as given in the template, and not finding it considers the resource as a new resource and creates it. This, I believe, is where the logic is wrong - it should instead look for a resource on the cluster using the original apiVersion rather than the new apiVersion. This way it will find the original resource and can upgrade then upgrade it.

I will look into the code changes that would be needed to achieve this
MichaelMorrisEst: I believe the solution of considering resources to be different if the apiVersion is different is consistent with the behaviour of 'kubectl apply', 'kubectl diff'
The change proposed results in the old resource that was being left orphaned, now being deleted
jnonino: For our use case that would be the expected behaviour, the old resource being deleted rather that left orphaned. You can always know if something like is going to happen by running the command with dry run and analyse the result.
gjenkins8: I label this as Helm v4, so at least it won't get forgotten. IMHO it probably should be merged into Helm v3 as a bug fix, but that needs maintainer attention/thought
gjenkins8: @MichaelMorrisEst -- could you please e.g. rebase to kick CI off (again). We should merge this for Helm 4

Patch:
diff --git a/pkg/kube/resource.go b/pkg/kube/resource.go
index 600f256b3..d88b171f0 100644
--- a/pkg/kube/resource.go
+++ b/pkg/kube/resource.go
@@ -81,5 +81,5 @@ func (r ResourceList) Intersect(rs ResourceList) ResourceList {
 
 // isMatchingInfo returns true if infos match on Name and GroupVersionKind.
 func isMatchingInfo(a, b *resource.Info) bool {
-	return a.Name == b.Name && a.Namespace == b.Namespace && a.Mapping.GroupVersionKind.Kind == b.Mapping.GroupVersionKind.Kind && a.Mapping.GroupVersionKind.Group == b.Mapping.GroupVersionKind.Group
+	return a.Name == b.Name && a.Namespace == b.Namespace && a.Mapping.GroupVersionKind == b.Mapping.GroupVersionKind
 }


Test patch:
diff --git a/pkg/kube/resource_test.go b/pkg/kube/resource_test.go
index c405ca382..ccc613c1b 100644
--- a/pkg/kube/resource_test.go
+++ b/pkg/kube/resource_test.go
@@ -59,3 +59,42 @@ func TestResourceList(t *testing.T) {
 		t.Error("expected intersect to return bar")
 	}
 }
+
+func TestIsMatchingInfo(t *testing.T) {
+	gvk := schema.GroupVersionKind{Group: "group1", Version: "version1", Kind: "pod"}
+	resourceInfo := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvk}}
+
+	gvkDiffGroup := schema.GroupVersionKind{Group: "diff", Version: "version1", Kind: "pod"}
+	resourceInfoDiffGroup := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkDiffGroup}}
+	if isMatchingInfo(&resourceInfo, &resourceInfoDiffGroup) {
+		t.Error("expected resources not equal")
+	}
+
+	gvkDiffVersion := schema.GroupVersionKind{Group: "group1", Version: "diff", Kind: "pod"}
+	resourceInfoDiffVersion := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkDiffVersion}}
+	if isMatchingInfo(&resourceInfo, &resourceInfoDiffVersion) {
+		t.Error("expected resources not equal")
+	}
+
+	gvkDiffKind := schema.GroupVersionKind{Group: "group1", Version: "version1", Kind: "deployment"}
+	resourceInfoDiffKind := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkDiffKind}}
+	if isMatchingInfo(&resourceInfo, &resourceInfoDiffKind) {
+		t.Error("expected resources not equal")
+	}
+
+	resourceInfoDiffName := resource.Info{Name: "diff", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvk}}
+	if isMatchingInfo(&resourceInfo, &resourceInfoDiffName) {
+		t.Error("expected resources not equal")
+	}
+
+	resourceInfoDiffNamespace := resource.Info{Name: "name1", Namespace: "diff", Mapping: &meta.RESTMapping{GroupVersionKind: gvk}}
+	if isMatchingInfo(&resourceInfo, &resourceInfoDiffNamespace) {
+		t.Error("expected resources not equal")
+	}
+
+	gvkEqual := schema.GroupVersionKind{Group: "group1", Version: "version1", Kind: "pod"}
+	resourceInfoEqual := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkEqual}}
+	if !isMatchingInfo(&resourceInfo, &resourceInfoEqual) {
+		t.Error("expected resources to be equal")
+	}
+}