Skip to content

Commit 4dbfa5c

Browse files
k8s-infra-cherrypick-robotjosvazgsbueringer
authored
[release-0.23] 🐛 Fix fake client's SSA status patch resource version check (#3446)
* Add unit test to expose fake client bug Signed-off-by: jose.vazquez <jose.vazquez@mongodb.com> * Simplify reproducer test * Fix accessor to match old rv for subresources as well Signed-off-by: jose.vazquez <jose.vazquez@mongodb.com> * Also test that passing the wrong RV fails with expected error Signed-off-by: jose.vazquez <jose.vazquez@mongodb.com> * Simplify and abbrevaite fix * Update pkg/client/fake/client_test.go Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> * Update pkg/client/fake/client_test.go Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> --------- Signed-off-by: jose.vazquez <jose.vazquez@mongodb.com> Co-authored-by: jose.vazquez <jose.vazquez@mongodb.com> Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com>
1 parent f52bbb8 commit 4dbfa5c

2 files changed

Lines changed: 126 additions & 4 deletions

File tree

‎pkg/client/fake/client_test.go‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1847,6 +1847,94 @@ var _ = Describe("Fake client", func() {
18471847
Expect(cl.Status().Apply(ctx, node, client.FieldOwner("test-owner"))).To(Succeed())
18481848
})
18491849

1850+
It("should allow SSA apply on status without object has changed issues", func(ctx SpecContext) {
1851+
schemeBuilder := &scheme.Builder{GroupVersion: schema.GroupVersion{Group: "chaosapps.metamagical.io", Version: "v1"}}
1852+
schemeBuilder.Register(&ChaosPod{})
1853+
testScheme := runtime.NewScheme()
1854+
Expect(schemeBuilder.AddToScheme(testScheme)).NotTo(HaveOccurred())
1855+
1856+
customResource := &ChaosPod{
1857+
TypeMeta: metav1.TypeMeta{
1858+
APIVersion: "chaosapps.metamagical.io/v1",
1859+
Kind: "ChaosPod",
1860+
},
1861+
ObjectMeta: metav1.ObjectMeta{
1862+
Name: "test-chaospod",
1863+
Namespace: "default",
1864+
},
1865+
Spec: ChaosPodSpec{
1866+
Image: "test-image",
1867+
},
1868+
}
1869+
cl := NewClientBuilder().WithScheme(testScheme).WithStatusSubresource(customResource).WithObjects(customResource).Build()
1870+
1871+
// Create an unstructured apply configuration with status but no resourceVersion
1872+
resourceForApply := &unstructured.Unstructured{}
1873+
resourceForApply.SetName("test-chaospod")
1874+
resourceForApply.SetNamespace("default")
1875+
resourceForApply.SetAPIVersion("chaosapps.metamagical.io/v1")
1876+
resourceForApply.SetKind("ChaosPod")
1877+
resourceForApply.SetResourceVersion("")
1878+
Expect(unstructured.SetNestedField(resourceForApply.Object, map[string]any{"phase": "Ready"}, "status")).To(Succeed())
1879+
1880+
resourceAC := client.ApplyConfigurationFromUnstructured(resourceForApply)
1881+
1882+
err := cl.Status().Apply(ctx, resourceAC, client.FieldOwner("test-owner"), client.ForceOwnership)
1883+
Expect(err).NotTo(HaveOccurred(), "SSA apply on status should succeed when resourceVersion is not set")
1884+
1885+
// Verify the status was applied
1886+
finalResource := &ChaosPod{
1887+
TypeMeta: metav1.TypeMeta{
1888+
APIVersion: "chaosapps.metamagical.io/v1",
1889+
Kind: "ChaosPod",
1890+
},
1891+
ObjectMeta: metav1.ObjectMeta{
1892+
Name: "test-chaospod",
1893+
Namespace: "default",
1894+
},
1895+
}
1896+
Expect(cl.Get(ctx, client.ObjectKeyFromObject(finalResource), finalResource)).To(Succeed())
1897+
Expect(finalResource.Status.Phase).To(Equal("Ready"))
1898+
})
1899+
1900+
It("should block SSA apply on status when passing the wrong non empty resource version", func(ctx SpecContext) {
1901+
schemeBuilder := &scheme.Builder{GroupVersion: schema.GroupVersion{Group: "chaosapps.metamagical.io", Version: "v1"}}
1902+
schemeBuilder.Register(&ChaosPod{})
1903+
testScheme := runtime.NewScheme()
1904+
Expect(schemeBuilder.AddToScheme(testScheme)).NotTo(HaveOccurred())
1905+
1906+
customResource := &ChaosPod{
1907+
TypeMeta: metav1.TypeMeta{
1908+
APIVersion: "chaosapps.metamagical.io/v1",
1909+
Kind: "ChaosPod",
1910+
},
1911+
ObjectMeta: metav1.ObjectMeta{
1912+
Name: "test-chaospod",
1913+
Namespace: "default",
1914+
},
1915+
Spec: ChaosPodSpec{
1916+
Image: "test-image",
1917+
},
1918+
}
1919+
cl := NewClientBuilder().WithScheme(testScheme).WithStatusSubresource(customResource).WithObjects(customResource).Build()
1920+
1921+
// Create an unstructured apply configuration with status but wrong non empty resourceVersion
1922+
resourceForApply := &unstructured.Unstructured{}
1923+
resourceForApply.SetName("test-chaospod")
1924+
resourceForApply.SetNamespace("default")
1925+
resourceForApply.SetAPIVersion("chaosapps.metamagical.io/v1")
1926+
resourceForApply.SetKind("ChaosPod")
1927+
resourceForApply.SetResourceVersion("bad-resource-version") // forces the error
1928+
Expect(unstructured.SetNestedField(resourceForApply.Object, map[string]any{"phase": "Ready"}, "status")).To(Succeed())
1929+
1930+
resourceAC := client.ApplyConfigurationFromUnstructured(resourceForApply)
1931+
1932+
// This is expected to fail with the wrong rv value passed in in the applied config
1933+
err := cl.Status().Apply(ctx, resourceAC, client.FieldOwner("test-owner"), client.ForceOwnership)
1934+
Expect(err).To(HaveOccurred(), "SSA apply on status should not succeed when resourceVersion is wrongly set")
1935+
Expect(apierrors.IsConflict(err)).To(BeTrue())
1936+
})
1937+
18501938
It("should not be able to manually update the fieldManager through a status update", func(ctx SpecContext) {
18511939
node := &corev1.Node{
18521940
ObjectMeta: metav1.ObjectMeta{
@@ -3442,3 +3530,35 @@ func (in *EmbeddedPointerStructCRD) DeepCopyObject() runtime.Object {
34423530

34433531
return &out
34443532
}
3533+
3534+
// ChaosPod is a custom resource type used for testing SSA apply operations
3535+
// on custom resources with status subresources.
3536+
type ChaosPod struct {
3537+
metav1.TypeMeta `json:",inline"`
3538+
metav1.ObjectMeta `json:"metadata,omitempty"`
3539+
3540+
Spec ChaosPodSpec `json:"spec,omitempty"`
3541+
Status ChaosPodStatus `json:"status,omitempty"`
3542+
}
3543+
3544+
// ChaosPodSpec defines the spec for ChaosPod
3545+
type ChaosPodSpec struct {
3546+
Image string `json:"image,omitempty"`
3547+
}
3548+
3549+
// ChaosPodStatus defines the status for ChaosPod
3550+
type ChaosPodStatus struct {
3551+
Phase string `json:"phase,omitempty"`
3552+
}
3553+
3554+
func (in *ChaosPod) DeepCopyObject() runtime.Object {
3555+
if in == nil {
3556+
return nil
3557+
}
3558+
out := &ChaosPod{}
3559+
out.TypeMeta = in.TypeMeta
3560+
out.ObjectMeta = *in.ObjectMeta.DeepCopy()
3561+
out.Spec = in.Spec
3562+
out.Status = in.Status
3563+
return out
3564+
}

‎pkg/client/fake/versioned_tracker.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -267,13 +267,15 @@ func (t versionedTracker) updateObject(
267267
// apiserver accepts such a patch, but it does so we just copy that behavior.
268268
// Kubernetes apiserver behavior can be checked like this:
269269
// `kubectl patch configmap foo --patch '{"metadata":{"annotations":{"foo":"bar"},"resourceVersion":null}}' -v=9`
270-
case bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Patch")):
270+
case bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Patch")),
271+
bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).Patch")):
271272
// We apply patches using a client-go reaction that ends up calling the trackers Update. As we can't change
272-
// that reaction, we use the callstack to figure out if this originated from the "fakeClient.Patch" func.
273+
// that reaction, we use the callstack to figure out if this originated from the "fake[SubResource]Client.Patch" func.
273274
accessor.SetResourceVersion(oldAccessor.GetResourceVersion())
274-
case bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Apply")):
275+
case bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Apply")),
276+
bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).Apply")):
275277
// We apply patches using a client-go reaction that ends up calling the trackers Update. As we can't change
276-
// that reaction, we use the callstack to figure out if this originated from the "fakeClient.Apply" func.
278+
// that reaction, we use the callstack to figure out if this originated from the "fake[SubResource]Client.Apply" func.
277279
accessor.SetResourceVersion(oldAccessor.GetResourceVersion())
278280
}
279281
}

0 commit comments

Comments
 (0)