Skip to content

Commit 6ad68a8

Browse files
y-rabiey-rabie
committed
fix and add unit test for merging ancestors of ExtensionServerPolicies
Signed-off-by: y-rabie <[email protected]>
1 parent 67a46b9 commit 6ad68a8

File tree

3 files changed

+159
-49
lines changed

3 files changed

+159
-49
lines changed

‎internal/gatewayapi/extensionserverpolicy.go‎

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ func (t *Translator) ProcessExtensionServerPolicies(policies []unstructured.Unst
8181
}
8282
if accepted {
8383
res = append(res, *policy)
84-
policy.Object["status"] = policyStatusToUnstructured(policyStatus)
84+
policy.Object["status"] = PolicyStatusToUnstructured(policyStatus)
8585
}
8686
}
8787

@@ -108,14 +108,6 @@ func extractTargetRefs(policy *unstructured.Unstructured, gateways []*GatewayCon
108108
return ret, nil
109109
}
110110

111-
func policyStatusToUnstructured(policyStatus gwapiv1.PolicyStatus) map[string]any {
112-
ret := map[string]any{}
113-
// No need to check the marshal/unmarshal error here
114-
d, _ := json.Marshal(policyStatus)
115-
_ = json.Unmarshal(d, &ret)
116-
return ret
117-
}
118-
119111
func resolveExtServerPolicyGatewayTargetRef(policy *unstructured.Unstructured, target gwapiv1.LocalPolicyTargetReferenceWithSectionName, gateways map[types.NamespacedName]*policyGatewayTargetContext) *GatewayContext {
120112
// Check if the gateway exists
121113
key := types.NamespacedName{
@@ -132,6 +124,24 @@ func resolveExtServerPolicyGatewayTargetRef(policy *unstructured.Unstructured, t
132124
return gateway.GatewayContext
133125
}
134126

127+
func PolicyStatusToUnstructured(policyStatus gwapiv1.PolicyStatus) map[string]any {
128+
ret := map[string]any{}
129+
// No need to check the marshal/unmarshal error here
130+
d, _ := json.Marshal(policyStatus)
131+
_ = json.Unmarshal(d, &ret)
132+
return ret
133+
}
134+
135+
func UnstructuredToPolicyStatus(policyStatus map[string]any) gwapiv1.PolicyStatus {
136+
var ret gwapiv1.PolicyStatus
137+
// No need to check the json marshal/unmarshal error, the policyStatus was
138+
// created via a typed object so the marshalling/unmarshalling will always
139+
// work
140+
d, _ := json.Marshal(policyStatus)
141+
_ = json.Unmarshal(d, &ret)
142+
return ret
143+
}
144+
135145
func (t *Translator) translateExtServerPolicyForGateway(
136146
policy *unstructured.Unstructured,
137147
gateway *GatewayContext,
@@ -173,3 +183,29 @@ func (t *Translator) translateExtServerPolicyForGateway(
173183
}
174184
return found
175185
}
186+
187+
// Appends status ancestors from newPolicy into aggregatedPolicy's list of ancestors.
188+
func MergeAncestorsForExtensionServerPolicies(aggregatedPolicy, newPolicy *unstructured.Unstructured) {
189+
aggStatusObj := aggregatedPolicy.Object["status"]
190+
var aggStatus gwapiv1.PolicyStatus
191+
if _, ok := aggStatusObj.(map[string]any); ok {
192+
aggStatus = UnstructuredToPolicyStatus(aggStatusObj.(map[string]any))
193+
} else if _, ok := aggStatusObj.(gwapiv1.PolicyStatus); ok {
194+
aggStatus = aggStatusObj.(gwapiv1.PolicyStatus)
195+
} else {
196+
aggStatus = gwapiv1.PolicyStatus{}
197+
}
198+
199+
newStatusObj := newPolicy.Object["status"]
200+
var newStatus gwapiv1.PolicyStatus
201+
if _, ok := newStatusObj.(map[string]any); ok {
202+
newStatus = UnstructuredToPolicyStatus(newStatusObj.(map[string]any))
203+
} else if _, ok := newStatusObj.(gwapiv1.PolicyStatus); ok {
204+
newStatus = newStatusObj.(gwapiv1.PolicyStatus)
205+
} else {
206+
newStatus = gwapiv1.PolicyStatus{}
207+
}
208+
209+
aggStatus.Ancestors = append(aggStatus.Ancestors, newStatus.Ancestors...)
210+
aggregatedPolicy.Object["status"] = PolicyStatusToUnstructured(aggStatus)
211+
}

‎internal/gatewayapi/extensionserverpolicy_test.go‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,3 +123,115 @@ func TestExtractTargetRefs(t *testing.T) {
123123
})
124124
}
125125
}
126+
127+
func TestMergeAncestorsForExtensionServerPolicies(t *testing.T) {
128+
tests := []struct {
129+
aggStatus *gwapiv1.PolicyStatus
130+
newStatus *gwapiv1.PolicyStatus
131+
noStatus bool
132+
}{
133+
{
134+
aggStatus: &gwapiv1.PolicyStatus{
135+
Ancestors: []gwapiv1.PolicyAncestorStatus{
136+
{
137+
AncestorRef: gwapiv1.ParentReference{
138+
Name: "gateway-1",
139+
},
140+
},
141+
},
142+
},
143+
newStatus: &gwapiv1.PolicyStatus{
144+
Ancestors: []gwapiv1.PolicyAncestorStatus{
145+
{
146+
AncestorRef: gwapiv1.ParentReference{
147+
Name: "gateway-2",
148+
},
149+
},
150+
},
151+
},
152+
},
153+
{
154+
aggStatus: &gwapiv1.PolicyStatus{},
155+
newStatus: &gwapiv1.PolicyStatus{
156+
Ancestors: []gwapiv1.PolicyAncestorStatus{
157+
{
158+
AncestorRef: gwapiv1.ParentReference{
159+
Name: "gateway-2",
160+
},
161+
},
162+
},
163+
},
164+
},
165+
{
166+
aggStatus: &gwapiv1.PolicyStatus{
167+
Ancestors: []gwapiv1.PolicyAncestorStatus{
168+
{
169+
AncestorRef: gwapiv1.ParentReference{
170+
Name: "gateway-1",
171+
},
172+
},
173+
},
174+
},
175+
newStatus: &gwapiv1.PolicyStatus{},
176+
},
177+
{
178+
aggStatus: &gwapiv1.PolicyStatus{},
179+
newStatus: &gwapiv1.PolicyStatus{},
180+
},
181+
{
182+
aggStatus: nil,
183+
newStatus: &gwapiv1.PolicyStatus{
184+
Ancestors: []gwapiv1.PolicyAncestorStatus{
185+
{
186+
AncestorRef: gwapiv1.ParentReference{
187+
Name: "gateway-1",
188+
},
189+
},
190+
},
191+
},
192+
},
193+
{
194+
aggStatus: &gwapiv1.PolicyStatus{
195+
Ancestors: []gwapiv1.PolicyAncestorStatus{
196+
{
197+
AncestorRef: gwapiv1.ParentReference{
198+
Name: "gateway-1",
199+
},
200+
},
201+
},
202+
},
203+
newStatus: nil,
204+
},
205+
{
206+
aggStatus: nil,
207+
newStatus: nil,
208+
},
209+
}
210+
211+
for _, test := range tests {
212+
aggPolicy := unstructured.Unstructured{Object: make(map[string]interface{})}
213+
newPolicy := unstructured.Unstructured{Object: make(map[string]interface{})}
214+
desiredMergedStatus := gwapiv1.PolicyStatus{}
215+
216+
// aggStatus == nil, means simulate not setting status at all within the policy.
217+
if test.aggStatus != nil {
218+
aggPolicy.Object["status"] = PolicyStatusToUnstructured(*test.aggStatus)
219+
desiredMergedStatus.Ancestors = append(desiredMergedStatus.Ancestors, test.aggStatus.Ancestors...)
220+
}
221+
222+
// newStatus == nil, means simulate not setting status at all within the policy.
223+
if test.newStatus != nil {
224+
newPolicy.Object["status"] = PolicyStatusToUnstructured(*test.newStatus)
225+
desiredMergedStatus.Ancestors = append(desiredMergedStatus.Ancestors, test.newStatus.Ancestors...)
226+
}
227+
228+
MergeAncestorsForExtensionServerPolicies(&aggPolicy, &newPolicy)
229+
230+
// The product object will always have an existing `status`, even if with 0 ancestors.
231+
newAggPolicy := UnstructuredToPolicyStatus(aggPolicy.Object["status"].(map[string]any))
232+
require.Len(t, newAggPolicy.Ancestors, len(desiredMergedStatus.Ancestors))
233+
for i := range newAggPolicy.Ancestors {
234+
require.Equal(t, desiredMergedStatus.Ancestors[i].AncestorRef.Name, newAggPolicy.Ancestors[i].AncestorRef.Name)
235+
}
236+
}
237+
}

‎internal/gatewayapi/runner/runner.go‎

Lines changed: 2 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ package runner
88
import (
99
"context"
1010
"crypto/tls"
11-
"encoding/json"
1211
"fmt"
1312
"os"
1413
"path"
@@ -407,37 +406,10 @@ func (r *Runner) subscribeAndTranslate(sub <-chan watchable.Snapshot[string, *re
407406
break
408407
}
409408
}
410-
411409
// If already found (because processed in an earlier gatewayclass), append its status ancestors.
412410
// Otherwise, append it to the list of *Policies in the accumulated result.
413411
if aggregatedPolicy != nil {
414-
ancestorsObject, found, err := unstructured.NestedFieldCopy(aggregatedPolicy.Object, "status", "ancestors")
415-
if !found || err != nil {
416-
r.Logger.Error(err, "failed to get existing ancestors", "policyName", aggregatedPolicy.GetName(), "ancestorsFound", found)
417-
continue
418-
}
419-
ancestors, ok := ancestorsObject.([]gwapiv1.PolicyAncestorStatus)
420-
if !ok {
421-
r.Logger.Error(err, "failed to assert existing ancestors type", "policyName", aggregatedPolicy.GetName())
422-
continue
423-
}
424-
425-
newAncestorsObject, found, err := unstructured.NestedFieldCopy(extServerPolicy.Object, "status", "ancestors")
426-
if !found || err != nil {
427-
r.Logger.Error(err, "failed to get new ancestors", "policyName", aggregatedPolicy.GetName(), "ancestorsFound", found)
428-
continue
429-
}
430-
newAncestors, ok := newAncestorsObject.([]gwapiv1.PolicyAncestorStatus)
431-
if !ok {
432-
r.Logger.Error(err, "failed to assert new ancestors type", "policyName", aggregatedPolicy.GetName())
433-
continue
434-
}
435-
436-
ancestors = append(ancestors, newAncestors...)
437-
err = unstructured.SetNestedField(aggregatedPolicy.Object, ancestors, "status", "ancestors")
438-
if err != nil {
439-
r.Logger.Error(err, "failed to update ancestors of policy", "policyName", aggregatedPolicy.GetName())
440-
}
412+
gatewayapi.MergeAncestorsForExtensionServerPolicies(aggregatedPolicy, &extServerPolicy)
441413
} else {
442414
aggregatedPolicy = &extServerPolicy
443415
aggregatedResult.ExtensionServerPolicies = append(aggregatedResult.ExtensionServerPolicies, *aggregatedPolicy)
@@ -538,7 +510,7 @@ func (r *Runner) subscribeAndTranslate(sub <-chan watchable.Snapshot[string, *re
538510
}
539511
if statusObj, hasStatus := extServerPolicy.Object["status"]; hasStatus && statusObj != nil {
540512
if statusMap, ok := statusObj.(map[string]any); ok && len(statusMap) > 0 {
541-
policyStatus := unstructuredToPolicyStatus(statusMap)
513+
policyStatus := gatewayapi.UnstructuredToPolicyStatus(statusMap)
542514
r.ProviderResources.ExtensionPolicyStatuses.Store(key, &policyStatus)
543515
extensionServerPolicyStatusCount++
544516
}
@@ -622,16 +594,6 @@ func (r *Runner) loadTLSConfig(ctx context.Context) (*tls.Config, []byte, error)
622594
}
623595
}
624596

625-
func unstructuredToPolicyStatus(policyStatus map[string]any) gwapiv1.PolicyStatus {
626-
var ret gwapiv1.PolicyStatus
627-
// No need to check the json marshal/unmarshal error, the policyStatus was
628-
// created via a typed object so the marshalling/unmarshalling will always
629-
// work
630-
d, _ := json.Marshal(policyStatus)
631-
_ = json.Unmarshal(d, &ret)
632-
return ret
633-
}
634-
635597
// deleteAllIRKeys deletes all XdsIR and InfraIR using tracked keys
636598
func (r *Runner) deleteAllKeys() {
637599
// Delete IR keys

0 commit comments

Comments
 (0)