Skip to content

Commit

Permalink
an error will be returned by workloadspread webhook when `getObjectOf…
Browse files Browse the repository at this point in the history
…` pod's owner failed; prevent WorkloadSpread e2e panic

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
  • Loading branch information
AiRanthem committed Oct 29, 2024
1 parent cba1c8a commit cb3007a
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 59 deletions.
23 changes: 14 additions & 9 deletions pkg/util/workloadspread/workloadspread.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,16 @@ func (h *Handler) HandlePodCreation(pod *corev1.Pod) (skip bool, err error) {
continue
}
// determine if the reference of workloadSpread and pod is equal
if h.isReferenceEqual(ws.Spec.TargetReference, ref, pod.Namespace) {
if ok, err := h.isReferenceEqual(ws.Spec.TargetReference, ref, pod.Namespace); ok {
matchedWS = &ws
// pod has at most one matched workloadSpread
break
} else if err != nil {
klog.ErrorS(err, "failed to determine whether workloadspread refers pod's owner")
if errors.IsNotFound(err) {
klog.ErrorS(err, "pod owner not found")
return true, err
}

Check warning on line 236 in pkg/util/workloadspread/workloadspread.go

View check run for this annotation

Codecov / codecov/patch

pkg/util/workloadspread/workloadspread.go#L232-L236

Added lines #L232 - L236 were not covered by tests
}
}
// not found matched workloadSpread
Expand Down Expand Up @@ -687,35 +693,34 @@ func (h *Handler) getSuitableSubset(subsetStatuses []appsv1alpha1.WorkloadSpread
return nil
}

func (h *Handler) isReferenceEqual(target *appsv1alpha1.TargetReference, owner *metav1.OwnerReference, namespace string) bool {
func (h *Handler) isReferenceEqual(target *appsv1alpha1.TargetReference, owner *metav1.OwnerReference, namespace string) (bool, error) {
if owner == nil {
return false
return false, nil
}

targetGv, err := schema.ParseGroupVersion(target.APIVersion)
if err != nil {
klog.ErrorS(err, "parse TargetReference apiVersion failed", "apiVersion", target.APIVersion)
return false
return false, err

Check warning on line 704 in pkg/util/workloadspread/workloadspread.go

View check run for this annotation

Codecov / codecov/patch

pkg/util/workloadspread/workloadspread.go#L704

Added line #L704 was not covered by tests
}

ownerGv, err := schema.ParseGroupVersion(owner.APIVersion)
if err != nil {
klog.ErrorS(err, "parse OwnerReference apiVersion failed", "apiVersion", owner.APIVersion)
return false
return false, err

Check warning on line 710 in pkg/util/workloadspread/workloadspread.go

View check run for this annotation

Codecov / codecov/patch

pkg/util/workloadspread/workloadspread.go#L710

Added line #L710 was not covered by tests
}

if targetGv.Group == ownerGv.Group && target.Kind == owner.Kind && target.Name == owner.Name {
return true
return true, nil
}

if match, err := matchReference(owner); err != nil || !match {
return false
return false, err
}

ownerObject, err := h.getObjectOf(owner, namespace)
if err != nil {
klog.ErrorS(err, "Failed to get owner object", "owner", owner)
return false
return false, err
}

return h.isReferenceEqual(target, metav1.GetControllerOfNoCopy(ownerObject), namespace)
Expand Down
13 changes: 10 additions & 3 deletions pkg/util/workloadspread/workloadspread_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ import (
var (
scheme *runtime.Scheme

cloneSetDemo = &appsv1alpha1.CloneSet{
ObjectMeta: metav1.ObjectMeta{
Name: "cloneset-test",
Namespace: "default",
},
}

podDemo = &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test-pod-1",
Expand Down Expand Up @@ -1088,7 +1095,7 @@ func TestWorkloadSpreadMutatingPod(t *testing.T) {
expectWS := cs.expectWorkloadSpread()
podExpect := cs.expectPod()
fakeClient := fake.NewClientBuilder().WithScheme(scheme).
WithObjects(workloadSpreadIn).WithStatusSubresource(&appsv1alpha1.WorkloadSpread{}).Build()
WithObjects(workloadSpreadIn, cloneSetDemo).WithStatusSubresource(&appsv1alpha1.WorkloadSpread{}).Build()
handler := NewWorkloadSpreadHandler(fakeClient)

var err error
Expand Down Expand Up @@ -1260,7 +1267,7 @@ func TestIsReferenceEqual(t *testing.T) {
for _, cs := range cases {
t.Run(cs.name, func(t *testing.T) {
h := Handler{fake.NewClientBuilder().Build()}
if h.isReferenceEqual(cs.getTargetRef(), cs.getOwnerRef(), "") != cs.expectEqual {
if ok, _ := h.isReferenceEqual(cs.getTargetRef(), cs.getOwnerRef(), ""); ok != cs.expectEqual {
t.Fatalf("isReferenceEqual failed")
}
})
Expand Down Expand Up @@ -1400,7 +1407,7 @@ func TestIsReferenceEqual2(t *testing.T) {
handler := &Handler{Client: cli}
workloadsInWhiteListInitialized = false
initializeWorkloadsInWhiteList(cli)
result := handler.isReferenceEqual(&ref, metav1.GetControllerOf(pod), pod.GetNamespace())
result, _ := handler.isReferenceEqual(&ref, metav1.GetControllerOf(pod), pod.GetNamespace())
if result != cs.Expect {
t.Fatalf("got unexpected result")
}
Expand Down
Loading

0 comments on commit cb3007a

Please sign in to comment.