-
Notifications
You must be signed in to change notification settings - Fork 350
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
feat: Implement target selectors for policies. #3704
Conversation
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3704 +/- ##
==========================================
+ Coverage 68.81% 69.05% +0.24%
==========================================
Files 175 176 +1
Lines 21525 21720 +195
==========================================
+ Hits 14812 14999 +187
- Misses 5636 5639 +3
- Partials 1077 1082 +5 ☔ View full report in Codecov by Sentry. |
internal/gatewayapi/helpers.go
Outdated
@@ -495,3 +496,37 @@ func irConfigName(policy client.Object) string { | |||
strings.ToLower(policy.GetObjectKind().GroupVersionKind().Kind), | |||
utils.NamespacedName(policy).String()) | |||
} | |||
|
|||
func getPolicyTargetRefs[T client.Object](policy egv1a1.PolicyTargetReferences, potentialTargets []T) []gwapiv1a2.LocalPolicyTargetReferenceWithSectionName { | |||
dedup := map[gwapiv1a2.LocalPolicyTargetReferenceWithSectionName]byte{} |
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.
sets instead ?
internal/gatewayapi/helpers.go
Outdated
} | ||
|
||
ret := []gwapiv1a2.LocalPolicyTargetReferenceWithSectionName{} | ||
for key := range dedup { |
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.
can we garantee stability of list order with maps or sets ?
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.
also is there a way to make this stateless ? we are actively trying to reduce mem usage #3698
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.
can we garantee stability of list order with maps or sets ?
No. Sets are the same as the builtin maps, since they are implemented in the same way, and they do not guarantee the list order. When transforming the map to a list, I can sort the list. What would be the correct key in which the list should be ordered?
is there a way to make this stateless ? we are actively trying to reduce mem usage
Using a map
to deduplicate the set of relevant targets means that the code never allocates more memory than it would need to keep track of the targets of the policy. I don't see a way to collect the targets of the selectors and not keep some state. Note that the memory is only allocated for the duration of the time it takes to handle the policy - it's not some cache that is kept and never released.
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.
lets sort based on creation timestamp
// Sort based on timestamp
sort.Slice(ret, func(i, j int) bool {
return ret[i].CreationTimestamp.Before(&(ret[j].CreationTimestamp))
})
```
returned refs are consistent. Signed-off-by: Lior Okman <lior.okman@sap.com>
/retest |
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.
LGTM, Thanks!
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.
thanks !
@arkodg, @liorokman, I installed |
What this PR does / why we need it:
This PR implements target selectors for policies
Which issue(s) this PR fixes:
Fixes #3607