Skip to content

Commit fcd6873

Browse files
p-sethardeck
p-se
and
thardeck
authored
[v0.14] Prevent false drift from null field rendering (#4997)
* Prevent false drift from Helm v4 null field rendering (#4664) * Prevent false drift from Helm v4 null field rendering Helm v4 can emit explicit null values for omitted fields (e.g., spec.strategy.rollingUpdate: null), while Helm v3 omitted these fields entirely. When combined with Kubernetes server-side defaulting, this creates false drift detection: the merge patch interprets null as "delete this field," conflicting with defaulted live resources. Solution: - Add normalizeNullPatch to recursively strip null-valued fields from merge patches before drift comparison - Handle nested structures (maps and arrays) to cover all potential Helm v4 null emissions beyond just deployment strategy * Add fast-path optimization for null patch normalization in diff.go Add bytes.Contains fast-path check before expensive unmarshal/walk/ marshal operations when patch doesn't contain 'null'. This provides ~55x performance improvement for patches without nulls: - WithoutNulls: 127.9 ns/op (vs 7063 ns/op with nulls) - 97% fewer allocations (1 vs 72 allocs/op) - 97% less memory (176 B vs 5404 B per op) Also add benchmark tests and fast-path test coverage. (cherry picked from commit 7a7732f) Refers to #4969 * Wire normalizeActual call site missing from cherry-pick --------- Co-authored-by: Tim Hardeck <thardeck@suse.com>
1 parent 6a4b3a5 commit fcd6873

7 files changed

Lines changed: 675 additions & 0 deletions

File tree

‎e2e/assets/gitrepo/helm-recreate-drift-gitrepo.yaml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
kind: GitRepo
2+
apiVersion: fleet.cattle.io/v1alpha1
3+
metadata:
4+
name: {{ .Name }}
5+
spec:
6+
repo: {{ .Repo }}
7+
branch: {{ .Branch }}
8+
clientSecretName: git-auth
9+
paths:
10+
- helm-recreate-drift

‎e2e/assets/helm-recreate-drift/chart/Chart.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
apiVersion: v2
2+
name: helm-v4-drift-test-chart
3+
description: Test chart with RollingUpdate deployment strategy for Helm v4 drift detection
4+
type: application
5+
version: 0.1.0
6+
appVersion: "1.0.0"

‎e2e/assets/helm-recreate-drift/chart/templates/deployment.yaml‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
apiVersion: apps/v1
2+
kind: Deployment
3+
metadata:
4+
name: test-deployment
5+
spec:
6+
replicas: {{ .Values.replicas | default 1 }}
7+
selector:
8+
matchLabels:
9+
app: test-app
10+
strategy:
11+
type: RollingUpdate
12+
template:
13+
metadata:
14+
labels:
15+
app: test-app
16+
spec:
17+
containers:
18+
- name: nginx
19+
image: nginx:1.14.2
20+
ports:
21+
- containerPort: 80

‎e2e/assets/helm-recreate-drift/fleet.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
namespace: helm-recreate-drift-test
2+
3+
helm:
4+
chart: chart
5+
releaseName: recreate-strategy-test
6+
values:
7+
replicas: 1

‎e2e/single-cluster/helm_recreate_drift_test.go‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
package singlecluster_test
2+
3+
import (
4+
"fmt"
5+
"math/rand"
6+
"os"
7+
"path"
8+
"strings"
9+
10+
. "github.com/onsi/ginkgo/v2"
11+
. "github.com/onsi/gomega"
12+
13+
"github.com/rancher/fleet/e2e/testenv"
14+
"github.com/rancher/fleet/e2e/testenv/githelper"
15+
"github.com/rancher/fleet/e2e/testenv/kubectl"
16+
)
17+
18+
var _ = Describe("Helm v4 Null Field Drift Detection", Label("infra-setup"), func() {
19+
var (
20+
tmpDir string
21+
clonedir string
22+
k kubectl.Command
23+
gh *githelper.Git
24+
localRepoName string
25+
inClusterRepoURL string
26+
gitrepoName string
27+
r = rand.New(rand.NewSource(GinkgoRandomSeed()))
28+
gitServerPort int
29+
gitProtocol string
30+
)
31+
32+
BeforeEach(func() {
33+
k = env.Kubectl.Namespace(env.Namespace)
34+
gitServerPort = 8080
35+
gitProtocol = "http"
36+
localRepoName = "repo"
37+
})
38+
39+
JustBeforeEach(func() {
40+
// Build git repo URL reachable _within_ the cluster, for the GitRepo
41+
host := githelper.BuildGitHostname()
42+
43+
addr, err := githelper.GetExternalRepoAddr(env, gitServerPort, localRepoName)
44+
Expect(err).ToNot(HaveOccurred())
45+
addr = strings.Replace(addr, "http://", fmt.Sprintf("%s://", gitProtocol), 1)
46+
gh = githelper.NewHTTP(addr)
47+
48+
inClusterRepoURL = gh.GetInClusterURL(host, gitServerPort, localRepoName)
49+
50+
tmpDir, _ = os.MkdirTemp("", "fleet-")
51+
clonedir = path.Join(tmpDir, localRepoName)
52+
53+
gitrepoName = testenv.RandomFilename("helm-recreate-drift", r)
54+
})
55+
56+
AfterEach(func() {
57+
_ = os.RemoveAll(tmpDir)
58+
59+
_, _ = k.Delete("gitrepo", gitrepoName)
60+
_, _ = k.Delete("secret", "git-auth")
61+
_, _ = k.Delete("namespace", "helm-recreate-drift-test")
62+
63+
// Check that the bundle deployment resource has been deleted
64+
Eventually(func(g Gomega) {
65+
out, _ := k.Get(
66+
"bundledeployments",
67+
"-A",
68+
"-l",
69+
fmt.Sprintf("fleet.cattle.io/repo-name=%s", gitrepoName),
70+
)
71+
g.Expect(out).To(ContainSubstring("No resources found"))
72+
}).Should(Succeed())
73+
})
74+
75+
When("deploying a Helm chart with fields omitted by Helm v4", func() {
76+
JustBeforeEach(func() {
77+
// Create git auth secret
78+
_, err := k.Create(
79+
"secret", "generic", "git-auth", "--type", "kubernetes.io/basic-auth",
80+
"--from-literal=username="+os.Getenv("GIT_HTTP_USER"),
81+
"--from-literal=password="+os.Getenv("GIT_HTTP_PASSWORD"),
82+
)
83+
Expect(err).ToNot(HaveOccurred())
84+
85+
err = testenv.ApplyTemplate(k, testenv.AssetPath("gitrepo/helm-recreate-drift-gitrepo.yaml"), struct {
86+
Name string
87+
Repo string
88+
Branch string
89+
}{
90+
Name: gitrepoName,
91+
Repo: inClusterRepoURL,
92+
Branch: gh.Branch,
93+
})
94+
Expect(err).ToNot(HaveOccurred())
95+
96+
_, err = gh.Create(clonedir, testenv.AssetPath("helm-recreate-drift"), "helm-recreate-drift")
97+
Expect(err).ToNot(HaveOccurred())
98+
})
99+
100+
It("should not detect drift from Helm v4 rendering", func() {
101+
By("checking the bundle is ready")
102+
Eventually(func(g Gomega) {
103+
out, err := k.Get("bundles", "-A", "-l", fmt.Sprintf("fleet.cattle.io/repo-name=%s", gitrepoName))
104+
g.Expect(err).ToNot(HaveOccurred())
105+
g.Expect(out).To(ContainSubstring("fleet-local"))
106+
}, testenv.MediumTimeout, testenv.ShortTimeout).Should(Succeed())
107+
108+
By("checking the bundle deployment is ready")
109+
var bundleDeploymentName, bdNamespace string
110+
Eventually(func(g Gomega) {
111+
out, err := k.Get("bundledeployments", "-A", "-o", "jsonpath={.items[0]['metadata.name','metadata.namespace']}", "-l", fmt.Sprintf("fleet.cattle.io/repo-name=%s", gitrepoName))
112+
g.Expect(err).ToNot(HaveOccurred())
113+
g.Expect(out).ToNot(BeEmpty())
114+
115+
data := strings.Split(out, " ")
116+
g.Expect(data).To(HaveLen(2))
117+
bundleDeploymentName, bdNamespace = data[0], data[1]
118+
}, testenv.MediumTimeout, testenv.ShortTimeout).Should(Succeed())
119+
120+
Eventually(func(g Gomega) {
121+
out, err := k.Namespace(bdNamespace).Get("bundledeployments", bundleDeploymentName, "-o", "jsonpath={.status.ready}")
122+
g.Expect(err).ToNot(HaveOccurred())
123+
g.Expect(out).To(Equal("true"))
124+
}, testenv.MediumTimeout, testenv.ShortTimeout).Should(Succeed())
125+
126+
By("checking the deployment has RollingUpdate strategy")
127+
Eventually(func(g Gomega) {
128+
out, err := k.Namespace("helm-recreate-drift-test").Get("deployments", "test-deployment", "-o", "jsonpath={.spec.strategy.type}")
129+
g.Expect(err).ToNot(HaveOccurred())
130+
g.Expect(out).To(Equal("RollingUpdate"))
131+
}, testenv.MediumTimeout, testenv.ShortTimeout).Should(Succeed())
132+
133+
By("verifying that no drift is detected")
134+
Eventually(func(g Gomega) {
135+
out, err := k.Namespace(bdNamespace).Get("bundledeployments", bundleDeploymentName, "-o", "jsonpath={.status.nonModified}")
136+
g.Expect(err).ToNot(HaveOccurred())
137+
g.Expect(out).To(Equal("true"), "BundleDeployment should not show drift from Helm v4 rendering")
138+
139+
// Also check that modifiedStatus is empty
140+
out, err = k.Namespace(bdNamespace).Get("bundledeployments", bundleDeploymentName, "-o", "jsonpath={.status.modifiedStatus}")
141+
g.Expect(err).ToNot(HaveOccurred())
142+
g.Expect(out).To(BeEmpty(), "modifiedStatus should be empty when no drift is detected")
143+
}, testenv.MediumTimeout, testenv.ShortTimeout).Should(Succeed())
144+
})
145+
})
146+
})

0 commit comments

Comments
 (0)