Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make gwctl work with Policies having multiple targetRefs #3217

Closed
wants to merge 4 commits into from
Closed

Make gwctl work with Policies having multiple targetRefs #3217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
667e73b
Make gwctl work with Policies having multiple targetRefs
deszhou Jul 25, 2024
09fe7a8
chore:del TestPoliciesPrinter_Print_And_PrintDescribeView2
deszhou Jul 25, 2024
d13c46c
chore:del convertPoliciesToRefsTable and fix lint test
deszhou Jul 25, 2024
7408571
Add support for both targetRefs and targetRef fields in gwctl
deszhou Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 72 additions & 34 deletions gwctl/pkg/policymanager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,13 @@ import (
"fmt"
"strings"

"sigs.k8s.io/controller-runtime/pkg/client"

apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/dynamic"
"sigs.k8s.io/controller-runtime/pkg/client"

gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
"sigs.k8s.io/gateway-api/gwctl/pkg/common"
Expand Down Expand Up @@ -216,10 +215,10 @@ func (p PolicyCRD) IsClusterScoped() bool {

type Policy struct {
u unstructured.Unstructured
// targetRef references the target object this policy is attached to. This
// targetRefs references the target object this policy is attached to. This
// only makes sense in case of a directly-attached-policy, or an
// unmerged-inherited-policy.
targetRef common.ObjRef
targetRefs []common.ObjRef
// Indicates whether the policy is supposed to be "inherited" (as opposed to
// "direct").
inherited bool
Expand All @@ -230,29 +229,53 @@ func (p Policy) ClientObject() client.Object { return p.Unstructured() }
func PolicyFromUnstructured(u unstructured.Unstructured, policyCRDs map[PolicyCrdID]PolicyCRD) (Policy, error) {
result := Policy{u: u}

// Identify targetRef of Policy.
// Identify targetRefs of Policy.
type genericPolicy struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec struct {
TargetRef gatewayv1alpha2.NamespacedPolicyTargetReference
TargetRefs []gatewayv1alpha2.NamespacedPolicyTargetReference `json:"targetRefs"`
TargetRef *gatewayv1alpha2.NamespacedPolicyTargetReference `json:"targetRef"`
}
}
structuredPolicy := &genericPolicy{}
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.UnstructuredContent(), structuredPolicy); err != nil {
return Policy{}, fmt.Errorf("failed to convert unstructured policy resource to structured: %v", err)
}
result.targetRef = common.ObjRef{
Group: string(structuredPolicy.Spec.TargetRef.Group),
Kind: string(structuredPolicy.Spec.TargetRef.Kind),
Name: string(structuredPolicy.Spec.TargetRef.Name),
Namespace: structuredPolicy.GetNamespace(),
}
if result.targetRef.Namespace == "" {
result.targetRef.Namespace = result.u.GetNamespace()

// Process targetRefs
for _, targetRef := range structuredPolicy.Spec.TargetRefs {
objRef := common.ObjRef{
Group: string(targetRef.Group),
Kind: string(targetRef.Kind),
Name: string(targetRef.Name),
Namespace: u.GetNamespace(),
}
if objRef.Namespace == "" {
objRef.Namespace = u.GetNamespace()
}
Comment on lines +252 to +256
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be missing something, but I understand objRef.Namespace has been initialized to u.GetNamespace() already, no? Then, if it's an empty string, it's set again to the same u.GetNamespace()? Maybe you meant:

Suggested change
Namespace: u.GetNamespace(),
}
if objRef.Namespace == "" {
objRef.Namespace = u.GetNamespace()
}
Namespace: structuredPolicy.GetNamespace(),
}
if objRef.Namespace == "" {
objRef.Namespace = u.GetNamespace()
}

if targetRef.Namespace != nil {
objRef.Namespace = string(*targetRef.Namespace)
}
Comment on lines +257 to +259
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're willing to import k8s.io/utils/ptr, this kind of pattern can be written:

objRef.Namespace = ptr.Deref(targetRef.Namespace, structuredPolicy.GetNamespace())

result.targetRefs = append(result.targetRefs, objRef)
}
if structuredPolicy.Spec.TargetRef.Namespace != nil {
result.targetRef.Namespace = string(*structuredPolicy.Spec.TargetRef.Namespace)

// Process single targetRef (if present and targetRefs is empty)
if structuredPolicy.Spec.TargetRef != nil && len(result.targetRefs) == 0 {
targetRef := structuredPolicy.Spec.TargetRef
objRef := common.ObjRef{
Group: string(targetRef.Group),
Kind: string(targetRef.Kind),
Name: string(targetRef.Name),
Namespace: u.GetNamespace(),
}
if objRef.Namespace == "" {
objRef.Namespace = u.GetNamespace()
}
if targetRef.Namespace != nil {
objRef.Namespace = string(*targetRef.Namespace)
}
result.targetRefs = append(result.targetRefs, objRef)
}

// Get the CRD corresponding to this policy object.
Expand All @@ -274,8 +297,8 @@ func (p Policy) PolicyCrdID() PolicyCrdID {
return PolicyCrdID(p.u.GetObjectKind().GroupVersionKind().Kind + "." + p.u.GetObjectKind().GroupVersionKind().Group)
}

func (p Policy) TargetRef() common.ObjRef {
return p.targetRef
func (p Policy) TargetRefs() []common.ObjRef {
return p.targetRefs
}

func (p Policy) IsInherited() bool {
Expand All @@ -287,19 +310,24 @@ func (p Policy) IsDirect() bool {
}

func (p Policy) IsAttachedTo(objRef common.ObjRef) bool {
if p.targetRef.Kind == "Namespace" && p.targetRef.Name == "" {
p.targetRef.Name = "default"
}
if objRef.Kind == "Namespace" && objRef.Name == "" {
objRef.Name = "default"
}
if p.targetRef.Kind != "Namespace" && p.targetRef.Namespace == "" {
p.targetRef.Namespace = "default"
}
if objRef.Kind != "Namespace" && objRef.Namespace == "" {
objRef.Namespace = "default"
for _, targetRef := range p.targetRefs {
if targetRef.Kind == "Namespace" && targetRef.Name == "" {
targetRef.Name = "default"
}
if objRef.Kind == "Namespace" && objRef.Name == "" {
objRef.Name = "default"
}
Comment on lines +317 to +319
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this could be done outside the loop.

if targetRef.Kind != "Namespace" && targetRef.Namespace == "" {
targetRef.Namespace = "default"
}
if objRef.Kind != "Namespace" && objRef.Namespace == "" {
objRef.Namespace = "default"
}
Comment on lines +323 to +325
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. Could be done outside the loop.

if targetRef == objRef {
return true
}
}
return p.targetRef == objRef
return false
}

func (p Policy) Unstructured() *unstructured.Unstructured {
Expand All @@ -308,9 +336,9 @@ func (p Policy) Unstructured() *unstructured.Unstructured {

func (p Policy) DeepCopy() Policy {
clone := Policy{
u: *p.u.DeepCopy(),
targetRef: p.targetRef,
inherited: p.inherited,
u: *p.u.DeepCopy(),
targetRefs: p.targetRefs,
inherited: p.inherited,
}
return clone
}
Expand All @@ -320,18 +348,28 @@ func (p Policy) Spec() map[string]interface{} {
if err != nil || !ok {
return nil
}

result, ok := spec.(map[string]interface{})
if !ok {
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit out of scope, but we could probably use unstructured.NestedMap instead of unstructured.NestedFieldCopy at line 347 and thus save the extra type casting.

if targetRef, hasTargetRef := result["targetRef"]; hasTargetRef {
if _, hasTargetRefs := result["targetRefs"]; !hasTargetRefs {
result["targetRefs"] = []interface{}{targetRef}
} else {
result["targetRefs"] = append(result["targetRefs"].([]interface{}), targetRef)
}
delete(result, "targetRef")
}

return result
}

func (p Policy) EffectiveSpec() (map[string]interface{}, error) {
if !p.IsInherited() {
// No merging is required in case of Direct policies.
result := p.Spec()
delete(result, "targetRefs")
delete(result, "targetRef")
return result, nil
}
Expand Down
27 changes: 25 additions & 2 deletions gwctl/pkg/policymanager/merger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,14 @@ func MergePoliciesOfSimilarKind(policies []Policy) (map[PolicyCrdID]Policy, erro
return nil, err
}

result[policyCrdID] = mergedPolicies[policyCrdID]
mergedPolicy := mergedPolicies[policyCrdID]

// Check and set targetRefs to nil if it's an empty slice
if len(mergedPolicy.targetRefs) == 0 {
mergedPolicy.targetRefs = nil
}

result[policyCrdID] = mergedPolicy
}
return result, nil
}
Expand Down Expand Up @@ -149,10 +156,26 @@ func mergePolicy(parent, child Policy) (Policy, error) {
result.u.SetUnstructuredContent(resultUnstructured)
// Merging two policies means the targetRef no longer makes any sense since
// since they can be conflicting. So we unset the targetRef.
result.targetRef = common.ObjRef{}
result.targetRefs = mergeTargetRefs(parent.targetRefs, child.targetRefs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say parent has targetRefs: [a, b] and child has targetRefs: [a, c]. So the merged (effective) policy applies to [a, b, c]?

Sorry, I may need to educate myself on how policies to be merged are selected in the first place – or, more precisely, how the policies are assigned to a particular node in the resource model. It just triggered me the idea that, if you ask for the effective policy for a, it will return a merge of parent and child, along with the info that such effective policy also applies to c. However, that's not entire really true, because if you ask for the effective policy for c, it may return something identical to child only (not a necessarily the parent+child merge.)

Concrete example – Assume a == gateway-1, b == route-1, and c == route-2, though with route-1.parentRefs == [gateway-1] and route-2.parentRefs == [gateway-2]. (Notice route-2 is NOT parented by gateway-1.)

IOW:

kind: Gateway
metadata:
  name: gateway-1
---
kind: Gateway
metadata:
  name: gateway-2
---
kind: HTTPRoute
metadata:
  name: route-1
spec:
  parentRefs:
  - kind: Gateway
    name: gateway-1
---
kind: HTTPRoute
metadata:
  name: route-2
spec:
  parentRefs:
  - kind: Gateway
    name: gateway-2
---
kind: Policy
metadata:
  name: parent
spec:
  targetRefs:
  - kind: Gateway
    name: gateway-1
  - kind: HTTPRoute
    name: route-1
---
kind: Policy
metadata:
  name: child
spec:
  targetRefs:
  - kind: Gateway
    name: gateway-1
  - kind: HTTPRoute
    name: route-2

Effective policies:

gateway-1 → parent + child (targetRefs: [gateway-1, route-1, route-2])
gateway 2 → ∅
route-1 → parent (targetRefs: [gateway-1, route-1])
route-2 → child (targetRefs: [gateway-1, route-2])

By reading the first line, if you trust parent applies to route-2 (or that child applies to route-1 analogously), you would be wrong.

return result, nil
}

// Helper function to merge targetRefs from two policies
func mergeTargetRefs(refs1, refs2 []common.ObjRef) []common.ObjRef {
refMap := make(map[common.ObjRef]struct{})
for _, ref := range refs1 {
refMap[ref] = struct{}{}
}
for _, ref := range refs2 {
refMap[ref] = struct{}{}
}
result := make([]common.ObjRef, 0, len(refMap))
for ref := range refMap {
result = append(result, ref)
}
return result
}

func mergeUnstructured(parent, patch map[string]interface{}) (map[string]interface{}, error) {
currentJSON, err := json.Marshal(parent)
if err != nil {
Expand Down
24 changes: 15 additions & 9 deletions gwctl/pkg/policymanager/merger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,11 @@ func TestMergePoliciesOfSimilarKind(t *testing.T) {
"spec": map[string]interface{}{
"condition": "path=/def",
"seconds": float64(30),
"targetRef": map[string]interface{}{
"kind": "Namespace",
"name": "default",
"targetRefs": []interface{}{
map[string]interface{}{
"kind": "Namespace",
"name": "default",
},
},
},
},
Expand All @@ -106,9 +108,11 @@ func TestMergePoliciesOfSimilarKind(t *testing.T) {
"spec": map[string]interface{}{
"condition": "path=/abc",
"seconds": float64(60),
"targetRef": map[string]interface{}{
"kind": "Namespace",
"name": "default",
"targetRefs": []interface{}{
map[string]interface{}{
"kind": "Namespace",
"name": "default",
},
},
},
},
Expand Down Expand Up @@ -152,9 +156,11 @@ func TestMergePoliciesOfSimilarKind(t *testing.T) {
"spec": map[string]interface{}{
"condition": "path=/def",
"seconds": float64(30),
"targetRef": map[string]interface{}{
"kind": "Namespace",
"name": "default",
"targetRefs": []interface{}{
map[string]interface{}{
"kind": "Namespace",
"name": "default",
},
},
},
},
Expand Down
20 changes: 14 additions & 6 deletions gwctl/pkg/printer/backends_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,19 @@ func TestBackendsPrinter_Print(t *testing.T) {
"default": map[string]interface{}{
"key2": "value-parent-2",
},
"targetRef": map[string]interface{}{
"group": "",
"kind": "Service",
"name": "foo-svc-1",
"namespace": "ns1",
"targetRefs": []interface{}{
map[string]interface{}{
"group": "",
"kind": "Service",
"name": "foo-svc-1",
"namespace": "ns1",
},
map[string]interface{}{
"group": "",
"kind": "Service",
"name": "foo-svc-2",
"namespace": "ns1",
},
},
},
},
Expand Down Expand Up @@ -219,7 +227,7 @@ ns1 foo-svc-2 Service 2d
want2 := `
NAMESPACE NAME TYPE AGE REFERRED BY ROUTES POLICIES
ns1 foo-svc-1 Service 3d ns1/foo-httproute-1 1
ns1 foo-svc-2 Service 2d ns1/foo-httproute-2, ns1/foo-httproute-3 + 2 more 0
ns1 foo-svc-2 Service 2d ns1/foo-httproute-2, ns1/foo-httproute-3 + 2 more 1
`
if diff := cmp.Diff(common.YamlString(want2), common.YamlString(got2), common.YamlStringTransformer); diff != "" {
t.Errorf("Unexpected diff\ngot=\n%v\nwant=\n%v\ndiff (-want +got)=\n%v", got2, want2, diff)
Expand Down
35 changes: 18 additions & 17 deletions gwctl/pkg/printer/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,26 +171,27 @@ func convertPoliciesToRefsTable(policies []*resourcediscovery.PolicyNode, includ
policyName = fmt.Sprintf("%v/%v", ns, policyName)
}

targetKind := policyNode.Policy.TargetRef().Kind

targetName := policyNode.Policy.TargetRef().Name
if ns := policyNode.Policy.TargetRef().Namespace; ns != "" {
targetName = fmt.Sprintf("%v/%v", ns, targetName)
}
// Iterate over each TargetRef
for _, targetRef := range policyNode.Policy.TargetRefs() {
targetKind := targetRef.Kind
targetName := targetRef.Name
if ns := targetRef.Namespace; ns != "" {
targetName = fmt.Sprintf("%v/%v", ns, targetName)
}

row := []string{
policyType, // Type
policyName, // Name
}
row := []string{
policyType, // Type
policyName, // Name
}

if includeTarget {
row = append(row,
targetKind, // Target Kind
targetName, // Target Name
)
if includeTarget {
row = append(row,
targetKind, // Target Kind
targetName, // Target Name
)
}
table.Rows = append(table.Rows, row)
}

table.Rows = append(table.Rows, row)
}
return table
}
Expand Down
10 changes: 6 additions & 4 deletions gwctl/pkg/printer/gatewayclasses_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,12 @@ func TestGatewayClassesPrinter_PrintDescribeView(t *testing.T) {
"name": "policy-name",
},
"spec": map[string]interface{}{
"targetRef": map[string]interface{}{
"group": "gateway.networking.k8s.io",
"kind": "GatewayClass",
"name": "foo-gatewayclass",
"targetRefs": []interface{}{
map[string]interface{}{
"group": "gateway.networking.k8s.io",
"kind": "GatewayClass",
"name": "foo-gatewayclass",
},
},
},
},
Expand Down
Loading