-
Notifications
You must be signed in to change notification settings - Fork 457
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.
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
Make gwctl work with Policies having multiple targetRefs #3217
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deszhou The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks @deszhou. I'll give a more detailed look later, but just wanted to call out that the tricky thing here would be that we would like to support both |
SGTM, and for consistency, we should use the |
Namespace: u.GetNamespace(), | ||
} | ||
if objRef.Namespace == "" { | ||
objRef.Namespace = u.GetNamespace() | ||
} |
There was a problem hiding this comment.
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:
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) | ||
} |
There was a problem hiding this comment.
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())
if objRef.Kind == "Namespace" && objRef.Name == "" { | ||
objRef.Name = "default" | ||
} |
There was a problem hiding this comment.
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 objRef.Kind != "Namespace" && objRef.Namespace == "" { | ||
objRef.Namespace = "default" | ||
} |
There was a problem hiding this comment.
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.
result, ok := spec.(map[string]interface{}) | ||
if !ok { | ||
return nil | ||
} | ||
|
There was a problem hiding this comment.
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.
@@ -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) |
There was a problem hiding this comment.
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.
Thanks, @guicassolato! I’ll update this PR and fix the issue after #3244. |
@deszhou: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #3196
Does this PR introduce a user-facing change?:
gwctl get policies -A
gwctl describe policies health-check-gateway